public inbox for gdb-patches@sourceware.org
 help / color / mirror / Atom feed
* [PATCH] [PR python/29603] Fix deletion of Watchpoints
@ 2022-09-25  5:33 Johnson Sun
  2022-09-25 18:10 ` Lancelot SIX
  2022-10-01  5:20 ` Johnson Sun
  0 siblings, 2 replies; 34+ messages in thread
From: Johnson Sun @ 2022-09-25  5:33 UTC (permalink / raw)
  To: gdb-patches; +Cc: j3.soon777

Currently, during normal stop (i.e., stop and waiting for the next command),
GDB automatically deletes local Watchpoints that are out of scope. However,
local Watchpoints are not deleted if a Python script decides not to normal
stop upon hit, causing them to be hit many more times than they should, as
reported in PR python/29603. This was happening because the watchpoint is not
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.

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 | 44 ++++++++++++++++++++++
 gdb/testsuite/gdb.python/py-watchpoint.py  | 31 +++++++++++++++
 4 files changed, 103 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 bff3bac7d1a..b78ae9c4993 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 00000000000..6d02e87e571
--- /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 ()
+{
+  int i;
+  for (i = 0; i < 3; i++)
+    printf ("%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 00000000000..863f3eff66a
--- /dev/null
+++ b/gdb/testsuite/gdb.python/py-watchpoint.exp
@@ -0,0 +1,44 @@
+# 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/>.
+
+# This file is part of the GDB testsuite.  It checks Watchpoints
+# are deleted after used.
+
+load_lib gdb-python.exp
+
+standard_testfile
+
+if {[prepare_for_testing "failed to prepare" $testfile $srcfile]} {
+    return -1
+}
+
+# Skip all tests if Python scripting is not enabled.
+if { [skip_python_tests] } { continue }
+
+if ![runto_main] then {
+    return 0
+}
+
+#
+# Check Watchpoints are deleted after used
+#
+
+gdb_test "set can-use-hw-watchpoints 0" ".*" "Don't use hardware watchpoints"
+gdb_test "python print (len(gdb.breakpoints()))" "1" "check default BP count"
+gdb_test "source ${srcdir}/${subdir}/${testfile}.py" ".*Python script imported.*" \
+	 "import python scripts"
+gdb_test "python print (len(gdb.breakpoints()))" "2" "check modified BP count"
+gdb_test "continue" ".*Watchpoint Hit: 4.*" "run until program stops"
+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 00000000000..855c820b245
--- /dev/null
+++ b/gdb/testsuite/gdb.python/py-watchpoint.py
@@ -0,0 +1,31 @@
+# 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/>.
+
+# This file is part of the GDB testsuite.  It checks Watchpoints
+# are deleted after used.
+
+class MyBreakpoint(gdb.Breakpoint):
+    def __init__(self, *args, **kwargs):
+        gdb.Breakpoint.__init__(self, *args, **kwargs)
+        self.i = 0
+    def stop(self):
+        self.i += 1
+        print("Watchpoint Hit:", self.i)
+        gdb.execute('print i')
+        return False
+
+MyBreakpoint('i', gdb.BP_WATCHPOINT)
+
+print("Python script imported")
-- 
2.34.1

base-commit: c99b2113a478a075e8d8f7c3848a92f1ce73f847

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH] [PR python/29603] Fix deletion of Watchpoints
  2022-09-25  5:33 [PATCH] [PR python/29603] Fix deletion of Watchpoints Johnson Sun
@ 2022-09-25 18:10 ` Lancelot SIX
  2022-10-01  5:20 ` Johnson Sun
  1 sibling, 0 replies; 34+ messages in thread
From: Lancelot SIX @ 2022-09-25 18:10 UTC (permalink / raw)
  To: Johnson Sun; +Cc: gdb-patches

Hi,

Thanks for the patch!

First, I have not really explored why, but running your testcase with
current master GDB gives me:

    $ make check-gdb TESTS=gdb.python/py-watchpoint.exp
    Running .../gdb/testsuite/gdb.python/py-watchpoint.exp ...

                === gdb Summary ===

    # of expected passes            6

which is strange.  I was expecting FAILs there.  Is it only on my system
(ubuntu 22.04 on x86_64)?

I can however reproduce the problem by running the ticket’s testcase
manually and observe the issue you describe.

On Sun, Sep 25, 2022 at 01:33:50PM +0800, Johnson Sun via Gdb-patches wrote:
> Currently, during normal stop (i.e., stop and waiting for the next command),
> GDB automatically deletes local Watchpoints that are out of scope. However,
> local Watchpoints are not deleted if a Python script decides not to normal
> stop upon hit, causing them to be hit many more times than they should, as
> reported in PR python/29603. This was happening because the watchpoint is not
> 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.
> 
> 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 | 44 ++++++++++++++++++++++
>  gdb/testsuite/gdb.python/py-watchpoint.py  | 31 +++++++++++++++
>  4 files changed, 103 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 bff3bac7d1a..b78ae9c4993 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);

From what I understand, the problem you are trying to solve is linked to
extension language (python in this case), so it seems odd to me that the
fix is in a non-extension related code path.

If my understanding is correct, the python extension you provide should
work similarly to typing say "watch i if 0".  So my approach would be to
have something similar between checking the watchpoint condition and
evaluating the `stop` method from you python class.

In the condition case, `disable_breakpoint` is not used before the
breakpoint is deleted.  Instead, the code checks "b->disposition !=
disp_del_at_next_stop" (in bpstat_check_breakpoint_conditions) before
trying to evaluate the condition.  I guess that this is what you also
want to do in your case.

In the same function, you can have the following change:

    diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
    index 002f4a935b1..e8d3fd0d50a 100644
    --- a/gdb/breakpoint.c
    +++ b/gdb/breakpoint.c
    @@ -5339,7 +5339,8 @@ bpstat_check_breakpoint_conditions (bpstat *bs, thread_info *thread)
     
       /* Evaluate extension language breakpoints that have a "stop" method
          implemented.  */
    -  bs->stop = breakpoint_ext_lang_cond_says_stop (b);
    +  if (b->disposition != disp_del_at_next_stop)
    +    bs->stop = breakpoint_ext_lang_cond_says_stop (b);
     
       if (is_watchpoint (b))
         {

I believe this is what you are looking for.  That being said, this is my
approach to solve your problem, a maintainer might have a better way to
address this.

>  }
>  
>  /* 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 00000000000..6d02e87e571
> --- /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 ()
> +{
> +  int i;
> +  for (i = 0; i < 3; i++)
> +    printf ("%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 00000000000..863f3eff66a
> --- /dev/null
> +++ b/gdb/testsuite/gdb.python/py-watchpoint.exp
> @@ -0,0 +1,44 @@
> +# 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/>.
> +
> +# This file is part of the GDB testsuite.  It checks Watchpoints
> +# are deleted after used.
> +
> +load_lib gdb-python.exp
> +
> +standard_testfile
> +
> +if {[prepare_for_testing "failed to prepare" $testfile $srcfile]} {
> +    return -1
> +}
> +
> +# Skip all tests if Python scripting is not enabled.
> +if { [skip_python_tests] } { continue }
> +
> +if ![runto_main] then {
> +    return 0
> +}
> +
> +#
> +# Check Watchpoints are deleted after used
> +#
> +
> +gdb_test "set can-use-hw-watchpoints 0" ".*" "Don't use hardware watchpoints"

In this case, you could probably use gdb_test_no_output

> +gdb_test "python print (len(gdb.breakpoints()))" "1" "check default BP count"

It looks odd to have a space after print and not before the other
parenthesis.  To follow python coding convention, I would remove the
space between "print" and the "(".  Same remark holds for the two other
occurrences of this pattern below.

Best,
Lancelot.

> +gdb_test "source ${srcdir}/${subdir}/${testfile}.py" ".*Python script imported.*" \
> +	 "import python scripts"
> +gdb_test "python print (len(gdb.breakpoints()))" "2" "check modified BP count"
> +gdb_test "continue" ".*Watchpoint Hit: 4.*" "run until program stops"
> +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 00000000000..855c820b245
> --- /dev/null
> +++ b/gdb/testsuite/gdb.python/py-watchpoint.py
> @@ -0,0 +1,31 @@
> +# 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/>.
> +
> +# This file is part of the GDB testsuite.  It checks Watchpoints
> +# are deleted after used.
> +
> +class MyBreakpoint(gdb.Breakpoint):
> +    def __init__(self, *args, **kwargs):
> +        gdb.Breakpoint.__init__(self, *args, **kwargs)
> +        self.i = 0
> +    def stop(self):
> +        self.i += 1
> +        print("Watchpoint Hit:", self.i)
> +        gdb.execute('print i')
> +        return False
> +
> +MyBreakpoint('i', gdb.BP_WATCHPOINT)
> +
> +print("Python script imported")
> -- 
> 2.34.1
> 
> base-commit: c99b2113a478a075e8d8f7c3848a92f1ce73f847

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH] [PR python/29603] Fix deletion of Watchpoints
  2022-09-25  5:33 [PATCH] [PR python/29603] Fix deletion of Watchpoints Johnson Sun
  2022-09-25 18:10 ` Lancelot SIX
@ 2022-10-01  5:20 ` Johnson Sun
  2022-10-01  5:27   ` [PATCH v2] [PR python/29603] Disable out-of-scope watchpoints Johnson Sun
  1 sibling, 1 reply; 34+ messages in thread
From: Johnson Sun @ 2022-10-01  5:20 UTC (permalink / raw)
  To: Lancelot SIX, gdb-patches; +Cc: j3.soon777

Hi Lancelot,

Thanks for reviewing this!  I revised the patch and 

will send version 2 shortly.

Cheers,
Johnson


^ permalink raw reply	[flat|nested] 34+ messages in thread

* [PATCH v2] [PR python/29603] Disable out-of-scope watchpoints
  2022-10-01  5:20 ` Johnson Sun
@ 2022-10-01  5:27   ` Johnson Sun
  2022-10-20 17:57     ` Johnson Sun
  0 siblings, 1 reply; 34+ messages in thread
From: Johnson Sun @ 2022-10-01  5:27 UTC (permalink / raw)
  To: Lancelot SIX, gdb-patches; +Cc: j3.soon777

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

There are two other solutions that seem to solve this issue but don't:
1. Automatically delete breakpoints on all kinds of stops
   (in `fetch_inferior_event'). This solution is very slow since the deletion
   requires O(N) time for N breakpoints.
2. Disable the watchpoint when the watchpoint's disposition is set to
   `delete at next stop' (in `watchpoint_del_at_next_stop'). This solution
   will not trigger a breakpoint stop callback when the watchpoint goes out of
   scope.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29603
---
 gdb/breakpoint.c                           |  2 +
 gdb/testsuite/gdb.python/py-watchpoint.c   | 27 ++++++++++++
 gdb/testsuite/gdb.python/py-watchpoint.exp | 50 ++++++++++++++++++++++
 gdb/testsuite/gdb.python/py-watchpoint.py  | 30 +++++++++++++
 4 files changed, 109 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 bff3bac7d1a..15f4ae2131c 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -5340,6 +5340,8 @@ bpstat_check_breakpoint_conditions (bpstat *bs, thread_info *thread)
   /* Evaluate extension language breakpoints that have a "stop" method
      implemented.  */
   bs->stop = breakpoint_ext_lang_cond_says_stop (b);
