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

[Luis Machado <luis.machado@linaro.org>]

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