Re: [PATCH 1/2] Fix C++ virtual method pointer resolution

[Patrick Palka <patrick at parcs dot ath dot cx>]

On Fri, Nov 21, 2014 at 6:35 AM, Yao Qi <yao@codesourcery.com> wrote:
> Patrick Palka <patrick@parcs.ath.cx> writes:
>
> Hi,
> Sorry for the delayed review...
>
>> The issue lies in the initial creation of the virtual method pointer as
>> seen by GDB.  In gnuv3_make_method_ptr() we fail to shift the vtable
>> offset when storing the first word of a virtual method pointer.  This is
>> important because functions that read this first word (namely the
>> callers of gnuv3_decode_method_ptr()) expect that the vtable offset is a
>> multiple of "sizeof (vtable_ptrdiff_t)".  Also it ensures that the vbit
>> tag does not collide with the bits used to store the actual offset.
>>
>> So when writing the virtual method pointer contents we need to shift the
>> vtable offset so as to be in symmetry with what the readers of the
>> vtable offset do -- which is, xor the vbit tag and then shift back the
>> offset.  (The prominent readers of the vtable offset are
>> gnuv3_print_method_ptr() and gnuv3_method_ptr_to_value().)
>
> I am not familiar with how gdb handle c++ virtual method, but your
> analysis looks right to me.  I spend the whole day reading c++ abi, but
> still don't know how to connect the abi with the code here :(

It is most likely the case that the GDB encoding is totally not in
line with the C++ ABI. I don't think GDB has any tests that check this
(e.g. by calling a compiled function that takes a member pointer).

>
>> diff --git a/gdb/gnu-v3-abi.c b/gdb/gnu-v3-abi.c
>> index d5ed355..ccb0be6 100644
>> --- a/gdb/gnu-v3-abi.c
>> +++ b/gdb/gnu-v3-abi.c
>> @@ -683,7 +683,12 @@ gnuv3_make_method_ptr (struct type *type, gdb_byte *contents,
>>
>>    if (!gdbarch_vbit_in_delta (gdbarch))
>>      {
>> -      store_unsigned_integer (contents, size, byte_order, value | is_virtual);
>> +      if (is_virtual != 0)
>> +     {
>> +       value = value * TYPE_LENGTH (vtable_ptrdiff_type (gdbarch));
>
> We need to hoist this shift out of "if" block, so that the path goes to
> "else" branch can be covered too.  Otherwise, fails in
> gdb.cp/method-ptr.exp can't be fixed on arm-linux target (on which
> vbit_in_delta is zero).

OK. This will at least make GDB consistent with its idea of the
encoding of a member pointer, but it will still probably won't be in
line with the ABI.

>
>> +
>> +get_debug_format
>> +
>> +if ![test_debug_format "DWARF 2"] {
>> +    return 0
>> +}
>
> Why do we need to check test_debug_format here?

Because GDB only supports C++ method pointers with the DWARF debug
format. So I'd assume that this test would fail for non-DWARF.

>
> --
> Yao (éå)