+  if (b->disposition == disp_del_at_next_stop)
+    disable_breakpoint(b);
 
   if (is_watchpoint (b))
     {
diff --git a/gdb/testsuite/gdb.python/py-watchpoint.c b/gdb/testsuite/gdb.python/py-watchpoint.c
new file mode 100644
index 00000000000..6d02e87e571
--- /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 ()
+{
+  int i;
+  for (i = 0; i < 3; i++)
+    printf ("%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 00000000000..a14ab139a3f
--- /dev/null
+++ b/gdb/testsuite/gdb.python/py-watchpoint.exp
@@ -0,0 +1,50 @@
+# 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/>.
+
+# This file is part of the GDB testsuite.  It checks Watchpoints
+# are deleted after used.
+
+load_lib gdb-python.exp
+
+standard_testfile
+
+if {[prepare_for_testing "failed to prepare" $testfile $srcfile]} {
+    return -1
+}
+
+# Skip all tests if Python scripting is not enabled.
+if { [skip_python_tests] } { continue }
+
+if ![runto_main] then {
+    return 0
+}
+
+#
+# Check Watchpoints are deleted after used
+#
+
+gdb_test_no_output "set can-use-hw-watchpoints 0" "Don't use hardware watchpoints"
+gdb_test "python print(len(gdb.breakpoints()))" "1" "check default BP count"
+gdb_test "source ${srcdir}/${subdir}/${testfile}.py" ".*Python script imported.*" \
+	 "import python scripts"
+gdb_test "python print(len(gdb.breakpoints()))" "2" "check modified BP count"
+gdb_test_sequence "continue" "run until program stops" {
+    "Watchpoint Hit: ."
+    "\[\r\n\]+Watchpoint . deleted because the program has left the block in"
+    "\[\r\n\]+which its expression is valid\."
+    "Watchpoint Hit: ."
+    "\[\r\n\]+012\\[Inferior 1 \\(process .*\\) exited normally\\]"
+}
+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 00000000000..2c668aeca77
--- /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/>.
+
+# This file is part of the GDB testsuite.  It checks Watchpoints
+# are deleted after used.
+
+class MyBreakpoint(gdb.Breakpoint):
+    def __init__(self, *args, **kwargs):
+        gdb.Breakpoint.__init__(self, *args, **kwargs)
+        self.i = 0
+    def stop(self):
+        self.i += 1
+        print("Watchpoint Hit:", self.i)
+        return False
+
+MyBreakpoint('i', gdb.BP_WATCHPOINT)
+
+print("Python script imported")
-- 
2.34.1

base-commit: c99b2113a478a075e8d8f7c3848a92f1ce73f847

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH v2] [PR python/29603] Disable out-of-scope watchpoints
  2022-10-01  5:27   ` [PATCH v2] [PR python/29603] Disable out-of-scope watchpoints Johnson Sun
@ 2022-10-20 17:57     ` Johnson Sun
  2022-10-20 18:11       ` [PATCH v3] " Johnson Sun
  0 siblings, 1 reply; 34+ messages in thread
From: Johnson Sun @ 2022-10-20 17:57 UTC (permalink / raw)
  To: Lancelot SIX, gdb-patches; +Cc: j3.soon777

Hi,

I revised the patch after further investigation and 

will send version 3 shortly.

Cheers,
Johnson


^ permalink raw reply	[flat|nested] 34+ messages in thread

* [PATCH v3] [PR python/29603] Disable out-of-scope watchpoints
  2022-10-20 17:57     ` Johnson Sun
@ 2022-10-20 18:11       ` Johnson Sun
  2022-11-18 12:17         ` [PING] " Johnson Sun
  2023-01-13 15:40         ` Simon Marchi
  0 siblings, 2 replies; 34+ messages in thread
From: Johnson Sun @ 2022-10-20 18:11 UTC (permalink / raw)
  To: Lancelot SIX, gdb-patches; +Cc: j3.soon777

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.

Two other solutions seem to solve this issue, but are in fact inappropriate:
1. Automatically delete breakpoints on all kinds of stops
   (in `fetch_inferior_event'). This solution is very slow since the deletion
   requires O(N) time for N breakpoints.
2. Disable the watchpoint after the watchpoint's disposition is set to
   `delete at next stop' (in `watchpoint_del_at_next_stop'). This solution
   modifies a non-extension-related code path, and isn't preferred since this
   issue cannot occur without extension languages. (gdb scripts always normal
   stop before continuing execution)

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29603
---
 gdb/breakpoint.c                           |  2 +
 gdb/testsuite/gdb.python/py-watchpoint.c   | 27 ++++++++++++
 gdb/testsuite/gdb.python/py-watchpoint.exp | 48 ++++++++++++++++++++++
 gdb/testsuite/gdb.python/py-watchpoint.py  | 30 ++++++++++++++
 4 files changed, 107 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 bff3bac7d1a..15f4ae2131c 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -5340,6 +5340,8 @@ bpstat_check_breakpoint_conditions (bpstat *bs, thread_info *thread)
   /* Evaluate extension language breakpoints that have a "stop" method
      implemented.  */
   bs->stop = breakpoint_ext_lang_cond_says_stop (b);
+  if (b->disposition == disp_del_at_next_stop)
+    disable_breakpoint(b);
 
   if (is_watchpoint (b))
     {
diff --git a/gdb/testsuite/gdb.python/py-watchpoint.c b/gdb/testsuite/gdb.python/py-watchpoint.c
new file mode 100644
index 00000000000..4e1760bb05b
--- /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;
+  for (i = 0; i < 3; i++)
+    printf ("%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 00000000000..ac58d75c523
--- /dev/null
+++ b/gdb/testsuite/gdb.python/py-watchpoint.exp
@@ -0,0 +1,48 @@
+# 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
+}
+
+# Skip all tests if Python scripting is not enabled.
+if { [skip_python_tests] } { continue }
+
+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"
+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_sequence "continue" "run until program stops" {
+    "Watchpoint Hit: ."
+    "\[\r\n\]+Watchpoint . deleted because the program has left the block in"
+    "\[\r\n\]+which its expression is valid\."
+    "Watchpoint Hit: ."
+    "\[\r\n\]+012\\[Inferior 1 \\(process .*\\) exited normally\\]"
+}
+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 00000000000..ce5dee118ad
--- /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, *args, **kwargs):
+        gdb.Breakpoint.__init__(self, *args, **kwargs)
+        self.i = 0
+
+    def stop(self):
+        self.i += 1
+        print("Watchpoint Hit:", self.i)
+        return False
+
+
+MyBreakpoint("i", gdb.BP_WATCHPOINT)
+
+print("Python script imported")
-- 
2.34.1

base-commit: c99b2113a478a075e8d8f7c3848a92f1ce73f847

^ permalink raw reply	[flat|nested] 34+ messages in thread

* [PING] [PATCH v3] [PR python/29603] Disable out-of-scope watchpoints
  2022-10-20 18:11       ` [PATCH v3] " Johnson Sun
@ 2022-11-18 12:17         ` Johnson Sun
  2022-11-25 15:11           ` [PING^2] " Johnson Sun
  2023-01-13 15:40         ` Simon Marchi
  1 sibling, 1 reply; 34+ messages in thread
From: Johnson Sun @ 2022-11-18 12:17 UTC (permalink / raw)
  To: Lancelot SIX, gdb-patches

Ping for: 
<https://sourceware.org/pipermail/gdb-patches/2022-October/192909.html>.

Johnson

On 10/21/2022 2:11 AM, 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.
>
> Two other solutions seem to solve this issue, but are in fact inappropriate:
> 1. Automatically delete breakpoints on all kinds of stops
>     (in `fetch_inferior_event'). This solution is very slow since the deletion
>     requires O(N) time for N breakpoints.
> 2. Disable the watchpoint after the watchpoint's disposition is set to
>     `delete at next stop' (in `watchpoint_del_at_next_stop'). This solution
>     modifies a non-extension-related code path, and isn't preferred since this
>     issue cannot occur without extension languages. (gdb scripts always normal
>     stop before continuing execution)
>
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29603
> ---
>   gdb/breakpoint.c                           |  2 +
>   gdb/testsuite/gdb.python/py-watchpoint.c   | 27 ++++++++++++
>   gdb/testsuite/gdb.python/py-watchpoint.exp | 48 ++++++++++++++++++++++
>   gdb/testsuite/gdb.python/py-watchpoint.py  | 30 ++++++++++++++
>   4 files changed, 107 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 bff3bac7d1a..15f4ae2131c 100644
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -5340,6 +5340,8 @@ bpstat_check_breakpoint_conditions (bpstat *bs, thread_info *thread)
>     /* Evaluate extension language breakpoints that have a "stop" method
>        implemented.  */
>     bs->stop = breakpoint_ext_lang_cond_says_stop (b);
> +  if (b->disposition == disp_del_at_next_stop)
> +    disable_breakpoint(b);
>   
>     if (is_watchpoint (b))
>       {
> diff --git a/gdb/testsuite/gdb.python/py-watchpoint.c b/gdb/testsuite/gdb.python/py-watchpoint.c
> new file mode 100644
> index 00000000000..4e1760bb05b
> --- /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;
> +  for (i = 0; i < 3; i++)
> +    printf ("%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 00000000000..ac58d75c523
> --- /dev/null
> +++ b/gdb/testsuite/gdb.python/py-watchpoint.exp
> @@ -0,0 +1,48 @@
> +# 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
> +}
> +
> +# Skip all tests if Python scripting is not enabled.
> +if { [skip_python_tests] } { continue }
> +
> +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"
> +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_sequence "continue" "run until program stops" {
> +    "Watchpoint Hit: ."
> +    "\[\r\n\]+Watchpoint . deleted because the program has left the block in"
> +    "\[\r\n\]+which its expression is valid\."
> +    "Watchpoint Hit: ."
> +    "\[\r\n\]+012\\[Inferior 1 \\(process .*\\) exited normally\\]"
> +}
> +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 00000000000..ce5dee118ad
> --- /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, *args, **kwargs):
> +        gdb.Breakpoint.__init__(self, *args, **kwargs)
> +        self.i = 0
> +
> +    def stop(self):
> +        self.i += 1
> +        print("Watchpoint Hit:", self.i)
> +        return False
> +
> +
> +MyBreakpoint("i", gdb.BP_WATCHPOINT)
> +
> +print("Python script imported")

^ permalink raw reply	[flat|nested] 34+ messages in thread

* [PING^2] [PATCH v3] [PR python/29603] Disable out-of-scope watchpoints
  2022-11-18 12:17         ` [PING] " Johnson Sun
@ 2022-11-25 15:11           ` Johnson Sun
  2022-12-04 16:45             ` [PING^3] " Johnson Sun
  0 siblings, 1 reply; 34+ messages in thread
From: Johnson Sun @ 2022-11-25 15:11 UTC (permalink / raw)
  To: LancelotSIX, gdb-patches

Ping for: 
<https://sourceware.org/pipermail/gdb-patches/2022-October/192909.html>.

Johnson

On 11/18/2022 8:17 PM, JohnsonSun wrote:
> Ping for: 
> <https://sourceware.org/pipermail/gdb-patches/2022-October/192909.html>.
>
> Johnson
>
> On 10/21/2022 2:11 AM, 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.
>>
>> Two other solutions seem to solve this issue, but are in fact 
>> inappropriate:
>> 1. Automatically delete breakpoints on all kinds of stops
>>     (in `fetch_inferior_event'). This solution is very slow since the 
>> deletion
>>     requires O(N) time for N breakpoints.
>> 2. Disable the watchpoint after the watchpoint's disposition is set to
>>     `delete at next stop' (in `watchpoint_del_at_next_stop'). This 
>> solution
>>     modifies a non-extension-related code path, and isn't preferred 
>> since this
>>     issue cannot occur without extension languages. (gdb scripts 
>> always normal
>>     stop before continuing execution)
>>
>> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29603
>> ---
>>   gdb/breakpoint.c                           |  2 +
>>   gdb/testsuite/gdb.python/py-watchpoint.c   | 27 ++++++++++++
>>   gdb/testsuite/gdb.python/py-watchpoint.exp | 48 ++++++++++++++++++++++
>>   gdb/testsuite/gdb.python/py-watchpoint.py  | 30 ++++++++++++++
>>   4 files changed, 107 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 bff3bac7d1a..15f4ae2131c 100644
>> --- a/gdb/breakpoint.c
>> +++ b/gdb/breakpoint.c
>> @@ -5340,6 +5340,8 @@ bpstat_check_breakpoint_conditions (bpstat *bs, 
>> thread_info *thread)
>>     /* Evaluate extension language breakpoints that have a "stop" method
>>        implemented.  */
>>     bs->stop = breakpoint_ext_lang_cond_says_stop (b);
>> +  if (b->disposition == disp_del_at_next_stop)
>> +    disable_breakpoint(b);
>>       if (is_watchpoint (b))
>>       {
>> diff --git a/gdb/testsuite/gdb.python/py-watchpoint.c 
>> b/gdb/testsuite/gdb.python/py-watchpoint.c
>> new file mode 100644
>> index 00000000000..4e1760bb05b
>> --- /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;
>> +  for (i = 0; i < 3; i++)
>> +    printf ("%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 00000000000..ac58d75c523
>> --- /dev/null
>> +++ b/gdb/testsuite/gdb.python/py-watchpoint.exp
>> @@ -0,0 +1,48 @@
>> +# 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
>> +}
>> +
>> +# Skip all tests if Python scripting is not enabled.
>> +if { [skip_python_tests] } { continue }
>> +
>> +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"
>> +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_sequence "continue" "run until program stops" {
>> +    "Watchpoint Hit: ."
>> +    "\[\r\n\]+Watchpoint . deleted because the program has left the 
>> block in"
>> +    "\[\r\n\]+which its expression is valid\."
>> +    "Watchpoint Hit: ."
>> +    "\[\r\n\]+012\\[Inferior 1 \\(process .*\\) exited normally\\]"
>> +}
>> +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 00000000000..ce5dee118ad
>> --- /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, *args, **kwargs):
>> +        gdb.Breakpoint.__init__(self, *args, **kwargs)
>> +        self.i = 0
>> +
>> +    def stop(self):
>> +        self.i += 1
>> +        print("Watchpoint Hit:", self.i)
>> +        return False
>> +
>> +
>> +MyBreakpoint("i", gdb.BP_WATCHPOINT)
>> +
>> +print("Python script imported")
>

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PING^3] [PATCH v3] [PR python/29603] Disable out-of-scope watchpoints
  2022-11-25 15:11           ` [PING^2] " Johnson Sun
@ 2022-12-04 16:45             ` Johnson Sun
  2022-12-12 21:44               ` [PING^4] " Johnson Sun
  0 siblings, 1 reply; 34+ messages in thread
From: Johnson Sun @ 2022-12-04 16:45 UTC (permalink / raw)
  To: LancelotSIX, gdb-patches

Ping for: 
<https://sourceware.org/pipermail/gdb-patches/2022-October/192909.html>.

Johnson

On 11/25/2022 11:11 PM, Johnson Sun wrote:
> Ping for: 
> <https://sourceware.org/pipermail/gdb-patches/2022-October/192909.html>.
>
> Johnson
>
> On 11/18/2022 8:17 PM, JohnsonSun wrote:
>> Ping for: 
>> <https://sourceware.org/pipermail/gdb-patches/2022-October/192909.html>.
>>
>> Johnson
>>
>> On 10/21/2022 2:11 AM, 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.
>>>
>>> Two other solutions seem to solve this issue, but are in fact 
>>> inappropriate:
>>> 1. Automatically delete breakpoints on all kinds of stops
>>>     (in `fetch_inferior_event'). This solution is very slow since 
>>> the deletion
>>>     requires O(N) time for N breakpoints.
>>> 2. Disable the watchpoint after the watchpoint's disposition is set to
>>>     `delete at next stop' (in `watchpoint_del_at_next_stop'). This 
>>> solution
>>>     modifies a non-extension-related code path, and isn't preferred 
>>> since this
>>>     issue cannot occur without extension languages. (gdb scripts 
>>> always normal
>>>     stop before continuing execution)
>>>
>>> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29603
>>> ---
>>>   gdb/breakpoint.c                           |  2 +
>>>   gdb/testsuite/gdb.python/py-watchpoint.c   | 27 ++++++++++++
>>>   gdb/testsuite/gdb.python/py-watchpoint.exp | 48 
>>> ++++++++++++++++++++++
>>>   gdb/testsuite/gdb.python/py-watchpoint.py  | 30 ++++++++++++++
>>>   4 files changed, 107 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 bff3bac7d1a..15f4ae2131c 100644
>>> --- a/gdb/breakpoint.c
>>> +++ b/gdb/breakpoint.c
>>> @@ -5340,6 +5340,8 @@ bpstat_check_breakpoint_conditions (bpstat 
>>> *bs, thread_info *thread)
>>>     /* Evaluate extension language breakpoints that have a "stop" 
>>> method
>>>        implemented.  */
>>>     bs->stop = breakpoint_ext_lang_cond_says_stop (b);
>>> +  if (b->disposition == disp_del_at_next_stop)
>>> +    disable_breakpoint(b);
>>>       if (is_watchpoint (b))
>>>       {
>>> diff --git a/gdb/testsuite/gdb.python/py-watchpoint.c 
>>> b/gdb/testsuite/gdb.python/py-watchpoint.c
>>> new file mode 100644
>>> index 00000000000..4e1760bb05b
>>> --- /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;
>>> +  for (i = 0; i < 3; i++)
>>> +    printf ("%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 00000000000..ac58d75c523
>>> --- /dev/null
>>> +++ b/gdb/testsuite/gdb.python/py-watchpoint.exp
>>> @@ -0,0 +1,48 @@
>>> +# 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
>>> +}
>>> +
>>> +# Skip all tests if Python scripting is not enabled.
>>> +if { [skip_python_tests] } { continue }
>>> +
>>> +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"
>>> +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_sequence "continue" "run until program stops" {
>>> +    "Watchpoint Hit: ."
>>> +    "\[\r\n\]+Watchpoint . deleted because the program has left the 
>>> block in"
>>> +    "\[\r\n\]+which its expression is valid\."
>>> +    "Watchpoint Hit: ."
>>> +    "\[\r\n\]+012\\[Inferior 1 \\(process .*\\) exited normally\\]"
>>> +}
>>> +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 00000000000..ce5dee118ad
>>> --- /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, *args, **kwargs):
>>> +        gdb.Breakpoint.__init__(self, *args, **kwargs)
>>> +        self.i = 0
>>> +
>>> +    def stop(self):
>>> +        self.i += 1
>>> +        print("Watchpoint Hit:", self.i)
>>> +        return False
>>> +
>>> +
>>> +MyBreakpoint("i", gdb.BP_WATCHPOINT)
>>> +
>>> +print("Python script imported")
>>

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PING^4] [PATCH v3] [PR python/29603] Disable out-of-scope watchpoints
  2022-12-04 16:45             ` [PING^3] " Johnson Sun
@ 2022-12-12 21:44               ` Johnson Sun
  2022-12-20 22:08                 ` [PING^5] " Johnson Sun
  0 siblings, 1 reply; 34+ messages in thread
From: Johnson Sun @ 2022-12-12 21:44 UTC (permalink / raw)
  To: LancelotSIX, gdb-patches

Ping for: 
<https://sourceware.org/pipermail/gdb-patches/2022-October/192909.html>.

Johnson

On 12/5/2022 12:45 AM, Johnson Sun wrote:
> Ping for: 
> <https://sourceware.org/pipermail/gdb-patches/2022-October/192909.html>.
>
> Johnson
>
> On 11/25/2022 11:11 PM, Johnson Sun wrote:
>> Ping for: 
>> <https://sourceware.org/pipermail/gdb-patches/2022-October/192909.html>.
>>
>> Johnson
>>
>> On 11/18/2022 8:17 PM, JohnsonSun wrote:
>>> Ping for: 
>>> <https://sourceware.org/pipermail/gdb-patches/2022-October/192909.html>.
>>>
>>> Johnson
>>>
>>> On 10/21/2022 2:11 AM, 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.
>>>>
>>>> Two other solutions seem to solve this issue, but are in fact 
>>>> inappropriate:
>>>> 1. Automatically delete breakpoints on all kinds of stops
>>>>     (in `fetch_inferior_event'). This solution is very slow since 
>>>> the deletion
>>>>     requires O(N) time for N breakpoints.
>>>> 2. Disable the watchpoint after the watchpoint's disposition is set to
>>>>     `delete at next stop' (in `watchpoint_del_at_next_stop'). This 
>>>> solution
>>>>     modifies a non-extension-related code path, and isn't preferred 
>>>> since this
>>>>     issue cannot occur without extension languages. (gdb scripts 
>>>> always normal
>>>>     stop before continuing execution)
>>>>
>>>> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29603
>>>> ---
>>>>   gdb/breakpoint.c                           |  2 +
>>>>   gdb/testsuite/gdb.python/py-watchpoint.c   | 27 ++++++++++++
>>>>   gdb/testsuite/gdb.python/py-watchpoint.exp | 48 
>>>> ++++++++++++++++++++++
>>>>   gdb/testsuite/gdb.python/py-watchpoint.py  | 30 ++++++++++++++
>>>>   4 files changed, 107 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 bff3bac7d1a..15f4ae2131c 100644
>>>> --- a/gdb/breakpoint.c
>>>> +++ b/gdb/breakpoint.c
>>>> @@ -5340,6 +5340,8 @@ bpstat_check_breakpoint_conditions (bpstat 
>>>> *bs, thread_info *thread)
>>>>     /* Evaluate extension language breakpoints that have a "stop" 
>>>> method
>>>>        implemented.  */
>>>>     bs->stop = breakpoint_ext_lang_cond_says_stop (b);
>>>> +  if (b->disposition == disp_del_at_next_stop)
>>>> +    disable_breakpoint(b);
>>>>       if (is_watchpoint (b))
>>>>       {
>>>> diff --git a/gdb/testsuite/gdb.python/py-watchpoint.c 
>>>> b/gdb/testsuite/gdb.python/py-watchpoint.c
>>>> new file mode 100644
>>>> index 00000000000..4e1760bb05b
>>>> --- /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;
>>>> +  for (i = 0; i < 3; i++)
>>>> +    printf ("%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 00000000000..ac58d75c523
>>>> --- /dev/null
>>>> +++ b/gdb/testsuite/gdb.python/py-watchpoint.exp
>>>> @@ -0,0 +1,48 @@
>>>> +# 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
>>>> +}
>>>> +
>>>> +# Skip all tests if Python scripting is not enabled.
>>>> +if { [skip_python_tests] } { continue }
>>>> +
>>>> +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"
>>>> +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_sequence "continue" "run until program stops" {
>>>> +    "Watchpoint Hit: ."
>>>> +    "\[\r\n\]+Watchpoint . deleted because the program has left 
>>>> the block in"
>>>> +    "\[\r\n\]+which its expression is valid\."
>>>> +    "Watchpoint Hit: ."
>>>> +    "\[\r\n\]+012\\[Inferior 1 \\(process .*\\) exited normally\\]"
>>>> +}
>>>> +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 00000000000..ce5dee118ad
>>>> --- /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, *args, **kwargs):
>>>> +        gdb.Breakpoint.__init__(self, *args, **kwargs)
>>>> +        self.i = 0
>>>> +
>>>> +    def stop(self):
>>>> +        self.i += 1
>>>> +        print("Watchpoint Hit:", self.i)
>>>> +        return False
>>>> +
>>>> +
>>>> +MyBreakpoint("i", gdb.BP_WATCHPOINT)
>>>> +
>>>> +print("Python script imported")
>>>

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PING^5] [PATCH v3] [PR python/29603] Disable out-of-scope watchpoints
  2022-12-12 21:44               ` [PING^4] " Johnson Sun
@ 2022-12-20 22:08                 ` Johnson Sun
  2022-12-27 16:40                   ` [PING^6] " Johnson Sun
  0 siblings, 1 reply; 34+ messages in thread
From: Johnson Sun @ 2022-12-20 22:08 UTC (permalink / raw)
  To: LancelotSIX, gdb-patches

Ping for: 
<https://sourceware.org/pipermail/gdb-patches/2022-October/192909.html>.

Johnson

On 12/13/2022 5:44 AM, Johnson Sun wrote:
> Ping for: 
> <https://sourceware.org/pipermail/gdb-patches/2022-October/192909.html>.
>
> Johnson
>
> On 12/5/2022 12:45 AM, Johnson Sun wrote:
>> Ping for: 
>> <https://sourceware.org/pipermail/gdb-patches/2022-October/192909.html>.
>>
>> Johnson
>>
>> On 11/25/2022 11:11 PM, Johnson Sun wrote:
>>> Ping for: 
>>> <https://sourceware.org/pipermail/gdb-patches/2022-October/192909.html>.
>>>
>>> Johnson
>>>
>>> On 11/18/2022 8:17 PM, JohnsonSun wrote:
>>>> Ping for: 
>>>> <https://sourceware.org/pipermail/gdb-patches/2022-October/192909.html>.
>>>>
>>>> Johnson
>>>>
>>>> On 10/21/2022 2:11 AM, 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.
>>>>>
>>>>> Two other solutions seem to solve this issue, but are in fact 
>>>>> inappropriate:
>>>>> 1. Automatically delete breakpoints on all kinds of stops
>>>>>     (in `fetch_inferior_event'). This solution is very slow since 
>>>>> the deletion
>>>>>     requires O(N) time for N breakpoints.
>>>>> 2. Disable the watchpoint after the watchpoint's disposition is 
>>>>> set to
>>>>>     `delete at next stop' (in `watchpoint_del_at_next_stop'). This 
>>>>> solution
>>>>>     modifies a non-extension-related code path, and isn't 
>>>>> preferred since this
>>>>>     issue cannot occur without extension languages. (gdb scripts 
>>>>> always normal
>>>>>     stop before continuing execution)
>>>>>
>>>>> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29603
>>>>> ---
>>>>>   gdb/breakpoint.c                           |  2 +
>>>>>   gdb/testsuite/gdb.python/py-watchpoint.c   | 27 ++++++++++++
>>>>>   gdb/testsuite/gdb.python/py-watchpoint.exp | 48 
>>>>> ++++++++++++++++++++++
>>>>>   gdb/testsuite/gdb.python/py-watchpoint.py  | 30 ++++++++++++++
>>>>>   4 files changed, 107 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 bff3bac7d1a..15f4ae2131c 100644
>>>>> --- a/gdb/breakpoint.c
>>>>> +++ b/gdb/breakpoint.c
>>>>> @@ -5340,6 +5340,8 @@ bpstat_check_breakpoint_conditions (bpstat 
>>>>> *bs, thread_info *thread)
>>>>>     /* Evaluate extension language breakpoints that have a "stop" 
>>>>> method
>>>>>        implemented.  */
>>>>>     bs->stop = breakpoint_ext_lang_cond_says_stop (b);
>>>>> +  if (b->disposition == disp_del_at_next_stop)
>>>>> +    disable_breakpoint(b);
>>>>>       if (is_watchpoint (b))
>>>>>       {
>>>>> diff --git a/gdb/testsuite/gdb.python/py-watchpoint.c 
>>>>> b/gdb/testsuite/gdb.python/py-watchpoint.c
>>>>> new file mode 100644
>>>>> index 00000000000..4e1760bb05b
>>>>> --- /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;
>>>>> +  for (i = 0; i < 3; i++)
>>>>> +    printf ("%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 00000000000..ac58d75c523
>>>>> --- /dev/null
>>>>> +++ b/gdb/testsuite/gdb.python/py-watchpoint.exp
>>>>> @@ -0,0 +1,48 @@
>>>>> +# 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
>>>>> +}
>>>>> +
>>>>> +# Skip all tests if Python scripting is not enabled.
>>>>> +if { [skip_python_tests] } { continue }
>>>>> +
>>>>> +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"
>>>>> +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_sequence "continue" "run until program stops" {
>>>>> +    "Watchpoint Hit: ."
>>>>> +    "\[\r\n\]+Watchpoint . deleted because the program has left 
>>>>> the block in"
>>>>> +    "\[\r\n\]+which its expression is valid\."
>>>>> +    "Watchpoint Hit: ."
>>>>> +    "\[\r\n\]+012\\[Inferior 1 \\(process .*\\) exited normally\\]"
>>>>> +}
>>>>> +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 00000000000..ce5dee118ad
>>>>> --- /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, *args, **kwargs):
>>>>> +        gdb.Breakpoint.__init__(self, *args, **kwargs)
>>>>> +        self.i = 0
>>>>> +
>>>>> +    def stop(self):
>>>>> +        self.i += 1
>>>>> +        print("Watchpoint Hit:", self.i)
>>>>> +        return False
>>>>> +
>>>>> +
>>>>> +MyBreakpoint("i", gdb.BP_WATCHPOINT)
>>>>> +
>>>>> +print("Python script imported")
>>>>

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PING^6] [PATCH v3] [PR python/29603] Disable out-of-scope watchpoints
  2022-12-20 22:08                 ` [PING^5] " Johnson Sun
@ 2022-12-27 16:40                   ` Johnson Sun
  2023-01-12 18:34                     ` [PING^7] " Johnson Sun
  0 siblings, 1 reply; 34+ messages in thread
From: Johnson Sun @ 2022-12-27 16:40 UTC (permalink / raw)
  To: LancelotSIX, gdb-patches

Ping for: 
<https://sourceware.org/pipermail/gdb-patches/2022-October/192909.html>.

Johnson

On 12/21/2022 6:08 AM, Johnson Sun wrote:
> Ping for: 
> <https://sourceware.org/pipermail/gdb-patches/2022-October/192909.html>.
>
> Johnson
>
> On 12/13/2022 5:44 AM, Johnson Sun wrote:
>> Ping for: 
>> <https://sourceware.org/pipermail/gdb-patches/2022-October/192909.html>.
>>
>> Johnson
>>
>> On 12/5/2022 12:45 AM, Johnson Sun wrote:
>>> Ping for: 
>>> <https://sourceware.org/pipermail/gdb-patches/2022-October/192909.html>.
>>>
>>> Johnson
>>>
>>> On 11/25/2022 11:11 PM, Johnson Sun wrote:
>>>> Ping for: 
>>>> <https://sourceware.org/pipermail/gdb-patches/2022-October/192909.html>.
>>>>
>>>> Johnson
>>>>
>>>> On 11/18/2022 8:17 PM, JohnsonSun wrote:
>>>>> Ping for: 
>>>>> <https://sourceware.org/pipermail/gdb-patches/2022-October/192909.html>.
>>>>>
>>>>> Johnson
>>>>>
>>>>> On 10/21/2022 2:11 AM, 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.
>>>>>>
>>>>>> Two other solutions seem to solve this issue, but are in fact 
>>>>>> inappropriate:
>>>>>> 1. Automatically delete breakpoints on all kinds of stops
>>>>>>     (in `fetch_inferior_event'). This solution is very slow since 
>>>>>> the deletion
>>>>>>     requires O(N) time for N breakpoints.
>>>>>> 2. Disable the watchpoint after the watchpoint's disposition is 
>>>>>> set to
>>>>>>     `delete at next stop' (in `watchpoint_del_at_next_stop'). 
>>>>>> This solution
>>>>>>     modifies a non-extension-related code path, and isn't 
>>>>>> preferred since this
>>>>>>     issue cannot occur without extension languages. (gdb scripts 
>>>>>> always normal
>>>>>>     stop before continuing execution)
>>>>>>
>>>>>> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29603
>>>>>> ---
>>>>>>   gdb/breakpoint.c                           |  2 +
>>>>>>   gdb/testsuite/gdb.python/py-watchpoint.c   | 27 ++++++++++++
>>>>>>   gdb/testsuite/gdb.python/py-watchpoint.exp | 48 
>>>>>> ++++++++++++++++++++++
>>>>>>   gdb/testsuite/gdb.python/py-watchpoint.py  | 30 ++++++++++++++
>>>>>>   4 files changed, 107 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 bff3bac7d1a..15f4ae2131c 100644
>>>>>> --- a/gdb/breakpoint.c
>>>>>> +++ b/gdb/breakpoint.c
>>>>>> @@ -5340,6 +5340,8 @@ bpstat_check_breakpoint_conditions (bpstat 
>>>>>> *bs, thread_info *thread)
>>>>>>     /* Evaluate extension language breakpoints that have a "stop" 
>>>>>> method
>>>>>>        implemented.  */
>>>>>>     bs->stop = breakpoint_ext_lang_cond_says_stop (b);
>>>>>> +  if (b->disposition == disp_del_at_next_stop)
>>>>>> +    disable_breakpoint(b);
>>>>>>       if (is_watchpoint (b))
>>>>>>       {
>>>>>> diff --git a/gdb/testsuite/gdb.python/py-watchpoint.c 
>>>>>> b/gdb/testsuite/gdb.python/py-watchpoint.c
>>>>>> new file mode 100644
>>>>>> index 00000000000..4e1760bb05b
>>>>>> --- /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;
>>>>>> +  for (i = 0; i < 3; i++)
>>>>>> +    printf ("%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 00000000000..ac58d75c523
>>>>>> --- /dev/null
>>>>>> +++ b/gdb/testsuite/gdb.python/py-watchpoint.exp
>>>>>> @@ -0,0 +1,48 @@
>>>>>> +# 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
>>>>>> +}
>>>>>> +
>>>>>> +# Skip all tests if Python scripting is not enabled.
>>>>>> +if { [skip_python_tests] } { continue }
>>>>>> +
>>>>>> +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"
>>>>>> +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_sequence "continue" "run until program stops" {
>>>>>> +    "Watchpoint Hit: ."
>>>>>> +    "\[\r\n\]+Watchpoint . deleted because the program has left 
>>>>>> the block in"
>>>>>> +    "\[\r\n\]+which its expression is valid\."
>>>>>> +    "Watchpoint Hit: ."
>>>>>> +    "\[\r\n\]+012\\[Inferior 1 \\(process .*\\) exited normally\\]"
>>>>>> +}
>>>>>> +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 00000000000..ce5dee118ad
>>>>>> --- /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, *args, **kwargs):
>>>>>> +        gdb.Breakpoint.__init__(self, *args, **kwargs)
>>>>>> +        self.i = 0
>>>>>> +
>>>>>> +    def stop(self):
>>>>>> +        self.i += 1
>>>>>> +        print("Watchpoint Hit:", self.i)
>>>>>> +        return False
>>>>>> +
>>>>>> +
>>>>>> +MyBreakpoint("i", gdb.BP_WATCHPOINT)
>>>>>> +
>>>>>> +print("Python script imported")
>>>>>

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PING^7] [PATCH v3] [PR python/29603] Disable out-of-scope watchpoints
  2022-12-27 16:40                   ` [PING^6] " Johnson Sun
@ 2023-01-12 18:34                     ` Johnson Sun
  0 siblings, 0 replies; 34+ messages in thread
