[PATCH v2 1/1] gdb, breakpoint: add a destructor to the watchpoint struct

[PATCH v2 1/1] gdb, breakpoint: add a destructor to the watchpoint struct

[Mohamed Bouhaouel]

Make sure to unlink the related breakpoint when the watchpoint instance
is deleted.  This prevents having a wp-related breakpoint that is
linked to a NULL watchpoint (e.g.  the watchpoint instance is being
deleted when the 'watch' command fails).  With the below scenario,
having such a left out breakpoint will lead to a GDB hang, and this
is due to an infinite loop when deleting all inferior breakpoints.

Scenario:
	(gdb) set can-use-hw-watchpoints 0
	(gdb) awatch <SCOPE VAR>
	Can't set read/access watchpoint when hardware watchpoints are disabled.
	(gdb) rwatch <SCOPE VAR>
	Can't set read/access watchpoint when hardware watchpoints are disabled.
	(gdb) <continue the program until the end>
	>> HANG <<

Signed-off-by: Mohamed Bouhaouel <mohamed.bouhaouel@intel.com>
---
 gdb/breakpoint.c                              | 15 +++++++
 gdb/breakpoint.h                              |  3 ++
 .../gdb.base/scope-hw-watch-disable.c         | 26 ++++++++++++
 .../gdb.base/scope-hw-watch-disable.exp       | 40 +++++++++++++++++++
 4 files changed, 84 insertions(+)
 create mode 100644 gdb/testsuite/gdb.base/scope-hw-watch-disable.c
 create mode 100644 gdb/testsuite/gdb.base/scope-hw-watch-disable.exp

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index ebe97940f54..862e9a372ab 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -9594,6 +9594,21 @@ break_range_command (const char *arg, int from_tty)
   install_breakpoint (false, std::move (br), true);
 }
 
+/* See breakpoint.h.  */
+
+watchpoint::~watchpoint ()
+{
+  /* Make sure to unlink the destroyed watchpoint from the related
+     breakpoint ring.  */
+
+  breakpoint *bpt = this;
+  breakpoint *related = bpt;
+  while (related->related_breakpoint != bpt)
+    related = related->related_breakpoint;
+
+  related->related_breakpoint = bpt->related_breakpoint;
+}
+
 /*  Return non-zero if EXP is verified as constant.  Returned zero
     means EXP is variable.  Also the constant detection may fail for
     some constant expressions and in such case still falsely return
diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
index 7c5cf3f2bef..cccd331e6e8 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -933,6 +933,9 @@ struct watchpoint : public breakpoint
   void print_recreate (struct ui_file *fp) const override;
   bool explains_signal (enum gdb_signal) override;
 
+  /* Destructor for WATCHPOINT.  */
+  ~watchpoint ();
+
   /* String form of exp to use for displaying to the user (malloc'd),
      or NULL if none.  */
   gdb::unique_xmalloc_ptr<char> exp_string;
