[v2,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. A 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, we need to use "vptr" in the virtual class object,
which indexes to its derived class's vtable, to fetch the virtual base
offset. The offset is then added to the virtual class object to access
its member in value_struct_elt_for_reference().

The new function add_virtual_base_offset searches recursively for the
base class along the class hierarchy and returns the virtual base offset
for the virtual class.

Tested on amd64-linux-gnu. No regressions.
---
 gdb/ChangeLog                      |    7 +++++
 gdb/testsuite/ChangeLog            |    6 ++++
 gdb/testsuite/gdb.cp/virtbase2.cc  |   21 +++++++++++++++
 gdb/testsuite/gdb.cp/virtbase2.exp |   50 ++++++++++++++++++++++++++++++++++++
 gdb/valops.c                       |   38 +++++++++++++++++++++++++++
 5 files changed, 122 insertions(+), 0 deletions(-)
 create mode 100644 gdb/testsuite/gdb.cp/virtbase2.cc
 create mode 100644 gdb/testsuite/gdb.cp/virtbase2.exp
  

>>>>> ">" == Weimin Pan <weimin.pan@oracle.com> writes:

>> 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. A simple test case
>> below demonstrates the problem:

Thank you for the patch.

>> diff --git a/gdb/testsuite/gdb.cp/virtbase2.cc b/gdb/testsuite/gdb.cp/virtbase2.cc
>> new file mode 100644
>> index 0000000..4f7631e
>> --- /dev/null
>> +++ b/gdb/testsuite/gdb.cp/virtbase2.cc
>> @@ -0,0 +1,21 @@
>> +struct base { 

New test cases should come with the GPL header.  You can copy one from
some other .cc file in gdb.cp, just make sure to fix the copyright line.

>> index 9bdbf22..754e7d0 100644
>> --- a/gdb/valops.c
>> +++ b/gdb/valops.c
>> @@ -3329,6 +3329,35 @@ 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 and return its offset,
>> +   relative to VT, if it is a virtual base member.  */

The comment says this "return"s the offset, but really it stores the
offset in BOFFS and returns a boolean.  The comment should explain this.

>> +static int
>> +add_virtual_base_offset (struct type *vt, struct type *cls,
>> +                            struct value *v, int &boffs)

I think the function is misnamed, because it doesn't actually add
anything.

Also, gdb style is not to use non-const references for out parameters.
Instead make "boffs" a pointer.

>> +{
>> +  for (int i = 0; i < TYPE_N_BASECLASSES (vt); i++)
>> +    {
>> +      struct type *ftype = TYPE_FIELD_TYPE (vt, i);
>> +      if (check_typedef (ftype) == cls)

You probably want types_equal rather than == here.  The difference is
that two types will not be == if they came from different shared
libraries.  However, they may in fact be "the same" type from the user
point of view.

You don't have to call check_typedef for types_equal.

However I do think you have to call check_typedef at the recursion spot.
I recall there being bugs in the past where gdb didn't check_typedef a
base type and ran into problems.

>> @@ -3393,6 +3422,15 @@ 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)

Probably types_equal here too.

>> +		    {
>> +		      struct value *v2 = value_of_this_silent (current_language);

Hah, I was going to write that this is incorrect, but I see it is in the
code already.

I suppose this makes sense, because this whole code block is about the
implicit this case.  Which seems like the wrong place to put it, but
none of this is your problem.

However, rather than calling value_of_this_silent for a second time,
just save the original value and re-use it.


Sorry about all the nits.  This area is tricky.  In any case the patch
is very close to ready.

Tom
  

Hi Tom,

Thanks very much for the comments.

On 9/7/2018 2:42 PM, Tom Tromey wrote:
>>>>>> ">" == Weimin Pan <weimin.pan@oracle.com> writes:
>>> 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. A simple test case
>>> below demonstrates the problem:
> Thank you for the patch.
>
>>> diff --git a/gdb/testsuite/gdb.cp/virtbase2.cc b/gdb/testsuite/gdb.cp/virtbase2.cc
>>> new file mode 100644
>>> index 0000000..4f7631e
>>> --- /dev/null
>>> +++ b/gdb/testsuite/gdb.cp/virtbase2.cc
>>> @@ -0,0 +1,21 @@
>>> +struct base {
> New test cases should come with the GPL header.  You can copy one from
> some other .cc file in gdb.cp, just make sure to fix the copyright line.

OK, done.

>>> index 9bdbf22..754e7d0 100644
>>> --- a/gdb/valops.c
>>> +++ b/gdb/valops.c
>>> @@ -3329,6 +3329,35 @@ 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 and return its offset,
>>> +   relative to VT, if it is a virtual base member.  */
> The comment says this "return"s the offset, but really it stores the
> offset in BOFFS and returns a boolean.  The comment should explain this.

Done.

>
>>> +static int
>>> +add_virtual_base_offset (struct type *vt, struct type *cls,
>>> +                            struct value *v, int &boffs)
> I think the function is misnamed, because it doesn't actually add
> anything.
>
> Also, gdb style is not to use non-const references for out parameters.
> Instead make "boffs" a pointer.

Done, Simone made the same comment.


>>> +{
>>> +  for (int i = 0; i < TYPE_N_BASECLASSES (vt); i++)
>>> +    {
>>> +      struct type *ftype = TYPE_FIELD_TYPE (vt, i);
>>> +      if (check_typedef (ftype) == cls)
> You probably want types_equal rather than == here.  The difference is
> that two types will not be == if they came from different shared
> libraries.  However, they may in fact be "the same" type from the user
> point of view.
>
> You don't have to call check_typedef for types_equal.
>
> However I do think you have to call check_typedef at the recursion spot.

OK, move check_typedef at the recursive call site and use types_equal 
for type comparison.

> I recall there being bugs in the past where gdb didn't check_typedef a
> base type and ran into problems.
>
>>> @@ -3393,6 +3422,15 @@ 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)
> Probably types_equal here too.

Done.

>
>>> +		    {
>>> +		      struct value *v2 = value_of_this_silent (current_language);
> Hah, I was going to write that this is incorrect, but I see it is in the
> code already.
>
> I suppose this makes sense, because this whole code block is about the
> implicit this case.  Which seems like the wrong place to put it, but
> none of this is your problem.
>
> However, rather than calling value_of_this_silent for a second time,
> just save the original value and re-use it.

Done, introduce local variable "this_v" to save its original value.

>
> Sorry about all the nits.  This area is tricky.  In any case the patch
> is very close to ready.

Currently working on failures from Simon's suggested test case run.

Thanks again,
Weimin

>
> Tom