From: Johnson Sun @ 2023-01-12 18:34 UTC (permalink / raw)
  To: LancelotSIX, gdb-patches

Ping for: 
<https://sourceware.org/pipermail/gdb-patches/2022-October/192909.html>.

Johnson

On 12/28/2022 12:40 AM, JohnsonSun wrote:
> Ping for: 
> <https://sourceware.org/pipermail/gdb-patches/2022-October/192909.html>.
>
> Johnson
>
> On 12/21/2022 6:08 AM, Johnson Sun wrote:
>> Ping for: 
>> <https://sourceware.org/pipermail/gdb-patches/2022-October/192909.html>.
>>
>> Johnson
>>
>> On 12/13/2022 5:44 AM, Johnson Sun wrote:
>>> Ping for: 
>>> <https://sourceware.org/pipermail/gdb-patches/2022-October/192909.html>.
>>>
>>> Johnson
>>>
>>> On 12/5/2022 12:45 AM, Johnson Sun wrote:
>>>> Ping for: 
>>>> <https://sourceware.org/pipermail/gdb-patches/2022-October/192909.html>.
>>>>
>>>> Johnson
>>>>
>>>> On 11/25/2022 11:11 PM, Johnson Sun wrote:
>>>>> Ping for: 
>>>>> <https://sourceware.org/pipermail/gdb-patches/2022-October/192909.html>.
>>>>>
>>>>> Johnson
>>>>>
>>>>> On 11/18/2022 8:17 PM, JohnsonSun wrote:
>>>>>> Ping for: 
>>>>>> <https://sourceware.org/pipermail/gdb-patches/2022-October/192909.html>.
>>>>>>
>>>>>> Johnson
>>>>>>
>>>>>> On 10/21/2022 2:11 AM, 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.
>>>>>>>
>>>>>>> Two other solutions seem to solve this issue, but are in fact 
>>>>>>> inappropriate:
>>>>>>> 1. Automatically delete breakpoints on all kinds of stops
>>>>>>>     (in `fetch_inferior_event'). This solution is very slow 
>>>>>>> since the deletion
>>>>>>>     requires O(N) time for N breakpoints.
>>>>>>> 2. Disable the watchpoint after the watchpoint's disposition is 
>>>>>>> set to
>>>>>>>     `delete at next stop' (in `watchpoint_del_at_next_stop'). 
>>>>>>> This solution
>>>>>>>     modifies a non-extension-related code path, and isn't 
>>>>>>> preferred since this
>>>>>>>     issue cannot occur without extension languages. (gdb scripts 
>>>>>>> always normal
>>>>>>>     stop before continuing execution)
>>>>>>>
>>>>>>> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29603
>>>>>>> ---
>>>>>>>   gdb/breakpoint.c                           |  2 +
>>>>>>>   gdb/testsuite/gdb.python/py-watchpoint.c   | 27 ++++++++++++
>>>>>>>   gdb/testsuite/gdb.python/py-watchpoint.exp | 48 
>>>>>>> ++++++++++++++++++++++
>>>>>>>   gdb/testsuite/gdb.python/py-watchpoint.py  | 30 ++++++++++++++
>>>>>>>   4 files changed, 107 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 bff3bac7d1a..15f4ae2131c 100644
>>>>>>> --- a/gdb/breakpoint.c
>>>>>>> +++ b/gdb/breakpoint.c
>>>>>>> @@ -5340,6 +5340,8 @@ bpstat_check_breakpoint_conditions (bpstat 
>>>>>>> *bs, thread_info *thread)
>>>>>>>     /* Evaluate extension language breakpoints that have a 
>>>>>>> "stop" method
>>>>>>>        implemented.  */
>>>>>>>     bs->stop = breakpoint_ext_lang_cond_says_stop (b);
>>>>>>> +  if (b->disposition == disp_del_at_next_stop)
>>>>>>> +    disable_breakpoint(b);
>>>>>>>       if (is_watchpoint (b))
>>>>>>>       {
>>>>>>> diff --git a/gdb/testsuite/gdb.python/py-watchpoint.c 
>>>>>>> b/gdb/testsuite/gdb.python/py-watchpoint.c
>>>>>>> new file mode 100644
>>>>>>> index 00000000000..4e1760bb05b
>>>>>>> --- /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;
>>>>>>> +  for (i = 0; i < 3; i++)
>>>>>>> +    printf ("%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 00000000000..ac58d75c523
>>>>>>> --- /dev/null
>>>>>>> +++ b/gdb/testsuite/gdb.python/py-watchpoint.exp
>>>>>>> @@ -0,0 +1,48 @@
>>>>>>> +# 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
>>>>>>> +}
>>>>>>> +
>>>>>>> +# Skip all tests if Python scripting is not enabled.
>>>>>>> +if { [skip_python_tests] } { continue }
>>>>>>> +
>>>>>>> +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"
>>>>>>> +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_sequence "continue" "run until program stops" {
>>>>>>> +    "Watchpoint Hit: ."
>>>>>>> +    "\[\r\n\]+Watchpoint . deleted because the program has left 
>>>>>>> the block in"
>>>>>>> +    "\[\r\n\]+which its expression is valid\."
>>>>>>> +    "Watchpoint Hit: ."
>>>>>>> +    "\[\r\n\]+012\\[Inferior 1 \\(process .*\\) exited 
>>>>>>> normally\\]"
>>>>>>> +}
>>>>>>> +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 00000000000..ce5dee118ad
>>>>>>> --- /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, *args, **kwargs):
>>>>>>> +        gdb.Breakpoint.__init__(self, *args, **kwargs)
>>>>>>> +        self.i = 0
>>>>>>> +
>>>>>>> +    def stop(self):
>>>>>>> +        self.i += 1
>>>>>>> +        print("Watchpoint Hit:", self.i)
>>>>>>> +        return False
>>>>>>> +
>>>>>>> +
>>>>>>> +MyBreakpoint("i", gdb.BP_WATCHPOINT)
>>>>>>> +
>>>>>>> +print("Python script imported")
>>>>>>
>

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH v3] [PR python/29603] Disable out-of-scope watchpoints
  2022-10-20 18:11       ` [PATCH v3] " Johnson Sun
  2022-11-18 12:17         ` [PING] " Johnson Sun