diff --git a/gdb/testsuite/gdb.base/scope-hw-watch-disable.c b/gdb/testsuite/gdb.base/scope-hw-watch-disable.c
new file mode 100644
index 00000000000..30956fe1b84
--- /dev/null
+++ b/gdb/testsuite/gdb.base/scope-hw-watch-disable.c
@@ -0,0 +1,26 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2023 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/>.  */
+
+int
+main ()
+{
+  int a = 0, b = 0;
+  b = a;
+  a = b + 10;
+
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.base/scope-hw-watch-disable.exp b/gdb/testsuite/gdb.base/scope-hw-watch-disable.exp
new file mode 100644
index 00000000000..54ebb4e4226
--- /dev/null
+++ b/gdb/testsuite/gdb.base/scope-hw-watch-disable.exp
@@ -0,0 +1,40 @@
+# Copyright 2023 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/>.
+
+# Test that GDB displays the correct error message when hardware watchpoints
+# are not supported or explicitly disabled.  Test also that GDB terminates
+# successfully after several attempts to insert a hardware watchpoint.
+
+standard_testfile
+
+if {[prepare_for_testing "failed to prepare" ${testfile} ${srcfile}]} {
+    return -1
+}
+
+gdb_test_no_output "set can-use-hw-watchpoints 0"
+
+if {![runto_main]} {
+    return -1
+}
+
+gdb_test "awatch a" \
+    "Can't set read/access watchpoint when hardware watchpoints are disabled." \
+    "unsuccessful attempt to create an access watchpoint"
+gdb_test "rwatch b" \
+    "Can't set read/access watchpoint when hardware watchpoints are disabled." \
+    "unsuccessful attempt to create a read watchpoint"
+
+# The program continues until termination.
+gdb_continue_to_end
-- 
2.25.1

Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

RE: [PING][PATCH v2 1/1] gdb, breakpoint: add a destructor to the watchpoint struct

[Bouhaouel, Mohamed]

Dear all,

I kindly request that you review this patch.

Thanks,
-Mohamed

> -----Original Message-----
> From: Gdb-patches <gdb-patches-
> bounces+felix.willgerodt=intel.com@sourceware.org> On Behalf Of Mohamed
> Bouhaouel via Gdb-patches
> Sent: Wednesday, April 5, 2023 3:35 PM
> To: gdb-patches@sourceware.org
> Cc: tom@tromey.com; aburgess@redhat.com
> Subject: [PATCH v2 1/1] gdb, breakpoint: add a destructor to the watchpoint
> struct
> 
> Make sure to unlink the related breakpoint when the watchpoint instance
> is deleted.  This prevents having a wp-related breakpoint that is
> linked to a NULL watchpoint (e.g.  the watchpoint instance is being
> deleted when the 'watch' command fails).  With the below scenario,
> having such a left out breakpoint will lead to a GDB hang, and this
> is due to an infinite loop when deleting all inferior breakpoints.
> 
> Scenario:
> 	(gdb) set can-use-hw-watchpoints 0
> 	(gdb) awatch <SCOPE VAR>
> 	Can't set read/access watchpoint when hardware watchpoints are
> disabled.
> 	(gdb) rwatch <SCOPE VAR>
> 	Can't set read/access watchpoint when hardware watchpoints are
> disabled.
> 	(gdb) <continue the program until the end>
> 	>> HANG <<
> 
> Signed-off-by: Mohamed Bouhaouel <mohamed.bouhaouel@intel.com>
> ---
>  gdb/breakpoint.c                              | 15 +++++++
>  gdb/breakpoint.h                              |  3 ++
>  .../gdb.base/scope-hw-watch-disable.c         | 26 ++++++++++++
>  .../gdb.base/scope-hw-watch-disable.exp       | 40 +++++++++++++++++++
>  4 files changed, 84 insertions(+)
>  create mode 100644 gdb/testsuite/gdb.base/scope-hw-watch-disable.c
>  create mode 100644 gdb/testsuite/gdb.base/scope-hw-watch-disable.exp
> 
> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
> index ebe97940f54..862e9a372ab 100644
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -9594,6 +9594,21 @@ break_range_command (const char *arg, int
> from_tty)
>    install_breakpoint (false, std::move (br), true);
>  }
> 
> +/* See breakpoint.h.  */
> +
> +watchpoint::~watchpoint ()
> +{
> +  /* Make sure to unlink the destroyed watchpoint from the related
> +     breakpoint ring.  */
> +
> +  breakpoint *bpt = this;
> +  breakpoint *related = bpt;
> +  while (related->related_breakpoint != bpt)
> +    related = related->related_breakpoint;
> +
> +  related->related_breakpoint = bpt->related_breakpoint;
> +}
> +
>  /*  Return non-zero if EXP is verified as constant.  Returned zero
>      means EXP is variable.  Also the constant detection may fail for
>      some constant expressions and in such case still falsely return
> diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
> index 7c5cf3f2bef..cccd331e6e8 100644
> --- a/gdb/breakpoint.h
> +++ b/gdb/breakpoint.h
> @@ -933,6 +933,9 @@ struct watchpoint : public breakpoint
>    void print_recreate (struct ui_file *fp) const override;
>    bool explains_signal (enum gdb_signal) override;
> 
> +  /* Destructor for WATCHPOINT.  */
> +  ~watchpoint ();
> +
>    /* String form of exp to use for displaying to the user (malloc'd),
>       or NULL if none.  */
>    gdb::unique_xmalloc_ptr<char> exp_string;
> diff --git a/gdb/testsuite/gdb.base/scope-hw-watch-disable.c
> b/gdb/testsuite/gdb.base/scope-hw-watch-disable.c
> new file mode 100644
> index 00000000000..30956fe1b84
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/scope-hw-watch-disable.c
> @@ -0,0 +1,26 @@
> +/* This testcase is part of GDB, the GNU debugger.
> +
> +   Copyright 2023 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/>.  */
> +
> +int
> +main ()
> +{
> +  int a = 0, b = 0;
> +  b = a;
> +  a = b + 10;
> +
> +  return 0;
> +}
> diff --git a/gdb/testsuite/gdb.base/scope-hw-watch-disable.exp
> b/gdb/testsuite/gdb.base/scope-hw-watch-disable.exp
> new file mode 100644
> index 00000000000..54ebb4e4226
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/scope-hw-watch-disable.exp
> @@ -0,0 +1,40 @@
> +# Copyright 2023 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/>.
> +
> +# Test that GDB displays the correct error message when hardware
> watchpoints
> +# are not supported or explicitly disabled.  Test also that GDB terminates
> +# successfully after several attempts to insert a hardware watchpoint.
> +
> +standard_testfile
> +
> +if {[prepare_for_testing "failed to prepare" ${testfile} ${srcfile}]} {
> +    return -1
> +}
> +
> +gdb_test_no_output "set can-use-hw-watchpoints 0"
> +
> +if {![runto_main]} {
> +    return -1
> +}
> +
> +gdb_test "awatch a" \
> +    "Can't set read/access watchpoint when hardware watchpoints are
> disabled." \
> +    "unsuccessful attempt to create an access watchpoint"
> +gdb_test "rwatch b" \
> +    "Can't set read/access watchpoint when hardware watchpoints are
> disabled." \
> +    "unsuccessful attempt to create a read watchpoint"
> +
> +# The program continues until termination.
> +gdb_continue_to_end
> --
> 2.25.1
> 
> Intel Deutschland GmbH
> Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
> Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
> Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva
> Chairperson of the Supervisory Board: Nicole Lau
> Registered Office: Munich
> Commercial Register: Amtsgericht Muenchen HRB 186928

Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

