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

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

[Weimin Pan]

Fix failure to find member of a typedef base class

The test case below demonstrates the problem, as described in this PR's Comment 5:

typedef struct {
        int x;
} A;

struct C : A {
        int y;
};

int main()
{
        C c;
        return 55;
}

$ gdb a.out
(gdb) ptype C::x
Internal error: non-aggregate type to value_struct_elt_for_reference

In value_struct_elt_for_reference(), need to call check_typedef() on
the aggregate type to handle the case of *curtype being ptr->typedef.

Tested on x86_64-linux. No regressions.
---
 gdb/ChangeLog                               |  6 +++++
 gdb/testsuite/ChangeLog                     |  6 +++++
 gdb/testsuite/gdb.cp/typedef-base-ptype.cc  | 30 +++++++++++++++++++++++
 gdb/testsuite/gdb.cp/typedef-base-ptype.exp | 37 +++++++++++++++++++++++++++++
 gdb/valops.c                                |  2 +-
 5 files changed, 80 insertions(+), 1 deletion(-)
 create mode 100644 gdb/testsuite/gdb.cp/typedef-base-ptype.cc
 create mode 100644 gdb/testsuite/gdb.cp/typedef-base-ptype.exp

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index ff2f145..99a3175 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,9 @@
+2018-06-12  Weimin Pan  <weimin.pan@oracle.com>
+
+	PR gdb/16841
+	* valops.c (value_struct_elt_for_reference): Call check_typedef on 
+	aggregate type to get its real type before accessing it.
+
 2018-05-29  Weimin Pan  <weimin.pan@oracle.com>
 
 	* minsyms.h (lookup_minimal_symbol_and_objfile): Remove declaration.
diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index b2938b1..78771d3 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,9 @@
+2018-06-12  Weimin Pan  <weimin.pan@oracle.com>
+
+	PR gdb/16841
+	* gdb/testsuite/gdb.cp/typedef-base-ptype.cc: New file.
+	* gdb/testsuite/gdb.cp/typedef-base-ptype.exp: New file.
+
 2018-05-24  Andrew Burgess  <andrew.burgess@embecosm.com>
 
 	PR gdb/23203