@ 2023-01-13 15:40         ` Simon Marchi
  2023-01-23 10:15           ` Johnson Sun
  1 sibling, 1 reply; 34+ messages in thread
From: Simon Marchi @ 2023-01-13 15:40 UTC (permalink / raw)
  To: Johnson Sun, Lancelot SIX, gdb-patches



On 10/20/22 14:11, Johnson Sun via Gdb-patches 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.
> 
> Two other solutions seem to solve this issue, but are in fact inappropriate:
> 1. Automatically delete breakpoints on all kinds of stops
>    (in `fetch_inferior_event'). This solution is very slow since the deletion
>    requires O(N) time for N breakpoints.
> 2. Disable the watchpoint after the watchpoint's disposition is set to
>    `delete at next stop' (in `watchpoint_del_at_next_stop'). This solution
>    modifies a non-extension-related code path, and isn't preferred since this
>    issue cannot occur without extension languages. (gdb scripts always normal
>    stop before continuing execution)

I have a bit of trouble reviewing this, because I'm not too familiar
with the lifecycle of watchpoints (I know the principle, but not the
specifically where things happen in GDB).  So it's difficult to tell
whether this is right or if there's a better way to handle it.

However, the supplied test does not pass for me:

    source /home/simark/build/binutils-gdb/gdb/testsuite/outputs/gdb.python/py-watchpoint/py-watchpoint.py
    Watchpoint 2: i
    Python script imported
    (gdb) PASS: gdb.python/py-watchpoint.exp: import python scripts
    python print(len(gdb.breakpoints()))
    2
    (gdb) PASS: gdb.python/py-watchpoint.exp: check modified BP count
    gdb_expect_list pattern: /Watchpoint Hit: ./
    continue
    Continuing.
    Watchpoint Hit: 1
    gdb_expect_list pattern: /[
    ]+Watchpoint . deleted because the program has left the block in/
    FAIL: gdb.python/py-watchpoint.exp: run until program stops (pattern 2) (timeout)
    gdb_expect_list pattern: /[
    ]+which its expression is valid./
    gdb_expect_list pattern: /Watchpoint Hit: ./
    gdb_expect_list pattern: /[
    ]+012\[Inferior 1 \(process .*\) exited normally\]/
    gdb_expect_list pattern: //
    python print(len(gdb.breakpoints()))
    Watchpoint Hit: 2
    Watchpoint Hit: 3
    Watchpoint Hit: 4

    Watchpoint 2 deleted because the program has left the block in
    which its expression is valid.
    Watchpoint Hit: 5
    012[Inferior 1 (process 2381681) exited normally]
    (gdb) FAIL: gdb.python/py-watchpoint.exp: check BP count (the program exited)

> 
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29603
> ---
>  gdb/breakpoint.c                           |  2 +
>  gdb/testsuite/gdb.python/py-watchpoint.c   | 27 ++++++++++++
>  gdb/testsuite/gdb.python/py-watchpoint.exp | 48 ++++++++++++++++++++++
>  gdb/testsuite/gdb.python/py-watchpoint.py  | 30 ++++++++++++++
>  4 files changed, 107 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 bff3bac7d1a..15f4ae2131c 100644
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -5340,6 +5340,8 @@ bpstat_check_breakpoint_conditions (bpstat *bs, thread_info *thread)
>    /* Evaluate extension language breakpoints that have a "stop" method
>       implemented.  */
>    bs->stop = breakpoint_ext_lang_cond_says_stop (b);
> +  if (b->disposition == disp_del_at_next_stop)
> +    disable_breakpoint(b);

Is there a reason to do it here in particular, and not, let's say as
soon as we change the disposition to disp_del_at_next_stop?

Other question: shouldn't marking the watchpoint as
disp_del_at_next_stop make it so it won't be inserted next time we
resume?  After all should_be_inserted returns false for breakpoint
locations that are disp_del_at_next_stop.  Perhaps it's because since we
don't do a "normal" stop, breakpoint locations stay inserted, and
there's nothing that pulls this location out of the target?  Therefore,
maybe a solution, to keep things coherent, would be to pull the
breakpoint locations out when marking the breakpoint as
disp_del_at_next_stop?  This way, we would respect the invariant that a
breakpoint disp_del_at_next_stop can't be inserted (I don't know if this
is really what is happening though, it's just speculation).

And, in a broader scope, why wait until the next normal stop to delete
the watchpoint?  A "normal" stop being a stop that we present to the
user (the normal_stop function).  We currently call
breakpoint_auto_delete in normal_stop, which is why we don't reach it
when your Breakpoint.stop method returns False.  At the end of, let's
say, handle_signal_stop, could we go over the bpstat chain and delete
any breakpoint we've just hit that is marked for deletion?

Simon

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH v3] [PR python/29603] Disable out-of-scope watchpoints
  2023-01-13 15:40         ` Simon Marchi
@ 2023-01-23 10:15           ` Johnson Sun
  2023-02-18 16:26             ` [PING] " Johnson Sun
  2023-03-13 16:00             ` Simon Marchi
  0 siblings, 2 replies; 34+ messages in thread
From: Johnson Sun @ 2023-01-23 10:15 UTC (permalink / raw)
  To: SimonMarchi, LancelotSIX, gdb-patches

Hi,

Thanks for reviewing this!

 > the supplied test does not pass for me

The current test doesn't seem to produce consistent results across different
machines, I'll try to fix it in the next version.

 > Is there a reason to do it here in particular, and not, let's say as
 > soon as we change the disposition to disp_del_at_next_stop?