Re: [PATCH v2 1/1] gdb, breakpoint: add a destructor to the watchpoint struct

[Bruno Larsen]

On 05/04/2023 15:34, Mohamed Bouhaouel via Gdb-patches wrote:
> Make sure to unlink the related breakpoint when the watchpoint instance
> is deleted.  This prevents having a wp-related breakpoint that is
> linked to a NULL watchpoint (e.g.  the watchpoint instance is being
> deleted when the 'watch' command fails).  With the below scenario,
> having such a left out breakpoint will lead to a GDB hang, and this
> is due to an infinite loop when deleting all inferior breakpoints.
>
> Scenario:
> 	(gdb) set can-use-hw-watchpoints 0
> 	(gdb) awatch <SCOPE VAR>
> 	Can't set read/access watchpoint when hardware watchpoints are disabled.
> 	(gdb) rwatch <SCOPE VAR>
> 	Can't set read/access watchpoint when hardware watchpoints are disabled.
> 	(gdb) <continue the program until the end>
> 	>> HANG <<
>
> Signed-off-by: Mohamed Bouhaouel <mohamed.bouhaouel@intel.com>

Hi Mohamed!

Thank you for working on this!

I have tried it, your test reproduces the issue, and your changes fix 
them. I only have one small nit inlined. Other than that:
Reviewed-By: Bruno Larsen <blarsen@redhat.com>

Do note that that tag is not enough to push, so here's hoping a 
maintainer looks at it soon!

