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

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

[Weimin Pan]

Finding data member in virtual base class

This patch fixes the original problem - printing member in a virtual base,
using various expressions, do not yield the same value. A simple test case
below demonstrates the problem:

% cat t.cc
struct base { int i; };
typedef base tbase;
struct derived: virtual tbase { void func() { } };
int main() { derived().func(); }
% g++ -g t.cc
% gdb a.out
(gdb) break derived::func
(gdb) run
(gdb) p i
$1 = 0
(gdb) p base::i
$2 = 0
(gdb) p derived::base::i
$3 = 0
(gdb) p derived::i
$4 = 4196392

To fix the problem, we need to use "vptr" in the virtual class object,
which indexes to its derived class's vtable, to fetch the virtual base
offset. The offset is then added to the virtual class object to access
its member in value_struct_elt_for_reference().

The new function add_virtual_base_offset searches recursively for the
base class along the class hierarchy and returns the virtual base offset
for the virtual class.

Tested on amd64-linux-gnu. No regressions.
---
 gdb/ChangeLog                      |    7 +++++
 gdb/testsuite/ChangeLog            |    6 ++++
 gdb/testsuite/gdb.cp/virtbase2.cc  |   21 +++++++++++++++
 gdb/testsuite/gdb.cp/virtbase2.exp |   50 ++++++++++++++++++++++++++++++++++++
 gdb/valops.c                       |   38 +++++++++++++++++++++++++++
 5 files changed, 122 insertions(+), 0 deletions(-)
 create mode 100644 gdb/testsuite/gdb.cp/virtbase2.cc
 create mode 100644 gdb/testsuite/gdb.cp/virtbase2.exp

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index c47c111..9d86f1c 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,10 @@
+2018-08-23  Weimin Pan  <weimin.pan@oracle.com>
+
+	PR gdb/16841
+	* valops.c (add_virtual_base_offset): New function.
+	(value_struct_elt_for_reference): Use it to get virtual base offset 
+	and add it in calculating class member address.
+
 2018-06-29  Pedro Alves  <palves@redhat.com>
 
 	* gdb/amd64-tdep.h (amd64_create_target_description): Add
diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index 93c849c..7be2e1d 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,9 @@
+2018-08-23  Weimin Pan  <weimin.pan@oracle.com>
+
+	PR gdb/16841
+	* gdb.cp/virtbase2.cc: New file.
+	* gdb.cp/virtbase2.exp: New file.
+
 2018-06-29  Pedro Alves  <palves@redhat.com>
 
 	* gdb.threads/names.exp: Adjust expected "info threads" output.
diff --git a/gdb/testsuite/gdb.cp/virtbase2.cc b/gdb/testsuite/gdb.cp/virtbase2.cc
new file mode 100644
index 0000000..4f7631e
--- /dev/null
+++ b/gdb/testsuite/gdb.cp/virtbase2.cc
@@ -0,0 +1,21 @@
+struct base { 
+  int i; double d; 
+  base() : i(55), d(6.6) {}
+};
+typedef base tbase;
+struct derived: virtual tbase 
+{ 
+  void func_d() { }
+};
+
+struct foo: virtual derived
+{ 
+  void func_f() { }
+};
+
+int main() 
+{ 
+  derived().func_d();
+  foo().func_f();
+}
+
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)
+
+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"
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.  */
+
+static int
+add_virtual_base_offset (struct type *vt, struct type *cls,
+                            struct value *v, int &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)
+		    {
+		      struct value *v2 = value_of_this_silent (current_language);
+		      struct type *vtype = check_typedef (value_type (v2));
+		      struct type *vt = check_typedef (TYPE_TARGET_TYPE (vtype));
+    		      int base_offs = 0;
+	    	      if (add_virtual_base_offset (vt, curtype, v, base_offs))
+      		        mem_offset += base_offs;
+		    }
 		  tmp = lookup_pointer_type (TYPE_TARGET_TYPE (type));
 		  result = value_from_pointer (tmp,
 					       value_as_long (v) + mem_offset);
-- 
1.7.1

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

[Simon Marchi]

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

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

[Simon Marchi]

Here's a suggestion of modifications for the test, implementing what
I mentioned.  Feel free to change it as you wish.


From 70cc479b7465b119c2aacd092167ebb5c60dd70b Mon Sep 17 00:00:00 2001
From: Simon Marchi <simon.marchi@ericsson.com>
Date: Fri, 7 Sep 2018 23:00:28 +0100
Subject: [PATCH] Modifications to the test