I have implemented this in the v1 patch, I called `disable_breakpoint' as
soon as we change the disposition to `disp_del_at_next_stop'
(in `watchpoint_del_at_next_stop'). However,
LancelotSIX <lsix@lancelotsix.com> mentioned that the fix is in a
non-extension-related code path, and suggested disabling it in
`bpstat_check_breakpoint_conditions' instead (the v3 patch).
See: https://sourceware.org/pipermail/gdb-patches/2022-September/192120.html

Both the v1 and v3 patch fixes the issue. I personally prefer the v1 patch.
(i.e., modifying `watchpoint_del_at_next_stop')

 > shouldn't marking the watchpoint as
 > disp_del_at_next_stop make it so it won't be inserted next time we
 > resume?  After all should_be_inserted returns false for breakpoint
 > locations that are disp_del_at_next_stop.  Perhaps it's because since we
 > don't do a "normal" stop, breakpoint locations stay inserted, and
 > there's nothing that pulls this location out of the target? Therefore,
 > maybe a solution, to keep things coherent, would be to pull the
 > breakpoint locations out when marking the breakpoint as
 > disp_del_at_next_stop?  This way, we would respect the invariant that a
 > breakpoint disp_del_at_next_stop can't be inserted (I don't know if this
 > is really what is happening though, it's just speculation).

For software watchpoints, they cannot be pulled out of the target, since
they may not be inserted into the target in the first place:

      /* Locations of type bp_loc_other and
     bp_loc_software_watchpoint are only maintained at GDB side,
     so there is no need to remove them.  */

     -- gdb/breakpoint.c:3840

Their expressions are re-evaluated by single-stepping through the program:

     Software watchpoints are very slow, since GDB needs to single-step
     the program being debugged and test the value of the watched
     expression(s) after each instruction.

     -- https://sourceware.org/gdb/wiki/Internals%20Watchpoints

Therefore, we must either disable or delete the software watchpoint.
We cannot pull it out of the target since it's only maintained on the
GDB side.

 > And, in a broader scope, why wait until the next normal stop to delete
 > the watchpoint?  A "normal" stop being a stop that we present to the
 > user (the normal_stop function).  We currently call
 > breakpoint_auto_delete in normal_stop, which is why we don't reach it
 > when your Breakpoint.stop method returns False.  At the end of, let's
 > say, handle_signal_stop, could we go over the bpstat chain and delete
 > any breakpoint we've just hit that is marked for deletion?

I believe this choice is due to performance issues.

In an early attempt, I tried to call `breakpoint_auto_delete' on all kinds
of stops. However, this implementation is very slow when we have a large
number of breakpoints, since we need to go over the entire bpstat chain on
all stops. I recall that this implementation fails on certain testcases with
a large number of breakpoints with many breakpoint stops.

Furthermore, the reason for using the `disp_del_at_next_stop' state remains
unclear, as mentioned in the comments:

     /* NOTE drow/2003-09-08: This state only exists for removing
        watchpoints.  It's not clear that it's necessary...  */

     -- gdb/breakpoint.c:2914

By tracing the git history, the `disp_del_at_next_stop' state is introduced
in commit c906108c21474dfb4ed285bcc0ac6fe02cd400cc, which doesn't provide
any proper explanation of this state.

I think the best way to deal with this is to avoid going over the entire
bpstat chain when deleting breakpoints. Potentially by keeping track of
a chain of breakpoints that should be deleted, and changing the bpstat chain
to a doubly linked list for the ease of deletion. Such changes deserve a
dedicated patch, though.

To sum up, I prefer modifying `watchpoint_del_at_next_stop' (i.e., the 
v1 patch).
If you also think it's appropriate, I'll fix the failing test and 
prepare the
v4 patch accordingly.

Johnson

On 1/13/2023 11:40 PM, SimonMarchi wrote:
>
> On 10/20/22 14:11, Johnson Sun via Gdb-patches 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.
>>
>> Two other solutions seem to solve this issue, but are in fact inappropriate:
>> 1. Automatically delete breakpoints on all kinds of stops
>>     (in `fetch_inferior_event'). This solution is very slow since the deletion
>>     requires O(N) time for N breakpoints.
>> 2. Disable the watchpoint after the watchpoint's disposition is set to
>>     `delete at next stop' (in `watchpoint_del_at_next_stop'). This solution
>>     modifies a non-extension-related code path, and isn't preferred since this
>>     issue cannot occur without extension languages. (gdb scripts always normal
>>     stop before continuing execution)
> I have a bit of trouble reviewing this, because I'm not too familiar
> with the lifecycle of watchpoints (I know the principle, but not the
> specifically where things happen in GDB).  So it's difficult to tell
> whether this is right or if there's a better way to handle it.
>
> However, the supplied test does not pass for me:
>
>      source /home/simark/build/binutils-gdb/gdb/testsuite/outputs/gdb.python/py-watchpoint/py-watchpoint.py
>      Watchpoint 2: i
>      Python script imported
>      (gdb) PASS: gdb.python/py-watchpoint.exp: import python scripts
>      python print(len(gdb.breakpoints()))
>      2
>      (gdb) PASS: gdb.python/py-watchpoint.exp: check modified BP count
>      gdb_expect_list pattern: /Watchpoint Hit: ./
>      continue
>      Continuing.
>      Watchpoint Hit: 1
>      gdb_expect_list pattern: /[
>      ]+Watchpoint . deleted because the program has left the block in/
>      FAIL: gdb.python/py-watchpoint.exp: run until program stops (pattern 2) (timeout)
>      gdb_expect_list pattern: /[
>      ]+which its expression is valid./
>      gdb_expect_list pattern: /Watchpoint Hit: ./
>      gdb_expect_list pattern: /[
>      ]+012\[Inferior 1 \(process .*\) exited normally\]/
>      gdb_expect_list pattern: //
>      python print(len(gdb.breakpoints()))
>      Watchpoint Hit: 2
>      Watchpoint Hit: 3
>      Watchpoint Hit: 4
>
>      Watchpoint 2 deleted because the program has left the block in
>      which its expression is valid.
>      Watchpoint Hit: 5
>      012[Inferior 1 (process 2381681) exited normally]
>      (gdb) FAIL: gdb.python/py-watchpoint.exp: check BP count (the program exited)
>
>> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29603
>> ---
>>   gdb/breakpoint.c                           |  2 +
>>   gdb/testsuite/gdb.python/py-watchpoint.c   | 27 ++++++++++++
>>   gdb/testsuite/gdb.python/py-watchpoint.exp | 48 ++++++++++++++++++++++
>>   gdb/testsuite/gdb.python/py-watchpoint.py  | 30 ++++++++++++++
>>   4 files changed, 107 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 bff3bac7d1a..15f4ae2131c 100644
>> --- a/gdb/breakpoint.c
>> +++ b/gdb/breakpoint.c
>> @@ -5340,6 +5340,8 @@ bpstat_check_breakpoint_conditions (bpstat *bs, thread_info *thread)
>>     /* Evaluate extension language breakpoints that have a "stop" method
>>        implemented.  */
>>     bs->stop = breakpoint_ext_lang_cond_says_stop (b);
>> +  if (b->disposition == disp_del_at_next_stop)
>> +    disable_breakpoint(b);
> Is there a reason to do it here in particular, and not, let's say as
> soon as we change the disposition to disp_del_at_next_stop?
>
> Other question: shouldn't marking the watchpoint as
> disp_del_at_next_stop make it so it won't be inserted next time we
> resume?  After all should_be_inserted returns false for breakpoint
> locations that are disp_del_at_next_stop.  Perhaps it's because since we
> don't do a "normal" stop, breakpoint locations stay inserted, and
> there's nothing that pulls this location out of the target?  Therefore,
> maybe a solution, to keep things coherent, would be to pull the
> breakpoint locations out when marking the breakpoint as
> disp_del_at_next_stop?  This way, we would respect the invariant that a
> breakpoint disp_del_at_next_stop can't be inserted (I don't know if this
> is really what is happening though, it's just speculation).
>
> And, in a broader scope, why wait until the next normal stop to delete
> the watchpoint?  A "normal" stop being a stop that we present to the
> user (the normal_stop function).  We currently call
> breakpoint_auto_delete in normal_stop, which is why we don't reach it
> when your Breakpoint.stop method returns False.  At the end of, let's
> say, handle_signal_stop, could we go over the bpstat chain and delete
> any breakpoint we've just hit that is marked for deletion?
>
> Simon
>

^ permalink raw reply	[flat|nested] 34+ messages in thread

* [PING] [PATCH v3] [PR python/29603] Disable out-of-scope watchpoints
  2023-01-23 10:15           ` Johnson Sun
@ 2023-02-18 16:26             ` Johnson Sun
  2023-02-26  6:16               ` [RFC] " Johnson Sun
  2023-03-13 16:00             ` Simon Marchi
  1 sibling, 1 reply; 34+ messages in thread
From: Johnson Sun @ 2023-02-18 16:26 UTC (permalink / raw)
  To: SimonMarchi, LancelotSIX, gdb-patches

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

I would like to ask for some feedback before submitting the v4 patch.

Johnson

On 1/23/2023 6:15 PM, Johnson Sun wrote:
> Hi,
>
> Thanks for reviewing this!
>
> > the supplied test does not pass for me
>
> The current test doesn't seem to produce consistent results across 
> different
> machines, I'll try to fix it in the next version.
>
> > Is there a reason to do it here in particular, and not, let's say as
> > soon as we change the disposition to disp_del_at_next_stop?
>
> I have implemented this in the v1 patch, I called `disable_breakpoint' as
> soon as we change the disposition to `disp_del_at_next_stop'
> (in `watchpoint_del_at_next_stop'). However,
> LancelotSIX <lsix@lancelotsix.com> mentioned that the fix is in a
> non-extension-related code path, and suggested disabling it in
> `bpstat_check_breakpoint_conditions' instead (the v3 patch).
> See: 
> https://sourceware.org/pipermail/gdb-patches/2022-September/192120.html
>
> Both the v1 and v3 patch fixes the issue. I personally prefer the v1 
> patch.
> (i.e., modifying `watchpoint_del_at_next_stop')
>
> > shouldn't marking the watchpoint as
> > disp_del_at_next_stop make it so it won't be inserted next time we
> > resume?  After all should_be_inserted returns false for breakpoint
> > locations that are disp_del_at_next_stop.  Perhaps it's because 
> since we
> > don't do a "normal" stop, breakpoint locations stay inserted, and
> > there's nothing that pulls this location out of the target? Therefore,
> > maybe a solution, to keep things coherent, would be to pull the
> > breakpoint locations out when marking the breakpoint as
> > disp_del_at_next_stop?  This way, we would respect the invariant that a
> > breakpoint disp_del_at_next_stop can't be inserted (I don't know if 
> this
> > is really what is happening though, it's just speculation).
>
> For software watchpoints, they cannot be pulled out of the target, since
> they may not be inserted into the target in the first place:
>
>      /* Locations of type bp_loc_other and
>     bp_loc_software_watchpoint are only maintained at GDB side,
>     so there is no need to remove them.  */
>
>     -- gdb/breakpoint.c:3840
>
> Their expressions are re-evaluated by single-stepping through the 
> program:
>
>     Software watchpoints are very slow, since GDB needs to single-step
>     the program being debugged and test the value of the watched
>     expression(s) after each instruction.
>
>     -- https://sourceware.org/gdb/wiki/Internals%20Watchpoints
>
> Therefore, we must either disable or delete the software watchpoint.
> We cannot pull it out of the target since it's only maintained on the
> GDB side.
>
> > And, in a broader scope, why wait until the next normal stop to delete
> > the watchpoint?  A "normal" stop being a stop that we present to the
> > user (the normal_stop function).  We currently call
> > breakpoint_auto_delete in normal_stop, which is why we don't reach it
> > when your Breakpoint.stop method returns False.  At the end of, let's
> > say, handle_signal_stop, could we go over the bpstat chain and delete
> > any breakpoint we've just hit that is marked for deletion?
>
> I believe this choice is due to performance issues.
>
> In an early attempt, I tried to call `breakpoint_auto_delete' on all 
> kinds
> of stops. However, this implementation is very slow when we have a large
> number of breakpoints, since we need to go over the entire bpstat 
> chain on
> all stops. I recall that this implementation fails on certain 
> testcases with
> a large number of breakpoints with many breakpoint stops.
>
> Furthermore, the reason for using the `disp_del_at_next_stop' state 
> remains
> unclear, as mentioned in the comments:
>
>     /* NOTE drow/2003-09-08: This state only exists for removing
>        watchpoints.  It's not clear that it's necessary...  */
>
>     -- gdb/breakpoint.c:2914
>
> By tracing the git history, the `disp_del_at_next_stop' state is 
> introduced
> in commit c906108c21474dfb4ed285bcc0ac6fe02cd400cc, which doesn't provide
> any proper explanation of this state.
>
> I think the best way to deal with this is to avoid going over the entire
> bpstat chain when deleting breakpoints. Potentially by keeping track of
> a chain of breakpoints that should be deleted, and changing the bpstat 
> chain
> to a doubly linked list for the ease of deletion. Such changes deserve a
> dedicated patch, though.
>
> To sum up, I prefer modifying `watchpoint_del_at_next_stop' (i.e., the 
> v1 patch).
> If you also think it's appropriate, I'll fix the failing test and 
> prepare the
> v4 patch accordingly.
>
> Johnson
>
> On 1/13/2023 11:40 PM, SimonMarchi wrote:
>>
>> On 10/20/22 14:11, Johnson Sun via Gdb-patches 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.
>>>
>>> Two other solutions seem to solve this issue, but are in fact 
>>> inappropriate:
>>> 1. Automatically delete breakpoints on all kinds of stops
>>>     (in `fetch_inferior_event'). This solution is very slow since 
>>> the deletion
>>>     requires O(N) time for N breakpoints.
>>> 2. Disable the watchpoint after the watchpoint's disposition is set to
>>>     `delete at next stop' (in `watchpoint_del_at_next_stop'). This 
>>> solution
>>>     modifies a non-extension-related code path, and isn't preferred 
>>> since this
>>>     issue cannot occur without extension languages. (gdb scripts 
>>> always normal
>>>     stop before continuing execution)
>> I have a bit of trouble reviewing this, because I'm not too familiar
>> with the lifecycle of watchpoints (I know the principle, but not the
>> specifically where things happen in GDB).  So it's difficult to tell
>> whether this is right or if there's a better way to handle it.
>>
>> However, the supplied test does not pass for me:
>>
>>      source 
>> /home/simark/build/binutils-gdb/gdb/testsuite/outputs/gdb.python/py-watchpoint/py-watchpoint.py
>>      Watchpoint 2: i
>>      Python script imported
>>      (gdb) PASS: gdb.python/py-watchpoint.exp: import python scripts
>>      python print(len(gdb.breakpoints()))
>>      2
>>      (gdb) PASS: gdb.python/py-watchpoint.exp: check modified BP count
>>      gdb_expect_list pattern: /Watchpoint Hit: ./
>>      continue
>>      Continuing.
>>      Watchpoint Hit: 1
>>      gdb_expect_list pattern: /[
>>      ]+Watchpoint . deleted because the program has left the block in/
>>      FAIL: gdb.python/py-watchpoint.exp: run until program stops 
>> (pattern 2) (timeout)
>>      gdb_expect_list pattern: /[
>>      ]+which its expression is valid./
>>      gdb_expect_list pattern: /Watchpoint Hit: ./
>>      gdb_expect_list pattern: /[
>>      ]+012\[Inferior 1 \(process .*\) exited normally\]/
>>      gdb_expect_list pattern: //
>>      python print(len(gdb.breakpoints()))
>>      Watchpoint Hit: 2
>>      Watchpoint Hit: 3
>>      Watchpoint Hit: 4
>>
>>      Watchpoint 2 deleted because the program has left the block in
>>      which its expression is valid.
>>      Watchpoint Hit: 5
>>      012[Inferior 1 (process 2381681) exited normally]
>>      (gdb) FAIL: gdb.python/py-watchpoint.exp: check BP count (the 
>> program exited)
>>
>>> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29603
>>> ---
>>>   gdb/breakpoint.c                           |  2 +
>>>   gdb/testsuite/gdb.python/py-watchpoint.c   | 27 ++++++++++++
>>>   gdb/testsuite/gdb.python/py-watchpoint.exp | 48 
>>> ++++++++++++++++++++++
>>>   gdb/testsuite/gdb.python/py-watchpoint.py  | 30 ++++++++++++++
>>>   4 files changed, 107 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 bff3bac7d1a..15f4ae2131c 100644
>>> --- a/gdb/breakpoint.c
>>> +++ b/gdb/breakpoint.c
>>> @@ -5340,6 +5340,8 @@ bpstat_check_breakpoint_conditions (bpstat 
>>> *bs, thread_info *thread)
>>>     /* Evaluate extension language breakpoints that have a "stop" 
>>> method
>>>        implemented.  */
>>>     bs->stop = breakpoint_ext_lang_cond_says_stop (b);
>>> +  if (b->disposition == disp_del_at_next_stop)
>>> +    disable_breakpoint(b);
>> Is there a reason to do it here in particular, and not, let's say as
>> soon as we change the disposition to disp_del_at_next_stop?
>>
>> Other question: shouldn't marking the watchpoint as
>> disp_del_at_next_stop make it so it won't be inserted next time we
>> resume?  After all should_be_inserted returns false for breakpoint
>> locations that are disp_del_at_next_stop.  Perhaps it's because since we
>> don't do a "normal" stop, breakpoint locations stay inserted, and
>> there's nothing that pulls this location out of the target? Therefore,
>> maybe a solution, to keep things coherent, would be to pull the
>> breakpoint locations out when marking the breakpoint as
>> disp_del_at_next_stop?  This way, we would respect the invariant that a
>> breakpoint disp_del_at_next_stop can't be inserted (I don't know if this
>> is really what is happening though, it's just speculation).
>>
>> And, in a broader scope, why wait until the next normal stop to delete
>> the watchpoint?  A "normal" stop being a stop that we present to the
>> user (the normal_stop function).  We currently call
>> breakpoint_auto_delete in normal_stop, which is why we don't reach it
>> when your Breakpoint.stop method returns False.  At the end of, let's
>> say, handle_signal_stop, could we go over the bpstat chain and delete
>> any breakpoint we've just hit that is marked for deletion?
>>
>> Simon
>>

^ permalink raw reply	[flat|nested] 34+ messages in thread

* [RFC] [PATCH v3] [PR python/29603] Disable out-of-scope watchpoints
  2023-02-18 16:26             ` [PING] " Johnson Sun
@ 2023-02-26  6:16               ` Johnson Sun
  2023-03-12 17:24                 ` [PING] " Johnson Sun
  0 siblings, 1 reply; 34+ messages in thread
From: Johnson Sun @ 2023-02-26  6:16 UTC (permalink / raw)
  To: SimonMarchi, LancelotSIX, gdb-patches

Request for comments: 
<https://sourceware.org/pipermail/gdb-patches/2023-January/196068.html>.

Johnson

On 2/19/2023 12:26 AM, JohnsonSun wrote:
> Ping for: 
> <https://sourceware.org/pipermail/gdb-patches/2023-January/196068.html>.
>
> I would like to ask for some feedback before submitting the v4 patch.
>
> Johnson
>
> On 1/23/2023 6:15 PM, Johnson Sun wrote:
>> Hi,
>>
>> Thanks for reviewing this!
>>
>> > the supplied test does not pass for me
>>
>> The current test doesn't seem to produce consistent results across 
>> different
>> machines, I'll try to fix it in the next version.
>>
>> > Is there a reason to do it here in particular, and not, let's say as
>> > soon as we change the disposition to disp_del_at_next_stop?
>>
>> I have implemented this in the v1 patch, I called 
>> `disable_breakpoint' as
>> soon as we change the disposition to `disp_del_at_next_stop'
>> (in `watchpoint_del_at_next_stop'). However,
>> LancelotSIX <lsix@lancelotsix.com> mentioned that the fix is in a
>> non-extension-related code path, and suggested disabling it in
>> `bpstat_check_breakpoint_conditions' instead (the v3 patch).
>> See: 
>> https://sourceware.org/pipermail/gdb-patches/2022-September/192120.html
>>
>> Both the v1 and v3 patch fixes the issue. I personally prefer the v1 
>> patch.
>> (i.e., modifying `watchpoint_del_at_next_stop')
>>
>> > shouldn't marking the watchpoint as
>> > disp_del_at_next_stop make it so it won't be inserted next time we
>> > resume?  After all should_be_inserted returns false for breakpoint
>> > locations that are disp_del_at_next_stop.  Perhaps it's because 
>> since we
>> > don't do a "normal" stop, breakpoint locations stay inserted, and
>> > there's nothing that pulls this location out of the target? Therefore,
>> > maybe a solution, to keep things coherent, would be to pull the
>> > breakpoint locations out when marking the breakpoint as
>> > disp_del_at_next_stop?  This way, we would respect the invariant 
>> that a
>> > breakpoint disp_del_at_next_stop can't be inserted (I don't know if 
>> this
>> > is really what is happening though, it's just speculation).
>>
>> For software watchpoints, they cannot be pulled out of the target, since
>> they may not be inserted into the target in the first place:
>>
>>      /* Locations of type bp_loc_other and
>>     bp_loc_software_watchpoint are only maintained at GDB side,
>>     so there is no need to remove them.  */
>>
>>     -- gdb/breakpoint.c:3840
>>
>> Their expressions are re-evaluated by single-stepping through the 
>> program:
>>
>>     Software watchpoints are very slow, since GDB needs to single-step
>>     the program being debugged and test the value of the watched
>>     expression(s) after each instruction.
>>
>>     -- https://sourceware.org/gdb/wiki/Internals%20Watchpoints
>>
>> Therefore, we must either disable or delete the software watchpoint.
>> We cannot pull it out of the target since it's only maintained on the
>> GDB side.
>>
>> > And, in a broader scope, why wait until the next normal stop to delete
>> > the watchpoint?  A "normal" stop being a stop that we present to the
>> > user (the normal_stop function).  We currently call
>> > breakpoint_auto_delete in normal_stop, which is why we don't reach it
>> > when your Breakpoint.stop method returns False.  At the end of, let's
>> > say, handle_signal_stop, could we go over the bpstat chain and delete
>> > any breakpoint we've just hit that is marked for deletion?
>>
>> I believe this choice is due to performance issues.
>>
>> In an early attempt, I tried to call `breakpoint_auto_delete' on all 
>> kinds
>> of stops. However, this implementation is very slow when we have a large
>> number of breakpoints, since we need to go over the entire bpstat 
>> chain on
>> all stops. I recall that this implementation fails on certain 
>> testcases with
>> a large number of breakpoints with many breakpoint stops.
>>
>> Furthermore, the reason for using the `disp_del_at_next_stop' state 
>> remains
>> unclear, as mentioned in the comments:
>>
>>     /* NOTE drow/2003-09-08: This state only exists for removing
>>        watchpoints.  It's not clear that it's necessary...  */
>>
>>     -- gdb/breakpoint.c:2914
>>
>> By tracing the git history, the `disp_del_at_next_stop' state is 
>> introduced
>> in commit c906108c21474dfb4ed285bcc0ac6fe02cd400cc, which doesn't 
>> provide
>> any proper explanation of this state.
>>
>> I think the best way to deal with this is to avoid going over the entire
>> bpstat chain when deleting breakpoints. Potentially by keeping track of
>> a chain of breakpoints that should be deleted, and changing the 
>> bpstat chain
>> to a doubly linked list for the ease of deletion. Such changes deserve a
>> dedicated patch, though.
>>
>> To sum up, I prefer modifying `watchpoint_del_at_next_stop' (i.e., 
>> the v1 patch).
>> If you also think it's appropriate, I'll fix the failing test and 
>> prepare the
>> v4 patch accordingly.
>>
>> Johnson
>>
>> On 1/13/2023 11:40 PM, SimonMarchi wrote:
>>>
>>> On 10/20/22 14:11, Johnson Sun via Gdb-patches 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.
>>>>
>>>> Two other solutions seem to solve this issue, but are in fact 
>>>> inappropriate:
>>>> 1. Automatically delete breakpoints on all kinds of stops
>>>>     (in `fetch_inferior_event'). This solution is very slow since 
>>>> the deletion
>>>>     requires O(N) time for N breakpoints.
>>>> 2. Disable the watchpoint after the watchpoint's disposition is set to
>>>>     `delete at next stop' (in `watchpoint_del_at_next_stop'). This 
>>>> solution
>>>>     modifies a non-extension-related code path, and isn't preferred 
>>>> since this
>>>>     issue cannot occur without extension languages. (gdb scripts 
>>>> always normal
>>>>     stop before continuing execution)
>>> I have a bit of trouble reviewing this, because I'm not too familiar
>>> with the lifecycle of watchpoints (I know the principle, but not the
>>> specifically where things happen in GDB).  So it's difficult to tell
>>> whether this is right or if there's a better way to handle it.
>>>
>>> However, the supplied test does not pass for me:
>>>
>>>      source 
>>> /home/simark/build/binutils-gdb/gdb/testsuite/outputs/gdb.python/py-watchpoint/py-watchpoint.py
>>>      Watchpoint 2: i
>>>      Python script imported
>>>      (gdb) PASS: gdb.python/py-watchpoint.exp: import python scripts
>>>      python print(len(gdb.breakpoints()))
>>>      2
>>>      (gdb) PASS: gdb.python/py-watchpoint.exp: check modified BP count
>>>      gdb_expect_list pattern: /Watchpoint Hit: ./
>>>      continue
>>>      Continuing.
>>>      Watchpoint Hit: 1
>>>      gdb_expect_list pattern: /[
>>>      ]+Watchpoint . deleted because the program has left the block in/
>>>      FAIL: gdb.python/py-watchpoint.exp: run until program stops 
>>> (pattern 2) (timeout)
>>>      gdb_expect_list pattern: /[
>>>      ]+which its expression is valid./
>>>      gdb_expect_list pattern: /Watchpoint Hit: ./
>>>      gdb_expect_list pattern: /[
>>>      ]+012\[Inferior 1 \(process .*\) exited normally\]/
>>>      gdb_expect_list pattern: //
>>>      python print(len(gdb.breakpoints()))
>>>      Watchpoint Hit: 2
>>>      Watchpoint Hit: 3
>>>      Watchpoint Hit: 4
>>>
>>>      Watchpoint 2 deleted because the program has left the block in
>>>      which its expression is valid.
>>>      Watchpoint Hit: 5
>>>      012[Inferior 1 (process 2381681) exited normally]
>>>      (gdb) FAIL: gdb.python/py-watchpoint.exp: check BP count (the 
>>> program exited)
>>>
>>>> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29603
>>>> ---
>>>>   gdb/breakpoint.c                           |  2 +
>>>>   gdb/testsuite/gdb.python/py-watchpoint.c   | 27 ++++++++++++
>>>>   gdb/testsuite/gdb.python/py-watchpoint.exp | 48 
>>>> ++++++++++++++++++++++
>>>>   gdb/testsuite/gdb.python/py-watchpoint.py  | 30 ++++++++++++++
>>>>   4 files changed, 107 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 bff3bac7d1a..15f4ae2131c 100644
>>>> --- a/gdb/breakpoint.c
>>>> +++ b/gdb/breakpoint.c
>>>> @@ -5340,6 +5340,8 @@ bpstat_check_breakpoint_conditions (bpstat 
>>>> *bs, thread_info *thread)
>>>>     /* Evaluate extension language breakpoints that have a "stop" 
>>>> method
>>>>        implemented.  */
>>>>     bs->stop = breakpoint_ext_lang_cond_says_stop (b);
>>>> +  if (b->disposition == disp_del_at_next_stop)
>>>> +    disable_breakpoint(b);
>>> Is there a reason to do it here in particular, and not, let's say as
>>> soon as we change the disposition to disp_del_at_next_stop?
>>>
>>> Other question: shouldn't marking the watchpoint as
>>> disp_del_at_next_stop make it so it won't be inserted next time we
>>> resume?  After all should_be_inserted returns false for breakpoint
>>> locations that are disp_del_at_next_stop.  Perhaps it's because 
>>> since we
>>> don't do a "normal" stop, breakpoint locations stay inserted, and
>>> there's nothing that pulls this location out of the target? Therefore,
>>> maybe a solution, to keep things coherent, would be to pull the
>>> breakpoint locations out when marking the breakpoint as
>>> disp_del_at_next_stop?  This way, we would respect the invariant that a
>>> breakpoint disp_del_at_next_stop can't be inserted (I don't know if 
>>> this
>>> is really what is happening though, it's just speculation).
>>>
>>> And, in a broader scope, why wait until the next normal stop to delete
>>> the watchpoint?  A "normal" stop being a stop that we present to the
>>> user (the normal_stop function).  We currently call
>>> breakpoint_auto_delete in normal_stop, which is why we don't reach it
>>> when your Breakpoint.stop method returns False.  At the end of, let's
>>> say, handle_signal_stop, could we go over the bpstat chain and delete
>>> any breakpoint we've just hit that is marked for deletion?
>>>
>>> Simon
>>>
>

^ permalink raw reply	[flat|nested] 34+ messages in thread

* [PING] [RFC] [PATCH v3] [PR python/29603] Disable out-of-scope watchpoints
  2023-02-26  6:16               ` [RFC] " Johnson Sun
@ 2023-03-12 17:24                 ` Johnson Sun
  0 siblings, 0 replies; 34+ messages in thread
From: Johnson Sun @ 2023-03-12 17:24 UTC (permalink / raw)
  To: SimonMarchi, LancelotSIX, gdb-patches

Ping for comments: 
<https://sourceware.org/pipermail/gdb-patches/2023-January/196068.html>.

Johnson

On 2/26/2023 2:16 PM, Johnson Sun wrote:
> Request for comments: 
> <https://sourceware.org/pipermail/gdb-patches/2023-January/196068.html>.
>
> Johnson
>
> On 2/19/2023 12:26 AM, JohnsonSun wrote:
>> Ping for: 
>> <https://sourceware.org/pipermail/gdb-patches/2023-January/196068.html>.
>>
>> I would like to ask for some feedback before submitting the v4 patch.
>>
>> Johnson
>>
>> On 1/23/2023 6:15 PM, Johnson Sun wrote:
>>> Hi,
>>>
>>> Thanks for reviewing this!
>>>
>>> > the supplied test does not pass for me
>>>
>>> The current test doesn't seem to produce consistent results across 
>>> different
>>> machines, I'll try to fix it in the next version.
>>>
>>> > Is there a reason to do it here in particular, and not, let's say as
>>> > soon as we change the disposition to disp_del_at_next_stop?
>>>
>>> I have implemented this in the v1 patch, I called 
>>> `disable_breakpoint' as
>>> soon as we change the disposition to `disp_del_at_next_stop'
>>> (in `watchpoint_del_at_next_stop'). However,
>>> LancelotSIX <lsix@lancelotsix.com> mentioned that the fix is in a
>>> non-extension-related code path, and suggested disabling it in
>>> `bpstat_check_breakpoint_conditions' instead (the v3 patch).
>>> See: 
>>> https://sourceware.org/pipermail/gdb-patches/2022-September/192120.html
>>>
>>> Both the v1 and v3 patch fixes the issue. I personally prefer the v1 
>>> patch.
>>> (i.e., modifying `watchpoint_del_at_next_stop')
>>>
>>> > shouldn't marking the watchpoint as
>>> > disp_del_at_next_stop make it so it won't be inserted next time we
>>> > resume?  After all should_be_inserted returns false for breakpoint
>>> > locations that are disp_del_at_next_stop.  Perhaps it's because 
>>> since we
>>> > don't do a "normal" stop, breakpoint locations stay inserted, and
>>> > there's nothing that pulls this location out of the target? 
>>> Therefore,
>>> > maybe a solution, to keep things coherent, would be to pull the
>>> > breakpoint locations out when marking the breakpoint as
>>> > disp_del_at_next_stop?  This way, we would respect the invariant 
>>> that a
>>> > breakpoint disp_del_at_next_stop can't be inserted (I don't know 
>>> if this
>>> > is really what is happening though, it's just speculation).
>>>
>>> For software watchpoints, they cannot be pulled out of the target, 
>>> since
>>> they may not be inserted into the target in the first place:
>>>
>>>      /* Locations of type bp_loc_other and
>>>     bp_loc_software_watchpoint are only maintained at GDB side,
>>>     so there is no need to remove them.  */
>>>
>>>     -- gdb/breakpoint.c:3840
>>>
>>> Their expressions are re-evaluated by single-stepping through the 
>>> program:
>>>
>>>     Software watchpoints are very slow, since GDB needs to single-step
>>>     the program being debugged and test the value of the watched
>>>     expression(s) after each instruction.
>>>
>>>     -- https://sourceware.org/gdb/wiki/Internals%20Watchpoints
>>>
>>> Therefore, we must either disable or delete the software watchpoint.
>>> We cannot pull it out of the target since it's only maintained on the
>>> GDB side.
>>>
>>> > And, in a broader scope, why wait until the next normal stop to 
>>> delete
>>> > the watchpoint?  A "normal" stop being a stop that we present to the
>>> > user (the normal_stop function).  We currently call
>>> > breakpoint_auto_delete in normal_stop, which is why we don't reach it
>>> > when your Breakpoint.stop method returns False.  At the end of, let's
>>> > say, handle_signal_stop, could we go over the bpstat chain and delete
>>> > any breakpoint we've just hit that is marked for deletion?
>>>
>>> I believe this choice is due to performance issues.
>>>
>>> In an early attempt, I tried to call `breakpoint_auto_delete' on all 
>>> kinds
>>> of stops. However, this implementation is very slow when we have a 
>>> large
>>> number of breakpoints, since we need to go over the entire bpstat 
>>> chain on
>>> all stops. I recall that this implementation fails on certain 
>>> testcases with
>>> a large number of breakpoints with many breakpoint stops.
>>>
>>> Furthermore, the reason for using the `disp_del_at_next_stop' state 
>>> remains
>>> unclear, as mentioned in the comments:
>>>
>>>     /* NOTE drow/2003-09-08: This state only exists for removing
>>>        watchpoints.  It's not clear that it's necessary...  */
>>>
>>>     -- gdb/breakpoint.c:2914
>>>
>>> By tracing the git history, the `disp_del_at_next_stop' state is 
>>> introduced
>>> in commit c906108c21474dfb4ed285bcc0ac6fe02cd400cc, which doesn't 
>>> provide
>>> any proper explanation of this state.
>>>
>>> I think the best way to deal with this is to avoid going over the 
>>> entire
>>> bpstat chain when deleting breakpoints. Potentially by keeping track of
>>> a chain of breakpoints that should be deleted, and changing the 
>>> bpstat chain
>>> to a doubly linked list for the ease of deletion. Such changes 
>>> deserve a
>>> dedicated patch, though.
>>>
>>> To sum up, I prefer modifying `watchpoint_del_at_next_stop' (i.e., 
>>> the v1 patch).
>>> If you also think it's appropriate, I'll fix the failing test and 
>>> prepare the
>>> v4 patch accordingly.
>>>
>>> Johnson
>>>
>>> On 1/13/2023 11:40 PM, SimonMarchi wrote:
>>>>
>>>> On 10/20/22 14:11, Johnson Sun via Gdb-patches 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.
>>>>>
>>>>> Two other solutions seem to solve this issue, but are in fact 
>>>>> inappropriate:
>>>>> 1. Automatically delete breakpoints on all kinds of stops
>>>>>     (in `fetch_inferior_event'). This solution is very slow since 
>>>>> the deletion
>>>>>     requires O(N) time for N breakpoints.
>>>>> 2. Disable the watchpoint after the watchpoint's disposition is 
>>>>> set to
>>>>>     `delete at next stop' (in `watchpoint_del_at_next_stop'). This 
>>>>> solution
>>>>>     modifies a non-extension-related code path, and isn't 
>>>>> preferred since this
>>>>>     issue cannot occur without extension languages. (gdb scripts 
>>>>> always normal
>>>>>     stop before continuing execution)
>>>> I have a bit of trouble reviewing this, because I'm not too familiar
>>>> with the lifecycle of watchpoints (I know the principle, but not the
>>>> specifically where things happen in GDB).  So it's difficult to tell
>>>> whether this is right or if there's a better way to handle it.
>>>>
>>>> However, the supplied test does not pass for me:
>>>>
>>>>      source 
>>>> /home/simark/build/binutils-gdb/gdb/testsuite/outputs/gdb.python/py-watchpoint/py-watchpoint.py
>>>>      Watchpoint 2: i
>>>>      Python script imported
>>>>      (gdb) PASS: gdb.python/py-watchpoint.exp: import python scripts
>>>>      python print(len(gdb.breakpoints()))
>>>>      2
>>>>      (gdb) PASS: gdb.python/py-watchpoint.exp: check modified BP count
>>>>      gdb_expect_list pattern: /Watchpoint Hit: ./
>>>>      continue
>>>>      Continuing.
>>>>      Watchpoint Hit: 1
>>>>      gdb_expect_list pattern: /[
>>>>      ]+Watchpoint . deleted because the program has left the block in/
>>>>      FAIL: gdb.python/py-watchpoint.exp: run until program stops 
>>>> (pattern 2) (timeout)
>>>>      gdb_expect_list pattern: /[
>>>>      ]+which its expression is valid./
>>>>      gdb_expect_list pattern: /Watchpoint Hit: ./
>>>>      gdb_expect_list pattern: /[
>>>>      ]+012\[Inferior 1 \(process .*\) exited normally\]/
>>>>      gdb_expect_list pattern: //
>>>>      python print(len(gdb.breakpoints()))
>>>>      Watchpoint Hit: 2
>>>>      Watchpoint Hit: 3
>>>>      Watchpoint Hit: 4
>>>>
>>>>      Watchpoint 2 deleted because the program has left the block in
>>>>      which its expression is valid.
>>>>      Watchpoint Hit: 5
>>>>      012[Inferior 1 (process 2381681) exited normally]
>>>>      (gdb) FAIL: gdb.python/py-watchpoint.exp: check BP count (the 
>>>> program exited)
>>>>
>>>>> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29603
>>>>> ---
>>>>>   gdb/breakpoint.c                           |  2 +
>>>>>   gdb/testsuite/gdb.python/py-watchpoint.c   | 27 ++++++++++++
>>>>>   gdb/testsuite/gdb.python/py-watchpoint.exp | 48 
>>>>> ++++++++++++++++++++++
>>>>>   gdb/testsuite/gdb.python/py-watchpoint.py  | 30 ++++++++++++++
>>>>>   4 files changed, 107 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 bff3bac7d1a..15f4ae2131c 100644
>>>>> --- a/gdb/breakpoint.c
>>>>> +++ b/gdb/breakpoint.c
>>>>> @@ -5340,6 +5340,8 @@ bpstat_check_breakpoint_conditions (bpstat 
>>>>> *bs, thread_info *thread)
>>>>>     /* Evaluate extension language breakpoints that have a "stop" 
>>>>> method
>>>>>        implemented.  */
>>>>>     bs->stop = breakpoint_ext_lang_cond_says_stop (b);
>>>>> +  if (b->disposition == disp_del_at_next_stop)
>>>>> +    disable_breakpoint(b);
>>>> Is there a reason to do it here in particular, and not, let's say as
>>>> soon as we change the disposition to disp_del_at_next_stop?
>>>>
>>>> Other question: shouldn't marking the watchpoint as
>>>> disp_del_at_next_stop make it so it won't be inserted next time we
>>>> resume?  After all should_be_inserted returns false for breakpoint
>>>> locations that are disp_del_at_next_stop.  Perhaps it's because 
>>>> since we
>>>> don't do a "normal" stop, breakpoint locations stay inserted, and
>>>> there's nothing that pulls this location out of the target? Therefore,
>>>> maybe a solution, to keep things coherent, would be to pull the
>>>> breakpoint locations out when marking the breakpoint as
>>>> disp_del_at_next_stop?  This way, we would respect the invariant 
>>>> that a
>>>> breakpoint disp_del_at_next_stop can't be inserted (I don't know if 
>>>> this
>>>> is really what is happening though, it's just speculation).
>>>>
>>>> And, in a broader scope, why wait until the next normal stop to delete
>>>> the watchpoint?  A "normal" stop being a stop that we present to the
>>>> user (the normal_stop function).  We currently call
>>>> breakpoint_auto_delete in normal_stop, which is why we don't reach it
>>>> when your Breakpoint.stop method returns False.  At the end of, let's
>>>> say, handle_signal_stop, could we go over the bpstat chain and delete
>>>> any breakpoint we've just hit that is marked for deletion?
>>>>
>>>> Simon
>>>>
>>

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH v3] [PR python/29603] Disable out-of-scope watchpoints
  2023-01-23 10:15           ` Johnson Sun
  2023-02-18 16:26             ` [PING] " Johnson Sun
@ 2023-03-13 16:00             ` Simon Marchi
  2023-03-23 18:25               ` Johnson Sun
  1 sibling, 1 reply; 34+ messages in thread
From: Simon Marchi @ 2023-03-13 16:00 UTC (permalink / raw)
  To: Johnson Sun, LancelotSIX, gdb-patches

On 1/23/23 05:15, Johnson Sun via Gdb-patches wrote:
> Hi,
> 
> Thanks for reviewing this!

Sorry for the delay.

> 
>> the supplied test does not pass for me
> 
> The current test doesn't seem to produce consistent results across different
> machines, I'll try to fix it in the next version.
> 
>> Is there a reason to do it here in particular, and not, let's say as
>> soon as we change the disposition to disp_del_at_next_stop?
> 
> I have implemented this in the v1 patch, I called `disable_breakpoint' as
> soon as we change the disposition to `disp_del_at_next_stop'
> (in `watchpoint_del_at_next_stop'). However,
> LancelotSIX <lsix@lancelotsix.com> mentioned that the fix is in a
> non-extension-related code path, and suggested disabling it in
> `bpstat_check_breakpoint_conditions' instead (the v3 patch).
> See: https://sourceware.org/pipermail/gdb-patches/2022-September/192120.html

Ah, sorry for missing that.  I now see that you also mentionned it in
your commit message.

> Both the v1 and v3 patch fixes the issue. I personally prefer the v1 patch.
> (i.e., modifying `watchpoint_del_at_next_stop')
> 
>> shouldn't marking the watchpoint as
>> disp_del_at_next_stop make it so it won't be inserted next time we
>> resume?  After all should_be_inserted returns false for breakpoint
>> locations that are disp_del_at_next_stop.  Perhaps it's because since we
>> don't do a "normal" stop, breakpoint locations stay inserted, and
>> there's nothing that pulls this location out of the target? Therefore,
>> maybe a solution, to keep things coherent, would be to pull the
>> breakpoint locations out when marking the breakpoint as
>> disp_del_at_next_stop?  This way, we would respect the invariant that a
>> breakpoint disp_del_at_next_stop can't be inserted (I don't know if this
>> is really what is happening though, it's just speculation).
> 
> For software watchpoints, they cannot be pulled out of the target, since
> they may not be inserted into the target in the first place:
> 
>      /* Locations of type bp_loc_other and
>     bp_loc_software_watchpoint are only maintained at GDB side,
>     so there is no need to remove them.  */
> 
>     -- gdb/breakpoint.c:3840
> 
> Their expressions are re-evaluated by single-stepping through the program:
> 
>     Software watchpoints are very slow, since GDB needs to single-step
>     the program being debugged and test the value of the watched
>     expression(s) after each instruction.
> 
>     -- https://sourceware.org/gdb/wiki/Internals%20Watchpoints
> 
> Therefore, we must either disable or delete the software watchpoint.
> We cannot pull it out of the target since it's only maintained on the
> GDB side.

Ah ok, I had not understood that we are talking about software
watchpoints.

> 
>> And, in a broader scope, why wait until the next normal stop to delete
>> the watchpoint?  A "normal" stop being a stop that we present to the
>> user (the normal_stop function).  We currently call
>> breakpoint_auto_delete in normal_stop, which is why we don't reach it
>> when your Breakpoint.stop method returns False.  At the end of, let's
>> say, handle_signal_stop, could we go over the bpstat chain and delete
>> any breakpoint we've just hit that is marked for deletion?
> 
> I believe this choice is due to performance issues.
> 
> In an early attempt, I tried to call `breakpoint_auto_delete' on all kinds
> of stops. However, this implementation is very slow when we have a large
> number of breakpoints, since we need to go over the entire bpstat chain on
> all stops. I recall that this implementation fails on certain testcases with
> a large number of breakpoints with many breakpoint stops.

There's a difference between going over all breakpoints, and going over
the bpstat chain.  Unecessarily going over all breakpoints is not a good
idea, indeed.  But the bpstat chain only contains elements for
breakpoints that were hit right now.  In a pathological case, you could
have a lot of elements in that chain, but I don't think it's the norm.
The typical case is to have just one element, when hitting a single
breakpoint.

breakpoint_auto_delete does both.  I was thinking that just looking at
the bpstat chain, let's say at the end of handle_inferior_event, could
be done for cheap.

But in any case, that's a larger change, I think we should focus on your
fix, and if somebody feels like going it, this can be another change
later.

> Furthermore, the reason for using the `disp_del_at_next_stop' state remains
> unclear, as mentioned in the comments:
> 
>     /* NOTE drow/2003-09-08: This state only exists for removing
>        watchpoints.  It's not clear that it's necessary...  */
> 
>     -- gdb/breakpoint.c:2914
> 
> By tracing the git history, the `disp_del_at_next_stop' state is introduced
> in commit c906108c21474dfb4ed285bcc0ac6fe02cd400cc, which doesn't provide
> any proper explanation of this state.
> 
> I think the best way to deal with this is to avoid going over the entire
> bpstat chain when deleting breakpoints. Potentially by keeping track of
> a chain of breakpoints that should be deleted, and changing the bpstat chain
> to a doubly linked list for the ease of deletion. Such changes deserve a
> dedicated patch, though.

Maybe the bpstat chain could use intrusive_list:

https://gitlab.com/gnutools/binutils-gdb/-/blob/master/gdbsupport/intrusive_list.h

> To sum up, I prefer modifying `watchpoint_del_at_next_stop' (i.e., the v1 patch).
> If you also think it's appropriate, I'll fix the failing test and prepare the
> v4 patch accordingly.

I think I prefer the v1 too, it puts the action of disabling the
watchpoint closer to where we decide to disable it, so it's easier to
follow.  I don't think it's a problem that the fix touches a
non-extension code path.  The change in v1 sounds logical to me: the
intent of watchpoint_del_at_next_stop is to get rid of the watchpoint,
make it so the user can't hit it anymore.  It's just that for some
reason (unknown to me at this point), we can't delete it right away.  If
disabling it is needed to ensure the watchpoint is not seen by the user
in some circumstances, then so be it.

I guess another solution would be to make watchpoint_check check
b->disposition, and skip it if it's disp_del_at_next_stop.  I don't
think it's any better though.

Simon

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH v3] [PR python/29603] Disable out-of-scope watchpoints
  2023-03-13 16:00             ` Simon Marchi
@ 2023-03-23 18:25               ` Johnson Sun
  2023-03-23 18:31                 ` [PATCH v4] " Johnson Sun
  0 siblings, 1 reply; 34+ messages in thread
From: Johnson Sun @ 2023-03-23 18:25 UTC (permalink / raw)
  To: SimonMarchi, JohnsonSun, LancelotSIX, gdb-patches

Hi Simon,

Thanks for reviewing this!  I revised the patch and

will send version 4 shortly.

Cheers,
Johnson


^ permalink raw reply	[flat|nested] 34+ messages in thread

* [PATCH v4] [PR python/29603] Disable out-of-scope watchpoints
  2023-03-23 18:25               ` Johnson Sun
@ 2023-03-23 18:31                 ` Johnson Sun
  2023-04-09 20:47                   ` Johnson Sun
  2023-04-17 18:56                   ` Simon Marchi
  0 siblings, 2 replies; 34+ messages in thread
From: Johnson Sun @ 2023-03-23 18:31 UTC (permalink / raw)
  To: SimonMarchi, Lancelot SIX, gdb-patches; +Cc: j3.soon777

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 | 54 ++++++++++++++++++++++
 gdb/testsuite/gdb.python/py-watchpoint.py  | 30 ++++++++++++
 4 files changed, 112 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..0c58b31647
--- /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++)
+    printf ("%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..5504736629
--- /dev/null
+++ b/gdb/testsuite/gdb.python/py-watchpoint.exp
@@ -0,0 +1,54 @@
+# 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
+}
+
+# Skip all tests if Python scripting is not enabled.
+if { [skip_python_tests] } { continue }
+
+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"
+gdb_test "break 24" ".*" "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_sequence "continue" "run until program stops" {
+    "Watchpoint Hit: 1"
+    "Watchpoint Hit: 2"
+    "Watchpoint Hit: 3"
+    "Watchpoint Hit: 4"
+    "Watchpoint 3 deleted because the program has left the block in"
+    "which its expression is valid\."
+    "Watchpoint Hit: 5"
+    "012\\[Inferior 1 \\(process .*\\) exited normally\\]"
+}
+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..c5fae00e93
--- /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, *args, **kwargs):
+        gdb.Breakpoint.__init__(self, *args, **kwargs)
+        self.n = 0
+
+    def stop(self):
+        self.n += 1
+        print("Watchpoint Hit:", self.n, flush=True)
+        return False
+
+
+MyBreakpoint("i", gdb.BP_WATCHPOINT)
+
+print("Python script imported")
-- 
2.34.1

base-commit: c99b2113a478a075e8d8f7c3848a92f1ce73f847

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH v4] [PR python/29603] Disable out-of-scope watchpoints
  2023-03-23 18:31                 ` [PATCH v4] " Johnson Sun
@ 2023-04-09 20:47                   ` Johnson Sun
  2023-04-09 20:49                     ` [PING] " Johnson Sun
  2023-04-17 18:56                   ` Simon Marchi
  1 sibling, 1 reply; 34+ messages in thread
From: Johnson Sun @ 2023-04-09 20:47 UTC (permalink / raw)
  To: SimonMarchi, Lancelot SIX, gdb-patches

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

Johnson

On 3/24/2023 2:31 AM, 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
> ---
>   gdb/breakpoint.c                           |  1 +
>   gdb/testsuite/gdb.python/py-watchpoint.c   | 27 +++++++++++
>   gdb/testsuite/gdb.python/py-watchpoint.exp | 54 ++++++++++++++++++++++
>   gdb/testsuite/gdb.python/py-watchpoint.py  | 30 ++++++++++++
>   4 files changed, 112 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..0c58b31647
> --- /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++)
> +    printf ("%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..5504736629
> --- /dev/null
> +++ b/gdb/testsuite/gdb.python/py-watchpoint.exp
> @@ -0,0 +1,54 @@
> +# 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
> +}
> +
> +# Skip all tests if Python scripting is not enabled.
> +if { [skip_python_tests] } { continue }
> +
> +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"
> +gdb_test "break 24" ".*" "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_sequence "continue" "run until program stops" {
> +    "Watchpoint Hit: 1"
> +    "Watchpoint Hit: 2"
> +    "Watchpoint Hit: 3"
> +    "Watchpoint Hit: 4"
> +    "Watchpoint 3 deleted because the program has left the block in"
> +    "which its expression is valid\."
> +    "Watchpoint Hit: 5"
> +    "012\\[Inferior 1 \\(process .*\\) exited normally\\]"
> +}
> +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..c5fae00e93
> --- /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, *args, **kwargs):
> +        gdb.Breakpoint.__init__(self, *args, **kwargs)
> +        self.n = 0
> +
> +    def stop(self):
> +        self.n += 1
> +        print("Watchpoint Hit:", self.n, flush=True)
> +        return False
> +
> +
> +MyBreakpoint("i", gdb.BP_WATCHPOINT)
> +
> +print("Python script imported")

