[PR,gdb/16841] virtual inheritance via typedef cannot find base
Commit Message
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. 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, the virtual-base offset, relative to its derived class,
needs to be fetched, via baseclass_offset(), and used in calculating the
address of its data member in value_struct_elt_for_reference().
Tested on amd64-linux-gnu. No regressions.
---
gdb/ChangeLog | 6 +++++
gdb/testsuite/ChangeLog | 6 +++++
gdb/testsuite/gdb.cp/virtbase2.cc | 28 ++++++++++++++++++++++++++
gdb/testsuite/gdb.cp/virtbase2.exp | 38 ++++++++++++++++++++++++++++++++++++
gdb/valops.c | 17 +++++++++++++++-
5 files changed, 94 insertions(+), 1 deletions(-)
create mode 100644 gdb/testsuite/gdb.cp/virtbase2.cc
create mode 100644 gdb/testsuite/gdb.cp/virtbase2.exp
Comments
On 2018-07-20 07:16 PM, Weimin Pan wrote:
> 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. 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, the virtual-base offset, relative to its derived class,
> needs to be fetched, via baseclass_offset(), and used in calculating the
> address of its data member in value_struct_elt_for_reference().
Hi Weimin,
I have looked at this a little bit, but unfortunately I don't really understand
what's going on (maybe somebody else does and could review it?). I understand
the issue, and can see that your patch fixes it, but I don't understand how it
does it. It would help if you could walk us through your code. It maybe also
means that some comments would be appropriate, to explain what's going on.
Thanks,
Simon
On 7/24/2018 3:18 PM, Simon Marchi wrote:
> On 2018-07-20 07:16 PM, Weimin Pan wrote:
>> 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. 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, the virtual-base offset, relative to its derived class,
>> needs to be fetched, via baseclass_offset(), and used in calculating the
>> address of its data member in value_struct_elt_for_reference().
> Hi Weimin,
>
> I have looked at this a little bit, but unfortunately I don't really understand
> what's going on (maybe somebody else does and could review it?). I understand
> the issue, and can see that your patch fixes it, but I don't understand how it
> does it. It would help if you could walk us through your code. It maybe also
> means that some comments would be appropriate, to explain what's going on.
>
> Thanks,
>
> Simon
Hi Simon,
Since a virtual base offset is stored in its derived class's vtable, my
code simply (1) checks if base
class "curtype" is virtual in its derived class "domain" and (2) calls
baseclass_offset() to get its
offset if it is.
Please check out the following article which provides a detailed
explanation about the layout:
Memory Layout for Multiple and Virtual Inheritance
https://web.archive.org/web/20160413064252/http://www.phpcompiler.org/articles/virtualinheritance.html
Thanks,
Weimin
Thanks for the patch. There are a few small issues with it.
First, git says that most of the lines in the valops.c part have a
trailing space. Please remove those.
Weimin> + for (index = 0; index < TYPE_NFIELDS (domain); index++)
I think the loop limit should be TYPE_N_BASECLASSES.
And, is it possible for the desired type to be a base class of a base class?
I suspect so, but I didn't try to check; in this case you will need some
recursion here, and also I think a new test for this case would be good.
Weimin> + if (TYPE_FIELDS (domain)[index].type == curtype)
It is more idiomatic to write TYPE_FIELD_TYPE (domain, index) here.
Also I think you need check_typedef on the left-hand-side.
Otherwise perhaps using a typedef for a base class would confuse gdb
(depending on whether the compiler emits typedefs here or not).
Weimin> + else
Weimin> + mem_offset = value_as_long (ptr);
This line is indented too far.
thanks,
Tom
Hi Tom,
Thanks very much for the comments.
On 7/28/2018 7:28 PM, Tom Tromey wrote:
> Thanks for the patch. There are a few small issues with it.
>
> First, git says that most of the lines in the valops.c part have a
> trailing space. Please remove those.
Will do.
> Weimin> + for (index = 0; index < TYPE_NFIELDS (domain); index++)
>
> I think the loop limit should be TYPE_N_BASECLASSES.
OK, will change it to TYPE_N_BASECLASSES. BTW, there are quite a number
of places that use:
for (i = 0; i < TYPE_NFIELDS (type); ++i)
> And, is it possible for the desired type to be a base class of a base class?
> I suspect so, but I didn't try to check; in this case you will need some
> recursion here, and also I think a new test for this case would be good.
Yes, I think it's possible and will need to do recursions here. Also
will expand the submitted
test with such classes.
>
> Weimin> + if (TYPE_FIELDS (domain)[index].type == curtype)
>
> It is more idiomatic to write TYPE_FIELD_TYPE (domain, index) here.
> Also I think you need check_typedef on the left-hand-side.
> Otherwise perhaps using a typedef for a base class would confuse gdb
> (depending on whether the compiler emits typedefs here or not).
Good point. Will change it to
if (check_typedef (TYPE_FIELD_TYPE (domain,index)) == curtype)
> Weimin> + else
> Weimin> + mem_offset = value_as_long (ptr);
>
> This line is indented too far.
OK.
Thanks again,
Weimin
>
> thanks,
> Tom
>>>>> ">" == Wei-min Pan <weimin.pan@oracle.com> writes:
Tom> I think the loop limit should be TYPE_N_BASECLASSES.
>> OK, will change it to TYPE_N_BASECLASSES. BTW, there are quite a
>> number of places that use:
>> for (i = 0; i < TYPE_NFIELDS (type); ++i)
Yes, it just depends on whether the intent at a given spot is to loop
over all fields, or to only loop over base classes.
Tom
@@ -1,3 +1,9 @@
+2018-07-20 Weimin Pan <weimin.pan@oracle.com>
+
+ PR gdb/16841
+ * valops.c (value_struct_elt_for_reference): Calculate virtual
+ baseclass offset, relative to its derived class.
+
2018-06-29 Pedro Alves <palves@redhat.com>
* gdb/amd64-tdep.h (amd64_create_target_description): Add
@@ -1,3 +1,9 @@
+2018-07-20 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.
new file mode 100644
@@ -0,0 +1,28 @@
+/* This test script is part of GDB, the GNU debugger.
+
+ 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/>. */
+
+struct left { int i; };
+struct right {int j;};
+struct derived: virtual left, virtual right
+{
+ void func() { }
+};
+
+int main()
+{
+ derived().func();
+}
new file mode 100644
@@ -0,0 +1,38 @@
+# 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"
+gdb_continue_to_breakpoint "continue to derived::func"
+gdb_test "print j" " = 0" "j in base class right"
+gdb_test "print i" " = 0" "i in base class left"
+gdb_test "print derived::j" " = 0" "j in base class right"
+gdb_test "print derived::i" " = 0" "i in base class left"
+gdb_test "print derived::right::j" " = 0" "j in base class right"
+gdb_test "print derived::left::i" " = 0" "i in base class left"
@@ -3385,6 +3385,7 @@ value_struct_elt_for_reference (struct type *domain, int offset,
struct value *ptr;
long mem_offset;
struct type *type, *tmp;
+ int index;
ptr = value_aggregate_elt (domain, name, NULL, 1, noside);
type = check_typedef (value_type (ptr));
@@ -3392,7 +3393,21 @@ value_struct_elt_for_reference (struct type *domain, int offset,
&& TYPE_CODE (type) == TYPE_CODE_MEMBERPTR);
tmp = lookup_pointer_type (TYPE_SELF_TYPE (type));
v = value_cast_pointers (tmp, v, 1);
- mem_offset = value_as_long (ptr);
+ for (index = 0; index < TYPE_NFIELDS (domain); index++)
+ {
+ if (TYPE_FIELDS (domain)[index].type == curtype)
+ break;
+ }
+ if (index != TYPE_NFIELDS (domain)
+ && BASETYPE_VIA_VIRTUAL (domain, index))
+ {
+ const gdb_byte *adr = value_contents_for_printing (ptr);
+ mem_offset = baseclass_offset (domain, index, adr,
+ value_offset (ptr),
+ value_as_long (v), ptr);
+ }
+ else
+ mem_offset = value_as_long (ptr);
tmp = lookup_pointer_type (TYPE_TARGET_TYPE (type));
result = value_from_pointer (tmp,
value_as_long (v) + mem_offset);