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

[Weimin Pan <weimin dot pan at oracle dot 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