[3/8] infcall, c++: collect more pass-by-reference information

Message ID 1556029914-21250-4-git-send-email-tankut.baris.aktemur@intel.com
State Superseded
Headers show
Series
  • Fix inferior call for C++ pass-by-reference arguments
Related show

Commit Message

Tankut Baris Aktemur April 23, 2019, 2:31 p.m.
Walk through a given type to collect information about whether the type
is copy constructible, destructible, trivially copyable, trivially copy
constructible, trivially destructible.  The previous algorithm returned
only a boolean result about whether the type is trivially copyable.
This patch computes more info.  Additionally, it utilizes DWARF attributes
that were previously not taken into account; namely,
DW_AT_deleted, DW_AT_defaulted, and DW_AT_calling_convention.

gdb/ChangeLog:

	* gnu-v3-abi.c (enum definition_style): New.
	(get_def_style): New.
	(is_user_provided_def): New.
	(is_implicit_def): New.
	(is_copy_or_move_constructor_type): New.
	(is_copy_constructor_type): New.
	(is_move_constructor_type): New.
	(gnuv3_pass_by_reference): Update.

---
 gdb/gnu-v3-abi.c | 259 +++++++++++++++++++++++++++++++++++++----------
 1 file changed, 208 insertions(+), 51 deletions(-)

-- 
2.21.0

Comments

Andrew Burgess May 22, 2019, 8:34 p.m. | #1
* Tankut Baris Aktemur <tankut.baris.aktemur@intel.com> [2019-04-23 16:31:49 +0200]:

> Walk through a given type to collect information about whether the type

> is copy constructible, destructible, trivially copyable, trivially copy

> constructible, trivially destructible.  The previous algorithm returned

> only a boolean result about whether the type is trivially copyable.

> This patch computes more info.  Additionally, it utilizes DWARF attributes

> that were previously not taken into account; namely,

> DW_AT_deleted, DW_AT_defaulted, and DW_AT_calling_convention.

> 

> gdb/ChangeLog:

> 

> 	* gnu-v3-abi.c (enum definition_style): New.

> 	(get_def_style): New.

> 	(is_user_provided_def): New.

> 	(is_implicit_def): New.

> 	(is_copy_or_move_constructor_type): New.

> 	(is_copy_constructor_type): New.

> 	(is_move_constructor_type): New.

> 	(gnuv3_pass_by_reference): Update.


I'm basically happy with this, a few formatting issues and questions
about comments below.

> 

> ---

>  gdb/gnu-v3-abi.c | 259 +++++++++++++++++++++++++++++++++++++----------

>  1 file changed, 208 insertions(+), 51 deletions(-)

> 

> diff --git a/gdb/gnu-v3-abi.c b/gdb/gnu-v3-abi.c

> index b39e5a5c585..9069305626f 100644

> --- a/gdb/gnu-v3-abi.c

> +++ b/gdb/gnu-v3-abi.c

> @@ -23,6 +23,7 @@

>  #include "cp-abi.h"

>  #include "cp-support.h"

>  #include "demangle.h"

> +#include "dwarf2.h"

>  #include "objfiles.h"

>  #include "valprint.h"

>  #include "c-lang.h"

> @@ -1228,6 +1229,122 @@ gnuv3_skip_trampoline (struct frame_info *frame, CORE_ADDR stop_pc)

>    return real_stop_pc;

>  }

>  

> +/* A member function is in one these states.  */

> +

> +enum definition_style

> +{

> +  DOES_NOT_EXIST_IN_SOURCE,

> +  DEFAULTED_INSIDE,

> +  DEFAULTED_OUTSIDE,

> +  DELETED,

> +  EXPLICIT,

> +};

> +

> +/* Return how the given field is defined.  */

> +

> +static definition_style

> +get_def_style (struct fn_field *fn, int fieldelem)

> +{

> +  if (TYPE_FN_FIELD_DELETED (fn, fieldelem))

> +    return DELETED;

> +

> +  if (TYPE_FN_FIELD_ARTIFICIAL (fn, fieldelem))

> +    return DOES_NOT_EXIST_IN_SOURCE;

> +

> +  switch (TYPE_FN_FIELD_DEFAULTED (fn, fieldelem))

> +    {

> +    case DW_DEFAULTED_no:

> +      return EXPLICIT;

> +    case DW_DEFAULTED_in_class:

> +      return DEFAULTED_INSIDE;

> +    case DW_DEFAULTED_out_of_class:

> +      return DEFAULTED_OUTSIDE;

> +    default:

> +      break;

> +    }

> +

> +  return EXPLICIT;

> +}

> +

> +/* Helper functions to determine whether the given definition style

> +   denotes that the definition is user-provided or implicit.

> +   Being defaulted outside the class decl counts as an explicit

> +   user-definition, while being defaulted inside is implicit.  */

> +

> +static bool

> +is_user_provided_def (definition_style def)

> +{

> +  return def == EXPLICIT || def == DEFAULTED_OUTSIDE;

> +}

> +

> +static bool

> +is_implicit_def (definition_style def)

> +{

> +  return def == DOES_NOT_EXIST_IN_SOURCE || def == DEFAULTED_INSIDE;

> +}

> +

> +/* Helper function to decide if METHOD_TYPE is a copy/move

> +   constructor type for CLASS_TYPE.  EXPECTED is the expected

> +   type code for the "right-hand-side" argument.

> +   This function is supposed to be used by the IS_COPY_CONSTRUCTOR_TYPE

> +   and IS_MOVE_CONSTRUCTOR_TYPE functions below.  Normally, you should

> +   not need to call this directly.  */

> +

> +static bool

> +is_copy_or_move_constructor_type (struct type *class_type,

> +				  struct type *method_type,

> +				  type_code expected)

