| Summary: | Printing ambiguous C++ field accesses randomly picks one | ||
|---|---|---|---|
| Product: | gdb | Reporter: | Tom de Vries <vries> |
| Component: | exp | Assignee: | Not yet assigned to anyone <unassigned> |
| Status: | RESOLVED FIXED | ||
| Severity: | normal | CC: | pedro |
| Priority: | P2 | ||
| Version: | HEAD | ||
| Target Milestone: | 11.1 | ||
| Host: | Target: | ||
| Build: | Last reconfirmed: | ||
|
Description
Tom de Vries
2020-09-11 13:47:54 UTC
Patch submitted here: https://sourceware.org/pipermail/gdb-patches/2020-August/171526.html The master branch has been updated by Tom de Vries <vries@sourceware.org>: https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=ad570dd76c3578bc5aaae0c2029ff43fd40ef9cd commit ad570dd76c3578bc5aaae0c2029ff43fd40ef9cd Author: Tom de Vries <tdevries@suse.de> Date: Fri Sep 11 15:56:44 2020 +0200 [gdb/testsuite] Kfail gdb.cp/ambiguous.exp FAILs for PR26602 Kfail these FAILs as caused by PR exp/26602: ... FAIL: gdb.cp/ambiguous.exp: print x.x FAIL: gdb.cp/ambiguous.exp: print n.x FAIL: gdb.cp/ambiguous.exp: print j.x FAIL: gdb.cp/ambiguous.exp: print jva1.x FAIL: gdb.cp/ambiguous.exp: print jva2.x FAIL: gdb.cp/ambiguous.exp: print (A1)j FAIL: gdb.cp/ambiguous.exp: print (A1)jva1 ... Tested on x86_64-linux. gdb/testsuite/ChangeLog: 2020-09-11 Tom de Vries <tdevries@suse.de> PR exp/26602 * gdb.cp/ambiguous.exp: Add KFAILs for PR26602. The master branch has been updated by Pedro Alves <palves@sourceware.org>: https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=87a37e5e078f506fa9905b74e9238593c537fcd5 commit 87a37e5e078f506fa9905b74e9238593c537fcd5 Author: Pedro Alves <pedro@palves.net> Date: Fri Aug 28 21:10:59 2020 +0100 Reject ambiguous C++ field accesses (PR exp/26602) 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 all the 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) Users can then fix their commands by casting or by specifying the baseclass explicitly, like: (gdb) p x.A1::x $1 = 1 (gdb) p x.A2::x $2 = 2 (gdb) p ((A1) x).x $3 = 1 (gdb) p ((A2) x).x $4 = 2 (gdb) p j.K::x $12 = 1 (gdb) p j.L::x $13 = 2 (gdb) p j.A1::x base class 'A1' is ambiguous in type 'J' The last error I've not touched; could be improved to also list the baseclass candidates. The showing the class "path" for each candidate was inspired by GCC's output when you try an ambiguous cast: gdb.cp/ambiguous.cc:161:8: error: ambiguous conversion from derived class 'const JVA1' to base class 'const A1': class JVA1 -> class KV -> class A1 class JVA1 -> class A1 (A1) jva1; ^~~~ I did not include the "class" word as it seemed unnecessarily repetitive, but I can include it if people prefer it: (gdb) print j.x Request for member 'x' is ambiguous in type 'J'. Candidates are: 'int A1::x' (class J -> class K -> class A1) 'int A1::x' (class J -> class L -> class A1) The testcase is adjusted accordingly. I also took the chance to modernize it at the same time. Also, as mentioned above, the testcase doesn't currently initialize the tested variables. This patch inializes them all, giving each field a distinct value, so that we can be sure that GDB is accessing the right fields / offsets. The testcase is extended accordingly. Unfortunately, this exposes a bug, not addressed in this patch. The bug is around a class that inherits from A1 directly and also inherits from two other distinct base classes that inherit virtually from A1 in turn: print jva1.KV::x $51 = 1431665544 (gdb) FAIL: gdb.cp/ambiguous.exp: all fields: print jva1.KV::x print jva1.KV::y $52 = 21845 (gdb) FAIL: gdb.cp/ambiguous.exp: all fields: print jva1.KV::y (gdb) print /x (KV)jva1 $4 = {<A1> = <invalid address>, _vptr.KV = 0x555555557b88 <vtable for JVA1+24>, i = 0x457} (gdb) print /x (A1)(KV)jva1 Cannot access memory at address 0x0 Since that's an orthogonal issue, I filed PR c++/26550 and kfailed the tests that fail because of it. gdb/ChangeLog: PR exp/26602 * valops.c (struct struct_field_searcher): New. (update_search_result): Rename to ... (struct_field_searcher::update_result): ... this. Simplify prototype. Record all found fields. (do_search_struct_field): Rename to ... (struct_field_searcher::search): ... this. Simplify prototype. Maintain stack of visited baseclass path. Call update_result for fields too. Keep searching fields in baseclasses instead of stopping at the first found field. (search_struct_field): Use struct_field_searcher. When looking for fields, report ambiguous access attempts. gdb/testsuite/ChangeLog: PR exp/26602 PR c++/26550 * gdb.cp/ambiguous.cc (marker1): Delete. (main): Initialize all the fields of the locals. Replace marker1 call with a "set breakpoint here" marker. * gdb.cp/ambiguous.exp: Modernize. Use gdb_continue_to_breakpoint instead of running to marker1. Add tests printing all the variables and all the fields of the variables. (test_ambiguous): New proc, expecting the new GDB output when a field access is ambiguous. Change all "warning: X ambiguous" tests to use it. Fix merged. |