Re: [PATCH v2] PR gdb/28480: Improve ambiguous member detection

[Andrew Burgess <aburgess@redhat.com>]

* Bruno Larsen via Gdb-patches <gdb-patches@sourceware.org> [2021-11-22 15:35:29 -0300]:

> Hi Andrew,
> 
> Thanks for the review. All of the easy addressable comments will be changed.
> 
> On 11/22/21 15:00, Andrew Burgess wrote:
> > * Bruno Larsen via Gdb-patches <gdb-patches@sourceware.org> [2021-11-08 15:27:22 -0300]:
> > 
> 
> > > diff --git a/gdb/valops.c b/gdb/valops.c
> > > index 9787cdbb513..2989a93df1a 100644
> > > --- a/gdb/valops.c
> > > +++ b/gdb/valops.c
> > > @@ -1962,6 +1962,33 @@ struct_field_searcher::update_result (struct value *v, LONGEST boffset)
> > >   	     space.  */
> > >   	  if (m_fields.empty () || m_last_boffset != boffset)
> > >   	    m_fields.push_back ({m_struct_path, v});
> > > +	  else
> > > +	  /* Some fields may occupy the same space and still be ambiguous.
> > > +	     This happens when [[no_unique_address]] is used by a member
> > > +	     of the class.  We assume that this only happens when the types are
> > > +	     different.  This is not necessarily complete, but a situation where
> > > +	     this assumption is incorrect is currently (2021) impossible.  */
> > 
> > This comment should be moved inside the "{ ... }" block.
> > 
> > I found this comment difficult to understand.  When you say "...when
> > the types are different", I guess this is referring to the path check
> > below maybe?  In which case I wonder if we can find a different way to
> > phrase this, rather than "different types" ... "paths to the two
> > fields are different" maybe?
> > 
> > Additional the whole final sentence just leaves me confused, it seems
> > to hint that there is a situation not covered by this code "This is
> > not necessarily complete...", but also that there is no such situation
> > "... is currently impossible".
> > 
> > I wonder if you are saying that should we ever have two fields of the
> > same name, in the same class, that occur at the same address, then
> > this code wouldn't cover that case?  But that seems a pretty weird
> > thing to worry about, so I assume I'm not understand you correctly.
> > 
> > Could you rephrase the last part please?
> 
> How does the following sound:
> 
> Some fields may occupy the same space and still be ambiguous. This
> happens when [[no_unique_address]] is used in the inferior's
> code. The current solution assumes that the compiler will only place
> 2 struct members in the same location if they are of different
> types. As of 2021, this is mandatory, but this may change in the
> future

I agree with you about what the standard says, but what I'm not
understanding is why that's relevant here.  We're looking up something
by name, right?  And we're trying to handle the case where two things
might exist at the same address _and_ have the same name.

So in this check:

  if(field.path.back () != m_struct_path.back ())
    ...
  else
    ...

We get to the if block when two things have the same address, but
their containing classes are different, in your original example A::e
and B::e, the 'A' and 'B' are different.

We get to the else block when two things have the same address, but
their containing classes are the same, so this might be A::e and A::e,
which I'm pretty sure can only happen when virtual inheritance is in
use, right?

But my point is, where in this logic do we check the type of 'e'?  I
don't see it.  If we imagine a world where A::e and B::e were the same
type, and placed at the same address, I think the existing code would
be fine.

My claim then, is that the following describes your code:

  Fields can occupy the same space and have the same name (be
  ambiguous).  This can happen when fields in two different base
  classes are marked [[no_unique_address]] and have the same name.
  The C++ standard says that such fields can only occupy the same
  space if they are of different type, but we don't rely on that in
  the following code.

If I'm not understanding something, please do let me know.

Thanks,
Andrew