diff --git a/gdb/testsuite/gdb.cp/typedef-base-ptype.cc b/gdb/testsuite/gdb.cp/typedef-base-ptype.cc
new file mode 100644
index 0000000..bfbcc4a
--- /dev/null
+++ b/gdb/testsuite/gdb.cp/typedef-base-ptype.cc
@@ -0,0 +1,30 @@
+/* This testcase 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/>.  */
+
+typedef struct {
+        int x;
+} A;
+
+struct C : A {
+        int y;
+};
+
+int main()
+{
+        C c;
+        return 55;
+}
diff --git a/gdb/testsuite/gdb.cp/typedef-base-ptype.exp b/gdb/testsuite/gdb.cp/typedef-base-ptype.exp
new file mode 100644
index 0000000..e588fd3
--- /dev/null
+++ b/gdb/testsuite/gdb.cp/typedef-base-ptype.exp
@@ -0,0 +1,37 @@
+# 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/>.
+
+if { [skip_cplus_tests] } { continue }
+
+standard_testfile .cc
+
+if [get_compiler_info "c++"] {
+    return -1
+}
+
+if {[prepare_for_testing "failed to prepare" $testfile $srcfile {debug c++}]} {
+    return -1
+}
+
+clean_restart $testfile
+
+if ![runto_main] {
+    untested "could not run to main"
+    return -1
+}
+
+gdb_test "ptype C::x" \
+         "type = int" \
+         "ptype typedef base struct member"
diff --git a/gdb/valops.c b/gdb/valops.c
index 62a86c0..3970a25 100644
--- a/gdb/valops.c
+++ b/gdb/valops.c
@@ -3343,7 +3343,7 @@ value_struct_elt_for_reference (struct type *domain, int offset,
 				int want_address,
 				enum noside noside)
 {
-  struct type *t = curtype;
+  struct type *t = check_typedef (curtype);
   int i;
   struct value *v, *result;
 
-- 
1.8.3.1

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

[Simon Marchi]

On 2018-06-15 07:45 PM, Weimin Pan wrote:
> Fix failure to find member of a typedef base class
> 
> The test case below demonstrates the problem, as described in this PR's Comment 5:
> 
> typedef struct {
>         int x;
> } A;
> 
> struct C : A {
>         int y;
> };
> 
> int main()
> {
>         C c;
>         return 55;
> }
> 
> $ gdb a.out
> (gdb) ptype C::x
> Internal error: non-aggregate type to value_struct_elt_for_reference
> 
> In value_struct_elt_for_reference(), need to call check_typedef() on
> the aggregate type to handle the case of *curtype being ptr->typedef.

Thanks, that makes sense.  I noted some nits, you can push the patch after
addressing them.

> diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
> index b2938b1..78771d3 100644
> --- a/gdb/testsuite/ChangeLog
> +++ b/gdb/testsuite/ChangeLog
> @@ -1,3 +1,9 @@
> +2018-06-12  Weimin Pan  <weimin.pan@oracle.com>
> +
> +	PR gdb/16841
> +	* gdb/testsuite/gdb.cp/typedef-base-ptype.cc: New file.
> +	* gdb/testsuite/gdb.cp/typedef-base-ptype.exp: New file.

Filenames should be relative to the ChangeLog file, so remove "gdb/testsuite/".

> diff --git a/gdb/testsuite/gdb.cp/typedef-base-ptype.cc b/gdb/testsuite/gdb.cp/typedef-base-ptype.cc
> new file mode 100644
> index 0000000..bfbcc4a
> --- /dev/null
> +++ b/gdb/testsuite/gdb.cp/typedef-base-ptype.cc
> @@ -0,0 +1,30 @@
> +/* This testcase 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/>.  */
> +
> +typedef struct {
> +        int x;
> +} A;
> +
> +struct C : A {
> +        int y;
> +};
> +
> +int main()
> +{
> +        C c;
> +        return 55;
> +}

We try to make the test code follow the GNU coding style (unless testing
a particular formatting is the purpose of the test).

> diff --git a/gdb/testsuite/gdb.cp/typedef-base-ptype.exp b/gdb/testsuite/gdb.cp/typedef-base-ptype.exp

I would suggest naming this test "typedef-base.exp", because we may add other tests
than ptype in it.  For example, David's original reproducer in PR 16841 still won't
work after this patch (AFAIK), and would be a good candidate to end up as a test in
this file.

> new file mode 100644
> index 0000000..e588fd3
> --- /dev/null
> +++ b/gdb/testsuite/gdb.cp/typedef-base-ptype.exp
> @@ -0,0 +1,37 @@
> +# 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/>.

Can you add a one-liner comment that states the purpose of the test?  Something
like

  # Make sure that inheritance through a typedef is well handled.

Thanks,

Simon

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

[Wei-min Pan]

On 6/16/2018 6:06 PM, Simon Marchi wrote:
> On 2018-06-15 07:45 PM, Weimin Pan wrote:
>> Fix failure to find member of a typedef base class
>>
>> The test case below demonstrates the problem, as described in this PR's Comment 5:
>>
>> typedef struct {
>>          int x;
>> } A;
>>
>> struct C : A {
>>          int y;
>> };
>>
>> int main()
>> {
>>          C c;
>>          return 55;
>> }
>>
>> $ gdb a.out
>> (gdb) ptype C::x
>> Internal error: non-aggregate type to value_struct_elt_for_reference
>>
>> In value_struct_elt_for_reference(), need to call check_typedef() on
>> the aggregate type to handle the case of *curtype being ptr->typedef.
> Thanks, that makes sense.  I noted some nits, you can push the patch after
> addressing them.
>
>> diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
>> index b2938b1..78771d3 100644
>> --- a/gdb/testsuite/ChangeLog
>> +++ b/gdb/testsuite/ChangeLog
>> @@ -1,3 +1,9 @@
>> +2018-06-12  Weimin Pan  <weimin.pan@oracle.com>
>> +
>> +	PR gdb/16841
>> +	* gdb/testsuite/gdb.cp/typedef-base-ptype.cc: New file.
>> +	* gdb/testsuite/gdb.cp/typedef-base-ptype.exp: New file.
> Filenames should be relative to the ChangeLog file, so remove "gdb/testsuite/".
>
>> diff --git a/gdb/testsuite/gdb.cp/typedef-base-ptype.cc b/gdb/testsuite/gdb.cp/typedef-base-ptype.cc
>> new file mode 100644
>> index 0000000..bfbcc4a
>> --- /dev/null
>> +++ b/gdb/testsuite/gdb.cp/typedef-base-ptype.cc
>> @@ -0,0 +1,30 @@
>> +/* This testcase 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/>.  */
>> +
>> +typedef struct {
>> +        int x;
>> +} A;
>> +
>> +struct C : A {
>> +        int y;
>> +};
>> +
>> +int main()
>> +{
>> +        C c;
>> +        return 55;
>> +}
> We try to make the test code follow the GNU coding style (unless testing
> a particular formatting is the purpose of the test).
>
>> diff --git a/gdb/testsuite/gdb.cp/typedef-base-ptype.exp b/gdb/testsuite/gdb.cp/typedef-base-ptype.exp
> I would suggest naming this test "typedef-base.exp", because we may add other tests
> than ptype in it.  For example, David's original reproducer in PR 16841 still won't
> work after this patch (AFAIK), and would be a good candidate to end up as a test in
> this file.
>
>> new file mode 100644
>> index 0000000..e588fd3
>> --- /dev/null
>> +++ b/gdb/testsuite/gdb.cp/typedef-base-ptype.exp
>> @@ -0,0 +1,37 @@
>> +# 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/>.
> Can you add a one-liner comment that states the purpose of the test?  Something
> like
>
>    # Make sure that inheritance through a typedef is well handled.
>
> Thanks,
>
> Simon
>

Hi Simon,

Will incorporate your comments into the patch and push it.

Thanks,
Weimin

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

[Tom Tromey]

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

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

[Wei-min Pan]

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

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

[Tom Tromey]

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

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

[Weimin Pan]

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

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

[Simon Marchi]

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

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

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index c47c111..1531ff1 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -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
diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index 93c849c..1577a54 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -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.
diff --git a/gdb/testsuite/gdb.cp/virtbase2.cc b/gdb/testsuite/gdb.cp/virtbase2.cc
new file mode 100644
index 0000000..33753b3
--- /dev/null
+++ b/gdb/testsuite/gdb.cp/virtbase2.cc
@@ -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();
+}
diff --git a/gdb/testsuite/gdb.cp/virtbase2.exp b/gdb/testsuite/gdb.cp/virtbase2.exp
new file mode 100644
index 0000000..01dd298
--- /dev/null
+++ b/gdb/testsuite/gdb.cp/virtbase2.exp
@@ -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"
diff --git a/gdb/valops.c b/gdb/valops.c
index 9bdbf22..f0a8118 100644
--- a/gdb/valops.c
+++ b/gdb/valops.c
@@ -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);
-- 
1.7.1