> +{

> +  /* The method should take at least two arguments...  */

> +  if (TYPE_NFIELDS (method_type) < 2)

> +    return false;

> +

> +  /* ...and the second argument should be the same as the class

> +     type, with the expected type code...  */

> +  struct type *arg_type = TYPE_FIELD_TYPE (method_type, 1);

> +

> +  if (TYPE_CODE (arg_type) != expected)

> +    return false;

> +

> +  struct type *target = check_typedef (TYPE_TARGET_TYPE (arg_type));

> +  if (!(class_types_same_p (target, class_type)))

> +    return false;

> +

> +  /* ...and the rest of the arguments should have default values.  */


This comment threw me for a while I read this to mean that I couldn't
legally write something like:

  struct A
  {
    int a;
    A (const A &other, int arg);
  };

which is daft, because obviously I can.  Then I realised that that
isn't a copy constructor, it's just a constructor that takes an 'A&',
doh!

But, I wonder if the comment could be made clearer to help those of us
a bit slower on the uptake, maybe something like:

  /* ...and if any of the remaining arguments don't have a default value
     then this is not a copy or move constructor, but just a constructor.  */

> +  for (int i = 2; i < TYPE_NFIELDS (method_type); i++)

> +    {

> +      arg_type = TYPE_FIELD_TYPE (method_type, i);

> +      /* FIXME taktemur/2019-04-23: As of this date, neither

> +	 clang++-7.0.0 nor g++-8.2.0 produce a DW_AT_default_value

> +	 attribute.  GDB is also not set to read this attribute, yet.

> +	 Hence, we immediately return false if there are more than

> +	 2 parameters.  */

> +      return false;

> +    }

> +

> +  return true;

> +}

> +

> +/* Return true if METHOD_TYPE is a copy ctor type for CLASS_TYPE.  */

> +

> +static bool

> +is_copy_constructor_type (struct type *class_type,

> +			  struct type *method_type)

> +{

> +  return is_copy_or_move_constructor_type (class_type, method_type,

> +					   TYPE_CODE_REF);

> +}

> +

> +/* Return true if METHOD_TYPE is a move ctor type for CLASS_TYPE.  */

> +

> +static bool

> +is_move_constructor_type (struct type *class_type,

> +			  struct type *method_type)

> +{

> +  return is_copy_or_move_constructor_type (class_type, method_type,

> +					   TYPE_CODE_RVALUE_REF);

> +}

> +

>  /* Return pass-by-reference information for the given TYPE.

>  

>     The rule in the v3 ABI document comes from section 3.1.1.  If the

> @@ -1236,16 +1353,15 @@ gnuv3_skip_trampoline (struct frame_info *frame, CORE_ADDR stop_pc)

>     is one or perform the copy itself otherwise), pass the address of

>     the copy, and then destroy the temporary (if necessary).

>  

> -   For return values with non-trivial copy constructors or

> +   For return values with non-trivial copy/move constructors or

>     destructors, space will be allocated in the caller, and a pointer

>     will be passed as the first argument (preceding "this").

>  

>     We don't have a bulletproof mechanism for determining whether a

> -   constructor or destructor is trivial.  For GCC and DWARF2 debug

> -   information, we can check the artificial flag.

> -

> -   We don't do anything with the constructors or destructors,

> -   but we have to get the argument passing right anyway.  */

> +   constructor or destructor is trivial.  For GCC and DWARF5 debug

> +   information, we can check the calling_convention attribute,

> +   the 'artificial' flag, the 'defaulted' attribute, and the

> +   'deleted' attribute.  */

>  

>  static struct language_pass_by_ref_info

>  gnuv3_pass_by_reference (struct type *type)

> @@ -1258,22 +1374,39 @@ gnuv3_pass_by_reference (struct type *type)

>    struct language_pass_by_ref_info info

>      = default_pass_by_reference (type);

>  

> -  /* FIXME: Currently, this implementation only fills in the

> -     'trivially-copyable' field.  */

> +  bool has_cc_attr = false;

> +  bool is_pass_by_value = false;

> +  bool is_dynamic = false;

> +  definition_style cctor_def = DOES_NOT_EXIST_IN_SOURCE;

> +  definition_style dtor_def = DOES_NOT_EXIST_IN_SOURCE;

> +  definition_style mctor_def = DOES_NOT_EXIST_IN_SOURCE;

>  

>    /* We're only interested in things that can have methods.  */

>    if (TYPE_CODE (type) != TYPE_CODE_STRUCT

>        && TYPE_CODE (type) != TYPE_CODE_UNION)

>      return info;

>  

> +  /* The compiler may have emitted the calling convention attribute.  */

> +  if (TYPE_CPLUS_CALLING_CONVENTION (type) == DW_CC_pass_by_value)

> +    {

> +      has_cc_attr = true;

> +      is_pass_by_value = true;

> +      /* Do not return immediately.  We have to find out if this type

> +	 is copy_constructible and destructible.  */

> +    }

> +

> +  if (TYPE_CPLUS_CALLING_CONVENTION (type) == DW_CC_pass_by_reference)

> +    {

> +      has_cc_attr = true;

> +      is_pass_by_value = false;

> +    }

> +

>    /* A dynamic class has a non-trivial copy constructor.

>       See c++98 section 12.8 Copying class objects [class.copy].  */

>    if (gnuv3_dynamic_class (type))

> -    {

> -      info.trivially_copyable = false;

> -      return info;

> -    }

> +    is_dynamic = true;

>  

> +  /* FIXME taktemur/2019-04-23: What if there are multiple cctors?  */


Can such a situation arise?  If you know how it could but don't know
how to handle it then can we expand the comment.  If you don't think
such a situation could arise then lets delete this comment and add an
assertion below.

>    for (fieldnum = 0; fieldnum < TYPE_NFN_FIELDS (type); fieldnum++)

>      for (fieldelem = 0; fieldelem < TYPE_FN_FIELDLIST_LENGTH (type, fieldnum);

>  	 fieldelem++)

> @@ -1282,49 +1415,53 @@ gnuv3_pass_by_reference (struct type *type)

>  	const char *name = TYPE_FN_FIELDLIST_NAME (type, fieldnum);

>  	struct type *fieldtype = TYPE_FN_FIELD_TYPE (fn, fieldelem);

