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

Message ID 1536800471-78975-1-git-send-email-weimin.pan@oracle.com
State New, archived
Headers

Commit Message

Weimin Pan Sept. 13, 2018, 1:01 a.m. UTC
  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, add function get_baseclass_offset() which searches
recursively for the base class along the class hierarchy. If the base
is virtual, it uses "vptr" in virtual class object, which indexes to
its derived class's vtable, to get and returns the baseclass offset.
If the base is non-virtual, it returns the accumulated offset of its
parent classes. The offset is then added to the address of the class
object to access its member in value_struct_elt_for_reference().

Tested on amd64-linux-gnu. No regressions.
---
 gdb/ChangeLog                      |    7 +++
 gdb/testsuite/ChangeLog            |    6 +++
 gdb/testsuite/gdb.cp/virtbase2.cc  |   46 +++++++++++++++++++++
 gdb/testsuite/gdb.cp/virtbase2.exp |   78 ++++++++++++++++++++++++++++++++++++
 gdb/valops.c                       |   63 ++++++++++++++++++++++++++++-
 5 files changed, 199 insertions(+), 1 deletions(-)
 create mode 100644 gdb/testsuite/gdb.cp/virtbase2.cc
 create mode 100644 gdb/testsuite/gdb.cp/virtbase2.exp
  

Comments

Simon Marchi Sept. 14, 2018, 6:26 p.m. UTC | #1
On 2018-09-12 09:01 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, add function get_baseclass_offset() which searches
> recursively for the base class along the class hierarchy. If the base
> is virtual, it uses "vptr" in virtual class object, which indexes to
> its derived class's vtable, to get and returns the baseclass offset.
> If the base is non-virtual, it returns the accumulated offset of its
> parent classes. The offset is then added to the address of the class
> object to access its member in value_struct_elt_for_reference().
> 
> Tested on amd64-linux-gnu. No regressions.

I have some last nits about the test, but I'd like Tom to give the final approval,
since he clearly knows this stuff better than I do.  If everything is fine with him,
then there is no need to post a new version, just fix it up and push.