> ---
>   gdb/breakpoint.c                              | 15 +++++++
>   gdb/breakpoint.h                              |  3 ++
>   .../gdb.base/scope-hw-watch-disable.c         | 26 ++++++++++++
>   .../gdb.base/scope-hw-watch-disable.exp       | 40 +++++++++++++++++++
>   4 files changed, 84 insertions(+)
>   create mode 100644 gdb/testsuite/gdb.base/scope-hw-watch-disable.c
>   create mode 100644 gdb/testsuite/gdb.base/scope-hw-watch-disable.exp
>
> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
> index ebe97940f54..862e9a372ab 100644
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -9594,6 +9594,21 @@ break_range_command (const char *arg, int from_tty)
>     install_breakpoint (false, std::move (br), true);
>   }
>   
> +/* See breakpoint.h.  */
> +
> +watchpoint::~watchpoint ()
> +{
> +  /* Make sure to unlink the destroyed watchpoint from the related
> +     breakpoint ring.  */
> +
> +  breakpoint *bpt = this;
The bpt variable seems unnecessary to me. you could remove this line and 
substitute all occurrences of bpt in the following lines with "this"
> +  breakpoint *related = bpt;
and in doing that, I think it might make sense to rename this to bpt? 
This second one is just a suggestion, though, so no need to change if 
you prefer it like this.

-- 
Cheers,
Bruno

> +  while (related->related_breakpoint != bpt)
> +    related = related->related_breakpoint;
> +
> +  related->related_breakpoint = bpt->related_breakpoint;
> +}
> +
>   /*  Return non-zero if EXP is verified as constant.  Returned zero
>       means EXP is variable.  Also the constant detection may fail for
>       some constant expressions and in such case still falsely return
> diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
> index 7c5cf3f2bef..cccd331e6e8 100644
> --- a/gdb/breakpoint.h
> +++ b/gdb/breakpoint.h
> @@ -933,6 +933,9 @@ struct watchpoint : public breakpoint
>     void print_recreate (struct ui_file *fp) const override;
>     bool explains_signal (enum gdb_signal) override;
>   
> +  /* Destructor for WATCHPOINT.  */
> +  ~watchpoint ();
> +
>     /* String form of exp to use for displaying to the user (malloc'd),
>        or NULL if none.  */
>     gdb::unique_xmalloc_ptr<char> exp_string;
> diff --git a/gdb/testsuite/gdb.base/scope-hw-watch-disable.c b/gdb/testsuite/gdb.base/scope-hw-watch-disable.c
> new file mode 100644
> index 00000000000..30956fe1b84
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/scope-hw-watch-disable.c
> @@ -0,0 +1,26 @@
> +/* This testcase is part of GDB, the GNU debugger.
> +
> +   Copyright 2023 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/>.  */
> +
> +int
> +main ()
> +{
> +  int a = 0, b = 0;
> +  b = a;
> +  a = b + 10;
> +
> +  return 0;
> +}
> diff --git a/gdb/testsuite/gdb.base/scope-hw-watch-disable.exp b/gdb/testsuite/gdb.base/scope-hw-watch-disable.exp
> new file mode 100644
> index 00000000000..54ebb4e4226
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/scope-hw-watch-disable.exp
> @@ -0,0 +1,40 @@
> +# Copyright 2023 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/>.
> +
> +# Test that GDB displays the correct error message when hardware watchpoints
> +# are not supported or explicitly disabled.  Test also that GDB terminates
> +# successfully after several attempts to insert a hardware watchpoint.
> +
> +standard_testfile
> +
> +if {[prepare_for_testing "failed to prepare" ${testfile} ${srcfile}]} {
> +    return -1
> +}
> +
> +gdb_test_no_output "set can-use-hw-watchpoints 0"
> +
> +if {![runto_main]} {
> +    return -1
> +}
> +
> +gdb_test "awatch a" \
> +    "Can't set read/access watchpoint when hardware watchpoints are disabled." \
> +    "unsuccessful attempt to create an access watchpoint"
> +gdb_test "rwatch b" \
> +    "Can't set read/access watchpoint when hardware watchpoints are disabled." \
> +    "unsuccessful attempt to create a read watchpoint"
> +
> +# The program continues until termination.
> +gdb_continue_to_end