This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


* 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
> 


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]