> ---
>  gdb/ChangeLog                      |    7 +++
>  gdb/testsuite/ChangeLog            |    6 +++
>  gdb/testsuite/gdb.cp/virtbase2.cc  |   46 +++++++++++++++++++++
>  gdb/testsuite/gdb.cp/virtbase2.exp |   78 ++++++++++++++++++++++++++++++++++++
>  gdb/valops.c                       |   63 ++++++++++++++++++++++++++++-
>  5 files changed, 199 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..9cbd8ca 100644
> --- a/gdb/ChangeLog
> +++ b/gdb/ChangeLog
> @@ -1,3 +1,10 @@
> +2018-09-12  Weimin Pan  <weimin.pan@oracle.com>
> +
> +	PR gdb/16841
> +	* valops.c (get_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..46f16d7 100644
> --- a/gdb/testsuite/ChangeLog
> +++ b/gdb/testsuite/ChangeLog
> @@ -1,3 +1,9 @@
> +2018-09-12  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..60c95c4
> --- /dev/null
> +++ b/gdb/testsuite/gdb.cp/virtbase2.cc
> @@ -0,0 +1,46 @@
> +/* 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/>.  */
> +
> +struct superbase {
> +  int x;
> +  superbase () : x(22) {}
> +};
> +
> +struct base : superbase {
> +  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();
> +}
> +
> +

Remove the trailing empty lines here, git shows a warning when applying the patch:

Applying: virtual inheritance via typedef cannot find base
Using index info to reconstruct a base tree...
M       gdb/ChangeLog
M       gdb/testsuite/ChangeLog
M       gdb/valops.c
.git/rebase-apply/patch:90: new blank line at EOF.
+
warning: 1 line adds whitespace errors.

> diff --git a/gdb/testsuite/gdb.cp/virtbase2.exp b/gdb/testsuite/gdb.cp/virtbase2.exp
> new file mode 100644
> index 0000000..4c14419
> --- /dev/null
> +++ b/gdb/testsuite/gdb.cp/virtbase2.exp
> @@ -0,0 +1,78 @@
> +# 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 works 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
> +}
> +> +proc make_scope_list { scopes } {

Please add a comment for this proc, since it's not very intuitive.  Here's
a suggestion:

# From a list of nested scopes, generate all possible ways of accessing something
# in those scopes.  For example, with the argument {foo bar baz}, this proc will
# return:
#  - {} (empty string)
#  - baz::
#  - bar::
#  - bar::baz::
#  - foo::
#  - foo::baz::
#  - foo::bar::
#  - foo::bar::baz::

Thanks!

Simon
  
Weimin Pan Sept. 14, 2018, 9:48 p.m. UTC | #2
On 9/14/2018 11:26 AM, Simon Marchi wrote:
> On 2018-09-12 09:01 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, add function get_baseclass_offset() which searches
>> recursively for the base class along the class hierarchy. If the base
>> is virtual, it uses "vptr" in virtual class object, which indexes to
>> its derived class's vtable, to get and returns the baseclass offset.
>> If the base is non-virtual, it returns the accumulated offset of its
>> parent classes. The offset is then added to the address of the class
>> object to access its member in value_struct_elt_for_reference().
>>
>> Tested on amd64-linux-gnu. No regressions.
> I have some last nits about the test, but I'd like Tom to give the final approval,
> since he clearly knows this stuff better than I do.  If everything is fine with him,
> then there is no need to post a new version, just fix it up and push.
>
>> ---
>>   gdb/ChangeLog                      |    7 +++
>>   gdb/testsuite/ChangeLog            |    6 +++
>>   gdb/testsuite/gdb.cp/virtbase2.cc  |   46 +++++++++++++++++++++
>>   gdb/testsuite/gdb.cp/virtbase2.exp |   78 ++++++++++++++++++++++++++++++++++++
>>   gdb/valops.c                       |   63 ++++++++++++++++++++++++++++-
>>   5 files changed, 199 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..9cbd8ca 100644
>> --- a/gdb/ChangeLog
>> +++ b/gdb/ChangeLog
>> @@ -1,3 +1,10 @@
>> +2018-09-12  Weimin Pan  <weimin.pan@oracle.com>
>> +
>> +	PR gdb/16841
>> +	* valops.c (get_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..46f16d7 100644
>> --- a/gdb/testsuite/ChangeLog
>> +++ b/gdb/testsuite/ChangeLog
>> @@ -1,3 +1,9 @@
>> +2018-09-12  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..60c95c4
>> --- /dev/null
>> +++ b/gdb/testsuite/gdb.cp/virtbase2.cc
>> @@ -0,0 +1,46 @@
>> +/* 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/>.  */
>> +
>> +struct superbase {
>> +  int x;
>> +  superbase () : x(22) {}
>> +};
>> +
>> +struct base : superbase {
>> +  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();
>> +}
>> +
>> +
> Remove the trailing empty lines here, git shows a warning when applying the patch:
>
> Applying: virtual inheritance via typedef cannot find base
> Using index info to reconstruct a base tree...
> M       gdb/ChangeLog
> M       gdb/testsuite/ChangeLog
> M       gdb/valops.c
> .git/rebase-apply/patch:90: new blank line at EOF.
> +
> warning: 1 line adds whitespace errors.
>
>> diff --git a/gdb/testsuite/gdb.cp/virtbase2.exp b/gdb/testsuite/gdb.cp/virtbase2.exp
>> new file mode 100644
>> index 0000000..4c14419
>> --- /dev/null
>> +++ b/gdb/testsuite/gdb.cp/virtbase2.exp
>> @@ -0,0 +1,78 @@
>> +# 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 works 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
>> +}
>> +> +proc make_scope_list { scopes } {
> Please add a comment for this proc, since it's not very intuitive.  Here's
> a suggestion:
>
> # From a list of nested scopes, generate all possible ways of accessing something
> # in those scopes.  For example, with the argument {foo bar baz}, this proc will
> # return:
> #  - {} (empty string)
> #  - baz::
> #  - bar::
> #  - bar::baz::
> #  - foo::
> #  - foo::baz::
> #  - foo::bar::
> #  - foo::bar::baz::
>
> Thanks!
>
> Simon

Hi Simon,

OK, Thanks for your comments. I've incorporated them into the patch.

Weimin
  
Tom Tromey Sept. 18, 2018, 11:59 a.m. UTC | #3
>>>>> ">" == Weimin Pan <weimin.pan@oracle.com> writes:

>> +2018-09-12  Weimin Pan  <weimin.pan@oracle.com>
>> +
>> +	PR gdb/16841

This should read "PR c++/16841".  The robot doesn't care but I suppose
it's a bit clearer for humans.

>> +	PR gdb/16841

Here too.

>> +        gdb_test "print ${scope}d" " = 6.5999999999999996"

It's best with floats to use a number that is exact.
So, instead of 6.6, 6.25 or something like that.

The code has a loop to deal with multiple inheritance but the test
doesn't exercise this case.  I think an additional for that would be
good.

>> +		      if (get_baseclass_offset (domain, curtype, v, &boff,
>> +						&isvirt))
>> +		        mem_offset += boff;
>> +		      else
>> +		        {
>> +		          struct type *t = check_typedef (value_type (this_v));
>> +		          t = check_typedef (TYPE_TARGET_TYPE (t));
>> +		          if (get_baseclass_offset (t, curtype, this_v,
>> +						    &boff, &isvirt))
>> +		            mem_offset += boff;

Could you explain this "else" block?  I did not understand the reason
for it.  I suppose it could use a comment.

thanks,
Tom
  
Weimin Pan Sept. 18, 2018, 11:25 p.m. UTC | #4
On 9/18/2018 4:59 AM, Tom Tromey wrote:
>>>>>> ">" == Weimin Pan <weimin.pan@oracle.com> writes:
>>> +2018-09-12  Weimin Pan  <weimin.pan@oracle.com>
>>> +
>>> +	PR gdb/16841
> This should read "PR c++/16841".  The robot doesn't care but I suppose
> it's a bit clearer for humans.

Done.

>>> +	PR gdb/16841
> Here too.

Done.

>>> +        gdb_test "print ${scope}d" " = 6.5999999999999996"
> It's best with floats to use a number that is exact.
> So, instead of 6.6, 6.25 or something like that.

Using 6.6 will result in an inexact number as well. Will change it to 
"6.25".

> The code has a loop to deal with multiple inheritance but the test
> doesn't exercise this case.  I think an additional for that would be
> good.

Sorry, which case is not exercised? Could it be printing superbase class 
member in base?

>>> +		      if (get_baseclass_offset (domain, curtype, v, &boff,
>>> +						&isvirt))
>>> +		        mem_offset += boff;
>>> +		      else
>>> +		        {
>>> +		          struct type *t = check_typedef (value_type (this_v));
>>> +		          t = check_typedef (TYPE_TARGET_TYPE (t));
>>> +		          if (get_baseclass_offset (t, curtype, this_v,
>>> +						    &boff, &isvirt))
>>> +		            mem_offset += boff;
> Could you explain this "else" block?  I did not understand the reason
> for it.  I suppose it could use a comment.

There is a brief comment:

                     /* Find class offset of type CURTYPE from either its
                          parent type DOMAIN or the type of implied 
this.  */

i.e. trying to find the class offset for CURTYPE first from its parent 
type DOMAIN.
If failed,  try to find it from the type of the implied this.

Thanks for your comments.

Weimin

>
> thanks,
> Tom
  
Tom Tromey Oct. 4, 2018, 10:19 p.m. UTC | #5
Sorry, I forgot about this.

Tom> The code has a loop to deal with multiple inheritance but the test
Tom> doesn't exercise this case.  I think an additional for that would be
Tom> good.

Weimin> Sorry, which case is not exercised? Could it be printing superbase
Weimin> class member in base?

There's this loop in get_baseclass_offset:

+  for (int i = 0; i < TYPE_N_BASECLASSES (vt); i++)

However, all the classes in virtbase2.cc use single inheritance, so when
testing, the loop only loops once.  I think it would be good to test a
multiple inheritance case.

Tom
  
Weimin Pan Oct. 4, 2018, 11:51 p.m. UTC | #6
On 10/4/2018 3:19 PM, Tom Tromey wrote:
> multiple inheritance case

Thank you for the clarification. I have expanded both virtbase2.cc
and virtbase2.exp (attached) to test the multiple inheritance case.
/* 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/>.  */

struct super {
  int w;
  super () : w(17) {}
};

struct superbase {
  int x;
  superbase () : x(22) {}
};

struct base : superbase {
  int i;
  double d;
  base() : i(55), d(6.25) {}
};

typedef base tbase;
struct derived: virtual super, virtual tbase
{
  void func_d() { }
};

struct foo: virtual derived
{
  void func_f() { }
};

int main()
{
  derived().func_d();
  foo().func_f();
}
# 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 works 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
}

# From a list of nested scopes, generate all possible ways of accessing something
# in those scopes.  For example, with the argument {foo bar baz}, this proc will
# return:
#  - {} (empty string)
#  - baz::
#  - bar::
#  - bar::baz::
#  - foo::
#  - foo::baz::
#  - foo::bar::
#  - foo::bar::baz::

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.25"
        gdb_test "print ${scope}x" " = 22"
    }
}

proc test_variables_in_superbase { scopes } {
    foreach scope [make_scope_list $scopes] {
        gdb_test "print ${scope}x" " = 22"
    }
}

proc test_variables_in_super { scopes } {
    foreach scope [make_scope_list $scopes] {
        gdb_test "print ${scope}w" " = 17"
    }
}

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}
    test_variables_in_superbase {base superbase}
    test_variables_in_superbase {derived superbase}
    test_variables_in_superbase {superbase}
    test_variables_in_superbase {base}
    test_variables_in_super {super}
    test_variables_in_super {derived super}
}

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_base {foo base}
    test_variables_in_base {base}
    test_variables_in_superbase {superbase}
    test_variables_in_superbase {foo superbase}
    test_variables_in_superbase {foo derived superbase}
    test_variables_in_superbase {foo derived base superbase}
    test_variables_in_super {super}
    test_variables_in_super {foo super}
    test_variables_in_super {foo derived super}
}
  
