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

[Weimin Pan <weimin.pan@oracle.com>]

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