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

[Tom Tromey <tom@tromey.com>]

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