^ permalink raw reply	[flat|nested] 34+ messages in thread

* [PING] [PATCH v4] [PR python/29603] Disable out-of-scope watchpoints
  2023-04-09 20:47                   ` Johnson Sun
@ 2023-04-09 20:49                     ` Johnson Sun
  2023-04-17 18:18                       ` [PING^2] " Johnson Sun
  0 siblings, 1 reply; 34+ messages in thread
From: Johnson Sun @ 2023-04-09 20:49 UTC (permalink / raw)
  To: SimonMarchi, LancelotSIX, gdb-patches

I forgot to add the `[PING]` prefix in the last email. Corrected in this 
email. Thanks.

Johnson

On 4/10/2023 4:47 AM, JohnsonSun wrote:
> Ping for: 
> <https://sourceware.org/pipermail/gdb-patches/2023-March/198253.html>
>
> Johnson
>
> On 3/24/2023 2:31 AM, 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
>> ---
>>   gdb/breakpoint.c                           |  1 +
>>   gdb/testsuite/gdb.python/py-watchpoint.c   | 27 +++++++++++
>>   gdb/testsuite/gdb.python/py-watchpoint.exp | 54 ++++++++++++++++++++++
>>   gdb/testsuite/gdb.python/py-watchpoint.py  | 30 ++++++++++++
>>   4 files changed, 112 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..0c58b31647
>> --- /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++)
>> +    printf ("%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..5504736629
>> --- /dev/null
>> +++ b/gdb/testsuite/gdb.python/py-watchpoint.exp
>> @@ -0,0 +1,54 @@
>> +# 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
>> +}
>> +
>> +# Skip all tests if Python scripting is not enabled.
>> +if { [skip_python_tests] } { continue }
>> +
>> +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"
>> +gdb_test "break 24" ".*" "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_sequence "continue" "run until program stops" {
>> +    "Watchpoint Hit: 1"
>> +    "Watchpoint Hit: 2"
>> +    "Watchpoint Hit: 3"
>> +    "Watchpoint Hit: 4"
>> +    "Watchpoint 3 deleted because the program has left the block in"
>> +    "which its expression is valid\."
>> +    "Watchpoint Hit: 5"
>> +    "012\\[Inferior 1 \\(process .*\\) exited normally\\]"
>> +}
>> +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..c5fae00e93
>> --- /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, *args, **kwargs):
>> +        gdb.Breakpoint.__init__(self, *args, **kwargs)
>> +        self.n = 0
>> +
>> +    def stop(self):
>> +        self.n += 1
>> +        print("Watchpoint Hit:", self.n, flush=True)
>> +        return False
>> +
>> +
>> +MyBreakpoint("i", gdb.BP_WATCHPOINT)
>> +
>> +print("Python script imported")
>

^ permalink raw reply	[flat|nested] 34+ messages in thread

* [PING^2] [PATCH v4] [PR python/29603] Disable out-of-scope watchpoints
  2023-04-09 20:49                     ` [PING] " Johnson Sun