>  

> -	/* If this function is marked as artificial, it is compiler-generated,

> -	   and we assume it is trivial.  */

> -	if (TYPE_FN_FIELD_ARTIFICIAL (fn, fieldelem))

> -	  continue;

> -

> -	/* If we've found a destructor, we must pass this by reference.  */

>  	if (name[0] == '~')

>  	  {

> -	    info.trivially_copyable = false;

> -	    return info;

> +	    /* We've found a destructor.  */

> +	    dtor_def = get_def_style (fn, fieldelem);

> +	    info.dtor_name = TYPE_FN_FIELD_PHYSNAME (fn, fieldelem);


Maybe we should have an error or at least a warning if
'info.dtor_name' is not nullptr before this assignment - this would
indicate multiple destructors, which seems weird, right?

>  	  }

> -

> -	/* If the mangled name of this method doesn't indicate that it

> -	   is a constructor, we're not interested.

> -

> -	   FIXME drow/2007-09-23: We could do this using the name of

> -	   the method and the name of the class instead of dealing

> -	   with the mangled name.  We don't have a convenient function

> -	   to strip off both leading scope qualifiers and trailing

> -	   template arguments yet.  */

> -	if (!is_constructor_name (TYPE_FN_FIELD_PHYSNAME (fn, fieldelem))

> -	    && !TYPE_FN_FIELD_CONSTRUCTOR (fn, fieldelem))

> -	  continue;

> -

> -	/* If this method takes two arguments, and the second argument is

> -	   a reference to this class, then it is a copy constructor.  */

> -	if (TYPE_NFIELDS (fieldtype) == 2)

> +	else if (is_constructor_name (TYPE_FN_FIELD_PHYSNAME (fn, fieldelem))

> +		 || TYPE_FN_FIELD_CONSTRUCTOR (fn, fieldelem))

>  	  {

> -	    struct type *arg_type = TYPE_FIELD_TYPE (fieldtype, 1);

> -

> -	    if (TYPE_CODE (arg_type) == TYPE_CODE_REF)

> +	    /* FIXME drow/2007-09-23: We could do this using the name of

> +	       the method and the name of the class instead of dealing

> +	       with the mangled name.  We don't have a convenient function

> +	       to strip off both leading scope qualifiers and trailing

> +	       template arguments yet.  */

> +	    if (is_copy_constructor_type (type, fieldtype))

>  	      {

> -		struct type *arg_target_type

> -		  = check_typedef (TYPE_TARGET_TYPE (arg_type));

> -		if (class_types_same_p (arg_target_type, type))

> -		  {

> -		    info.trivially_copyable = false;

> -		    return info;

> -		  }

> +		cctor_def = get_def_style (fn, fieldelem);

> +		info.cctor_name = TYPE_FN_FIELD_PHYSNAME (fn, fieldelem);


This would be where we assert that we only have one cctor I think...

>  	      }

> +	    else if (is_move_constructor_type (type, fieldtype))

> +	      mctor_def = get_def_style (fn, fieldelem);

>  	  }

>        }

>  

> +  bool cctor_implicitly_deleted

> +    =  mctor_def != DOES_NOT_EXIST_IN_SOURCE

> +    && cctor_def == DOES_NOT_EXIST_IN_SOURCE;


I think this should be parenthesised like this:

  bool cctor_implicitly_deleted
    =  (mctor_def != DOES_NOT_EXIST_IN_SOURCE
        && cctor_def == DOES_NOT_EXIST_IN_SOURCE);

My reference is:
  https://www.gnu.org/prep/standards/standards.html#Formatting


> +

> +  bool cctor_explicitly_deleted = cctor_def == DELETED;

> +

> +  if (cctor_implicitly_deleted || cctor_explicitly_deleted)

> +    info.copy_constructible = false;

> +

> +  if (dtor_def == DELETED)

> +    info.destructible = false;

> +

> +  info.trivially_destructible = is_implicit_def (dtor_def);

> +

> +  info.trivially_copy_constructible

> +    =  is_implicit_def (cctor_def)

> +    && !is_dynamic;


Again extra ( ... ) needed I think.

> +

> +  info.trivially_copyable

> +    =  info.trivially_copy_constructible

> +    && info.trivially_destructible

> +    && !is_user_provided_def (mctor_def);


And again.

> +

>    /* Even if all the constructors and destructors were artificial, one

>       of them may have invoked a non-artificial constructor or

>       destructor in a base class.  If any base class needs to be passed

> @@ -1335,15 +1472,35 @@ gnuv3_pass_by_reference (struct type *type)

>    for (fieldnum = 0; fieldnum < TYPE_NFIELDS (type); fieldnum++)

>      if (!field_is_static (&TYPE_FIELD (type, fieldnum)))

>        {

> +	struct type *field_type = TYPE_FIELD_TYPE (type, fieldnum);

> +

> +	/* For arrays, make the decision based on the element type.  */

> +	if (TYPE_CODE (field_type) == TYPE_CODE_ARRAY)

> +	  field_type = field_type->main_type->target_type;

> +

>  	struct language_pass_by_ref_info field_info

> -	  = gnuv3_pass_by_reference (TYPE_FIELD_TYPE (type, fieldnum));

> +	  = gnuv3_pass_by_reference (field_type);

> +

> +	if (!field_info.copy_constructible)

> +	  info.copy_constructible = false;

> +	if (!field_info.destructible)

> +	  info.destructible = false;

>  	if (!field_info.trivially_copyable)

> -	  {

> -	    info.trivially_copyable = false;

> -	    return info;

> -	  }

> +	  info.trivially_copyable = false;

> +	if (!field_info.trivially_copy_constructible)

> +	  info.trivially_copy_constructible = false;

> +	if (!field_info.trivially_destructible)

> +	  info.trivially_destructible = false;

>        }

>  

> +  /* Consistency check.  */

> +  if (has_cc_attr && info.trivially_copyable != is_pass_by_value)

