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

[Bruno Larsen <blarsen@redhat.com>]

On 11/24/21 14:09, Andrew Burgess wrote:
> * 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.


Good catch. I thought that field.path included the type of 'e', but reviewing the code, you're right in saying that it doesn't.
However, as far as I understand, this is still fine, seeing as otherwise, we'd have two different A classes in the same address, which is also blocked by the C++ standard.

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

I feel like this is a good description, I'll add it to the code and push it. Thanks again for the review!

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


-- 
Cheers!
Bruno Larsen