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

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

Hi Simon,

Thanks very much for your comments.


On 9/7/2018 2:26 PM, Simon Marchi wrote:
> Hi Weimin,
>
> It seems like the typedef is not really a factor here.  The bug is present even
> if you remove the typedef from you test case.  Could you update the title of the
> patch to reflect this?  Your patch does not need to have the same title as the
> PR that motivated it.
>
> Testing with a typedef is a good idea though.
>
> Your patch still has trailing whitespaces.  Try to do:
>
> $ git format-patch HEAD^
> $ git am 0001-virtual-inheritance-via-typedef-cannot-find-base.patch
>
> and make sure you have no message other than
>
>    Applying: virtual inheritance via typedef cannot find base

It's nice to know this "git am" command. Do I use the "git am --abort" 
command to
restore my original patch, correct problems, then use another "git am" 
command?

>> diff --git a/gdb/testsuite/gdb.cp/virtbase2.exp b/gdb/testsuite/gdb.cp/virtbase2.exp
>> new file mode 100644
>> index 0000000..c29ff1c
>> --- /dev/null
>> +++ b/gdb/testsuite/gdb.cp/virtbase2.exp
>> @@ -0,0 +1,50 @@
>> +# Copyright 2018 Free Software Foundation, Inc.
>> +
>> +# This program is free software; you can redistribute it and/or modify
>> +# it under the terms of the GNU General Public License as published by
>> +# the Free Software Foundation; either version 3 of the License, or
>> +# (at your option) any later version.
>> +#
>> +# This program is distributed in the hope that it will be useful,
>> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> +# GNU General Public License for more details.
>> +#
>> +# You should have received a copy of the GNU General Public License
>> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
>> +
>> +# Make sure printing virtual base class data member correctly (PR16841)
> It sounds like it's missing a word... "works correctly"?

Word "works" has been added.

>> +
>> +if { [skip_cplus_tests] } { continue }
>> +
>> +standard_testfile .cc
>> +
>> +if {[prepare_for_testing "failed to prepare" $testfile $srcfile {debug c++}]} {
>> +    return -1
>> +}
>> +
>> +if {![runto_main]} then {
>> +    perror "couldn't run to main"
>> +    continue
>> +}
>> +
>> +gdb_breakpoint "derived::func_d"
>> +gdb_continue_to_breakpoint "continue to derived::func_d"
>> +gdb_test "print i" " = 55" "i in base class"
>> +gdb_test "print derived::i" " = 55" "i in base class"
>> +gdb_test "print derived::base::i" " = 55" "i in base class"
>> +gdb_test "print base::i" " = 55" "i in base class"
>> +gdb_test "print d" " = 6.5999999999999996" "d in base class"
>> +gdb_test "print derived::d" " = 6.5999999999999996" "d in base class"
>> +gdb_test "print derived::base::d" " = 6.5999999999999996" "d in base class"
>> +gdb_test "print base::d" " = 6.5999999999999996" "d in base class"
>> +gdb_breakpoint "foo::func_f"
>> +gdb_continue_to_breakpoint "continue to foo::func_f"
>> +gdb_test "print i" " = 55" "i in base class"
>> +gdb_test "print derived::i" " = 55" "i in base class"
>> +gdb_test "print derived::base::i" " = 55" "i in base class"
>> +gdb_test "print base::i" " = 55" "i in base class"
>> +gdb_test "print d" " = 6.5999999999999996" "d in base class"
>> +gdb_test "print derived::d" " = 6.5999999999999996" "d in base class"
>> +gdb_test "print derived::base::d" " = 6.5999999999999996" "d in base class"
>> +gdb_test "print base::d" " = 6.5999999999999996" "d in base class"
> Make sure test names are unique:
> https://sourceware.org/gdb/wiki/GDBTestcaseCookbook#Make_sure_test_messages_are_unique
>
> In these cases, I think you can omit the test names, gdb_test will default
> to use the command as the test name.

I'm going to use the new virtbase2.exp that you suggested in your 
separate email which, exercise more
possible combinations of scopes as you described below, and reveal more 
test failures (: Looking into
these problems now.

> I think I found another problematic case.  If you modify your test like this:
>
> diff --git a/gdb/testsuite/gdb.cp/virtbase2.cc b/gdb/testsuite/gdb.cp/virtbase2.cc
> index 4f7631e..4620ef5 100644
> --- a/gdb/testsuite/gdb.cp/virtbase2.cc
> +++ b/gdb/testsuite/gdb.cp/virtbase2.cc
> @@ -1,4 +1,9 @@
> -struct base {
> +struct superbase {
> +  int x;
> +  superbase () : x(22) {}
> +};
> +
> +struct base : superbase {
>     int i; double d;
>     base() : i(55), d(6.6) {}
>   };
>
>
> ... and try to print the x field with "print derived::x", you get a wrong value.

Yes, "print derived::x" shows a wrong value while "print x" and "print 
derived::base::x" work correctly.

> It would be nice to have the test case generate all possible combinations of scopes.
> For example, with
>
> struct base { int x; }
> struct derived : base {}
>
> We should test with
>
>    - x
>    - base::x
>    - derived::x
>    - derived::base::x
>
>> diff --git a/gdb/valops.c b/gdb/valops.c
>> 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.  */
> Can you describe the return value and the parameter V?

Will do. Tom also has pointed this out.

> +
> +static int
> Return bool.
>
>> +add_virtual_base_offset (struct type *vt, struct type *cls,
>> +                            struct value *v, int &boffs)
> I suggest naming this function find_virtual_base_offset or get_virtual_base_offset,
> since it doesn't do the actual adding.

Will change the function name to get_virtual_base_offset. Tom made a similar comment.

> The last line is too much indented.

Fixed.

>
> Please make boffs a pointer:

Done, Tom also suggested it.

>
> https://sourceware.org/gdb/wiki/Internals%20GDB-C-Coding-Standards#Avoid_non-const_reference_parameters.2C_use_pointers_instead
>
> The function comment should mention that the offset is returned in *BOFFS.

Done, Tom 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)
>> +        {
>> +          if (BASETYPE_VIA_VIRTUAL (vt, i))
>> +            {
>> +              const gdb_byte *adr = value_contents_for_printing (v);
>> +              boffs = baseclass_offset (vt, i, adr, value_offset (v),
>> +                                        value_as_long (v), v);
>> +            }
>> +          return true;
>> +        }
>> +
>> +      if (add_virtual_base_offset (ftype, cls, v, boffs))
>> +        return true;
>> +    }
>> +
>> +  return false;
>> +}
>> +
>>   /* C++: Given an aggregate type CURTYPE, and a member name NAME,
>>      return the address of this member as a "pointer to member" type.
>>      If INTYPE is non-null, then it will be the type of the member we
>> @@ -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)
>> +		    {
> Can you add a small comment about what this block of code is doing?

Will do.

Thanks again,
Weimin

>
> Simon