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

[Bruno Larsen <blarsen@redhat.com>]

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

Or I can remove the last sentence, if that is still confusing or unnecessary

-- 
Cheers!
Bruno Larsen