Re: [PATCH] Reject ambiguous C++ field accesses

[Pedro Alves <pedro@palves.net>]

On 8/27/20 8:58 PM, Luis Machado wrote:

>> diff --git a/gdb/testsuite/gdb.cp/ambiguous.exp b/gdb/testsuite/gdb.cp/ambiguous.exp
>> index 17fb29f5350..74783cd0725 100644
>> --- a/gdb/testsuite/gdb.cp/ambiguous.exp
>> +++ b/gdb/testsuite/gdb.cp/ambiguous.exp
>> @@ -15,15 +15,9 @@
>>     # This file is part of the gdb testsuite
>>   -# tests relating to ambiguous class members
>> -# Written by Satish Pai <pai@apollo.hp.com> 1997-07-28
>> -
>> -# This file is part of the gdb testsuite
>> -
>> -#
>> -# test running programs
>> -#
>> -
>> +# Print out various class objects' members and check that the error
>> +# about the field or baseclass being ambiguious is emitted at the
>> +# right times.
> 
> Typo, ambiguious.

Thanks, fixed.

> 
>>     if { [skip_cplus_tests] } { continue }
>>   @@ -47,180 +41,225 @@ if {[prepare_for_testing "failed to prepare" $testfile $srcfile \
>>       return -1
>>   }
>>   -#
>> -# set it up at a breakpoint so we can play with the variable values
>> -#
>> +# Set it up at a breakpoint so we can play with the variable values.
>> +
> 
> This old comment reads funny. Set what up? I'd rephrase a bit.

This comment is just before the starting up the inferior and
running to a breakpoint.  It's talking about that.  Setting
up the inferior ready for testing.  I changed it to:

 # Run to a breakpoint after the variables have been initialized so we
 # can play with the variable values.

But I guess I could just remove it altogether, since it's
pretty obvious what we're doing.

>> diff --git a/gdb/valops.c b/gdb/valops.c
>> index 0eb2b096211..0e030a03ecd 100644
>> --- a/gdb/valops.c
>> +++ b/gdb/valops.c
>> @@ -1766,25 +1766,105 @@ typecmp (int staticp, int varargs, int nargs,
>>     return i + 1;
>>   }
>>   -/* Helper class for do_search_struct_field that updates *RESULT_PTR
>> -   and *LAST_BOFFSET, and possibly throws an exception if the field
>> -   search has yielded ambiguous results.  */
>> +/* Helper class for search_struct_field that keeps track of found
>> +   results.  See search_struct_field for description of
>> +   LOOKING_FOR_BASECLASS.  If LOOKING_FOR_BASECLASS is true, possibly
>> +   throws an exception if the base class search has yielded ambiguous
> 
> "throw" an exception instead?

I'll handle this one in response to Gary.

> 
>> +   results.  If LOOKING_FOR_BASECLASS is false, found fields are
>> +   accumulated and the caller (search_struct_field takes) care of
> 
> "takes" outside the parenthesis.

Fixed.

> 
>> +   throwing an error if the field search yielded ambiguous results.
>> +   The latter is done that way so that the error message can include a
>> +   list of all the found candidates.  */
>> +
>> +struct struct_field_searcher
>> +{
>> +  /* A found field.  */
>> +  struct found_field
>> +  {
>> +    /* Patch to the structure where the field was found.  */
> 
> Typo... Path.

:-)  Fixed.

>> +  /* Whether we're looking for a baseclass, or a field.  */
>> +  const bool m_looking_for_baseclass;
>> +
>> +  /* The offset of base class containing the field/baseclass we last
>> +     recorded.  */
> 
> Maybe "offset into base class"?

Thanks.  However, it's the offset of the class itself, not the field.
So I've changed it to:

  /* The offset of the baseclass containing the field/baseclass we
     last recorded.  */

> Otherwise LGTM. I exercised this for aarch64-linux and see no more failures and 10 KFAIL's.

Great, thanks!