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

[Pedro Alves <pedro@palves.net>]

On 8/27/20 7:45 PM, Luis Machado wrote:
> On 8/27/20 3:02 PM, Pedro Alves wrote:
>> The gdb.cp/ambiguous.exp testcase had been disabled for many years,
>> but recently it was re-enabled.  However, it is failing everywhere.
>> That is because it is testing an old feature that is gone from GDB.
>>
>> The testcase is expecting to see an ambiguous field warning, like:
>>
>>   # X is derived from A1 and A2; both A1 and A2 have a member 'x'
>>   send_gdb "print x.x\n"
>>   gdb_expect {
>>      -re "warning: x ambiguous; using X::A2::x. Use a cast to disambiguate.\r\n\\$\[0-9\]* = \[-\]*\[0-9\]*\r\n$gdb_prompt $" {
>>     pass "print x.x"
>>      }
>>      -re "warning: x ambiguous; using X::A1::x. Use a cast to disambiguate.\r\n\\$\[0-9\]* = \[-\]*\[0-9\]*\r\n$gdb_prompt $" {
>>     pass "print x.x"
>>      }
>>      -re ".*$gdb_prompt $" { fail "print x.x" }
>>      timeout { fail "(timeout) print x.x" }
>>    }
>>
>> However, GDB just accesses one of the candidates without warning or
>> error:
>>
>>   print x.x
>>   $1 = 1431655296
>>   (gdb) FAIL: gdb.cp/ambiguous.exp: print x.x
>>
>> (The weird number is because the testcase does not initialize the
>> variables.)
>>
>> The testcase come in originally with the big HP merge:
>>
>>   +Sun Jan 10 23:44:11 1999  David Taylor  <taylor@texas.cygnus.com>
>>   +
>>   +
>>   +       The following files are part of the HP merge; some had longer
>>   +       names at HP, but have been renamed to be no more than 14
>>   +       characters in length.
>>
>> Looking at the tree back then, we find that warning:
>>
>>   /* Helper function used by value_struct_elt to recurse through baseclasses.
>>      Look for a field NAME in ARG1. Adjust the address of ARG1 by OFFSET bytes,
>>      and search in it assuming it has (class) type TYPE.
>>      If found, return value, else return NULL.
>>
>>      If LOOKING_FOR_BASECLASS, then instead of looking for struct fields,
>>      look for a baseclass named NAME.  */
>>
>>   static value_ptr
>>   search_struct_field (name, arg1, offset, type, looking_for_baseclass)
>>        char *name;
>>        register value_ptr arg1;
>>        int offset;
>>        register struct type *type;
>>        int looking_for_baseclass;
>>   {
>>     int found = 0;
>>     char found_class[1024];
>>     value_ptr v;
>>     struct type *vbase = NULL;
>>
>>     found_class[0] = '\000';
>>
>>     v = search_struct_field_aux (name, arg1, offset, type, looking_for_baseclass, &found, found_class, &vbase);
>>     if (found > 1)
>>       warning ("%s ambiguous; using %s::%s. Use a cast to disambiguate.",
>>                name, found_class, name);
>>
>>     return v;
>>   }
>>
>>
>> However, in current GDB, search_struct_field does not handle the
>> ambiguous field case, nor is that warning found anywhere.  Somehow it
>> got lost over the years.  That seems like a regression, because the
>> compiler (as per language rules) rejects the ambiguous accesses as
>> well.  E.g.:
>>
>>   gdb.cp/ambiguous.cc:98:5: error: request for member 'x' is ambiguous
>>      98 |   x.x = 1;
>>         |     ^
>>   gdb.cp/ambiguous.cc:10:7: note: candidates are: 'int A2::x'
>>      10 |   int x;
>>         |       ^
>>   gdb.cp/ambiguous.cc:4:7: note:                 'int A1::x'
>>       4 |   int x;
>>         |       ^
>>
>>
>> This patch restores the feature, though implemented differently and
>> with better user experience, IMHO.  An ambiguous access is now an
>> error instead of a warning, and also GDB shows you the all candidates,
>> like:
>>
>>   (gdb) print x.x
>>   Request for member 'x' is ambiguous in type 'X'. Candidates are:
>>     'int A1::x' (X -> A1)
>>     'int A2::x' (X -> A2)
>>   (gdb) print j.x
>>   Request for member 'x' is ambiguous in type 'J'. Candidates are:
>>     'int A1::x' (J -> K -> A1)
>>     'int A1::x' (J -> L -> A1)
> 
> Would it make sense to spare the users the touble of having to re-type/cast things and, instead, display all the possible values with an indication of what the fields are for each value?
> 
> That would be more intuitive to me, given the debugger is only inspecting things and not enforcing a particular syntax for a source file.
> 
> Users tend to be lazy. They just want to see values, not be syntactically correct. :-)
> 

No, that would not work with not-as-simple expressions.  Like "p foo(x.x + 2)".