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

  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
  

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