diff mbox

Fix watching structs in C++

Message ID m34lm2paqs.fsf@oc1027705133.ibm.com
State New, archived
Headers

Commit Message

Andreas Arnez Feb. 27, 2018, 6:07 p.m. UTC 
  Some of the watchpoint logic depends on the fact that the head of the
value chain represents the user-specified value to watch.  Thus no
additional values should be added to the value chain after that.  However,
this may happen in gnuv3_rrti_type, where value_addr is invoked.  If no
RTTI is found, then the pointer value built by value_addr, rather than the
original value, stays in front of the value chain.

With such a "polluted" value chain the watchpoint logic does not recognize
when the user intended to watch a struct, and can_use_hardware_watchpoint
returns zero.  Instead of a hardware watchpoint, a software watchpoint
will then be set for no apparent reason.

This is fixed by adding an early exit to gnuv3_rtti_type when the input
value is not a dynamic class object.

gdb/testsuite/ChangeLog:

	* gdb.cp/watch-cp.cc: New test.
	* gdb.cp/watch-cp.exp: New file.

gdb/ChangeLog:

	* gnu-v3-abi.c (gnuv3_rtti_type): Add early exit if the given
	value is not a dynamic class object.
---
 gdb/gnu-v3-abi.c                  |  5 +++--
 gdb/testsuite/gdb.cp/watch-cp.cc  | 28 ++++++++++++++++++++++++++++
 gdb/testsuite/gdb.cp/watch-cp.exp | 35 +++++++++++++++++++++++++++++++++++
 3 files changed, 66 insertions(+), 2 deletions(-)
 create mode 100644 gdb/testsuite/gdb.cp/watch-cp.cc
 create mode 100644 gdb/testsuite/gdb.cp/watch-cp.exp

Comments

Andreas Arnez March 5, 2018, 12:43 p.m. UTC | #1 
Ping:

https://sourceware.org/ml/gdb-patches/2018-02/msg00428.html

On Tue, Feb 27 2018, Andreas Arnez wrote:

> Some of the watchpoint logic depends on the fact that the head of the
> value chain represents the user-specified value to watch.  Thus no
> additional values should be added to the value chain after that.  However,
> this may happen in gnuv3_rrti_type, where value_addr is invoked.  If no
> RTTI is found, then the pointer value built by value_addr, rather than the
> original value, stays in front of the value chain.
>
> With such a "polluted" value chain the watchpoint logic does not recognize
> when the user intended to watch a struct, and can_use_hardware_watchpoint
> returns zero.  Instead of a hardware watchpoint, a software watchpoint
> will then be set for no apparent reason.
>
> This is fixed by adding an early exit to gnuv3_rtti_type when the input
> value is not a dynamic class object.
>
> gdb/testsuite/ChangeLog:
>
> 	* gdb.cp/watch-cp.cc: New test.
> 	* gdb.cp/watch-cp.exp: New file.
>
> gdb/ChangeLog:
>
> 	* gnu-v3-abi.c (gnuv3_rtti_type): Add early exit if the given
> 	value is not a dynamic class object.
Yao Qi March 5, 2018, 1:46 p.m. UTC | #2 
Andreas Arnez <arnez@linux.vnet.ibm.com> writes:

Hi Andreas,

> Some of the watchpoint logic depends on the fact that the head of the
> value chain represents the user-specified value to watch.  Thus no
> additional values should be added to the value chain after that.  However,
> this may happen in gnuv3_rrti_type, where value_addr is invoked.  If no
> RTTI is found, then the pointer value built by value_addr, rather than the
> original value, stays in front of the value chain.

I sort of understand the value chain in watchpoint expressions, but it
is still a big gap from watchpoint value chain to the function you
changed in the patch.

Could you please add more information in commit log? especially about
how value chain is related to gnuv3_rrti_type.
Andreas Arnez March 5, 2018, 5:32 p.m. UTC | #3 
On Mon, Mar 05 2018, Yao Qi wrote:

> Andreas Arnez <arnez@linux.vnet.ibm.com> writes:
>
> Hi Andreas,
>
>> Some of the watchpoint logic depends on the fact that the head of the
>> value chain represents the user-specified value to watch.  Thus no
>> additional values should be added to the value chain after that.  However,
>> this may happen in gnuv3_rrti_type, where value_addr is invoked.  If no
>> RTTI is found, then the pointer value built by value_addr, rather than the
>> original value, stays in front of the value chain.
>
> I sort of understand the value chain in watchpoint expressions, but it
> is still a big gap from watchpoint value chain to the function you
> changed in the patch.
>
> Could you please add more information in commit log? especially about
> how value chain is related to gnuv3_rrti_type.

