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

["Aktemur, Tankut Baris" <tankut dot baris dot aktemur at intel dot com>]

* 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