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

[Simon Marchi <simon dot marchi at ericsson dot com>]

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