@ 2023-04-17 18:18                       ` Johnson Sun
  0 siblings, 0 replies; 34+ messages in thread
From: Johnson Sun @ 2023-04-17 18:18 UTC (permalink / raw)
  To: SimonMarchi, LancelotSIX, gdb-patches

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

Johnson

On 4/10/2023 4:49 AM, JohnsonSun wrote:
> I forgot to add the `[PING]` prefix in the last email. Corrected in 
> this email. Thanks.
>
> Johnson
>
> On 4/10/2023 4:47 AM, JohnsonSun wrote:
>> Ping for: 
>> <https://sourceware.org/pipermail/gdb-patches/2023-March/198253.html>
>>
>> Johnson
>>
>> On 3/24/2023 2:31 AM, 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
>>> ---
>>>   gdb/breakpoint.c                           |  1 +
>>>   gdb/testsuite/gdb.python/py-watchpoint.c   | 27 +++++++++++
>>>   gdb/testsuite/gdb.python/py-watchpoint.exp | 54 
>>> ++++++++++++++++++++++
>>>   gdb/testsuite/gdb.python/py-watchpoint.py  | 30 ++++++++++++
>>>   4 files changed, 112 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..0c58b31647
>>> --- /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++)
>>> +    printf ("%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..5504736629
>>> --- /dev/null
>>> +++ b/gdb/testsuite/gdb.python/py-watchpoint.exp
>>> @@ -0,0 +1,54 @@
>>> +# 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
>>> +}
>>> +
>>> +# Skip all tests if Python scripting is not enabled.
>>> +if { [skip_python_tests] } { continue }
>>> +
>>> +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"
>>> +gdb_test "break 24" ".*" "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_sequence "continue" "run until program stops" {
>>> +    "Watchpoint Hit: 1"
>>> +    "Watchpoint Hit: 2"
>>> +    "Watchpoint Hit: 3"
>>> +    "Watchpoint Hit: 4"
>>> +    "Watchpoint 3 deleted because the program has left the block in"
>>> +    "which its expression is valid\."
>>> +    "Watchpoint Hit: 5"
>>> +    "012\\[Inferior 1 \\(process .*\\) exited normally\\]"
>>> +}
>>> +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..c5fae00e93
>>> --- /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, *args, **kwargs):
>>> +        gdb.Breakpoint.__init__(self, *args, **kwargs)
>>> +        self.n = 0
>>> +
>>> +    def stop(self):
>>> +        self.n += 1
>>> +        print("Watchpoint Hit:", self.n, flush=True)
>>> +        return False
>>> +
>>> +
>>> +MyBreakpoint("i", gdb.BP_WATCHPOINT)
>>> +
>>> +print("Python script imported")
>>
>

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH v4] [PR python/29603] Disable out-of-scope watchpoints
  2023-03-23 18:31                 ` [PATCH v4] " Johnson Sun
  2023-04-09 20:47                   ` Johnson Sun
@ 2023-04-17 18:56                   ` Simon Marchi
  2023-04-23  9:46                     ` Johnson Sun
  1 sibling, 1 reply; 34+ messages in thread
From: Simon Marchi @ 2023-04-17 18:56 UTC (permalink / raw)
  To: Johnson Sun, Lancelot SIX, gdb-patches

> diff --git a/gdb/testsuite/gdb.python/py-watchpoint.exp b/gdb/testsuite/gdb.python/py-watchpoint.exp
> new file mode 100644
> index 0000000000..5504736629
> --- /dev/null
> +++ b/gdb/testsuite/gdb.python/py-watchpoint.exp
> @@ -0,0 +1,54 @@
> +# 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
> +}
> +
> +# Skip all tests if Python scripting is not enabled.
> +if { [skip_python_tests] } { continue }
This needs to be updated to:

  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"
> +gdb_test "break 24" ".*" "set breakpoint before loop"

Avoid using hardcoded line numbers.  Either break on a symbol (function)
name, or put a comment in the source code and use gdb_get_line_number.

> diff --git a/gdb/testsuite/gdb.python/py-watchpoint.py b/gdb/testsuite/gdb.python/py-watchpoint.py
> new file mode 100644
> index 0000000000..c5fae00e93
> --- /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, *args, **kwargs):
> +        gdb.Breakpoint.__init__(self, *args, **kwargs)
> +        self.n = 0

I think it would be a bit more straightforward to read if you passed the
arguments to the base class directly:

    def __init__(self):
      super().__init__("i", gdb.BP_WATCHPOINT)
      self.n = 0

I think we can use the super() notation, now that we only support Python
3?

> +
> +    def stop(self):
> +        self.n += 1
> +        print("Watchpoint Hit:", self.n, flush=True)
> +        return False
> +
> +
> +MyBreakpoint("i", gdb.BP_WATCHPOINT)
> +
> +print("Python script imported")

I tried your test with the native-gdbserver and
native-extended-gdbserver boards, and for some reason it doesn't work.
You can try with:

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

In gdb.log:

    ...
    Python script imported
    (gdb) PASS: gdb.python/py-watchpoint.exp: import python scripts
    python print(len(gdb.breakpoints()))
    2
    (gdb) PASS: gdb.python/py-watchpoint.exp: check modified BP count
    gdb_expect_list pattern: /Watchpoint Hit: 1/
    continue
    Continuing.
    Watchpoint Hit: 1
    gdb_expect_list pattern: /Watchpoint Hit: 2/
    FAIL: gdb.python/py-watchpoint.exp: run until program stops (pattern 2) (timeout)

I didn't dig to understand why, but we would need to fix that before
merging.

Simon


^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH v4] [PR python/29603] Disable out-of-scope watchpoints
  2023-04-17 18:56                   ` Simon Marchi
