Re: [PATCH v2 PR gdb/16841] virtual inheritance via typedef cannot find base

[Simon Marchi <simon.marchi@ericsson.com>]

Hi Weimin,

It seems like the typedef is not really a factor here.  The bug is present even
if you remove the typedef from you test case.  Could you update the title of the
patch to reflect this?  Your patch does not need to have the same title as the
PR that motivated it.

Testing with a typedef is a good idea though.

Your patch still has trailing whitespaces.  Try to do:

$ git format-patch HEAD^
$ git am 0001-virtual-inheritance-via-typedef-cannot-find-base.patch

and make sure you have no message other than

  Applying: virtual inheritance via typedef cannot find base

> diff --git a/gdb/testsuite/gdb.cp/virtbase2.exp b/gdb/testsuite/gdb.cp/virtbase2.exp
> new file mode 100644
> index 0000000..c29ff1c
> --- /dev/null
> +++ b/gdb/testsuite/gdb.cp/virtbase2.exp
> @@ -0,0 +1,50 @@
> +# Copyright 2018 Free Software Foundation, Inc.
> +
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +
> +# Make sure printing virtual base class data member correctly (PR16841)

It sounds like it's missing a word... "works correctly"?

> +
> +if { [skip_cplus_tests] } { continue }
> +
> +standard_testfile .cc
> +
> +if {[prepare_for_testing "failed to prepare" $testfile $srcfile {debug c++}]} {
> +    return -1
> +}
> +
> +if {![runto_main]} then {
> +    perror "couldn't run to main"
> +    continue
> +}
> +
> +gdb_breakpoint "derived::func_d"
> +gdb_continue_to_breakpoint "continue to derived::func_d"
> +gdb_test "print i" " = 55" "i in base class"
> +gdb_test "print derived::i" " = 55" "i in base class"
> +gdb_test "print derived::base::i" " = 55" "i in base class"
> +gdb_test "print base::i" " = 55" "i in base class"
> +gdb_test "print d" " = 6.5999999999999996" "d in base class"
> +gdb_test "print derived::d" " = 6.5999999999999996" "d in base class"
> +gdb_test "print derived::base::d" " = 6.5999999999999996" "d in base class"
> +gdb_test "print base::d" " = 6.5999999999999996" "d in base class"
> +gdb_breakpoint "foo::func_f"
> +gdb_continue_to_breakpoint "continue to foo::func_f"
> +gdb_test "print i" " = 55" "i in base class"
> +gdb_test "print derived::i" " = 55" "i in base class"
> +gdb_test "print derived::base::i" " = 55" "i in base class"
> +gdb_test "print base::i" " = 55" "i in base class"
> +gdb_test "print d" " = 6.5999999999999996" "d in base class"
> +gdb_test "print derived::d" " = 6.5999999999999996" "d in base class"
> +gdb_test "print derived::base::d" " = 6.5999999999999996" "d in base class"
> +gdb_test "print base::d" " = 6.5999999999999996" "d in base class"

Make sure test names are unique:
https://sourceware.org/gdb/wiki/GDBTestcaseCookbook#Make_sure_test_messages_are_unique

In these cases, I think you can omit the test names, gdb_test will default
to use the command as the test name.

I think I found another problematic case.  If you modify your test like this:

diff --git a/gdb/testsuite/gdb.cp/virtbase2.cc b/gdb/testsuite/gdb.cp/virtbase2.cc
index 4f7631e..4620ef5 100644
--- a/gdb/testsuite/gdb.cp/virtbase2.cc
+++ b/gdb/testsuite/gdb.cp/virtbase2.cc
@@ -1,4 +1,9 @@
-struct base {
+struct superbase {
+  int x;
+  superbase () : x(22) {}
+};
+
+struct base : superbase {
   int i; double d;
   base() : i(55), d(6.6) {}
 };


... and try to print the x field with "print derived::x", you get a wrong value.

It would be nice to have the test case generate all possible combinations of scopes.
For example, with

struct base { int x; }
struct derived : base {}

We should test with

  - x
  - base::x
  - derived::x
  - derived::base::x

> diff --git a/gdb/valops.c b/gdb/valops.c
> index 9bdbf22..754e7d0 100644
> --- a/gdb/valops.c
> +++ b/gdb/valops.c
> @@ -3329,6 +3329,35 @@ compare_parameters (struct type *t1, struct type *t2, int skip_artificial)
>    return 0;
>  }
>  
> +/* C++: Given an aggregate type VT, and a class type CLS,
> +   search recursively for CLS and return its offset,
> +   relative to VT, if it is a virtual base member.  */

Can you describe the return value and the parameter V?

> +
> +static int

Return bool.

> +add_virtual_base_offset (struct type *vt, struct type *cls,
> +                            struct value *v, int &boffs)

I suggest naming this function find_virtual_base_offset or get_virtual_base_offset,
since it doesn't do the actual adding.

The last line is too much indented.

Please make boffs a pointer:

https://sourceware.org/gdb/wiki/Internals%20GDB-C-Coding-Standards#Avoid_non-const_reference_parameters.2C_use_pointers_instead

The function comment should mention that the offset is returned in *BOFFS.

> +{
> +  for (int i = 0; i < TYPE_N_BASECLASSES (vt); i++)
> +    {
> +      struct type *ftype = TYPE_FIELD_TYPE (vt, i);
> +      if (check_typedef (ftype) == cls)
> +        {
> +          if (BASETYPE_VIA_VIRTUAL (vt, i))
> +            {
> +              const gdb_byte *adr = value_contents_for_printing (v);
> +              boffs = baseclass_offset (vt, i, adr, value_offset (v),
> +                                        value_as_long (v), v);
> +            }
> +          return true;
> +        }
> +
> +      if (add_virtual_base_offset (ftype, cls, v, boffs))
> +        return true;
> +    }
> +
> +  return false;
> +}
> +
>  /* C++: Given an aggregate type CURTYPE, and a member name NAME,
>     return the address of this member as a "pointer to member" type.
>     If INTYPE is non-null, then it will be the type of the member we
> @@ -3393,6 +3422,15 @@ value_struct_elt_for_reference (struct type *domain, int offset,
>  		  tmp = lookup_pointer_type (TYPE_SELF_TYPE (type));
>  		  v = value_cast_pointers (tmp, v, 1);
>  		  mem_offset = value_as_long (ptr);
> +		  if (domain != curtype)
> +		    {

Can you add a small comment about what this block of code is doing?

Simon