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

[Bruno Larsen <blarsen@redhat.com>]

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