---
 gdb/testsuite/gdb.cp/virtbase2.cc  |  7 +++-
 gdb/testsuite/gdb.cp/virtbase2.exp | 68 +++++++++++++++++++++++++++-----------
 2 files changed, 54 insertions(+), 21 deletions(-)

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) {}
 };
diff --git a/gdb/testsuite/gdb.cp/virtbase2.exp b/gdb/testsuite/gdb.cp/virtbase2.exp
index c29ff1c..854f81e 100644
--- a/gdb/testsuite/gdb.cp/virtbase2.exp
+++ b/gdb/testsuite/gdb.cp/virtbase2.exp
@@ -28,23 +28,51 @@ if {![runto_main]} then {
     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"
+proc make_scope_list { scopes } {
+    if { [llength $scopes] == 1 } {
+        return [list "" "${scopes}::"]
+    }
+
+    # Pop the first element, save the first scope.
+    set this_scope [lindex $scopes 0]
+    set scopes [lreplace $scopes 0 0]
+
+    set child_result [make_scope_list $scopes]
+
+    # Add a copy of the child's result without this scope...
+    set result $child_result
+
+    # ... and a copy of the child's result with this scope.
+    foreach r $child_result {
+        lappend result "${this_scope}::$r"
+    }
+
+    return $result
+}
+
+proc test_variables_in_base { scopes } {
+    foreach scope [make_scope_list $scopes] {
+        gdb_test "print ${scope}i" " = 55"
+        gdb_test "print ${scope}d" " = 6.5999999999999996"
+    }
+}
+
+proc test_variables_in_superbase { scopes } {
+    foreach scope [make_scope_list $scopes] {
+        gdb_test "print ${scope}x" " = 22"
+    }
+}
+
+with_test_prefix "derived::func_d" {
+    gdb_breakpoint "derived::func_d"
+    gdb_continue_to_breakpoint "continue to derived::func_d"
+    test_variables_in_base {derived base}
+    test_variables_in_superbase {derived base superbase}
+}
+
+with_test_prefix "foo::func_f" {
+    gdb_breakpoint "foo::func_f"
+    gdb_continue_to_breakpoint "continue to foo::func_f"
+    test_variables_in_base {foo derived base}
+    test_variables_in_superbase {foo derived base superbase}
+}
-- 
2.7.4

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

[Weimin Pan]

Hi Simon,

Thanks very much for your comments.


On 9/7/2018 2:26 PM, Simon Marchi wrote:
> 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

It's nice to know this "git am" command. Do I use the "git am --abort" 
command to
restore my original patch, correct problems, then use another "git am" 
command?

>> 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"?

Word "works" has been added.

>> +
>> +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'm going to use the new virtbase2.exp that you suggested in your 
separate email which, exercise more
possible combinations of scopes as you described below, and reveal more 
test failures (: Looking into
these problems now.

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

Yes, "print derived::x" shows a wrong value while "print x" and "print 
derived::base::x" work correctly.

> 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?

Will do. Tom also has pointed this out.

> +
> +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.

Will change the function name to get_virtual_base_offset. Tom made a similar comment.

> The last line is too much indented.

Fixed.

>
> Please make boffs a pointer:

Done, Tom also suggested it.

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

Done, Tom made the same comment.

>> +{
>> +  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?

Will do.

Thanks again,
Weimin

>
> Simon

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

[Simon Marchi]

On 2018-09-08 01:12, Weimin Pan wrote:
>> 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
> 
> It's nice to know this "git am" command. Do I use the "git am --abort"
> command to
> restore my original patch, correct problems, then use another "git am" 
> command?

You shouldn't need to use --abort in this case... In my instructions, I 
forgot to mention to remove your commit before applying it again (it's 
expected that applying the same patch twice doesn't work).  So it should 
be:

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

Modifications to the patch can be done by using "git commit --amend", as 
usual.

Thanks!

Simon

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

[Tom Tromey]

>>>>> ">" == Weimin Pan <weimin.pan@oracle.com> writes:

>> Finding data member in virtual base class
>> This patch fixes the original problem - printing member in a virtual base,
>> using various expressions, do not yield the same value. A simple test case
>> below demonstrates the problem:

Thank you for the patch.

>> diff --git a/gdb/testsuite/gdb.cp/virtbase2.cc b/gdb/testsuite/gdb.cp/virtbase2.cc
>> new file mode 100644
>> index 0000000..4f7631e
>> --- /dev/null
>> +++ b/gdb/testsuite/gdb.cp/virtbase2.cc
>> @@ -0,0 +1,21 @@
>> +struct base { 

New test cases should come with the GPL header.  You can copy one from
some other .cc file in gdb.cp, just make sure to fix the copyright line.

>> 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.  */

The comment says this "return"s the offset, but really it stores the
offset in BOFFS and returns a boolean.  The comment should explain this.

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

I think the function is misnamed, because it doesn't actually add
anything.

Also, gdb style is not to use non-const references for out parameters.
Instead make "boffs" a pointer.

>> +{
>> +  for (int i = 0; i < TYPE_N_BASECLASSES (vt); i++)
>> +    {
>> +      struct type *ftype = TYPE_FIELD_TYPE (vt, i);
>> +      if (check_typedef (ftype) == cls)

You probably want types_equal rather than == here.  The difference is
that two types will not be == if they came from different shared
libraries.  However, they may in fact be "the same" type from the user
point of view.

You don't have to call check_typedef for types_equal.

However I do think you have to call check_typedef at the recursion spot.
I recall there being bugs in the past where gdb didn't check_typedef a
base type and ran into problems.

>> @@ -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)

Probably types_equal here too.

>> +		    {
>> +		      struct value *v2 = value_of_this_silent (current_language);

Hah, I was going to write that this is incorrect, but I see it is in the
code already.

I suppose this makes sense, because this whole code block is about the
implicit this case.  Which seems like the wrong place to put it, but
none of this is your problem.

However, rather than calling value_of_this_silent for a second time,
just save the original value and re-use it.


Sorry about all the nits.  This area is tricky.  In any case the patch
is very close to ready.

Tom

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

[Weimin Pan]

Hi Tom,

Thanks very much for the comments.

On 9/7/2018 2:42 PM, Tom Tromey wrote:
>>>>>> ">" == Weimin Pan <weimin.pan@oracle.com> writes:
>>> Finding data member in virtual base class
>>> This patch fixes the original problem - printing member in a virtual base,
>>> using various expressions, do not yield the same value. A simple test case
>>> below demonstrates the problem:
> Thank you for the patch.
>
>>> diff --git a/gdb/testsuite/gdb.cp/virtbase2.cc b/gdb/testsuite/gdb.cp/virtbase2.cc
>>> new file mode 100644
>>> index 0000000..4f7631e
>>> --- /dev/null
>>> +++ b/gdb/testsuite/gdb.cp/virtbase2.cc
>>> @@ -0,0 +1,21 @@
>>> +struct base {
> New test cases should come with the GPL header.  You can copy one from
> some other .cc file in gdb.cp, just make sure to fix the copyright line.

OK, done.

>>> 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.  */
> The comment says this "return"s the offset, but really it stores the
> offset in BOFFS and returns a boolean.  The comment should explain this.

Done.

>
>>> +static int
>>> +add_virtual_base_offset (struct type *vt, struct type *cls,
>>> +                            struct value *v, int &boffs)
> I think the function is misnamed, because it doesn't actually add
> anything.
>
> Also, gdb style is not to use non-const references for out parameters.
> Instead make "boffs" a pointer.

Done, Simone made the same comment.


>>> +{
>>> +  for (int i = 0; i < TYPE_N_BASECLASSES (vt); i++)
>>> +    {
>>> +      struct type *ftype = TYPE_FIELD_TYPE (vt, i);
>>> +      if (check_typedef (ftype) == cls)
> You probably want types_equal rather than == here.  The difference is
> that two types will not be == if they came from different shared
> libraries.  However, they may in fact be "the same" type from the user
> point of view.
>
> You don't have to call check_typedef for types_equal.
>
> However I do think you have to call check_typedef at the recursion spot.

OK, move check_typedef at the recursive call site and use types_equal 
for type comparison.

> I recall there being bugs in the past where gdb didn't check_typedef a
> base type and ran into problems.
>
>>> @@ -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)
> Probably types_equal here too.

Done.

>
>>> +		    {
>>> +		      struct value *v2 = value_of_this_silent (current_language);
> Hah, I was going to write that this is incorrect, but I see it is in the
> code already.
>
> I suppose this makes sense, because this whole code block is about the
> implicit this case.  Which seems like the wrong place to put it, but
> none of this is your problem.
>
> However, rather than calling value_of_this_silent for a second time,
> just save the original value and re-use it.

Done, introduce local variable "this_v" to save its original value.

>
> Sorry about all the nits.  This area is tricky.  In any case the patch
> is very close to ready.

Currently working on failures from Simon's suggested test case run.

Thanks again,
Weimin

>
> Tom