> +    {

> +      /* DWARF CC attribute is not the same as the inferred value;

> +	 using the DWARF attribute.  */

> +      info.trivially_copyable = is_pass_by_value;

> +    }

> +

>    return info;

>  }

>  

> -- 

> 2.21.0

> 


Thanks,
Andrew
Tankut Baris Aktemur May 31, 2019, 1:55 p.m. | #2
> > Walk through a given type to collect information about whether the type

> > is copy constructible, destructible, trivially copyable, trivially copy

> > constructible, trivially destructible.  The previous algorithm returned

> > only a boolean result about whether the type is trivially copyable.

> > This patch computes more info.  Additionally, it utilizes DWARF attributes

> > that were previously not taken into account; namely,

> > DW_AT_deleted, DW_AT_defaulted, and DW_AT_calling_convention.

> 

> I'm basically happy with this, a few formatting issues and questions

> about comments below.

>


Thank you.

> >

> > +  /* FIXME taktemur/2019-04-23: What if there are multiple cctors?  */

> 

> Can such a situation arise?  If you know how it could but don't know

> how to handle it then can we expand the comment.  If you don't think

> such a situation could arise then lets delete this comment and add an

> assertion below.

> 


Such a situation can arise when there is a copy ctor that takes a
non-const &T and another that takes a const &T.  I'm planning to
add the example below to the comment:

  /* FIXME taktemur/2019-04-23: What if there are multiple copy ctors?
     E.g.:
       class C {
       public:
         C (C &c) { ... }
         C (const C &c) { ... }
       };
  */

The correct version shall be selected based on the type of the argument,
but I don't know how to express that in GDB.


> >    for (fieldnum = 0; fieldnum < TYPE_NFN_FIELDS (type); fieldnum++)

> >      for (fieldelem = 0; fieldelem < TYPE_FN_FIELDLIST_LENGTH (type, fieldnum);

> >  	 fieldelem++)

> > @@ -1282,49 +1415,53 @@ gnuv3_pass_by_reference (struct type *type)

> >  	const char *name = TYPE_FN_FIELDLIST_NAME (type, fieldnum);

> >  	struct type *fieldtype = TYPE_FN_FIELD_TYPE (fn, fieldelem);

> >

> > -	/* If this function is marked as artificial, it is compiler-generated,

> > -	   and we assume it is trivial.  */

> > -	if (TYPE_FN_FIELD_ARTIFICIAL (fn, fieldelem))

> > -	  continue;

> > -

> > -	/* If we've found a destructor, we must pass this by reference.  */

> >  	if (name[0] == '~')