@ 2023-04-23  9:46                     ` Johnson Sun
  2023-04-23  9:54                       ` [PATCH v5] " Johnson Sun
  0 siblings, 1 reply; 34+ messages in thread
From: Johnson Sun @ 2023-04-23  9:46 UTC (permalink / raw)
  To: SimonMarchi, JohnsonSun, LancelotSIX, gdb-patches

Hi Simon,

Thanks for reviewing this!  I revised the patch and

will send version 5 shortly.

The new version has been tested on the native-gdbserver and

native-extended-gdbserver boards.

Cheers,
Johnson


^ permalink raw reply	[flat|nested] 34+ messages in thread

* [PATCH v5] [PR python/29603] Disable out-of-scope watchpoints
  2023-04-23  9:46                     ` Johnson Sun
@ 2023-04-23  9:54                       ` Johnson Sun
  2023-05-06 19:06                         ` [PING] " Johnson Sun
  2023-05-09 18:50                         ` Simon Marchi
  0 siblings, 2 replies; 34+ messages in thread
From: Johnson Sun @ 2023-04-23  9:54 UTC (permalink / raw)
  To: SimonMarchi, Lancelot SIX, gdb-patches; +Cc: j3.soon777

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")
-- 
2.34.1

base-commit: c99b2113a478a075e8d8f7c3848a92f1ce73f847

^ permalink raw reply	[flat|nested] 34+ messages in thread

* [PING] [PATCH v5] [PR python/29603] Disable out-of-scope watchpoints
  2023-04-23  9:54                       ` [PATCH v5] " Johnson Sun
@ 2023-05-06 19:06                         ` Johnson Sun
  2023-05-09 18:50                         ` Simon Marchi
  1 sibling, 0 replies; 34+ messages in thread
From: Johnson Sun @ 2023-05-06 19:06 UTC (permalink / raw)
  To: JohnsonSun, SimonMarchi, LancelotSIX, gdb-patches

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

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH v5] [PR python/29603] Disable out-of-scope watchpoints
  2023-04-23  9:54                       ` [PATCH v5] " Johnson Sun
  2023-05-06 19:06                         ` [PING] " Johnson Sun
@ 2023-05-09 18:50                         ` Simon Marchi
  2023-05-10 17:22                           ` Johnson Sun
  1 sibling, 1 reply; 34+ messages in thread
From: Simon Marchi @ 2023-05-09 18:50 UTC (permalink / raw)
  To: Johnson Sun, Lancelot SIX, gdb-patches

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

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH v5] [PR python/29603] Disable out-of-scope watchpoints
  2023-05-09 18:50                         ` Simon Marchi
@ 2023-05-10 17:22                           ` Johnson Sun
  2023-05-11  2:08                             ` Simon Marchi
  0 siblings, 1 reply; 34+ messages in thread
From: Johnson Sun @ 2023-05-10 17:22 UTC (permalink / raw)
  To: SimonMarchi, JohnsonSun, LancelotSIX, gdb-patches

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
>

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH v5] [PR python/29603] Disable out-of-scope watchpoints
  2023-05-10 17:22                           ` Johnson Sun
@ 2023-05-11  2:08                             ` Simon Marchi
  2023-05-11 15:46                               ` [PATCH v6] " Johnson Sun
  2023-05-11 15:50                               ` [PATCH v5] " Johnson Sun
  0 siblings, 2 replies; 34+ messages in thread
From: Simon Marchi @ 2023-05-11  2:08 UTC (permalink / raw)
  To: Johnson Sun, LancelotSIX, gdb-patches

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

^ permalink raw reply	[flat|nested] 34+ messages in thread

* [PATCH v6] [PR python/29603] Disable out-of-scope watchpoints
  2023-05-11  2:08                             ` Simon Marchi
@ 2023-05-11 15:46                               ` Johnson Sun
  2023-05-11 16:09                                 ` Simon Marchi
  2023-05-11 15:50                               ` [PATCH v5] " Johnson Sun
  1 sibling, 1 reply; 34+ messages in thread
From: Johnson Sun @ 2023-05-11 15:46 UTC (permalink / raw)
  To: SimonMarchi, Lancelot SIX, gdb-patches; +Cc: j3.soon777

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   | 26 ++++++++++++
 gdb/testsuite/gdb.python/py-watchpoint.exp | 46 ++++++++++++++++++++++
 gdb/testsuite/gdb.python/py-watchpoint.py  | 30 ++++++++++++++
 4 files changed, 103 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 1cc9e84c11f..8f35b7f7951 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -1843,6 +1843,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 00000000000..a54f379660b
--- /dev/null
+++ b/gdb/testsuite/gdb.python/py-watchpoint.c
@@ -0,0 +1,26 @@
+/* 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)
+{
+  volatile int i = -1;
+  for (i = 0; i < 3; i++); /* main for */
+  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 00000000000..fd7dbe20eb2
--- /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 00000000000..647a653085e
--- /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")
-- 
2.34.1

base-commit: 39453f9d8cf03b382d34f3548706f1ae5916e34e

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH v5] [PR python/29603] Disable out-of-scope watchpoints
  2023-05-11  2:08                             ` Simon Marchi
  2023-05-11 15:46                               ` [PATCH v6] " Johnson Sun
@ 2023-05-11 15:50                               ` Johnson Sun
  1 sibling, 0 replies; 34+ messages in thread
From: Johnson Sun @ 2023-05-11 15:50 UTC (permalink / raw)
  To: SimonMarchi, JohnsonSun, LancelotSIX, gdb-patches

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



^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH v6] [PR python/29603] Disable out-of-scope watchpoints
  2023-05-11 15:46                               ` [PATCH v6] " Johnson Sun
@ 2023-05-11 16:09                                 ` Simon Marchi
  0 siblings, 0 replies; 34+ messages in thread
From: Simon Marchi @ 2023-05-11 16:09 UTC (permalink / raw)
  To: Johnson Sun, Lancelot SIX, gdb-patches

On 5/11/23 11:46, Johnson Sun via Gdb-patches 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

Thanks, this version of the test works fine for me.  I'll go ahead and
push the patch.

Simon

^ permalink raw reply	[flat|nested] 34+ messages in thread

end of thread, other threads:[~2023-05-11 16:09 UTC | newest]

Thread overview: 34+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-09-25  5:33 [PATCH] [PR python/29603] Fix deletion of Watchpoints Johnson Sun
2022-09-25 18:10 ` Lancelot SIX
2022-10-01  5:20 ` Johnson Sun
2022-10-01  5:27   ` [PATCH v2] [PR python/29603] Disable out-of-scope watchpoints Johnson Sun
2022-10-20 17:57     ` Johnson Sun
2022-10-20 18:11       ` [PATCH v3] " Johnson Sun
2022-11-18 12:17         ` [PING] " Johnson Sun
2022-11-25 15:11           ` [PING^2] " Johnson Sun
2022-12-04 16:45             ` [PING^3] " Johnson Sun
2022-12-12 21:44               ` [PING^4] " Johnson Sun
2022-12-20 22:08                 ` [PING^5] " Johnson Sun
2022-12-27 16:40                   ` [PING^6] " Johnson Sun
2023-01-12 18:34                     ` [PING^7] " Johnson Sun
2023-01-13 15:40         ` Simon Marchi
2023-01-23 10:15           ` Johnson Sun
2023-02-18 16:26             ` [PING] " Johnson Sun
2023-02-26  6:16               ` [RFC] " Johnson Sun
2023-03-12 17:24                 ` [PING] " Johnson Sun
2023-03-13 16:00             ` Simon Marchi
2023-03-23 18:25               ` Johnson Sun
2023-03-23 18:31                 ` [PATCH v4] " Johnson Sun
2023-04-09 20:47                   ` Johnson Sun
2023-04-09 20:49                     ` [PING] " Johnson Sun
2023-04-17 18:18                       ` [PING^2] " Johnson Sun
2023-04-17 18:56                   ` Simon Marchi
2023-04-23  9:46                     ` Johnson Sun
2023-04-23  9:54                       ` [PATCH v5] " Johnson Sun
2023-05-06 19:06                         ` [PING] " Johnson Sun
2023-05-09 18:50                         ` Simon Marchi
2023-05-10 17:22                           ` Johnson Sun
2023-05-11  2:08                             ` Simon Marchi
2023-05-11 15:46                               ` [PATCH v6] " Johnson Sun
2023-05-11 16:09                                 ` Simon Marchi
2023-05-11 15:50                               ` [PATCH v5] " Johnson Sun

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).