OK, how about this? --

... However, if a watchpoint is defined for a C++ structure/class
object, then run-time type information (RTTI) may be present.  Thus,
while constructing the value chain for the watchpoint, the dynamic type
is fetched by gnuv3_rrti_type, which invokes value_addr, which then adds
a new value to the head of the value chain.  This new value represents
the pointer to the structure instead of the structure itself.

--
Andreas
Yao Qi March 7, 2018, 11:36 a.m. UTC | #4 
On Mon, Mar 5, 2018 at 5:32 PM, Andreas Arnez <arnez@linux.vnet.ibm.com> wrote:
> OK, how about this? --
>
> ... However, if a watchpoint is defined for a C++ structure/class
> object, then run-time type information (RTTI) may be present.  Thus,
> while constructing the value chain for the watchpoint, the dynamic type
> is fetched by gnuv3_rrti_type, which invokes value_addr, which then adds
> a new value to the head of the value chain.  This new value represents
> the pointer to the structure instead of the structure itself.
>

I think I understand that part now.  Thanks for the explanation.  Patch is
good to me.
Andreas Arnez March 7, 2018, 2:45 p.m. UTC | #5 
On Wed, Mar 07 2018, Yao Qi wrote:

> On Mon, Mar 5, 2018 at 5:32 PM, Andreas Arnez <arnez@linux.vnet.ibm.com> wrote:
>> OK, how about this? --
>>
>> ... However, if a watchpoint is defined for a C++ structure/class
>> object, then run-time type information (RTTI) may be present.  Thus,
>> while constructing the value chain for the watchpoint, the dynamic type
>> is fetched by gnuv3_rrti_type, which invokes value_addr, which then adds
>> a new value to the head of the value chain.  This new value represents
>> the pointer to the structure instead of the structure itself.
>>
>
> I think I understand that part now.  Thanks for the explanation.  Patch is
> good to me.

Thanks.  Pushed:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=commit;h=e95a97d41a18

--
Andreas
diff mbox

Patch

diff --git a/gdb/gnu-v3-abi.c b/gdb/gnu-v3-abi.c
index 0965846ce6..859187f2e9 100644
--- a/gdb/gnu-v3-abi.c
+++ b/gdb/gnu-v3-abi.c
@@ -299,8 +299,9 @@  gnuv3_rtti_type (struct value *value,
   LONGEST offset_to_top;
   const char *atsign;
 
-  /* We only have RTTI for class objects.  */
-  if (TYPE_CODE (values_type) != TYPE_CODE_STRUCT)
+  /* We only have RTTI for dynamic class objects.  */
+  if (TYPE_CODE (values_type) != TYPE_CODE_STRUCT
+      || !gnuv3_dynamic_class (values_type))
     return NULL;
 
   /* Determine architecture.  */
diff --git a/gdb/testsuite/gdb.cp/watch-cp.cc b/gdb/testsuite/gdb.cp/watch-cp.cc
new file mode 100644
index 0000000000..6954e854c9
--- /dev/null
+++ b/gdb/testsuite/gdb.cp/watch-cp.cc
@@ -0,0 +1,28 @@ 
+/* 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/>.  */
+
+/* A class that does not have RTTI and is small enough to fit in a
+   hardware watchpoint on all targets.  */
+class smallstuff { public: int n; };
+
+smallstuff watchme[5];
+
+int
+main ()
+{
+  for (int i = 0; i < sizeof (watchme) / sizeof (watchme[0]); i++)
+    watchme[i].n = i;
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.cp/watch-cp.exp b/gdb/testsuite/gdb.cp/watch-cp.exp
new file mode 100644
index 0000000000..bdf0f13886
--- /dev/null
+++ b/gdb/testsuite/gdb.cp/watch-cp.exp
@@ -0,0 +1,35 @@ 
+# 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/>.
+
+if { [skip_cplus_tests] || [skip_hw_watchpoint_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_test "watch watchme\[3\]" "atchpoint .*: watchme.*" "set watchpoint"
+
+# Verify that a hardware watchpoint is used.
+gdb_test "info watchpoints" " hw watchpoint .* watchme.*"
+
+# Now see whether it actually works.
+gdb_test "continue" "Hardware watchpoint .*New value = \\{n = 3\\}.*"