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

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

[Patrick Palka]

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

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

[Patrick Palka]

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.

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

[Patrick Palka]

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.

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

[Yao Qi]

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 :(

> 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).

> +
> +get_debug_format
> +
> +if ![test_debug_format "DWARF 2"] {
> +    return 0
> +}

Why do we need to check test_debug_format here?

-- 
Yao (齐尧)

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

[Patrick Palka]

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 (齐尧)

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

[Yao Qi]

Patrick Palka <patrick@parcs.ath.cx> writes:

>>> +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.

I am not aware of this, any details?  If GDB doesn't support C++ method
pointers with non-DWARF, we should kfail or xfail it in the test.  See
gdb.cp/m-static.exp.

-- 
Yao (齐尧)

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

[Patrick Palka]

On Fri, Nov 21, 2014 at 8:37 AM, Yao Qi <yao@codesourcery.com> wrote:
> Patrick Palka <patrick@parcs.ath.cx> writes:
>
>>>> +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.
>
> I am not aware of this, any details?  If GDB doesn't support C++ method
> pointers with non-DWARF, we should kfail or xfail it in the test.  See
> gdb.cp/m-static.exp.

Err, sorry, I conflated the ABI (gnu-v3) with the debug format
(DWARF). That hunk is unnecessary.

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

[Patrick Palka]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 7406 bytes --]

On Fri, 21 Nov 2014, Yao Qi wrote:

> Patrick Palka <patrick@parcs.ath.cx> writes:
>
>>>> +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.
>
> I am not aware of this, any details?  If GDB doesn't support C++ method
> pointers with non-DWARF, we should kfail or xfail it in the test.  See
> gdb.cp/m-static.exp.
>
> -- 
> Yao (齐尧)
>


Here's an updated version of the patch which removes the
test_debug_format check and also shifts vtable offset on !vbit_in_delta
architectures.  Of course, the internal GDB encoding may still be
inconsistent with the ABI but at least it is self-consistent now.

-- >8 --

Subject: [PATCH] Fix C++ virtual method pointer resolution

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.

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                    |  9 +++++-
  gdb/testsuite/gdb.cp/method-ptr.cc  | 58 +++++++++++++++++++++++++++++++++++++
  gdb/testsuite/gdb.cp/method-ptr.exp | 54 ++++++++++++++++++++++++++++++++++
  3 files changed, 120 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 d673e77..5775d85 100644
--- a/gdb/gnu-v3-abi.c
+++ b/gdb/gnu-v3-abi.c
@@ -682,11 +682,18 @@ 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
      {
+      if (is_virtual != 0)
+	value = value * TYPE_LENGTH (vtable_ptrdiff_type (gdbarch));
        store_unsigned_integer (contents, size, byte_order, value);
        store_unsigned_integer (contents + size, size, byte_order, is_virtual);
      }
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..cd3d8ff
--- /dev/null
+++ b/gdb/testsuite/gdb.cp/method-ptr.exp
@@ -0,0 +1,54 @@
+# 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
+}
+
+# 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.2.0.rc1.23.gf570943