Tom Tromey Oct. 5, 2018, 2:02 p.m. UTC | #7
>>>>> ">" == Wei-min Pan <weimin.pan@oracle.com> writes:

>> Thank you for the clarification. I have expanded both virtbase2.cc
>> and virtbase2.exp (attached) to test the multiple inheritance case.

Thank you for persevering.  Please check in the patch.

Tom
  

Patch

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index c47c111..9cbd8ca 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,10 @@ 
+2018-09-12  Weimin Pan  <weimin.pan@oracle.com>
+
+	PR gdb/16841
+	* valops.c (get_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..46f16d7 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,9 @@ 
+2018-09-12  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..60c95c4
--- /dev/null
+++ b/gdb/testsuite/gdb.cp/virtbase2.cc
@@ -0,0 +1,46 @@ 
+/* 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/>.  */
+
+struct superbase {
+  int x;
+  superbase () : x(22) {}
+};
+
+struct base : superbase {
+  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..4c14419
--- /dev/null
+++ b/gdb/testsuite/gdb.cp/virtbase2.exp
@@ -0,0 +1,78 @@ 
+# 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 works 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
+}
+
+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}
+}
diff --git a/gdb/valops.c b/gdb/valops.c
index 9bdbf22..ce39330 100644
--- a/gdb/valops.c
+++ b/gdb/valops.c
@@ -3329,6 +3329,49 @@  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 using value V; If found, store the offset
+   which is either fetched from the virtual base pointer if CLS
+   is virtual or accumulated offset of its parent classes if
+   CLS is non-virtual in *BOFFS, set ISVIRT to indicate if CLS
+   is virtual, and return true.  If not found, return false.  */
+
+static bool
+get_baseclass_offset (struct type *vt, struct type *cls,
+		      struct value *v, int *boffs, bool *isvirt)
+{
+  for (int i = 0; i < TYPE_N_BASECLASSES (vt); i++)
+    {
+      struct type *t = TYPE_FIELD_TYPE (vt, i);
+      if (types_equal (t, 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);
+	      *isvirt = true;
+            }
+          else
+	    *isvirt = false;
+          return true;
+        }
+
+      if (get_baseclass_offset (check_typedef (t), cls, v, boffs, isvirt))
+        {
+	  if (*isvirt == false)	/* Add non-virtual base offset.  */
+	    {
+	      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;
+	}
+    }
+
+  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
@@ -3382,7 +3425,7 @@  value_struct_elt_for_reference (struct type *domain, int offset,
 	      v = value_of_this_silent (current_language);
 	      if (v != NULL)
 		{
-		  struct value *ptr;
+		  struct value *ptr, *this_v = v;
 		  long mem_offset;
 		  struct type *type, *tmp;
 
@@ -3393,6 +3436,24 @@  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)
+		    {
+		      /* Find class offset of type CURTYPE from either its
+			 parent type DOMAIN or the type of implied this.  */
+		      int boff = 0;
+		      bool isvirt = false;
+		      if (get_baseclass_offset (domain, curtype, v, &boff,
+						&isvirt))
+		        mem_offset += boff;
+		      else
+		        {
+		          struct type *t = check_typedef (value_type (this_v));
+		          t = check_typedef (TYPE_TARGET_TYPE (t));
+		          if (get_baseclass_offset (t, curtype, this_v,
+						    &boff, &isvirt))
+		            mem_offset += boff;
+		        }
+		    }
 		  tmp = lookup_pointer_type (TYPE_TARGET_TYPE (type));
 		  result = value_from_pointer (tmp,
 					       value_as_long (v) + mem_offset);