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

Patrick Palka
2014-11-16T03:57:19Z

On Tue, Sep 30, 2014 at 5:48 PM, Patrick Palka <patrick@parcs.ath.cx> wrote:
> On Sun, Sep 21, 2014 at 11:07 PM, Patrick Palka <patrick@parcs.ath.cx> wrote:
>> C++ virtual method pointer resolution is currently broken, in that a
>> virtual method pointer will sometimes resolve to the wrong symbol. A
>> minimal testcase is the following program:
>>
>> struct x
>> {
>> virtual void f ();
>> virtual void g ();
>> }
>>
>> void x::f () { }
>> void x::g () { }
>>
>> (gdb) print &x::f
>> $1 = &virtual x::f()
>> (gdb) print &x::g
>> $1 = &virtual x::f()
>>
>> As you can see, &x::f correctly resolves to x::f(), but &x::g
>> incorrectly resolves to x::f() instead of x::g().
>>
>> 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().)
>>
>> gdb/ChangeLog
>> * gnu-v3-abi.c (gnuv3_make_method_ptr): Shift the vtable offset
>> before setting the virtual bit.
>>
>> gdb/testsuite/ChangeLog
>> * gdb.cp/method-ptr.exp: New test.
>> * gdb.cp/method-ptr.cc: New testcase.
>>
>> Tested on x86_64-unknown-linux-gnu.
>> ---
>> gdb/gnu-v3-abi.c | 7 ++++-
>> gdb/testsuite/gdb.cp/method-ptr.cc | 58 +++++++++++++++++++++++++++++++++++
>> gdb/testsuite/gdb.cp/method-ptr.exp | 60 +++++++++++++++++++++++++++++++++++++
>> 3 files changed, 124 insertions(+), 1 deletion(-)
>> create mode 100644 gdb/testsuite/gdb.cp/method-ptr.cc
>> create mode 100644 gdb/testsuite/gdb.cp/method-ptr.exp
>>
>> 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));
>> + value = value | 1;
>> + }
>> + store_unsigned_integer (contents, size, byte_order, value);
>> store_unsigned_integer (contents + size, size, byte_order, 0);
>> }
>> else
>> diff --git a/gdb/testsuite/gdb.cp/method-ptr.cc b/gdb/testsuite/gdb.cp/method-ptr.cc
>> new file mode 100644
>> index 0000000..db47484
>> --- /dev/null
>> +++ b/gdb/testsuite/gdb.cp/method-ptr.cc
>> @@ -0,0 +1,58 @@
>> +/* This testcase is part of GDB, the GNU debugger.
>> +
>> + Copyright 2014 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/>. */
>> +
>> +struct x
>> +{
>> + virtual void f ();
>> + virtual void g ();
>> + virtual void h ();
>> +};
>> +
>> +void x::f () { }
>> +void x::g () { }
>> +void x::h () { }
>> +
>> +struct y : x
>> +{
>> + virtual void f ();
>> +
>> + virtual void j ();
>> + virtual void k ();
>> +};
>> +
>> +void y::f () { }
>> +void y::j () { }
>> +void y::k () { }
>> +
>> +struct z : y
>> +{
>> + virtual void g ();
>> + virtual void j ();
>> +
>> + virtual void l ();
>> + virtual void m ();
>> +};
>> +
>> +void z::g () { }
>> +void z::j () { }
>> +void z::l () { }
>> +void z::m () { }
>> +
>> +int
>> +main ()
>> +{
>> +}
>> diff --git a/gdb/testsuite/gdb.cp/method-ptr.exp b/gdb/testsuite/gdb.cp/method-ptr.exp
>> new file mode 100644
>> index 0000000..732b861
>> --- /dev/null
>> +++ b/gdb/testsuite/gdb.cp/method-ptr.exp
>> @@ -0,0 +1,60 @@
>> +# Copyright 2014 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/>.
>> +
>> +# This file is part of the gdb testsuite
>> +
>> +if [skip_cplus_tests] { continue }
>> +
>> +standard_testfile .cc
>> +
>> +if [prepare_for_testing $testfile.exp $testfile $srcfile {debug c++}] {
>> + return -1
>> +}
>> +
>> +if ![runto_main] {
>> + return -1
>> +}
>> +
>> +get_debug_format
>> +
>> +if ![test_debug_format "DWARF 2"] {
>> + return 0
>> +}
>> +
>> +# Check that the virtual method pointer NAME resolves to symbol SYMBOL.
>> +proc check_virtual_method_ptr_resolution { name symbol } {
>> + global decimal
>> +
>> + # Printing the expression &NAME should show the resolved symbol SYMBOL.
>> + gdb_test "print &$name" "\\$$decimal = &virtual $symbol\\(\\)\\s"
>> +}
>> +
>> +check_virtual_method_ptr_resolution "x::f" "x::f"
>> +check_virtual_method_ptr_resolution "x::g" "x::g"
>> +check_virtual_method_ptr_resolution "x::h" "x::h"
>> +
>> +check_virtual_method_ptr_resolution "y::f" "y::f"
>> +check_virtual_method_ptr_resolution "y::g" "x::g"
>> +check_virtual_method_ptr_resolution "y::h" "x::h"
>> +check_virtual_method_ptr_resolution "y::j" "y::j"
>> +check_virtual_method_ptr_resolution "y::k" "y::k"
>> +
>> +check_virtual_method_ptr_resolution "z::f" "y::f"
>> +check_virtual_method_ptr_resolution "z::g" "z::g"
>> +check_virtual_method_ptr_resolution "z::h" "x::h"
>> +check_virtual_method_ptr_resolution "z::j" "z::j"
>> +check_virtual_method_ptr_resolution "z::k" "y::k"
>> +check_virtual_method_ptr_resolution "z::l" "z::l"
>> +check_virtual_method_ptr_resolution "z::m" "z::m"
>> --
>> 2.1.1.273.g97b8860
>>
>
> Ping.

Ping.