[v5,PR,python/29603] Disable out-of-scope watchpoints

  Currently, when a local software watchpoint goes out of scope, GDB sets
the watchpoint's disposition to `delete at next stop' and then normal
stops (i.e., stop and wait for the next GDB command). When GDB normal
stops, it automatically deletes the breakpoints with their disposition
set to `delete at next stop'.

Suppose a Python script decides not to normal stop when a local
software watchpoint goes out of scope, the watchpoint will not be
automatically deleted even when its disposition is set to
`delete at next stop'.

Since GDB single-steps the program and tests the watched expression
after each instruction, not deleting the watchpoint causes the
watchpoint to be hit many more times than it should, as reported in
PR python/29603.

This was happening because the watchpoint is not deleted or disabled
when going out of scope.

This commit fixes this issue by disabling the watchpoint when going out
of scope. It also adds a test to ensure this feature isn't regressed in
the future.

Calling `breakpoint_auto_delete' on all kinds of stops (in
`fetch_inferior_event') seem to solve this issue, but is in fact
inappropriate, since `breakpoint_auto_delete' goes over all breakpoints
instead of just going through the bpstat chain (which only contains the
breakpoints that were hit right now).

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29603
---
 gdb/breakpoint.c                           |  1 +
 gdb/testsuite/gdb.python/py-watchpoint.c   | 27 +++++++++++++
 gdb/testsuite/gdb.python/py-watchpoint.exp | 46 ++++++++++++++++++++++
 gdb/testsuite/gdb.python/py-watchpoint.py  | 30 ++++++++++++++
 4 files changed, 104 insertions(+)
 create mode 100644 gdb/testsuite/gdb.python/py-watchpoint.c
 create mode 100644 gdb/testsuite/gdb.python/py-watchpoint.exp
 create mode 100644 gdb/testsuite/gdb.python/py-watchpoint.py
  

Ping for: 
<https://sourceware.org/pipermail/gdb-patches/2023-April/199034.html>

Johnson

On 4/23/2023 5:54 PM, JohnsonSun wrote:
> Currently, when a local software watchpoint goes out of scope, GDB sets
> the watchpoint's disposition to `delete at next stop' and then normal
> stops (i.e., stop and wait for the next GDB command). When GDB normal
> stops, it automatically deletes the breakpoints with their disposition
> set to `delete at next stop'.
>
> Suppose a Python script decides not to normal stop when a local
> software watchpoint goes out of scope, the watchpoint will not be
> automatically deleted even when its disposition is set to
> `delete at next stop'.
>
> Since GDB single-steps the program and tests the watched expression
> after each instruction, not deleting the watchpoint causes the
> watchpoint to be hit many more times than it should, as reported in
> PR python/29603.
>
> This was happening because the watchpoint is not deleted or disabled
> when going out of scope.
>
> This commit fixes this issue by disabling the watchpoint when going out
> of scope. It also adds a test to ensure this feature isn't regressed in
> the future.
>
> Calling `breakpoint_auto_delete' on all kinds of stops (in
> `fetch_inferior_event') seem to solve this issue, but is in fact
> inappropriate, since `breakpoint_auto_delete' goes over all breakpoints
> instead of just going through the bpstat chain (which only contains the
> breakpoints that were hit right now).
>
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29603
> ---
>   gdb/breakpoint.c                           |  1 +
>   gdb/testsuite/gdb.python/py-watchpoint.c   | 27 +++++++++++++
>   gdb/testsuite/gdb.python/py-watchpoint.exp | 46 ++++++++++++++++++++++
>   gdb/testsuite/gdb.python/py-watchpoint.py  | 30 ++++++++++++++
>   4 files changed, 104 insertions(+)
>   create mode 100644 gdb/testsuite/gdb.python/py-watchpoint.c
>   create mode 100644 gdb/testsuite/gdb.python/py-watchpoint.exp
>   create mode 100644 gdb/testsuite/gdb.python/py-watchpoint.py
>
> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
> index bff3bac7d1..47dcf1e127 100644
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -1832,6 +1832,7 @@ watchpoint_del_at_next_stop (struct watchpoint *w)
>         w->related_breakpoint = w;
>       }
>     w->disposition = disp_del_at_next_stop;
> +  disable_breakpoint (w);
>   }
>   
>   /* Extract a bitfield value from value VAL using the bit parameters contained in
> diff --git a/gdb/testsuite/gdb.python/py-watchpoint.c b/gdb/testsuite/gdb.python/py-watchpoint.c
> new file mode 100644
> index 0000000000..2eeae55021
> --- /dev/null
> +++ b/gdb/testsuite/gdb.python/py-watchpoint.c
> @@ -0,0 +1,27 @@
> +/* This testcase is part of GDB, the GNU debugger.
> +
> +   Copyright 2022 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/>.  */
> +
> +#include <stdio.h>
> +
> +int
> +main (void)
> +{
> +  int i = -1;
> +  for (i = 0; i < 3; i++) /* main for */
> +    printf ("i = %d; ", i);
> +  return 0;
> +}
> diff --git a/gdb/testsuite/gdb.python/py-watchpoint.exp b/gdb/testsuite/gdb.python/py-watchpoint.exp
> new file mode 100644
> index 0000000000..fd7dbe20eb
> --- /dev/null
> +++ b/gdb/testsuite/gdb.python/py-watchpoint.exp
> @@ -0,0 +1,46 @@
> +# Copyright (C) 2022 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/>.
> +
> +# Check that Watchpoints are deleted after use.
> +
> +load_lib gdb-python.exp
> +
> +standard_testfile
> +
> +if {[prepare_for_testing "failed to prepare" $testfile $srcfile]} {
> +    return -1
> +}
> +
> +require allow_python_tests
> +
> +if ![runto_main] then {
> +    return 0
> +}
> +
> +# For remote host testing
> +set pyfile [gdb_remote_download host ${srcdir}/${subdir}/${testfile}.py]
> +
> +gdb_test_no_output "set can-use-hw-watchpoints 0" "don't use hardware watchpoints"
> +set for_line_no [gdb_get_line_number "main for"]
> +gdb_test "break $for_line_no" ".*" "set breakpoint before loop"
> +gdb_test "continue" ".*" "run until reaching loop"
> +gdb_test "clear" ".*" "delete the breakpoint before loop"
> +gdb_test "python print(len(gdb.breakpoints()))" "1" "check default BP count"
> +gdb_test "source $pyfile" ".*Python script imported.*" \
> +    "import python scripts"
> +gdb_test "python print(len(gdb.breakpoints()))" "2" "check modified BP count"
> +gdb_test "continue" ".*" "run until program stops"
> +gdb_test "python print(bpt.n)" "5" "check watchpoint hits"
> +gdb_test "python print(len(gdb.breakpoints()))" "1" "check BP count"
> diff --git a/gdb/testsuite/gdb.python/py-watchpoint.py b/gdb/testsuite/gdb.python/py-watchpoint.py
> new file mode 100644
> index 0000000000..647a653085
> --- /dev/null
> +++ b/gdb/testsuite/gdb.python/py-watchpoint.py
> @@ -0,0 +1,30 @@
> +# Copyright (C) 2022 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/>.
> +
> +
> +class MyBreakpoint(gdb.Breakpoint):
> +    def __init__(self):
> +        super().__init__("i", gdb.BP_WATCHPOINT)
> +        self.n = 0
> +
> +    def stop(self):
> +        self.n += 1
> +        print("Watchpoint Hit:", self.n, flush=True)
> +        return False
> +
> +
> +bpt = MyBreakpoint()
> +
> +print("Python script imported")
  

On 4/23/23 05:54, Johnson Sun wrote:
> Currently, when a local software watchpoint goes out of scope, GDB sets
> the watchpoint's disposition to `delete at next stop' and then normal
> stops (i.e., stop and wait for the next GDB command). When GDB normal
> stops, it automatically deletes the breakpoints with their disposition
> set to `delete at next stop'.
> 
> Suppose a Python script decides not to normal stop when a local
> software watchpoint goes out of scope, the watchpoint will not be
> automatically deleted even when its disposition is set to
> `delete at next stop'.
> 
> Since GDB single-steps the program and tests the watched expression
> after each instruction, not deleting the watchpoint causes the
> watchpoint to be hit many more times than it should, as reported in
> PR python/29603.
> 
> This was happening because the watchpoint is not deleted or disabled
> when going out of scope.
> 
> This commit fixes this issue by disabling the watchpoint when going out
> of scope. It also adds a test to ensure this feature isn't regressed in
> the future.
> 
> Calling `breakpoint_auto_delete' on all kinds of stops (in
> `fetch_inferior_event') seem to solve this issue, but is in fact
> inappropriate, since `breakpoint_auto_delete' goes over all breakpoints
> instead of just going through the bpstat chain (which only contains the
> breakpoints that were hit right now).
> 
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29603

Hi,

Hmm, re-testing the patch, it now just times out like this:

    (gdb) PASS: gdb.python/py-watchpoint.exp: check modified BP count
    continue
    Continuing.
    Watchpoint Hit: 1
    FAIL: gdb.python/py-watchpoint.exp: run until program stops (timeout)
    python print(bpt.n)

It basically hangs there.  Do you see this?

Simon
  

Hi,

I just applied the patch to commit 
39453f9d8cf03b382d34f3548706f1ae5916e34e, and tested with the following 
commands on a clean Ubuntu 22.04 LTS machine:

     make check TESTS="gdb.python/py-watchpoint.exp"
     make check TESTS="gdb.python/py-watchpoint.exp" 
RUNTESTFLAGS="--target_board=native-gdbserver"
     make check TESTS="gdb.python/py-watchpoint.exp" 
RUNTESTFLAGS="--target_board=native-extended-gdbserver"

all 3 commands above passed the tests (10/10) on my machine.

I'm unsure why the test times out on your machine. Could you provide 
information regarding the base commit that was used and the operating 
system installed?

Best regards,
Johnson

On 5/10/2023 2:50 AM, SimonMarchi wrote:
> On 4/23/23 05:54, Johnson Sun wrote:
>> Currently, when a local software watchpoint goes out of scope, GDB sets
>> the watchpoint's disposition to `delete at next stop' and then normal
>> stops (i.e., stop and wait for the next GDB command). When GDB normal
>> stops, it automatically deletes the breakpoints with their disposition
>> set to `delete at next stop'.
>>
>> Suppose a Python script decides not to normal stop when a local
>> software watchpoint goes out of scope, the watchpoint will not be
>> automatically deleted even when its disposition is set to
>> `delete at next stop'.
>>
>> Since GDB single-steps the program and tests the watched expression
>> after each instruction, not deleting the watchpoint causes the
>> watchpoint to be hit many more times than it should, as reported in
>> PR python/29603.
>>
>> This was happening because the watchpoint is not deleted or disabled
>> when going out of scope.
>>
>> This commit fixes this issue by disabling the watchpoint when going out
>> of scope. It also adds a test to ensure this feature isn't regressed in
>> the future.
>>
>> Calling `breakpoint_auto_delete' on all kinds of stops (in
>> `fetch_inferior_event') seem to solve this issue, but is in fact
>> inappropriate, since `breakpoint_auto_delete' goes over all breakpoints
>> instead of just going through the bpstat chain (which only contains the
>> breakpoints that were hit right now).
>>
>> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29603
> Hi,
>
> Hmm, re-testing the patch, it now just times out like this:
>
>      (gdb) PASS: gdb.python/py-watchpoint.exp: check modified BP count
>      continue
>      Continuing.
>      Watchpoint Hit: 1
>      FAIL: gdb.python/py-watchpoint.exp: run until program stops (timeout)
>      python print(bpt.n)
>
> It basically hangs there.  Do you see this?
>
> Simon
>
  

On 5/10/23 13:22, Johnson Sun wrote:
> Hi,
> 
> I just applied the patch to commit 39453f9d8cf03b382d34f3548706f1ae5916e34e, and tested with the following commands on a clean Ubuntu 22.04 LTS machine:
> 
>     make check TESTS="gdb.python/py-watchpoint.exp"
>     make check TESTS="gdb.python/py-watchpoint.exp" RUNTESTFLAGS="--target_board=native-gdbserver"
>     make check TESTS="gdb.python/py-watchpoint.exp" RUNTESTFLAGS="--target_board=native-extended-gdbserver"
> 
> all 3 commands above passed the tests (10/10) on my machine.
> 
> I'm unsure why the test times out on your machine. Could you provide information regarding the base commit that was used and the operating system installed?

I just retested on latest master as of now (38b95a529385).

Looking at the output of "set debug infrun", it looks like the program
is making progress, just that it's really slow, since GDB is single
stepping all instructions due to the software watchpoint.

Is the printf necessary for the test?  If not, we can remove it.  If the
loop executes just a few instructions, the program should execute
relatively quickly, even if single stepping all the way through.

Simon
  

On 5/11/2023 10:08 AM, SimonMarchi wrote:
> On 5/10/23 13:22, Johnson Sun wrote:
>> Hi,
>>
>> I just applied the patch to commit 39453f9d8cf03b382d34f3548706f1ae5916e34e, and tested with the following commands on a clean Ubuntu 22.04 LTS machine:
>>
>>      make check TESTS="gdb.python/py-watchpoint.exp"
>>      make check TESTS="gdb.python/py-watchpoint.exp" RUNTESTFLAGS="--target_board=native-gdbserver"
>>      make check TESTS="gdb.python/py-watchpoint.exp" RUNTESTFLAGS="--target_board=native-extended-gdbserver"
>>
>> all 3 commands above passed the tests (10/10) on my machine.
>>
>> I'm unsure why the test times out on your machine. Could you provide information regarding the base commit that was used and the operating system installed?
> I just retested on latest master as of now (38b95a529385).
>
> Looking at the output of "set debug infrun", it looks like the program
> is making progress, just that it's really slow, since GDB is single
> stepping all instructions due to the software watchpoint.
>
> Is the printf necessary for the test?  If not, we can remove it.  If the
> loop executes just a few instructions, the program should execute
> relatively quickly, even if single stepping all the way through.
>
> Simon


Hi,

The `printf` statement is not necessary. The tests will still pass after 
removing the `printf` statement and declaring the variable `i` as volatile.

I just sent an updated patch: 
<https://sourceware.org/pipermail/gdb-patches/2023-May/199516.html>

Thank you,

Johnson