[Bug c++/26550] New: Object of class with virtual base classes printed incorrectly (gdb.cp/ambiguous.exp)

[Bug c++/26550] New: Object of class with virtual base classes printed incorrectly (gdb.cp/ambiguous.exp)

[palves at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=26550

            Bug ID: 26550
           Summary: Object of class with virtual base classes printed
                    incorrectly (gdb.cp/ambiguous.exp)
           Product: gdb
           Version: unknown
            Status: NEW
          Severity: normal
          Priority: P2
         Component: c++
          Assignee: unassigned at sourceware dot org
          Reporter: palves at redhat dot com
  Target Milestone: ---

The gdb.cp/ambiguous.exp testcase ( as soon as these changes land in master:
https://sourceware.org/pipermail/gdb-patches/2020-August/171526.html )
runs into a GDB bug where GDB prints a value with virtual base classes
incorrectly.

The class in question inherits from A1 directly and also inherits
from two other distinct base classes that inherit virtually from A1 in
turn:

class A1 {
public:
  int x;
  int y;
};

class LV : public virtual A1 {
public:
  int z;
};

class KV : public virtual A1 {
public:
  int i;
};

class JVA1 : public KV, public LV, public A1 {
public:
  int jva1;
};

and then is initialized like this:

  JVA1 jva1;
  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;

The testcase shows:

 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

And with the following we see how GDB is clearly confusing the offset of
_vptr.KV with the offset of A1:

 (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
      ^^^^^^

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug c++/26550] Object of class with virtual base classes printed incorrectly (gdb.cp/ambiguous.exp)

[cvs-commit at gcc dot gnu.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=26550

--- Comment #1 from cvs-commit at gcc dot gnu.org <cvs-commit at gcc dot gnu.org> ---
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.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug c++/26550] Object of class with virtual base classes printed incorrectly (gdb.cp/ambiguous.exp)

[blarsen at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=26550

B. Larsen <blarsen at redhat dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |blarsen at redhat dot com

--- Comment #2 from B. Larsen <blarsen at redhat dot com> ---
Hi! I've stated looking at this bug, and I have a new insight

> And with the following we see how GDB is clearly confusing the offset of _vptr.KV
> with the offset of A1:
> (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

I think the problem runs a little deeper because of the following tests:

(gdb) print &jva1.KV::x
$1 = (int *) 0x7fffffffe0f0

(gdb) print &jva1.LV::x
$2 = (int *) 0x7fffffffe100

(gdb) p &jva1
$3 = (JVA1 *) 0x7fffffffe0f0

and if I understand virtual inheritance correctly (and dissassembly of the
code), $1 and $2 should be the same, both 0x7f...fe100

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug c++/26550] Object of class with virtual base classes printed incorrectly (gdb.cp/ambiguous.exp)

[blarsen at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=26550

--- Comment #3 from B. Larsen <blarsen at redhat dot com> ---
After 2 weeks looking at this I know why this is happening, but I haven't
figured out how to fix it, so I'm gonna document what I found in case someone
can help me out.

When GDB is processing the expression "jva1.KV::x", the processing ends up at
structop_member_operation::evaluate, where it gets broken into:
  * get address of jva1 (addr)
  * cast JVA1 into KV, and get the offset of x within that class (offset)
  * get the value of `addr + offset`

And the problem lies on that middle part. it happens by calling:

1. expr::unop_addr_operation::evaluate
2. expr::scope_operation::evaluate_for_address
3. value_aggregate_elt
4. value_struct_elt_for_reference

and 4. is where the problem happens. When we get there, our "domain type" is
already KV, we don't have any idea what the original memory address is, and
because KV was contructed through virtual inheritances, the offset for the
virtual baseclass A1 needs stuff in the inferior's memory.

I brought the stack trace up because my best guess on how to solve the problem
would involve changing `class expr::operation` to have a base_addr field, and a
method for setting it, so that scope_operation::evaluate_for_address may access
that info, but this does sound like wasted space for every other use case of
operations, so I'm reluctant to doing it

-- 
You are receiving this mail because:
You are on the CC list for the bug.