> >  	  {

> > -	    info.trivially_copyable = false;

> > -	    return info;

> > +	    /* We've found a destructor.  */

> > +	    dtor_def = get_def_style (fn, fieldelem);

> > +	    info.dtor_name = TYPE_FN_FIELD_PHYSNAME (fn, fieldelem);

> 

> Maybe we should have an error or at least a warning if

> 'info.dtor_name' is not nullptr before this assignment - this would

> indicate multiple destructors, which seems weird, right?

> 


I believe the 'info.dtor_name' field can be nullptr even if a destructor
definition exists, if the destructor is inlined and hence its code does
not exist in the object file.  (Such a case also requires special treatment
and is handled at the client side, in gdb/infcall.c in part 7/8 of this patch.)
So, I thought I should gdb_assert on 'dtor_def == DOES_NOT_EXIST_IN_SOURCE'
instead.  Is this OK?

> > +	    if (is_copy_constructor_type (type, fieldtype))

> >  	      {

> > -		struct type *arg_target_type

> > -		  = check_typedef (TYPE_TARGET_TYPE (arg_type));

> > -		if (class_types_same_p (arg_target_type, type))

> > -		  {

> > -		    info.trivially_copyable = false;

> > -		    return info;

> > -		  }

> > +		cctor_def = get_def_style (fn, fieldelem);

> > +		info.cctor_name = TYPE_FN_FIELD_PHYSNAME (fn, fieldelem);

> 

> This would be where we assert that we only have one cctor I think...

> 


Similarly, I'm planning to assert 'cctor_def == DOES_NOT_EXIST_IN_SOURCE'.

> > +  bool cctor_implicitly_deleted

> > +    =  mctor_def != DOES_NOT_EXIST_IN_SOURCE

> > +    && cctor_def == DOES_NOT_EXIST_IN_SOURCE;

> 

> I think this should be parenthesised like this:

> 

>   bool cctor_implicitly_deleted

>     =  (mctor_def != DOES_NOT_EXIST_IN_SOURCE

>         && cctor_def == DOES_NOT_EXIST_IN_SOURCE);

> 

> My reference is:

>   https://www.gnu.org/prep/standards/standards.html#Formatting

> 


Thanks for the pointer.  I'll address these formatting issues in the next update.

> 

> Thanks,

> Andrew


Regards,
-Baris

Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Gary Kershaw
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928
Andrew Burgess June 23, 2019, 7:23 p.m. | #3
* Aktemur, Tankut Baris <tankut.baris.aktemur@intel.com> [2019-05-31 13:55:58 +0000]:

> > > Walk through a given type to collect information about whether the type

> > > is copy constructible, destructible, trivially copyable, trivially copy

> > > constructible, trivially destructible.  The previous algorithm returned

> > > only a boolean result about whether the type is trivially copyable.

> > > This patch computes more info.  Additionally, it utilizes DWARF attributes

> > > that were previously not taken into account; namely,

> > > DW_AT_deleted, DW_AT_defaulted, and DW_AT_calling_convention.

> > 

> > I'm basically happy with this, a few formatting issues and questions

> > about comments below.

> >

> 

> Thank you.

> 

> > >

> > > +  /* FIXME taktemur/2019-04-23: What if there are multiple cctors?  */

> > 

> > Can such a situation arise?  If you know how it could but don't know

> > how to handle it then can we expand the comment.  If you don't think

> > such a situation could arise then lets delete this comment and add an

> > assertion below.

> > 

> 

> Such a situation can arise when there is a copy ctor that takes a

> non-const &T and another that takes a const &T.  I'm planning to

> add the example below to the comment:

> 

>   /* FIXME taktemur/2019-04-23: What if there are multiple copy ctors?

>      E.g.:

>        class C {

>        public:

>          C (C &c) { ... }

>          C (const C &c) { ... }

>        };

>   */


That would be great, I find information like this in comments very
helpful so thank you.

> 

> The correct version shall be selected based on the type of the argument,

> but I don't know how to express that in GDB.

> 

> 

> > >    for (fieldnum = 0; fieldnum < TYPE_NFN_FIELDS (type); fieldnum++)

> > >      for (fieldelem = 0; fieldelem < TYPE_FN_FIELDLIST_LENGTH (type, fieldnum);

> > >  	 fieldelem++)

> > > @@ -1282,49 +1415,53 @@ gnuv3_pass_by_reference (struct type *type)

> > >  	const char *name = TYPE_FN_FIELDLIST_NAME (type, fieldnum);

> > >  	struct type *fieldtype = TYPE_FN_FIELD_TYPE (fn, fieldelem);

> > >

> > > -	/* If this function is marked as artificial, it is compiler-generated,

> > > -	   and we assume it is trivial.  */

> > > -	if (TYPE_FN_FIELD_ARTIFICIAL (fn, fieldelem))

> > > -	  continue;

> > > -

> > > -	/* If we've found a destructor, we must pass this by reference.  */

> > >  	if (name[0] == '~')

> > >  	  {

> > > -	    info.trivially_copyable = false;

> > > -	    return info;

> > > +	    /* We've found a destructor.  */

> > > +	    dtor_def = get_def_style (fn, fieldelem);

> > > +	    info.dtor_name = TYPE_FN_FIELD_PHYSNAME (fn, fieldelem);

> > 

> > Maybe we should have an error or at least a warning if

> > 'info.dtor_name' is not nullptr before this assignment - this would

> > indicate multiple destructors, which seems weird, right?

> > 

> 

> I believe the 'info.dtor_name' field can be nullptr even if a destructor

> definition exists, if the destructor is inlined and hence its code does

> not exist in the object file.  (Such a case also requires special treatment

> and is handled at the client side, in gdb/infcall.c in part 7/8 of this patch.)

> So, I thought I should gdb_assert on 'dtor_def == DOES_NOT_EXIST_IN_SOURCE'

> instead.  Is this OK?

> 

> > > +	    if (is_copy_constructor_type (type, fieldtype))

> > >  	      {

> > > -		struct type *arg_target_type

> > > -		  = check_typedef (TYPE_TARGET_TYPE (arg_type));

> > > -		if (class_types_same_p (arg_target_type, type))

> > > -		  {

> > > -		    info.trivially_copyable = false;

> > > -		    return info;

> > > -		  }

> > > +		cctor_def = get_def_style (fn, fieldelem);

> > > +		info.cctor_name = TYPE_FN_FIELD_PHYSNAME (fn, fieldelem);

> > 

> > This would be where we assert that we only have one cctor I think...

> > 

> 

> Similarly, I'm planning to assert 'cctor_def == DOES_NOT_EXIST_IN_SOURCE'.

>


OK, that sounds reasonable.

If nobody else reviews the remaining patches in this series then I
will get around to them soon I hope.  I've been crazy busy the last
couple of weeks and am still trying to catch up on everything.

Thanks,
Andrew

> > > +  bool cctor_implicitly_deleted

> > > +    =  mctor_def != DOES_NOT_EXIST_IN_SOURCE

> > > +    && cctor_def == DOES_NOT_EXIST_IN_SOURCE;

> > 

> > I think this should be parenthesised like this:

> > 

> >   bool cctor_implicitly_deleted

> >     =  (mctor_def != DOES_NOT_EXIST_IN_SOURCE

> >         && cctor_def == DOES_NOT_EXIST_IN_SOURCE);

> > 

> > My reference is:

> >   https://www.gnu.org/prep/standards/standards.html#Formatting

> > 

> 

> Thanks for the pointer.  I'll address these formatting issues in the next update.

> 

> > 

> > Thanks,

> > Andrew

> 

> Regards,

> -Baris

> 

> Intel Deutschland GmbH

> Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany

> Tel: +49 89 99 8853-0, www.intel.de

> Managing Directors: Christin Eisenschmid, Gary Kershaw

> Chairperson of the Supervisory Board: Nicole Lau

> Registered Office: Munich

> Commercial Register: Amtsgericht Muenchen HRB 186928

>
Tankut Baris Aktemur June 24, 2019, 7:28 a.m. | #4
* On Sunday, June 23, 2019 9:24 PM, Andrew Burgess wrote:
> * Aktemur, Tankut Baris <tankut.baris.aktemur@intel.com> [2019-05-31 13:55:58 +0000]:

> 

> > > > Walk through a given type to collect information about whether the type

> > > > is copy constructible, destructible, trivially copyable, trivially copy

> > > > constructible, trivially destructible.  The previous algorithm returned

> > > > only a boolean result about whether the type is trivially copyable.

> > > > This patch computes more info.  Additionally, it utilizes DWARF attributes

> > > > that were previously not taken into account; namely,

> > > > DW_AT_deleted, DW_AT_defaulted, and DW_AT_calling_convention.

> > >

> > > I'm basically happy with this, a few formatting issues and questions

> > > about comments below.

> > >

> >

> > Thank you.

> >

> > > >

> > > > +  /* FIXME taktemur/2019-04-23: What if there are multiple cctors?  */

> > >

> > > Can such a situation arise?  If you know how it could but don't know

> > > how to handle it then can we expand the comment.  If you don't think

> > > such a situation could arise then lets delete this comment and add an

> > > assertion below.

> > >

> >

> > Such a situation can arise when there is a copy ctor that takes a

> > non-const &T and another that takes a const &T.  I'm planning to

> > add the example below to the comment:

> >

> >   /* FIXME taktemur/2019-04-23: What if there are multiple copy ctors?

> >      E.g.:

> >        class C {

> >        public:

> >          C (C &c) { ... }

> >          C (const C &c) { ... }

> >        };

> >   */

> 

> That would be great, I find information like this in comments very

> helpful so thank you.

> 

> >

> > The correct version shall be selected based on the type of the argument,

> > but I don't know how to express that in GDB.

> >

> >

> > > >    for (fieldnum = 0; fieldnum < TYPE_NFN_FIELDS (type); fieldnum++)

> > > >      for (fieldelem = 0; fieldelem < TYPE_FN_FIELDLIST_LENGTH (type, fieldnum);

> > > >  	 fieldelem++)

> > > > @@ -1282,49 +1415,53 @@ gnuv3_pass_by_reference (struct type *type)

> > > >  	const char *name = TYPE_FN_FIELDLIST_NAME (type, fieldnum);

> > > >  	struct type *fieldtype = TYPE_FN_FIELD_TYPE (fn, fieldelem);

> > > >

> > > > -	/* If this function is marked as artificial, it is compiler-generated,

> > > > -	   and we assume it is trivial.  */

> > > > -	if (TYPE_FN_FIELD_ARTIFICIAL (fn, fieldelem))

> > > > -	  continue;

> > > > -

> > > > -	/* If we've found a destructor, we must pass this by reference.  */

> > > >  	if (name[0] == '~')

> > > >  	  {

> > > > -	    info.trivially_copyable = false;

> > > > -	    return info;

> > > > +	    /* We've found a destructor.  */

> > > > +	    dtor_def = get_def_style (fn, fieldelem);

> > > > +	    info.dtor_name = TYPE_FN_FIELD_PHYSNAME (fn, fieldelem);

> > >

> > > Maybe we should have an error or at least a warning if

> > > 'info.dtor_name' is not nullptr before this assignment - this would

> > > indicate multiple destructors, which seems weird, right?

> > >

> >

> > I believe the 'info.dtor_name' field can be nullptr even if a destructor

> > definition exists, if the destructor is inlined and hence its code does

> > not exist in the object file.  (Such a case also requires special treatment

> > and is handled at the client side, in gdb/infcall.c in part 7/8 of this patch.)

> > So, I thought I should gdb_assert on 'dtor_def == DOES_NOT_EXIST_IN_SOURCE'

> > instead.  Is this OK?

> >

> > > > +	    if (is_copy_constructor_type (type, fieldtype))

> > > >  	      {

> > > > -		struct type *arg_target_type

> > > > -		  = check_typedef (TYPE_TARGET_TYPE (arg_type));

> > > > -		if (class_types_same_p (arg_target_type, type))

> > > > -		  {

> > > > -		    info.trivially_copyable = false;

> > > > -		    return info;

> > > > -		  }

> > > > +		cctor_def = get_def_style (fn, fieldelem);

> > > > +		info.cctor_name = TYPE_FN_FIELD_PHYSNAME (fn, fieldelem);

> > >

> > > This would be where we assert that we only have one cctor I think...

> > >

> >

> > Similarly, I'm planning to assert 'cctor_def == DOES_NOT_EXIST_IN_SOURCE'.

> >

> 

> OK, that sounds reasonable.

> 

> If nobody else reviews the remaining patches in this series then I

> will get around to them soon I hope.  I've been crazy busy the last

> couple of weeks and am still trying to catch up on everything.

> 

> Thanks,

> Andrew

>


Thank you! The remaining patches have not received a review so far.
I'll submit today a v2 of the series in which your comments have been addressed
in patches 1-3.

-Baris
 
> > > > +  bool cctor_implicitly_deleted

> > > > +    =  mctor_def != DOES_NOT_EXIST_IN_SOURCE

> > > > +    && cctor_def == DOES_NOT_EXIST_IN_SOURCE;

> > >

> > > I think this should be parenthesised like this:

> > >

> > >   bool cctor_implicitly_deleted

> > >     =  (mctor_def != DOES_NOT_EXIST_IN_SOURCE

> > >         && cctor_def == DOES_NOT_EXIST_IN_SOURCE);

> > >

> > > My reference is:

> > >   https://www.gnu.org/prep/standards/standards.html#Formatting

> > >

> >

> > Thanks for the pointer.  I'll address these formatting issues in the next update.

> >

> > >

> > > Thanks,

> > > Andrew

> >

> > Regards,

> > -Baris

> >

Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Gary Kershaw
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

Patch

diff --git a/gdb/gnu-v3-abi.c b/gdb/gnu-v3-abi.c
index b39e5a5c585..9069305626f 100644
--- a/gdb/gnu-v3-abi.c
+++ b/gdb/gnu-v3-abi.c
@@ -23,6 +23,7 @@ 
 #include "cp-abi.h"
 #include "cp-support.h"
 #include "demangle.h"
+#include "dwarf2.h"
 #include "objfiles.h"
 #include "valprint.h"
 #include "c-lang.h"
@@ -1228,6 +1229,122 @@  gnuv3_skip_trampoline (struct frame_info *frame, CORE_ADDR stop_pc)
   return real_stop_pc;
 }
 
+/* A member function is in one these states.  */
+
+enum definition_style
+{
+  DOES_NOT_EXIST_IN_SOURCE,
+  DEFAULTED_INSIDE,
+  DEFAULTED_OUTSIDE,
+  DELETED,
+  EXPLICIT,
+};
+
+/* Return how the given field is defined.  */
+
+static definition_style
+get_def_style (struct fn_field *fn, int fieldelem)
+{
+  if (TYPE_FN_FIELD_DELETED (fn, fieldelem))
+    return DELETED;
+
+  if (TYPE_FN_FIELD_ARTIFICIAL (fn, fieldelem))
+    return DOES_NOT_EXIST_IN_SOURCE;
+
+  switch (TYPE_FN_FIELD_DEFAULTED (fn, fieldelem))
+    {
+    case DW_DEFAULTED_no:
+      return EXPLICIT;
+    case DW_DEFAULTED_in_class:
+      return DEFAULTED_INSIDE;
+    case DW_DEFAULTED_out_of_class:
+      return DEFAULTED_OUTSIDE;
+    default:
+      break;
+    }
+
+  return EXPLICIT;
+}
+
+/* Helper functions to determine whether the given definition style
+   denotes that the definition is user-provided or implicit.
+   Being defaulted outside the class decl counts as an explicit
+   user-definition, while being defaulted inside is implicit.  */
+
+static bool
+is_user_provided_def (definition_style def)
+{
+  return def == EXPLICIT || def == DEFAULTED_OUTSIDE;
+}
+
+static bool
+is_implicit_def (definition_style def)
+{
+  return def == DOES_NOT_EXIST_IN_SOURCE || def == DEFAULTED_INSIDE;
+}
+
+/* Helper function to decide if METHOD_TYPE is a copy/move
+   constructor type for CLASS_TYPE.  EXPECTED is the expected
+   type code for the "right-hand-side" argument.
+   This function is supposed to be used by the IS_COPY_CONSTRUCTOR_TYPE
+   and IS_MOVE_CONSTRUCTOR_TYPE functions below.  Normally, you should
+   not need to call this directly.  */
+
+static bool
+is_copy_or_move_constructor_type (struct type *class_type,
+				  struct type *method_type,
+				  type_code expected)
+{
+  /* The method should take at least two arguments...  */
+  if (TYPE_NFIELDS (method_type) < 2)
+    return false;
+
+  /* ...and the second argument should be the same as the class
+     type, with the expected type code...  */
+  struct type *arg_type = TYPE_FIELD_TYPE (method_type, 1);
+
+  if (TYPE_CODE (arg_type) != expected)
+    return false;
+
+  struct type *target = check_typedef (TYPE_TARGET_TYPE (arg_type));
+  if (!(class_types_same_p (target, class_type)))
+    return false;
+
+  /* ...and the rest of the arguments should have default values.  */
+  for (int i = 2; i < TYPE_NFIELDS (method_type); i++)
+    {
+      arg_type = TYPE_FIELD_TYPE (method_type, i);
+      /* FIXME taktemur/2019-04-23: As of this date, neither
+	 clang++-7.0.0 nor g++-8.2.0 produce a DW_AT_default_value
+	 attribute.  GDB is also not set to read this attribute, yet.
+	 Hence, we immediately return false if there are more than
+	 2 parameters.  */
+      return false;
+    }
+
+  return true;
+}
+
+/* Return true if METHOD_TYPE is a copy ctor type for CLASS_TYPE.  */
+
+static bool
+is_copy_constructor_type (struct type *class_type,
+			  struct type *method_type)
+{
+  return is_copy_or_move_constructor_type (class_type, method_type,
+					   TYPE_CODE_REF);
+}
+
+/* Return true if METHOD_TYPE is a move ctor type for CLASS_TYPE.  */
+
+static bool
+is_move_constructor_type (struct type *class_type,
+			  struct type *method_type)
+{
+  return is_copy_or_move_constructor_type (class_type, method_type,
+					   TYPE_CODE_RVALUE_REF);
+}
+
 /* Return pass-by-reference information for the given TYPE.
 
    The rule in the v3 ABI document comes from section 3.1.1.  If the
@@ -1236,16 +1353,15 @@  gnuv3_skip_trampoline (struct frame_info *frame, CORE_ADDR stop_pc)
    is one or perform the copy itself otherwise), pass the address of
    the copy, and then destroy the temporary (if necessary).
 
-   For return values with non-trivial copy constructors or
+   For return values with non-trivial copy/move constructors or
    destructors, space will be allocated in the caller, and a pointer
    will be passed as the first argument (preceding "this").
 
    We don't have a bulletproof mechanism for determining whether a
-   constructor or destructor is trivial.  For GCC and DWARF2 debug
-   information, we can check the artificial flag.
-
-   We don't do anything with the constructors or destructors,
-   but we have to get the argument passing right anyway.  */
+   constructor or destructor is trivial.  For GCC and DWARF5 debug
+   information, we can check the calling_convention attribute,
+   the 'artificial' flag, the 'defaulted' attribute, and the
+   'deleted' attribute.  */
 
 static struct language_pass_by_ref_info
 gnuv3_pass_by_reference (struct type *type)
@@ -1258,22 +1374,39 @@  gnuv3_pass_by_reference (struct type *type)
   struct language_pass_by_ref_info info
     = default_pass_by_reference (type);
 
-  /* FIXME: Currently, this implementation only fills in the
-     'trivially-copyable' field.  */
+  bool has_cc_attr = false;
+  bool is_pass_by_value = false;
+  bool is_dynamic = false;
+  definition_style cctor_def = DOES_NOT_EXIST_IN_SOURCE;
+  definition_style dtor_def = DOES_NOT_EXIST_IN_SOURCE;
+  definition_style mctor_def = DOES_NOT_EXIST_IN_SOURCE;
 
   /* We're only interested in things that can have methods.  */
   if (TYPE_CODE (type) != TYPE_CODE_STRUCT
       && TYPE_CODE (type) != TYPE_CODE_UNION)
     return info;
 
+  /* The compiler may have emitted the calling convention attribute.  */
+  if (TYPE_CPLUS_CALLING_CONVENTION (type) == DW_CC_pass_by_value)
+    {
+      has_cc_attr = true;
+      is_pass_by_value = true;
+      /* Do not return immediately.  We have to find out if this type
+	 is copy_constructible and destructible.  */
+    }
+
+  if (TYPE_CPLUS_CALLING_CONVENTION (type) == DW_CC_pass_by_reference)
+    {
+      has_cc_attr = true;
+      is_pass_by_value = false;
+    }
+
   /* A dynamic class has a non-trivial copy constructor.
      See c++98 section 12.8 Copying class objects [class.copy].  */
   if (gnuv3_dynamic_class (type))
-    {
-      info.trivially_copyable = false;
-      return info;
-    }
+    is_dynamic = true;
 
+  /* FIXME taktemur/2019-04-23: What if there are multiple cctors?  */
   for (fieldnum = 0; fieldnum < TYPE_NFN_FIELDS (type); fieldnum++)
     for (fieldelem = 0; fieldelem < TYPE_FN_FIELDLIST_LENGTH (type, fieldnum);
 	 fieldelem++)
@@ -1282,49 +1415,53 @@  gnuv3_pass_by_reference (struct type *type)
 	const char *name = TYPE_FN_FIELDLIST_NAME (type, fieldnum);
 	struct type *fieldtype = TYPE_FN_FIELD_TYPE (fn, fieldelem);
 
-	/* If this function is marked as artificial, it is compiler-generated,
-	   and we assume it is trivial.  */
-	if (TYPE_FN_FIELD_ARTIFICIAL (fn, fieldelem))
-	  continue;
-
-	/* If we've found a destructor, we must pass this by reference.  */
 	if (name[0] == '~')
 	  {
-	    info.trivially_copyable = false;
-	    return info;
+	    /* We've found a destructor.  */
+	    dtor_def = get_def_style (fn, fieldelem);
+	    info.dtor_name = TYPE_FN_FIELD_PHYSNAME (fn, fieldelem);
 	  }
-
-	/* If the mangled name of this method doesn't indicate that it
-	   is a constructor, we're not interested.
-
-	   FIXME drow/2007-09-23: We could do this using the name of
-	   the method and the name of the class instead of dealing
-	   with the mangled name.  We don't have a convenient function
-	   to strip off both leading scope qualifiers and trailing
-	   template arguments yet.  */
-	if (!is_constructor_name (TYPE_FN_FIELD_PHYSNAME (fn, fieldelem))
-	    && !TYPE_FN_FIELD_CONSTRUCTOR (fn, fieldelem))
-	  continue;
-
-	/* If this method takes two arguments, and the second argument is
-	   a reference to this class, then it is a copy constructor.  */
-	if (TYPE_NFIELDS (fieldtype) == 2)
+	else if (is_constructor_name (TYPE_FN_FIELD_PHYSNAME (fn, fieldelem))
+		 || TYPE_FN_FIELD_CONSTRUCTOR (fn, fieldelem))
 	  {
-	    struct type *arg_type = TYPE_FIELD_TYPE (fieldtype, 1);
-
-	    if (TYPE_CODE (arg_type) == TYPE_CODE_REF)
+	    /* FIXME drow/2007-09-23: We could do this using the name of
+	       the method and the name of the class instead of dealing
+	       with the mangled name.  We don't have a convenient function
+	       to strip off both leading scope qualifiers and trailing
+	       template arguments yet.  */
+	    if (is_copy_constructor_type (type, fieldtype))
 	      {
-		struct type *arg_target_type
-		  = check_typedef (TYPE_TARGET_TYPE (arg_type));
-		if (class_types_same_p (arg_target_type, type))
-		  {
-		    info.trivially_copyable = false;
-		    return info;
-		  }
+		cctor_def = get_def_style (fn, fieldelem);
+		info.cctor_name = TYPE_FN_FIELD_PHYSNAME (fn, fieldelem);
 	      }
+	    else if (is_move_constructor_type (type, fieldtype))
+	      mctor_def = get_def_style (fn, fieldelem);
 	  }
       }
 
+  bool cctor_implicitly_deleted
+    =  mctor_def != DOES_NOT_EXIST_IN_SOURCE
+    && cctor_def == DOES_NOT_EXIST_IN_SOURCE;
+
+  bool cctor_explicitly_deleted = cctor_def == DELETED;
+
+  if (cctor_implicitly_deleted || cctor_explicitly_deleted)
+    info.copy_constructible = false;
+
+  if (dtor_def == DELETED)
+    info.destructible = false;
+
+  info.trivially_destructible = is_implicit_def (dtor_def);
+
+  info.trivially_copy_constructible
+    =  is_implicit_def (cctor_def)
+    && !is_dynamic;
+
+  info.trivially_copyable
+    =  info.trivially_copy_constructible
+    && info.trivially_destructible
+    && !is_user_provided_def (mctor_def);
+
   /* Even if all the constructors and destructors were artificial, one
      of them may have invoked a non-artificial constructor or
      destructor in a base class.  If any base class needs to be passed
@@ -1335,15 +1472,35 @@  gnuv3_pass_by_reference (struct type *type)
   for (fieldnum = 0; fieldnum < TYPE_NFIELDS (type); fieldnum++)
     if (!field_is_static (&TYPE_FIELD (type, fieldnum)))
       {
+	struct type *field_type = TYPE_FIELD_TYPE (type, fieldnum);
+
+	/* For arrays, make the decision based on the element type.  */
+	if (TYPE_CODE (field_type) == TYPE_CODE_ARRAY)
+	  field_type = field_type->main_type->target_type;
+
 	struct language_pass_by_ref_info field_info
-	  = gnuv3_pass_by_reference (TYPE_FIELD_TYPE (type, fieldnum));
+	  = gnuv3_pass_by_reference (field_type);
+
+	if (!field_info.copy_constructible)
+	  info.copy_constructible = false;
+	if (!field_info.destructible)
+	  info.destructible = false;
 	if (!field_info.trivially_copyable)
-	  {
-	    info.trivially_copyable = false;
-	    return info;
-	  }
+	  info.trivially_copyable = false;
+	if (!field_info.trivially_copy_constructible)
+	  info.trivially_copy_constructible = false;
+	if (!field_info.trivially_destructible)
+	  info.trivially_destructible = false;
       }
 
+  /* Consistency check.  */
+  if (has_cc_attr && info.trivially_copyable != is_pass_by_value)
+    {
+      /* DWARF CC attribute is not the same as the inferred value;
+	 using the DWARF attribute.  */
+      info.trivially_copyable = is_pass_by_value;
+    }
+
   return info;
 }