[PATCH] Reject ambiguous C++ field accesses
Luis Machado
luis.machado@linaro.org
Thu Aug 27 19:58:23 GMT 2020
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)
>
> 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 some bugs, not addressed in this patch.
> The bug is around a class that inherits from A1 directly and also
> inherits from with 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
>
> This shows how GDB is clearly reading the confusing the offset of the
> _vptr.KV with the A1 offset:
>
> (gdb) print /x jva1
> $1 = {<KV> = {<A1> = {x = 0x2, y = 0x16}, _vptr.KV = 0x555555557b88 <vtable for JVA1+24>, i = 0x457}, ......}
> ^^^^^^^^^^^^^^
> (gdb) print /x jva1.KV::x
> $2 = 0x55557b88
> ^^^^^^^^^^
> (gdb) print /x jva1.KV::y
> $3 = 0x5555
> ^^^^^^
>
> (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'm kfailing the tests that fail
> because of it. (I'll file a bug soon, unless someone can fix it
> quickly!)
>
> gdb/ChangeLog:
>
> * 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:
>
> * 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.
> ---
> gdb/testsuite/gdb.cp/ambiguous.cc | 87 ++++++++--
> gdb/testsuite/gdb.cp/ambiguous.exp | 321 +++++++++++++++++++++----------------
> gdb/valops.c | 219 ++++++++++++++++++-------
> 3 files changed, 418 insertions(+), 209 deletions(-)
>
> diff --git a/gdb/testsuite/gdb.cp/ambiguous.cc b/gdb/testsuite/gdb.cp/ambiguous.cc
> index 6ee7bc18ea9..a55686547f2 100644
> --- a/gdb/testsuite/gdb.cp/ambiguous.cc
> +++ b/gdb/testsuite/gdb.cp/ambiguous.cc
> @@ -1,9 +1,4 @@
>
> -void marker1()
> -{
> - return;
> -}
> -
> class A1 {
> public:
> int x;
> @@ -102,9 +97,81 @@ int main()
>
> i += k.i + m.w + a1.x + a2.x + a3.x + x.z + l.z + n.r + j.j;
>
> - marker1();
> -
> + /* Initialize all the fields. Keep the order the same as in the
> + .exp file. */
> +
> + a1.x = 1;
> + a1.y = 2;
> +
> + a2.x = 1;
> + a2.y = 2;
> +
> + a3.x = 1;
> + a3.y = 2;
> +
> + x.A1::x = 1;
> + x.A1::y = 2;
> + x.A2::x = 3;
> + x.A2::y = 4;
> + x.z = 5;
> +
> + l.x = 1;
> + l.y = 2;
> + l.z = 3;
> +
> + m.x = 1;
> + m.y = 2;
> + m.w = 3;
> +
> + n.A1::x = 1;
> + n.A1::y = 2;
> + n.A2::x = 3;
> + n.A2::y = 4;
> + n.w = 5;
> + n.r = 6;
> + n.z = 7;
> +
> + k.x = 1;
> + k.y = 2;
> + k.i = 3;
> +
> + j.K::x = 1;
> + j.K::y = 2;
> + j.L::x = 3;
> + j.L::y = 4;
> + j.i = 5;
> + j.z = 6;
> + j.j = 7;
> +
> + jv.x = 1;
> + jv.y = 2;
> + jv.i = 3;
> + jv.z = 4;
> + jv.jv = 5;
> +
> + jva1.KV::x = 1;
> + jva1.KV::y = 2;
> + jva1.LV::x = 3;
> + jva1.LV::y = 4;
> + jva1.z = 5;
> + jva1.i = 6;
> + jva1.jva1 = 7;
> +
> + jva2.KV::x = 1;
> + jva2.KV::y = 2;
> + jva2.LV::x = 3;
> + jva2.LV::y = 4;
> + jva2.A2::x = 5;
> + jva2.A2::y = 6;
> + jva2.z = 7;
> + jva2.i = 8;
> + jva2.jva2 = 9;
> +
> + jva1v.x = 1;
> + jva1v.y = 2;
> + jva1v.z = 3;
> + jva1v.i = 4;
> + jva1v.jva1v = 5;
> +
> + return 0; /* set breakpoint here */
> }
> -
> -
> -
> 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.
>
> 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.
> 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?
> + results. If LOOKING_FOR_BASECLASS is false, found fields are
> + accumulated and the caller (search_struct_field takes) care of
"takes" outside the parenthesis.
> + 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.
> + std::vector<struct type *> path;
> +
> + /* The field found. */
> + struct value *field_value;
> + };
> +
> + struct_field_searcher (const char *name,
> + struct type *outermost_type,
> + bool looking_for_baseclass)
> + : m_name (name),
> + m_looking_for_baseclass (looking_for_baseclass),
> + m_outermost_type (outermost_type)
> + {
> + }
>
> -static void
> -update_search_result (struct value **result_ptr, struct value *v,
> - LONGEST *last_boffset, LONGEST boffset,
> - const char *name, struct type *type)
> + void search (struct value *arg1, LONGEST offset,
> + struct type *type);
> +
> + const std::vector<found_field> &fields ()
> + {
> + return m_fields;
> + }
> +
> + struct value *baseclass ()
> + {
> + return m_baseclass;
> + }
> +
> +private:
> + /* Update results to include V, a found field/baseclass. */
> + void update_result (struct value *v, LONGEST boffset);
> +
> + /* The name of the field/baseclass we're searching for. */
> + const char *m_name;
> +
> + /* 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"?
> + LONGEST m_last_boffset = 0;
> +
> + /* If looking for a baseclass, then the result is stored here. */
> + struct value *m_baseclass = nullptr;
> +
> + /* When looking for fields, the found candidates are stored
> + here. */
> + std::vector<found_field> m_fields;
> +
> + /* The type of the initial type passed to search_struct_field; this
> + is used for error reporting when the lookup is ambiguous. */
> + struct type *m_outermost_type;
> +
> + /* The full path to the struct being inspected. E.g. for field 'x'
> + defined in class B inherited by class A, we have A and B pushed
> + on the path. */
> + std::vector <struct type *> m_struct_path;
> +};
> +
> +void
> +struct_field_searcher::update_result (struct value *v, LONGEST boffset)
> {
> if (v != NULL)
> {
> - if (*result_ptr != NULL
> - /* The result is not ambiguous if all the classes that are
> - found occupy the same space. */
> - && *last_boffset != boffset)
> - error (_("base class '%s' is ambiguous in type '%s'"),
> - name, TYPE_SAFE_NAME (type));
> - *result_ptr = v;
> - *last_boffset = boffset;
> + if (m_looking_for_baseclass)
> + {
> + if (m_baseclass != nullptr
> + /* The result is not ambiguous if all the classes that are
> + found occupy the same space. */
> + && m_last_boffset != boffset)
> + error (_("base class '%s' is ambiguous in type '%s'"),
> + m_name, TYPE_SAFE_NAME (m_outermost_type));
> +
> + m_baseclass = v;
> + m_last_boffset = boffset;
> + }
> + else
> + {
> + /* The field is not ambiguous if it occupies the same
> + space. */
> + if (m_fields.empty () || m_last_boffset != boffset)
> + m_fields.push_back ({m_struct_path, v});
> + }
> }
> }
>
> @@ -1795,25 +1875,25 @@ update_search_result (struct value **result_ptr, struct value *v,
> search_struct_field; this is used for error reporting when the
> lookup is ambiguous. */
>
> -static void
> -do_search_struct_field (const char *name, struct value *arg1, LONGEST offset,
> - struct type *type, int looking_for_baseclass,
> - struct value **result_ptr,
> - LONGEST *last_boffset,
> - struct type *outermost_type)
> +void
> +struct_field_searcher::search (struct value *arg1, LONGEST offset,
> + struct type *type)
> {
> int i;
> int nbases;
>
> + m_struct_path.push_back (type);
> + SCOPE_EXIT { m_struct_path.pop_back (); };
> +
> type = check_typedef (type);
> nbases = TYPE_N_BASECLASSES (type);
>
> - if (!looking_for_baseclass)
> + if (!m_looking_for_baseclass)
> for (i = type->num_fields () - 1; i >= nbases; i--)
> {
> const char *t_field_name = TYPE_FIELD_NAME (type, i);
>
> - if (t_field_name && (strcmp_iw (t_field_name, name) == 0))
> + if (t_field_name && (strcmp_iw (t_field_name, m_name) == 0))
> {
> struct value *v;
>
> @@ -1821,7 +1901,8 @@ do_search_struct_field (const char *name, struct value *arg1, LONGEST offset,
> v = value_static_field (type, i);
> else
> v = value_primitive_field (arg1, offset, i, type);
> - *result_ptr = v;
> +
> + update_result (v, offset);
> return;
> }
>
> @@ -1845,7 +1926,6 @@ do_search_struct_field (const char *name, struct value *arg1, LONGEST offset,
> represented as a struct, with a member for each
> <variant field>. */
>
> - struct value *v = NULL;
> LONGEST new_offset = offset;
>
> /* This is pretty gross. In G++, the offset in an
> @@ -1859,16 +1939,7 @@ do_search_struct_field (const char *name, struct value *arg1, LONGEST offset,
> && TYPE_FIELD_BITPOS (field_type, 0) == 0))
> new_offset += TYPE_FIELD_BITPOS (type, i) / 8;
>
> - do_search_struct_field (name, arg1, new_offset,
> - field_type,
> - looking_for_baseclass, &v,
> - last_boffset,
> - outermost_type);
> - if (v)
> - {
> - *result_ptr = v;
> - return;
> - }
> + search (arg1, new_offset, field_type);
> }
> }
> }
> @@ -1880,10 +1951,10 @@ do_search_struct_field (const char *name, struct value *arg1, LONGEST offset,
> /* If we are looking for baseclasses, this is what we get when
> we hit them. But it could happen that the base part's member
> name is not yet filled in. */
> - int found_baseclass = (looking_for_baseclass
> + int found_baseclass = (m_looking_for_baseclass
> && TYPE_BASECLASS_NAME (type, i) != NULL
> - && (strcmp_iw (name,
> - TYPE_BASECLASS_NAME (type,
> + && (strcmp_iw (m_name,
> + TYPE_BASECLASS_NAME (type,
> i)) == 0));
> LONGEST boffset = value_embedded_offset (arg1) + offset;
>
> @@ -1924,28 +1995,17 @@ do_search_struct_field (const char *name, struct value *arg1, LONGEST offset,
> if (found_baseclass)
> v = v2;
> else
> - {
> - do_search_struct_field (name, v2, 0,
> - TYPE_BASECLASS (type, i),
> - looking_for_baseclass,
> - result_ptr, last_boffset,
> - outermost_type);
> - }
> + search (v2, 0, TYPE_BASECLASS (type, i));
> }
> else if (found_baseclass)
> v = value_primitive_field (arg1, offset, i, type);
> else
> {
> - do_search_struct_field (name, arg1,
> - offset + TYPE_BASECLASS_BITPOS (type,
> - i) / 8,
> - basetype, looking_for_baseclass,
> - result_ptr, last_boffset,
> - outermost_type);
> + search (arg1, offset + TYPE_BASECLASS_BITPOS (type, i) / 8,
> + basetype);
> }
>
> - update_search_result (result_ptr, v, last_boffset,
> - boffset, name, outermost_type);
> + update_result (v, boffset);
> }
> }
>
> @@ -1960,12 +2020,55 @@ static struct value *
> search_struct_field (const char *name, struct value *arg1,
> struct type *type, int looking_for_baseclass)
> {
> - struct value *result = NULL;
> - LONGEST boffset = 0;
> + struct_field_searcher searcher (name, type, looking_for_baseclass);
>
> - do_search_struct_field (name, arg1, 0, type, looking_for_baseclass,
> - &result, &boffset, type);
> - return result;
> + searcher.search (arg1, 0, type);
> +
> + if (!looking_for_baseclass)
> + {
> + const auto &fields = searcher.fields ();
> +
> + if (fields.empty ())
> + return nullptr;
> + else if (fields.size () > 1)
> + {
> + std::string candidates;
> +
> + for (auto &&candidate : fields)
> + {
> + gdb_assert (!candidate.path.empty ());
> +
> + struct type *field_type = value_type (candidate.field_value);
> + struct type *struct_type = candidate.path.back ();
> +
> + std::string path;
> + bool first = true;
> + for (struct type *t : candidate.path)
> + {
> + if (first)
> + first = false;
> + else
> + path += " -> ";
> + path += t->name ();
> + }
> +
> + candidates += string_printf ("\n '%s %s::%s' (%s)",
> + TYPE_SAFE_NAME (field_type),
> + TYPE_SAFE_NAME (struct_type),
> + name,
> + path.c_str ());
> + }
> +
> + error (_("Request for member '%s' is ambiguous in type '%s'."
> + " Candidates are:%s"),
> + name, TYPE_SAFE_NAME (type),
> + candidates.c_str ());
> + }
> + else
> + return fields[0].field_value;
> + }
> + else
> + return searcher.baseclass ();
> }
>
> /* Helper function used by value_struct_elt to recurse through
>
> base-commit: 8c51f2f291a5459e1eabd000b2c52e5de52b4c56
>
Otherwise LGTM. I exercised this for aarch64-linux and see no more
failures and 10 KFAIL's.
More information about the Gdb-patches
mailing list