public inbox for gdb-patches@sourceware.org
 help / color / mirror / Atom feed
* [PATCH 0/3] [PR gdb/18655] Fix Python FinishBreakpoints not deleted bug
@ 2022-09-20 17:24 Johnson Sun
  2022-09-20 17:24 ` [PATCH 1/3] [gdb/testsuite] Add gdb.python/py-finish-breakpoint-deletion.exp for Bug 18655 Johnson Sun
                   ` (4 more replies)
  0 siblings, 5 replies; 24+ messages in thread
From: Johnson Sun @ 2022-09-20 17:24 UTC (permalink / raw)
  To: gdb-patches; +Cc: j3.soon777

This adds tests and fixes Bug 18655: Python FinishBreakpoints are not deleted.

Patch #1 adds a testcase.
Patch #2 is the core of the fix.
Patch #3 fixes some comments.

I ran the testsuite on an x86_64 Ubuntu 22.04 VM and find no test regression.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=18655

 gdb/python/py-finishbreakpoint.c                   |  5 ++-
 .../gdb.python/py-finish-breakpoint-deletion.c     | 28 +++++++++++++
 .../gdb.python/py-finish-breakpoint-deletion.exp   | 46 ++++++++++++++++++++++
 .../gdb.python/py-finish-breakpoint-deletion.py    | 33 ++++++++++++++++
 4 files changed, 110 insertions(+), 2 deletions(-)


base-commit: c99b2113a478a075e8d8f7c3848a92f1ce73f847


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

* [PATCH 1/3] [gdb/testsuite] Add gdb.python/py-finish-breakpoint-deletion.exp for Bug 18655
  2022-09-20 17:24 [PATCH 0/3] [PR gdb/18655] Fix Python FinishBreakpoints not deleted bug Johnson Sun
@ 2022-09-20 17:24 ` Johnson Sun
  2022-09-21 13:44   ` Bruno Larsen
  2022-09-20 17:24 ` [PATCH 2/3] [gdb/python] Fix " Johnson Sun
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 24+ messages in thread
From: Johnson Sun @ 2022-09-20 17:24 UTC (permalink / raw)
  To: gdb-patches; +Cc: j3.soon777

This adds a minimal test that will fail (KFAIL) due to not deleting Python FinishBreakpoints.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=18655
---
 .../py-finish-breakpoint-deletion.c           | 28 +++++++++++
 .../py-finish-breakpoint-deletion.exp         | 47 +++++++++++++++++++
 .../py-finish-breakpoint-deletion.py          | 33 +++++++++++++
 3 files changed, 108 insertions(+)
 create mode 100644 gdb/testsuite/gdb.python/py-finish-breakpoint-deletion.c
 create mode 100644 gdb/testsuite/gdb.python/py-finish-breakpoint-deletion.exp
 create mode 100644 gdb/testsuite/gdb.python/py-finish-breakpoint-deletion.py

diff --git a/gdb/testsuite/gdb.python/py-finish-breakpoint-deletion.c b/gdb/testsuite/gdb.python/py-finish-breakpoint-deletion.c
new file mode 100644
index 00000000000..d2e6783799c
--- /dev/null
+++ b/gdb/testsuite/gdb.python/py-finish-breakpoint-deletion.c
@@ -0,0 +1,28 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2011-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/>.  */
+
+int subroutine(int a) {
+  return a;
+}
+
+int main() {
+  int i;
+  for (i = 0; i < 5; i++) {
+    subroutine(i);
+  }
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.python/py-finish-breakpoint-deletion.exp b/gdb/testsuite/gdb.python/py-finish-breakpoint-deletion.exp
new file mode 100644
index 00000000000..778b53fbeda
--- /dev/null
+++ b/gdb/testsuite/gdb.python/py-finish-breakpoint-deletion.exp
@@ -0,0 +1,47 @@
+# Copyright (C) 2011-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 tests the mechanism
+# exposing values to Python.
+
+load_lib gdb-python.exp
+
+standard_testfile
+
+if {[prepare_for_testing "failed to prepare" $testfile $srcfile {debug c++}]} {
+    return -1
+}
+
+# Skip all tests if Python scripting is not enabled.
+if { [skip_python_tests] } { continue }
+
+if ![runto_main] then {
+    return 0
+}
+
+set pyfile [gdb_remote_download host \
+		${srcdir}/${subdir}/${testfile}.py]
+
+#
+# Check FinishBreakpoints are deleted after used
+#
+
+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" "Breakpoint.*at.*" "run until FinishBreakpoint stops"
+setup_kfail "gdb/18655" "*-*-*"
+gdb_test "python print (len(gdb.breakpoints()))" "2" "check BP count"
diff --git a/gdb/testsuite/gdb.python/py-finish-breakpoint-deletion.py b/gdb/testsuite/gdb.python/py-finish-breakpoint-deletion.py
new file mode 100644
index 00000000000..8146940d41f
--- /dev/null
+++ b/gdb/testsuite/gdb.python/py-finish-breakpoint-deletion.py
@@ -0,0 +1,33 @@
+# Copyright (C) 2011-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 tests python Finish
+# Breakpoints.
+
+
+class MyFinishBreakpoint(gdb.FinishBreakpoint):
+    def stop(self):
+        print("stopped at MyFinishBreakpoint")
+        return self.return_value == 4
+
+class MyBreakpoint(gdb.Breakpoint):
+    def stop(self):
+        print("stopped at MyBreakpoint")
+        MyFinishBreakpoint()
+        return False
+
+MyBreakpoint("subroutine", internal=True)
+
+print("Python script imported")
-- 
2.34.1


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

* [PATCH 2/3] [gdb/python] Fix gdb.python/py-finish-breakpoint-deletion.exp for Bug 18655
  2022-09-20 17:24 [PATCH 0/3] [PR gdb/18655] Fix Python FinishBreakpoints not deleted bug Johnson Sun
  2022-09-20 17:24 ` [PATCH 1/3] [gdb/testsuite] Add gdb.python/py-finish-breakpoint-deletion.exp for Bug 18655 Johnson Sun
@ 2022-09-20 17:24 ` Johnson Sun
  2022-09-21 14:02   ` Bruno Larsen
  2022-09-20 17:24 ` [PATCH 3/3] [gdb/python] Replace gdbpy_should_stop with gdbpy_breakpoint_cond_says_stop Johnson Sun
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 24+ messages in thread
From: Johnson Sun @ 2022-09-20 17:24 UTC (permalink / raw)
  To: gdb-patches; +Cc: j3.soon777

This removes the KFAIL in the testcase, and fixes the bug by setting the
correct disposition type for deletion.

The original code fails since it does not correctly delete the temporary
FinishBreakpoints in `gdb/python/py-finishbreakpoint.c':

	/* Can't delete it here, but it will be removed at the next stop.  */
	disable_breakpoint (bp_obj->bp);
	gdb_assert (bp_obj->bp->disposition == disp_del);

The code above aims to delete the breakpoint on the next hit by setting the
disposition to `disp_del'. However, the FinishBreakpoint is disabled, so it
will never be hit (thus never be deleted before the program stops).

The fix only consists of a single line, setting the disposition to
`disp_del_at_next_stop':

	bp_obj->bp->disposition = disp_del_at_next_stop;

The code above allows the breakpoint to be deleted in `breakpoint_auto_delete':

	for (breakpoint *b : all_breakpoints_safe ())
	  if (b->disposition == disp_del_at_next_stop)
	    delete_breakpoint (b);

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=18655
---
 gdb/python/py-finishbreakpoint.c                           | 1 +
 gdb/testsuite/gdb.python/py-finish-breakpoint-deletion.exp | 1 -
 2 files changed, 1 insertion(+), 1 deletion(-)

diff --git a/gdb/python/py-finishbreakpoint.c b/gdb/python/py-finishbreakpoint.c
index c80096f6810..a219bc82f15 100644
--- a/gdb/python/py-finishbreakpoint.c
+++ b/gdb/python/py-finishbreakpoint.c
@@ -146,6 +146,7 @@ bpfinishpy_post_stop_hook (struct gdbpy_breakpoint_object *bp_obj)
       /* Can't delete it here, but it will be removed at the next stop.  */
       disable_breakpoint (bp_obj->bp);
       gdb_assert (bp_obj->bp->disposition == disp_del);
+      bp_obj->bp->disposition = disp_del_at_next_stop;
     }
   catch (const gdb_exception &except)
     {
diff --git a/gdb/testsuite/gdb.python/py-finish-breakpoint-deletion.exp b/gdb/testsuite/gdb.python/py-finish-breakpoint-deletion.exp
index 778b53fbeda..ac5e5d8ac2e 100644
--- a/gdb/testsuite/gdb.python/py-finish-breakpoint-deletion.exp
+++ b/gdb/testsuite/gdb.python/py-finish-breakpoint-deletion.exp
@@ -43,5 +43,4 @@ 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" "Breakpoint.*at.*" "run until FinishBreakpoint stops"
-setup_kfail "gdb/18655" "*-*-*"
 gdb_test "python print (len(gdb.breakpoints()))" "2" "check BP count"
-- 
2.34.1


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

* [PATCH 3/3] [gdb/python] Replace gdbpy_should_stop with gdbpy_breakpoint_cond_says_stop
  2022-09-20 17:24 [PATCH 0/3] [PR gdb/18655] Fix Python FinishBreakpoints not deleted bug Johnson Sun
  2022-09-20 17:24 ` [PATCH 1/3] [gdb/testsuite] Add gdb.python/py-finish-breakpoint-deletion.exp for Bug 18655 Johnson Sun
  2022-09-20 17:24 ` [PATCH 2/3] [gdb/python] Fix " Johnson Sun
@ 2022-09-20 17:24 ` Johnson Sun
  2022-09-21 14:09   ` Bruno Larsen
  2022-09-23  5:41 ` [PATCH v2] [PR python/18655] Fix deletion of FinishBreakpoints Johnson Sun
  2022-09-23  6:00 ` [PATCH v2] [gdb/python] Replace gdbpy_should_stop with gdbpy_breakpoint_cond_says_stop Johnson Sun
  4 siblings, 1 reply; 24+ messages in thread
From: Johnson Sun @ 2022-09-20 17:24 UTC (permalink / raw)
  To: gdb-patches; +Cc: j3.soon777

In 2014, the function `gdbpy_should_stop' has been replaced with
`gdbpy_breakpoint_cond_says_stop'

This replaces `gdbpy_should_stop' with `gdbpy_breakpoint_cond_says_stop' in the
comments.

Since `gdbpy_should_stop' has been renamed as noted in `gdb/ChangeLog-2014':

	* python/py-breakpoint.c (gdbpy_breakpoint_cond_says_stop): Renamed
	from gdbpy_should_stop.  Change result type to enum scr_bp_stop.
---
 gdb/python/py-finishbreakpoint.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/gdb/python/py-finishbreakpoint.c b/gdb/python/py-finishbreakpoint.c
index a219bc82f15..d608c95be0f 100644
--- a/gdb/python/py-finishbreakpoint.c
+++ b/gdb/python/py-finishbreakpoint.c
@@ -90,7 +90,7 @@ bpfinishpy_dealloc (PyObject *self)
   Py_TYPE (self)->tp_free (self);
 }
 
-/* Triggered when gdbpy_should_stop is about to execute the `stop' callback
+/* Triggered when gdbpy_breakpoint_cond_says_stop is about to execute the `stop' callback
    of the gdb.FinishBreakpoint object BP_OBJ.  Will compute and cache the
    `return_value', if possible.  */
 
@@ -134,7 +134,7 @@ bpfinishpy_pre_stop_hook (struct gdbpy_breakpoint_object *bp_obj)
     }
 }
 
-/* Triggered when gdbpy_should_stop has triggered the `stop' callback
+/* Triggered when gdbpy_breakpoint_cond_says_stop has triggered the `stop' callback
    of the gdb.FinishBreakpoint object BP_OBJ.  */
 
 void
-- 
2.34.1


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

* Re: [PATCH 1/3] [gdb/testsuite] Add gdb.python/py-finish-breakpoint-deletion.exp for Bug 18655
  2022-09-20 17:24 ` [PATCH 1/3] [gdb/testsuite] Add gdb.python/py-finish-breakpoint-deletion.exp for Bug 18655 Johnson Sun
@ 2022-09-21 13:44   ` Bruno Larsen
  2022-09-23  5:16     ` Johnson Sun
  0 siblings, 1 reply; 24+ messages in thread
From: Bruno Larsen @ 2022-09-21 13:44 UTC (permalink / raw)
  To: Johnson Sun, gdb-patches


On 20/09/2022 19:24, Johnson Sun via Gdb-patches wrote:
> This adds a minimal test that will fail (KFAIL) due to not deleting Python FinishBreakpoints.
>
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=18655

Hi Johnson!

Thanks for working on this!

In general, I think it is better to add the testcase in the same patch 
that fixes the behavior, so I think you can squash this commit into the 
next one. I also have some comments on code styling, which have been 
inlined.

> ---
>   .../py-finish-breakpoint-deletion.c           | 28 +++++++++++
>   .../py-finish-breakpoint-deletion.exp         | 47 +++++++++++++++++++
>   .../py-finish-breakpoint-deletion.py          | 33 +++++++++++++
>   3 files changed, 108 insertions(+)
>   create mode 100644 gdb/testsuite/gdb.python/py-finish-breakpoint-deletion.c
>   create mode 100644 gdb/testsuite/gdb.python/py-finish-breakpoint-deletion.exp
>   create mode 100644 gdb/testsuite/gdb.python/py-finish-breakpoint-deletion.py
>
> diff --git a/gdb/testsuite/gdb.python/py-finish-breakpoint-deletion.c b/gdb/testsuite/gdb.python/py-finish-breakpoint-deletion.c
> new file mode 100644
> index 00000000000..d2e6783799c
> --- /dev/null
> +++ b/gdb/testsuite/gdb.python/py-finish-breakpoint-deletion.c
> @@ -0,0 +1,28 @@
> +/* This testcase is part of GDB, the GNU debugger.
> +
> +   Copyright 2011-2022 Free Software Foundation, Inc.
Since the file is only being added in 2022, you can remove the range and 
just leave 2022 here. Goes for all copyright blurbs.
> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see  <http://www.gnu.org/licenses/>.  */
> +
> +int subroutine(int a) {
> +  return a;
> +}

Even for tests, whenever possible, code should follow the GNU coding 
style, so this function should look like:

int

subroutine (int a)

{

   return a;

}

Same goes for the main function.

> +
> +int main() {
> +  int i;
> +  for (i = 0; i < 5; i++) {
> +    subroutine(i);
> +  }
> +  return 0;
> +}
> diff --git a/gdb/testsuite/gdb.python/py-finish-breakpoint-deletion.exp b/gdb/testsuite/gdb.python/py-finish-breakpoint-deletion.exp
> new file mode 100644
> index 00000000000..778b53fbeda
> --- /dev/null
> +++ b/gdb/testsuite/gdb.python/py-finish-breakpoint-deletion.exp
> @@ -0,0 +1,47 @@
> +# Copyright (C) 2011-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 tests the mechanism
> +# exposing values to Python.

The description of the test here seems a bit off, is it a copy-paste 
mistake?

I think the "check FinishBreakpoints (...)" comment can be used here 
instead.

> +
> +load_lib gdb-python.exp
> +
> +standard_testfile
> +
> +if {[prepare_for_testing "failed to prepare" $testfile $srcfile {debug c++}]} {
is there a specific reason you decided to compile the testcase as C++ 
when the code is pure C?
> +    return -1
> +}
> +
> +# Skip all tests if Python scripting is not enabled.
> +if { [skip_python_tests] } { continue }
> +
> +if ![runto_main] then {
> +    return 0
> +}
> +
> +set pyfile [gdb_remote_download host \
> +		${srcdir}/${subdir}/${testfile}.py]
This looks unnecessary, I think you can just "source 
${srcdir}/${subdir}/${testfile}.py" down below.
> +
> +#
> +# Check FinishBreakpoints are deleted after used
> +#
> +
> +gdb_test "python print (len(gdb.breakpoints()))" "1" "check default BP count"
> +gdb_test "source $pyfile" ".*Python script imported.*" \
> +         "import python scripts"

change the 8 spaces here for a tab.

Cheers,
Bruno


> +gdb_test "python print (len(gdb.breakpoints()))" "2" "check modified BP count"
> +gdb_test "continue" "Breakpoint.*at.*" "run until FinishBreakpoint stops"
> +setup_kfail "gdb/18655" "*-*-*"
> +gdb_test "python print (len(gdb.breakpoints()))" "2" "check BP count"
> diff --git a/gdb/testsuite/gdb.python/py-finish-breakpoint-deletion.py b/gdb/testsuite/gdb.python/py-finish-breakpoint-deletion.py
> new file mode 100644
> index 00000000000..8146940d41f
> --- /dev/null
> +++ b/gdb/testsuite/gdb.python/py-finish-breakpoint-deletion.py
> @@ -0,0 +1,33 @@
> +# Copyright (C) 2011-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 tests python Finish
> +# Breakpoints.
> +
> +
> +class MyFinishBreakpoint(gdb.FinishBreakpoint):
> +    def stop(self):
> +        print("stopped at MyFinishBreakpoint")
> +        return self.return_value == 4
> +
> +class MyBreakpoint(gdb.Breakpoint):
> +    def stop(self):
> +        print("stopped at MyBreakpoint")
> +        MyFinishBreakpoint()
> +        return False
> +
> +MyBreakpoint("subroutine", internal=True)
> +
> +print("Python script imported")


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

* Re: [PATCH 2/3] [gdb/python] Fix gdb.python/py-finish-breakpoint-deletion.exp for Bug 18655
  2022-09-20 17:24 ` [PATCH 2/3] [gdb/python] Fix " Johnson Sun
@ 2022-09-21 14:02   ` Bruno Larsen
  0 siblings, 0 replies; 24+ messages in thread
From: Bruno Larsen @ 2022-09-21 14:02 UTC (permalink / raw)
  To: Johnson Sun, gdb-patches

Hi Johnson,

Thanks for working on this!

The code looks fine, but the description can be improved a bit. We try 
to make commit titles as descriptive as possible on their own. I think 
describing this as "gdb/python: Fix deletion of FinishBreakpoints" would 
be easier when referencing this in the future.

On 20/09/2022 19:24, Johnson Sun via Gdb-patches wrote:
> This removes the KFAIL in the testcase, and fixes the bug by setting the
> correct disposition type for deletion.
>
> The original code fails since it does not correctly delete the temporary
> FinishBreakpoints in `gdb/python/py-finishbreakpoint.c':
>
> 	/* Can't delete it here, but it will be removed at the next stop.  */
> 	disable_breakpoint (bp_obj->bp);
> 	gdb_assert (bp_obj->bp->disposition == disp_del);
>
> The code above aims to delete the breakpoint on the next hit by setting the
> disposition to `disp_del'. However, the FinishBreakpoint is disabled, so it
> will never be hit (thus never be deleted before the program stops).
>
> The fix only consists of a single line, setting the disposition to
> `disp_del_at_next_stop':
>
> 	bp_obj->bp->disposition = disp_del_at_next_stop;

This may be personal preference, but I would usually describe the fix in 
less computer-y terms, like:

"This commit fixes this by setting the disposition to 
disp_del_at_next_stop instead, which gets cleaned up in 
breakpoint_auto_delete".

>
> The code above allows the breakpoint to be deleted in `breakpoint_auto_delete':
>
> 	for (breakpoint *b : all_breakpoints_safe ())
> 	  if (b->disposition == disp_del_at_next_stop)
> 	    delete_breakpoint (b);
>
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=18655

Also, we tend to refer to bugs as PR <component>/<number>, so this would 
be PR python/18655. This way bugzilla can automatically link things for us.

Cheers,
Bruno

> ---
>   gdb/python/py-finishbreakpoint.c                           | 1 +
>   gdb/testsuite/gdb.python/py-finish-breakpoint-deletion.exp | 1 -
>   2 files changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/gdb/python/py-finishbreakpoint.c b/gdb/python/py-finishbreakpoint.c
> index c80096f6810..a219bc82f15 100644
> --- a/gdb/python/py-finishbreakpoint.c
> +++ b/gdb/python/py-finishbreakpoint.c
> @@ -146,6 +146,7 @@ bpfinishpy_post_stop_hook (struct gdbpy_breakpoint_object *bp_obj)
>         /* Can't delete it here, but it will be removed at the next stop.  */
>         disable_breakpoint (bp_obj->bp);
>         gdb_assert (bp_obj->bp->disposition == disp_del);
> +      bp_obj->bp->disposition = disp_del_at_next_stop;
>       }
>     catch (const gdb_exception &except)
>       {
> diff --git a/gdb/testsuite/gdb.python/py-finish-breakpoint-deletion.exp b/gdb/testsuite/gdb.python/py-finish-breakpoint-deletion.exp
> index 778b53fbeda..ac5e5d8ac2e 100644
> --- a/gdb/testsuite/gdb.python/py-finish-breakpoint-deletion.exp
> +++ b/gdb/testsuite/gdb.python/py-finish-breakpoint-deletion.exp
> @@ -43,5 +43,4 @@ 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" "Breakpoint.*at.*" "run until FinishBreakpoint stops"
> -setup_kfail "gdb/18655" "*-*-*"
>   gdb_test "python print (len(gdb.breakpoints()))" "2" "check BP count"


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

* Re: [PATCH 3/3] [gdb/python] Replace gdbpy_should_stop with gdbpy_breakpoint_cond_says_stop
  2022-09-20 17:24 ` [PATCH 3/3] [gdb/python] Replace gdbpy_should_stop with gdbpy_breakpoint_cond_says_stop Johnson Sun
@ 2022-09-21 14:09   ` Bruno Larsen
  0 siblings, 0 replies; 24+ messages in thread
From: Bruno Larsen @ 2022-09-21 14:09 UTC (permalink / raw)
  To: gdb-patches


On 20/09/2022 19:24, Johnson Sun via Gdb-patches wrote:
> In 2014, the function `gdbpy_should_stop' has been replaced with
> `gdbpy_breakpoint_cond_says_stop'
>
> This replaces `gdbpy_should_stop' with `gdbpy_breakpoint_cond_says_stop' in the
> comments.
>
> Since `gdbpy_should_stop' has been renamed as noted in `gdb/ChangeLog-2014':
>
> 	* python/py-breakpoint.c (gdbpy_breakpoint_cond_says_stop): Renamed
> 	from gdbpy_should_stop.  Change result type to enum scr_bp_stop.

Hi Johnson!

Thanks for cleaning this up! This looks obvious enough for  me, but I 
can't approve patches to be pushed. I hope a maintainer looks at it soon.

On that note, do you have a reason to have sent this along with the rest 
of the patch series? It doesn't look directly related, and patch series 
tend to take a longer time to be reviewed, so you may have better luck 
submitting unrelated things separately in the future :-)

Cheers,
Bruno

> ---
>   gdb/python/py-finishbreakpoint.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/gdb/python/py-finishbreakpoint.c b/gdb/python/py-finishbreakpoint.c
> index a219bc82f15..d608c95be0f 100644
> --- a/gdb/python/py-finishbreakpoint.c
> +++ b/gdb/python/py-finishbreakpoint.c
> @@ -90,7 +90,7 @@ bpfinishpy_dealloc (PyObject *self)
>     Py_TYPE (self)->tp_free (self);
>   }
>   
> -/* Triggered when gdbpy_should_stop is about to execute the `stop' callback
> +/* Triggered when gdbpy_breakpoint_cond_says_stop is about to execute the `stop' callback
>      of the gdb.FinishBreakpoint object BP_OBJ.  Will compute and cache the
>      `return_value', if possible.  */
>   
> @@ -134,7 +134,7 @@ bpfinishpy_pre_stop_hook (struct gdbpy_breakpoint_object *bp_obj)
>       }
>   }
>   
> -/* Triggered when gdbpy_should_stop has triggered the `stop' callback
> +/* Triggered when gdbpy_breakpoint_cond_says_stop has triggered the `stop' callback
>      of the gdb.FinishBreakpoint object BP_OBJ.  */
>   
>   void


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

* Re: [PATCH 1/3] [gdb/testsuite] Add gdb.python/py-finish-breakpoint-deletion.exp for Bug 18655
  2022-09-21 13:44   ` Bruno Larsen
@ 2022-09-23  5:16     ` Johnson Sun
  0 siblings, 0 replies; 24+ messages in thread
From: Johnson Sun @ 2022-09-23  5:16 UTC (permalink / raw)
  To: BrunoLarsen, gdb-patches; +Cc: j3.soon777

Hi Bruno,

Thanks for reviewing this!  I revised the patch and 
will send version 2 shortly.

Cheers,
Johnson


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

* [PATCH v2] [PR python/18655] Fix deletion of FinishBreakpoints
  2022-09-20 17:24 [PATCH 0/3] [PR gdb/18655] Fix Python FinishBreakpoints not deleted bug Johnson Sun
                   ` (2 preceding siblings ...)
  2022-09-20 17:24 ` [PATCH 3/3] [gdb/python] Replace gdbpy_should_stop with gdbpy_breakpoint_cond_says_stop Johnson Sun
@ 2022-09-23  5:41 ` Johnson Sun
  2022-09-23  8:32   ` Bruno Larsen
  2022-09-23 20:27   ` [PATCH v3] " Johnson Sun
  2022-09-23  6:00 ` [PATCH v2] [gdb/python] Replace gdbpy_should_stop with gdbpy_breakpoint_cond_says_stop Johnson Sun
  4 siblings, 2 replies; 24+ messages in thread
From: Johnson Sun @ 2022-09-23  5:41 UTC (permalink / raw)
  To: BrunoLarsen, gdb-patches; +Cc: j3.soon777

This commit fixes the deletion of FinishBreakpoints by setting the disposition
to disp_del_at_next_stop instead, which gets cleaned up in
breakpoint_auto_delete.

The original code fails since it does not correctly delete the temporary
FinishBreakpoints in `gdb/python/py-finishbreakpoint.c':

	/* Can't delete it here, but it will be removed at the next stop.  */
	disable_breakpoint (bp_obj->bp);
	gdb_assert (bp_obj->bp->disposition == disp_del);

The code above aims to delete the breakpoint on the next hit by setting the
disposition to `disp_del'. However, the FinishBreakpoint is disabled, so it
will never be hit (thus never be deleted before the program stops).

The fix only consists of a single line, setting the disposition to
`disp_del_at_next_stop':

	bp_obj->bp->disposition = disp_del_at_next_stop;

The code above allows the breakpoint to be deleted in `breakpoint_auto_delete':

	for (breakpoint *b : all_breakpoints_safe ())
	  if (b->disposition == disp_del_at_next_stop)
	    delete_breakpoint (b);

A minimal test is added to verify this issue.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=18655
---
 gdb/python/py-finishbreakpoint.c              |  1 +
 .../py-finish-breakpoint-deletion.c           | 31 +++++++++++++
 .../py-finish-breakpoint-deletion.exp         | 43 +++++++++++++++++++
 .../py-finish-breakpoint-deletion.py          | 33 ++++++++++++++
 4 files changed, 108 insertions(+)
 create mode 100644 gdb/testsuite/gdb.python/py-finish-breakpoint-deletion.c
 create mode 100644 gdb/testsuite/gdb.python/py-finish-breakpoint-deletion.exp
 create mode 100644 gdb/testsuite/gdb.python/py-finish-breakpoint-deletion.py

diff --git a/gdb/python/py-finishbreakpoint.c b/gdb/python/py-finishbreakpoint.c
index c80096f6810..a219bc82f15 100644
--- a/gdb/python/py-finishbreakpoint.c
+++ b/gdb/python/py-finishbreakpoint.c
@@ -146,6 +146,7 @@ bpfinishpy_post_stop_hook (struct gdbpy_breakpoint_object *bp_obj)
       /* Can't delete it here, but it will be removed at the next stop.  */
       disable_breakpoint (bp_obj->bp);
       gdb_assert (bp_obj->bp->disposition == disp_del);
+      bp_obj->bp->disposition = disp_del_at_next_stop;
     }
   catch (const gdb_exception &except)
     {
diff --git a/gdb/testsuite/gdb.python/py-finish-breakpoint-deletion.c b/gdb/testsuite/gdb.python/py-finish-breakpoint-deletion.c
new file mode 100644
index 00000000000..258c8dc3861
--- /dev/null
+++ b/gdb/testsuite/gdb.python/py-finish-breakpoint-deletion.c
@@ -0,0 +1,31 @@
+/* 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/>.  */
+
+int
+subroutine (int a)
+{
+  return a;
+}
+
+int
+main ()
+{
+  int i;
+  for (i = 0; i < 5; i++)
+    subroutine(i);
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.python/py-finish-breakpoint-deletion.exp b/gdb/testsuite/gdb.python/py-finish-breakpoint-deletion.exp
new file mode 100644
index 00000000000..bcf697266a1
--- /dev/null
+++ b/gdb/testsuite/gdb.python/py-finish-breakpoint-deletion.exp
@@ -0,0 +1,43 @@
+# 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 FinishBreakpoints
+# 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 FinishBreakpoints are deleted after used
+#
+
+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" "Breakpoint.*at.*" "run until FinishBreakpoint stops"
+gdb_test "python print (len(gdb.breakpoints()))" "2" "check BP count"
diff --git a/gdb/testsuite/gdb.python/py-finish-breakpoint-deletion.py b/gdb/testsuite/gdb.python/py-finish-breakpoint-deletion.py
new file mode 100644
index 00000000000..f207d0bd2bf
--- /dev/null
+++ b/gdb/testsuite/gdb.python/py-finish-breakpoint-deletion.py
@@ -0,0 +1,33 @@
+# 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 FinishBreakpoints
+# are deleted after used.
+
+
+class MyFinishBreakpoint(gdb.FinishBreakpoint):
+    def stop(self):
+        print("stopped at MyFinishBreakpoint")
+        return self.return_value == 4
+
+class MyBreakpoint(gdb.Breakpoint):
+    def stop(self):
+        print("stopped at MyBreakpoint")
+        MyFinishBreakpoint()
+        return False
+
+MyBreakpoint("subroutine", internal=True)
+
+print("Python script imported")
-- 
2.34.1

base-commit: c99b2113a478a075e8d8f7c3848a92f1ce73f847

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

* [PATCH v2] [gdb/python] Replace gdbpy_should_stop with gdbpy_breakpoint_cond_says_stop
  2022-09-20 17:24 [PATCH 0/3] [PR gdb/18655] Fix Python FinishBreakpoints not deleted bug Johnson Sun
                   ` (3 preceding siblings ...)
  2022-09-23  5:41 ` [PATCH v2] [PR python/18655] Fix deletion of FinishBreakpoints Johnson Sun
@ 2022-09-23  6:00 ` Johnson Sun
  2022-10-20 18:24   ` [PING] " Johnson Sun
  4 siblings, 1 reply; 24+ messages in thread
From: Johnson Sun @ 2022-09-23  6:00 UTC (permalink / raw)
  To: BrunoLarsen, gdb-patches; +Cc: j3.soon777

In 2014, the function `gdbpy_should_stop' has been replaced with
`gdbpy_breakpoint_cond_says_stop'

This replaces `gdbpy_should_stop' with `gdbpy_breakpoint_cond_says_stop' in the
comments.

Since `gdbpy_should_stop' has been renamed as noted in `gdb/ChangeLog-2014':

	* python/py-breakpoint.c (gdbpy_breakpoint_cond_says_stop): Renamed
	from gdbpy_should_stop.  Change result type to enum scr_bp_stop.
---
 gdb/python/py-finishbreakpoint.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/gdb/python/py-finishbreakpoint.c b/gdb/python/py-finishbreakpoint.c
index c80096f6810..dcf86317779 100644
--- a/gdb/python/py-finishbreakpoint.c
+++ b/gdb/python/py-finishbreakpoint.c
@@ -90,7 +90,7 @@ bpfinishpy_dealloc (PyObject *self)
   Py_TYPE (self)->tp_free (self);
 }
 
-/* Triggered when gdbpy_should_stop is about to execute the `stop' callback
+/* Triggered when gdbpy_breakpoint_cond_says_stop is about to execute the `stop' callback
    of the gdb.FinishBreakpoint object BP_OBJ.  Will compute and cache the
    `return_value', if possible.  */
 
@@ -134,7 +134,7 @@ bpfinishpy_pre_stop_hook (struct gdbpy_breakpoint_object *bp_obj)
     }
 }
 
-/* Triggered when gdbpy_should_stop has triggered the `stop' callback
+/* Triggered when gdbpy_breakpoint_cond_says_stop has triggered the `stop' callback
    of the gdb.FinishBreakpoint object BP_OBJ.  */
 
 void
-- 
2.34.1

base-commit: c99b2113a478a075e8d8f7c3848a92f1ce73f847

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

* Re: [PATCH v2] [PR python/18655] Fix deletion of FinishBreakpoints
  2022-09-23  5:41 ` [PATCH v2] [PR python/18655] Fix deletion of FinishBreakpoints Johnson Sun
@ 2022-09-23  8:32   ` Bruno Larsen
  2022-09-23 20:16     ` Johnson Sun
  2022-09-23 20:27   ` [PATCH v3] " Johnson Sun
  1 sibling, 1 reply; 24+ messages in thread
From: Bruno Larsen @ 2022-09-23  8:32 UTC (permalink / raw)
  To: Johnson Sun, gdb-patches

On 23/09/2022 07:41, Johnson Sun wrote:
> This commit fixes the deletion of FinishBreakpoints by setting the disposition
> to disp_del_at_next_stop instead, which gets cleaned up in
> breakpoint_auto_delete.
>
> The original code fails since it does not correctly delete the temporary
> FinishBreakpoints in `gdb/python/py-finishbreakpoint.c':
>
> 	/* Can't delete it here, but it will be removed at the next stop.  */
> 	disable_breakpoint (bp_obj->bp);
> 	gdb_assert (bp_obj->bp->disposition == disp_del);
>
> The code above aims to delete the breakpoint on the next hit by setting the
> disposition to `disp_del'. However, the FinishBreakpoint is disabled, so it
> will never be hit (thus never be deleted before the program stops).
>
> The fix only consists of a single line, setting the disposition to
> `disp_del_at_next_stop':
>
> 	bp_obj->bp->disposition = disp_del_at_next_stop;
>
> The code above allows the breakpoint to be deleted in `breakpoint_auto_delete':
>
> 	for (breakpoint *b : all_breakpoints_safe ())
> 	  if (b->disposition == disp_del_at_next_stop)
> 	    delete_breakpoint (b);
>
> A minimal test is added to verify this issue.
>
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=18655

Hi Johnson!

Thanks for the new version of the patch! I think I wasn't quite clear in 
my first review, but we tend to try to explain the problem and solution 
as separate from the code as possible, because some times code can 
change drastically, but knowing the logic behind a previous solution 
might help with current problems. With this in mind, I would usually 
write a commit message like this:


Currently, GDB creates FinishBreakpoints to execute the command finish, 
and these are meant to be temporary breakpoints. However, they are not 
being cleaned up after use, as reported in PR python/18655. This was 
happening because the disposition of the breakpoint was not being set 
correctly.

This commit fixes this issue by correctly setting the disposition in the 
post stop hook of the breakpoint. It also adds a test to ensure this 
feature isn't regressed in the future.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=18655


Feel free to change the wording to your preference, or to match better 
what GDB actually does (this is my first time even hearing about 
FinishBreakpoints), but I do think that this style of explanation would 
be more beneficial when referring back to this commit in the future.

I also have 2 minor comments below.

> ---
>   gdb/python/py-finishbreakpoint.c              |  1 +
>   .../py-finish-breakpoint-deletion.c           | 31 +++++++++++++
>   .../py-finish-breakpoint-deletion.exp         | 43 +++++++++++++++++++
>   .../py-finish-breakpoint-deletion.py          | 33 ++++++++++++++
>   4 files changed, 108 insertions(+)
>   create mode 100644 gdb/testsuite/gdb.python/py-finish-breakpoint-deletion.c
>   create mode 100644 gdb/testsuite/gdb.python/py-finish-breakpoint-deletion.exp
>   create mode 100644 gdb/testsuite/gdb.python/py-finish-breakpoint-deletion.py
>
> diff --git a/gdb/python/py-finishbreakpoint.c b/gdb/python/py-finishbreakpoint.c
> index c80096f6810..a219bc82f15 100644
> --- a/gdb/python/py-finishbreakpoint.c
> +++ b/gdb/python/py-finishbreakpoint.c
> @@ -146,6 +146,7 @@ bpfinishpy_post_stop_hook (struct gdbpy_breakpoint_object *bp_obj)
>         /* Can't delete it here, but it will be removed at the next stop.  */
>         disable_breakpoint (bp_obj->bp);
>         gdb_assert (bp_obj->bp->disposition == disp_del);
> +      bp_obj->bp->disposition = disp_del_at_next_stop;
I think you can remove the assert. There is no point checking the 
previous disposition if you'll rewrite it in the next line IMHO.
>       }
>     catch (const gdb_exception &except)
>       {
> diff --git a/gdb/testsuite/gdb.python/py-finish-breakpoint-deletion.c b/gdb/testsuite/gdb.python/py-finish-breakpoint-deletion.c
> new file mode 100644
> index 00000000000..258c8dc3861
> --- /dev/null
> +++ b/gdb/testsuite/gdb.python/py-finish-breakpoint-deletion.c
> @@ -0,0 +1,31 @@
> +/* 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/>.  */
> +
> +int
> +subroutine (int a)
> +{
> +  return a;
> +}
> +
> +int
> +main ()
> +{
> +  int i;
> +  for (i = 0; i < 5; i++)
> +    subroutine(i);
space before the (

Cheers,
Bruno


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

* Re: [PATCH v2] [PR python/18655] Fix deletion of FinishBreakpoints
  2022-09-23  8:32   ` Bruno Larsen
@ 2022-09-23 20:16     ` Johnson Sun
  0 siblings, 0 replies; 24+ messages in thread
From: Johnson Sun @ 2022-09-23 20:16 UTC (permalink / raw)
  To: BrunoLarsen, gdb-patches; +Cc: j3.soon777

Hi Bruno,

Thanks for reviewing this!  I totally agree with you.

I revised the patch and will send version 3 shortly.

Cheers,
Johnson


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

* [PATCH v3] [PR python/18655] Fix deletion of FinishBreakpoints
  2022-09-23  5:41 ` [PATCH v2] [PR python/18655] Fix deletion of FinishBreakpoints Johnson Sun
  2022-09-23  8:32   ` Bruno Larsen
@ 2022-09-23 20:27   ` Johnson Sun
  2022-10-12  1:53     ` Simon Marchi
  1 sibling, 1 reply; 24+ messages in thread
From: Johnson Sun @ 2022-09-23 20:27 UTC (permalink / raw)
  To: BrunoLarsen, gdb-patches; +Cc: j3.soon777

Currently, FinishBreakpoints are set at the return address of a frame based on
the `finish' command, and are meant to be temporary breakpoints. However, they
are not being cleaned up after use, as reported in PR python/18655. This was
happening because the disposition of the breakpoint was not being set
correctly.

This commit fixes this issue by correctly setting the disposition in the
post-stop hook of the breakpoint. It also adds a test to ensure this feature
isn't regressed in the future.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=18655
---
 gdb/python/py-finishbreakpoint.c              |  2 +-
 .../py-finish-breakpoint-deletion.c           | 31 +++++++++++++
 .../py-finish-breakpoint-deletion.exp         | 43 +++++++++++++++++++
 .../py-finish-breakpoint-deletion.py          | 33 ++++++++++++++
 4 files changed, 108 insertions(+), 1 deletion(-)
 create mode 100644 gdb/testsuite/gdb.python/py-finish-breakpoint-deletion.c
 create mode 100644 gdb/testsuite/gdb.python/py-finish-breakpoint-deletion.exp
 create mode 100644 gdb/testsuite/gdb.python/py-finish-breakpoint-deletion.py

diff --git a/gdb/python/py-finishbreakpoint.c b/gdb/python/py-finishbreakpoint.c
index c80096f6810..d8b1aff0e2b 100644
--- a/gdb/python/py-finishbreakpoint.c
+++ b/gdb/python/py-finishbreakpoint.c
@@ -145,7 +145,7 @@ bpfinishpy_post_stop_hook (struct gdbpy_breakpoint_object *bp_obj)
     {
       /* Can't delete it here, but it will be removed at the next stop.  */
       disable_breakpoint (bp_obj->bp);
-      gdb_assert (bp_obj->bp->disposition == disp_del);
+      bp_obj->bp->disposition = disp_del_at_next_stop;
     }
   catch (const gdb_exception &except)
     {
diff --git a/gdb/testsuite/gdb.python/py-finish-breakpoint-deletion.c b/gdb/testsuite/gdb.python/py-finish-breakpoint-deletion.c
new file mode 100644
index 00000000000..f09f58861e8
--- /dev/null
+++ b/gdb/testsuite/gdb.python/py-finish-breakpoint-deletion.c
@@ -0,0 +1,31 @@
+/* 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/>.  */
+
+int
+subroutine (int a)
+{
+  return a;
+}
+
+int
+main ()
+{
+  int i;
+  for (i = 0; i < 5; i++)
+    subroutine (i);
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.python/py-finish-breakpoint-deletion.exp b/gdb/testsuite/gdb.python/py-finish-breakpoint-deletion.exp
new file mode 100644
index 00000000000..25919900a59
--- /dev/null
+++ b/gdb/testsuite/gdb.python/py-finish-breakpoint-deletion.exp
@@ -0,0 +1,43 @@
+# 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 FinishBreakpoints
+# 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 FinishBreakpoints are deleted after used
+#
+
+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" "Breakpoint.*at.*" "run until FinishBreakpoint stops"
+gdb_test "python print (len(gdb.breakpoints()))" "2" "check BP count"
diff --git a/gdb/testsuite/gdb.python/py-finish-breakpoint-deletion.py b/gdb/testsuite/gdb.python/py-finish-breakpoint-deletion.py
new file mode 100644
index 00000000000..f207d0bd2bf
--- /dev/null
+++ b/gdb/testsuite/gdb.python/py-finish-breakpoint-deletion.py
@@ -0,0 +1,33 @@
+# 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 FinishBreakpoints
+# are deleted after used.
+
+
+class MyFinishBreakpoint(gdb.FinishBreakpoint):
+    def stop(self):
+        print("stopped at MyFinishBreakpoint")
+        return self.return_value == 4
+
+class MyBreakpoint(gdb.Breakpoint):
+    def stop(self):
+        print("stopped at MyBreakpoint")
+        MyFinishBreakpoint()
+        return False
+
+MyBreakpoint("subroutine", internal=True)
+
+print("Python script imported")
-- 
2.34.1

base-commit: c99b2113a478a075e8d8f7c3848a92f1ce73f847

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

* Re: [PATCH v3] [PR python/18655] Fix deletion of FinishBreakpoints
  2022-09-23 20:27   ` [PATCH v3] " Johnson Sun
@ 2022-10-12  1:53     ` Simon Marchi
  2022-10-20 17:34       ` Johnson Sun
  0 siblings, 1 reply; 24+ messages in thread
From: Simon Marchi @ 2022-10-12  1:53 UTC (permalink / raw)
  To: Johnson Sun, BrunoLarsen, gdb-patches

Hi,

On 2022-09-23 16:27, Johnson Sun via Gdb-patches wrote:
> Currently, FinishBreakpoints are set at the return address of a frame based on
> the `finish' command, and are meant to be temporary breakpoints. However, they
> are not being cleaned up after use, as reported in PR python/18655. This was
> happening because the disposition of the breakpoint was not being set
> correctly.
> 
> This commit fixes this issue by correctly setting the disposition in the
> post-stop hook of the breakpoint. It also adds a test to ensure this feature
> isn't regressed in the future.
> 
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=18655

Whoa, that bug was filed by a younger me, apparently.  Thanks for fixing
it.

Since your patch (the test case) is more than a few obvious lines, I
think that you will need a copyright assignment (unless you have one and
I just don't see it, or your employer has a blanket copyright assignment
and this is done on behalf of your employer).  If you need one, just
follow this form:

  https://git.savannah.gnu.org/cgit/gnulib.git/plain/doc/Copyright/request-assign.future

In the mean time, the patch mostly looks good to me, just some
additional nits.

> ---
>  gdb/python/py-finishbreakpoint.c              |  2 +-
>  .../py-finish-breakpoint-deletion.c           | 31 +++++++++++++
>  .../py-finish-breakpoint-deletion.exp         | 43 +++++++++++++++++++
>  .../py-finish-breakpoint-deletion.py          | 33 ++++++++++++++
>  4 files changed, 108 insertions(+), 1 deletion(-)
>  create mode 100644 gdb/testsuite/gdb.python/py-finish-breakpoint-deletion.c
>  create mode 100644 gdb/testsuite/gdb.python/py-finish-breakpoint-deletion.exp
>  create mode 100644 gdb/testsuite/gdb.python/py-finish-breakpoint-deletion.py
> 
> diff --git a/gdb/python/py-finishbreakpoint.c b/gdb/python/py-finishbreakpoint.c
> index c80096f6810..d8b1aff0e2b 100644
> --- a/gdb/python/py-finishbreakpoint.c
> +++ b/gdb/python/py-finishbreakpoint.c
> @@ -145,7 +145,7 @@ bpfinishpy_post_stop_hook (struct gdbpy_breakpoint_object *bp_obj)
>      {
>        /* Can't delete it here, but it will be removed at the next stop.  */
>        disable_breakpoint (bp_obj->bp);
> -      gdb_assert (bp_obj->bp->disposition == disp_del);
> +      bp_obj->bp->disposition = disp_del_at_next_stop;
>      }
>    catch (const gdb_exception &except)
>      {
> diff --git a/gdb/testsuite/gdb.python/py-finish-breakpoint-deletion.c b/gdb/testsuite/gdb.python/py-finish-breakpoint-deletion.c
> new file mode 100644
> index 00000000000..f09f58861e8
> --- /dev/null
> +++ b/gdb/testsuite/gdb.python/py-finish-breakpoint-deletion.c
> @@ -0,0 +1,31 @@
> +/* 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/>.  */
> +
> +int
> +subroutine (int a)
> +{
> +  return a;
> +}

Not super important, but subroutine could be static.

> +
> +int
> +main ()

main (void)

> +{
> +  int i;
> +  for (i = 0; i < 5; i++)
> +    subroutine (i);
> +  return 0;
> +}
> diff --git a/gdb/testsuite/gdb.python/py-finish-breakpoint-deletion.exp b/gdb/testsuite/gdb.python/py-finish-breakpoint-deletion.exp
> new file mode 100644
> index 00000000000..25919900a59
> --- /dev/null
> +++ b/gdb/testsuite/gdb.python/py-finish-breakpoint-deletion.exp
> @@ -0,0 +1,43 @@
> +# 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 FinishBreakpoints
> +# are deleted after used.

You can remove the "This file is part of the GDB testsuite" part.  Just
leave:

# Check that FinishBreakpoints 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
> +}
> +
> +#
> +# Check FinishBreakpoints are deleted after used
> +#

That comment can be removed, it's redundant with what's at the top of
the file.

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

For consistency, remove the space after "print".

> +gdb_test "source ${srcdir}/${subdir}/${testfile}.py" ".*Python script imported.*" \
> +	 "import python scripts"

Indent that last line with just 4 spaces.

In case somebody uses remote host testing, you should upload the .py
file to the test host first.  See py-unwind.exp for an example.

> +gdb_test "python print (len(gdb.breakpoints()))" "2" "check modified BP count"
> +gdb_test "continue" "Breakpoint.*at.*" "run until FinishBreakpoint stops"
> +gdb_test "python print (len(gdb.breakpoints()))" "2" "check BP count"
> diff --git a/gdb/testsuite/gdb.python/py-finish-breakpoint-deletion.py b/gdb/testsuite/gdb.python/py-finish-breakpoint-deletion.py
> new file mode 100644
> index 00000000000..f207d0bd2bf
> --- /dev/null
> +++ b/gdb/testsuite/gdb.python/py-finish-breakpoint-deletion.py
> @@ -0,0 +1,33 @@
> +# 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 FinishBreakpoints
> +# are deleted after used.

I don't think you need that comment on the .py file: the entry point to
the test is the .exp, this is where the purpose of the test is
documented.

Please format the file using `black`, as described here:

https://sourceware.org/gdb/wiki/Internals%20GDB-Python-Coding-Standards

Simon

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

* Re: [PATCH v3] [PR python/18655] Fix deletion of FinishBreakpoints
  2022-10-12  1:53     ` Simon Marchi
@ 2022-10-20 17:34       ` Johnson Sun
  2022-10-20 17:49         ` [PATCH v4] " Johnson Sun
  0 siblings, 1 reply; 24+ messages in thread
From: Johnson Sun @ 2022-10-20 17:34 UTC (permalink / raw)
  To: SimonMarchi, BrunoLarsen, gdb-patches; +Cc: j3.soon777

Hi Simon,

Thanks for reviewing this and filing the bug a few years ago!

I have signed the copyright form and revised the patch.

Will send version 4 shortly.

Cheers,
Johnson


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

* [PATCH v4] [PR python/18655] Fix deletion of FinishBreakpoints
  2022-10-20 17:34       ` Johnson Sun
@ 2022-10-20 17:49         ` Johnson Sun
  2022-11-18 12:16           ` [PING] " Johnson Sun
  2022-11-18 15:56           ` Simon Marchi
  0 siblings, 2 replies; 24+ messages in thread
From: Johnson Sun @ 2022-10-20 17:49 UTC (permalink / raw)
  To: SimonMarchi, BrunoLarsen, gdb-patches; +Cc: j3.soon777

Currently, FinishBreakpoints are set at the return address of a frame based on
the `finish' command, and are meant to be temporary breakpoints. However, they
are not being cleaned up after use, as reported in PR python/18655. This was
happening because the disposition of the breakpoint was not being set
correctly.

This commit fixes this issue by correctly setting the disposition in the
post-stop hook of the breakpoint. It also adds a test to ensure this feature
isn't regressed in the future.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=18655
---
 gdb/python/py-finishbreakpoint.c              |  2 +-
 .../py-finish-breakpoint-deletion.c           | 31 ++++++++++++++
 .../py-finish-breakpoint-deletion.exp         | 41 +++++++++++++++++++
 .../py-finish-breakpoint-deletion.py          | 32 +++++++++++++++
 4 files changed, 105 insertions(+), 1 deletion(-)
 create mode 100644 gdb/testsuite/gdb.python/py-finish-breakpoint-deletion.c
 create mode 100644 gdb/testsuite/gdb.python/py-finish-breakpoint-deletion.exp
 create mode 100644 gdb/testsuite/gdb.python/py-finish-breakpoint-deletion.py

diff --git a/gdb/python/py-finishbreakpoint.c b/gdb/python/py-finishbreakpoint.c
index c80096f6810..d8b1aff0e2b 100644
--- a/gdb/python/py-finishbreakpoint.c
+++ b/gdb/python/py-finishbreakpoint.c
@@ -145,7 +145,7 @@ bpfinishpy_post_stop_hook (struct gdbpy_breakpoint_object *bp_obj)
     {
       /* Can't delete it here, but it will be removed at the next stop.  */
       disable_breakpoint (bp_obj->bp);
-      gdb_assert (bp_obj->bp->disposition == disp_del);
+      bp_obj->bp->disposition = disp_del_at_next_stop;
     }
   catch (const gdb_exception &except)
     {
diff --git a/gdb/testsuite/gdb.python/py-finish-breakpoint-deletion.c b/gdb/testsuite/gdb.python/py-finish-breakpoint-deletion.c
new file mode 100644
index 00000000000..b6e71513373
--- /dev/null
+++ b/gdb/testsuite/gdb.python/py-finish-breakpoint-deletion.c
@@ -0,0 +1,31 @@
+/* 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/>.  */
+
+static int
+subroutine (int a)
+{
+  return a;
+}
+
+int
+main (void)
+{
+  int i;
+  for (i = 0; i < 5; i++)
+    subroutine (i);
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.python/py-finish-breakpoint-deletion.exp b/gdb/testsuite/gdb.python/py-finish-breakpoint-deletion.exp
new file mode 100644
index 00000000000..cd882cee91d
--- /dev/null
+++ b/gdb/testsuite/gdb.python/py-finish-breakpoint-deletion.exp
@@ -0,0 +1,41 @@
+# 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 FinishBreakpoints 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 "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" "Breakpoint.*at.*" "run until FinishBreakpoint stops"
+gdb_test "python print(len(gdb.breakpoints()))" "2" "check BP count"
diff --git a/gdb/testsuite/gdb.python/py-finish-breakpoint-deletion.py b/gdb/testsuite/gdb.python/py-finish-breakpoint-deletion.py
new file mode 100644
index 00000000000..ab05b0c35a3
--- /dev/null
+++ b/gdb/testsuite/gdb.python/py-finish-breakpoint-deletion.py
@@ -0,0 +1,32 @@
+# 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 MyFinishBreakpoint(gdb.FinishBreakpoint):
+    def stop(self):
+        print("stopped at MyFinishBreakpoint")
+        return self.return_value == 4
+
+
+class MyBreakpoint(gdb.Breakpoint):
+    def stop(self):
+        print("stopped at MyBreakpoint")
+        MyFinishBreakpoint()
+        return False
+
+
+MyBreakpoint("subroutine", internal=True)
+
+print("Python script imported")
-- 
2.34.1

base-commit: c99b2113a478a075e8d8f7c3848a92f1ce73f847

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

* [PING] [PATCH v2] [gdb/python] Replace gdbpy_should_stop with gdbpy_breakpoint_cond_says_stop
  2022-09-23  6:00 ` [PATCH v2] [gdb/python] Replace gdbpy_should_stop with gdbpy_breakpoint_cond_says_stop Johnson Sun
@ 2022-10-20 18:24   ` Johnson Sun
  2022-11-18 12:14     ` [PING^2] " Johnson Sun
  0 siblings, 1 reply; 24+ messages in thread
From: Johnson Sun @ 2022-10-20 18:24 UTC (permalink / raw)
  To: BrunoLarsen, gdb-patches; +Cc: j3.soon777

In 2014, the function `gdbpy_should_stop' has been replaced with
`gdbpy_breakpoint_cond_says_stop'

This replaces `gdbpy_should_stop' with `gdbpy_breakpoint_cond_says_stop' in the
comments.

Since `gdbpy_should_stop' has been renamed as noted in `gdb/ChangeLog-2014':

	* python/py-breakpoint.c (gdbpy_breakpoint_cond_says_stop): Renamed
	from gdbpy_should_stop.  Change result type to enum scr_bp_stop.
---
 gdb/python/py-finishbreakpoint.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/gdb/python/py-finishbreakpoint.c b/gdb/python/py-finishbreakpoint.c
index c80096f6810..dcf86317779 100644
--- a/gdb/python/py-finishbreakpoint.c
+++ b/gdb/python/py-finishbreakpoint.c
@@ -90,7 +90,7 @@ bpfinishpy_dealloc (PyObject *self)
   Py_TYPE (self)->tp_free (self);
 }
 
-/* Triggered when gdbpy_should_stop is about to execute the `stop' callback
+/* Triggered when gdbpy_breakpoint_cond_says_stop is about to execute the `stop' callback
    of the gdb.FinishBreakpoint object BP_OBJ.  Will compute and cache the
    `return_value', if possible.  */
 
@@ -134,7 +134,7 @@ bpfinishpy_pre_stop_hook (struct gdbpy_breakpoint_object *bp_obj)
     }
 }
 
-/* Triggered when gdbpy_should_stop has triggered the `stop' callback
+/* Triggered when gdbpy_breakpoint_cond_says_stop has triggered the `stop' callback
    of the gdb.FinishBreakpoint object BP_OBJ.  */
 
 void
-- 
2.34.1

base-commit: c99b2113a478a075e8d8f7c3848a92f1ce73f847

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

* [PING^2] [PATCH v2] [gdb/python] Replace gdbpy_should_stop with gdbpy_breakpoint_cond_says_stop
  2022-10-20 18:24   ` [PING] " Johnson Sun
@ 2022-11-18 12:14     ` Johnson Sun
  2022-11-25 15:10       ` [PING^3] " Johnson Sun
  0 siblings, 1 reply; 24+ messages in thread
From: Johnson Sun @ 2022-11-18 12:14 UTC (permalink / raw)
  To: BrunoLarsen, gdb-patches

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

Johnson

On 10/21/2022 2:24 AM, Johnson Sun wrote:
> In 2014, the function `gdbpy_should_stop' has been replaced with
> `gdbpy_breakpoint_cond_says_stop'
>
> This replaces `gdbpy_should_stop' with `gdbpy_breakpoint_cond_says_stop' in the
> comments.
>
> Since `gdbpy_should_stop' has been renamed as noted in `gdb/ChangeLog-2014':
>
> 	* python/py-breakpoint.c (gdbpy_breakpoint_cond_says_stop): Renamed
> 	from gdbpy_should_stop.  Change result type to enum scr_bp_stop.
> ---
>   gdb/python/py-finishbreakpoint.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/gdb/python/py-finishbreakpoint.c b/gdb/python/py-finishbreakpoint.c
> index c80096f6810..dcf86317779 100644
> --- a/gdb/python/py-finishbreakpoint.c
> +++ b/gdb/python/py-finishbreakpoint.c
> @@ -90,7 +90,7 @@ bpfinishpy_dealloc (PyObject *self)
>     Py_TYPE (self)->tp_free (self);
>   }
>   
> -/* Triggered when gdbpy_should_stop is about to execute the `stop' callback
> +/* Triggered when gdbpy_breakpoint_cond_says_stop is about to execute the `stop' callback
>      of the gdb.FinishBreakpoint object BP_OBJ.  Will compute and cache the
>      `return_value', if possible.  */
>   
> @@ -134,7 +134,7 @@ bpfinishpy_pre_stop_hook (struct gdbpy_breakpoint_object *bp_obj)
>       }
>   }
>   
> -/* Triggered when gdbpy_should_stop has triggered the `stop' callback
> +/* Triggered when gdbpy_breakpoint_cond_says_stop has triggered the `stop' callback
>      of the gdb.FinishBreakpoint object BP_OBJ.  */
>   
>   void

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

* [PING] [PATCH v4] [PR python/18655] Fix deletion of FinishBreakpoints
  2022-10-20 17:49         ` [PATCH v4] " Johnson Sun
@ 2022-11-18 12:16           ` Johnson Sun
  2022-11-18 15:56           ` Simon Marchi
  1 sibling, 0 replies; 24+ messages in thread
From: Johnson Sun @ 2022-11-18 12:16 UTC (permalink / raw)
  To: SimonMarchi, BrunoLarsen, gdb-patches

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

Johnson

On 10/21/2022 1:49 AM, Johnson Sun wrote:
> Currently, FinishBreakpoints are set at the return address of a frame based on
> the `finish' command, and are meant to be temporary breakpoints. However, they
> are not being cleaned up after use, as reported in PR python/18655. This was
> happening because the disposition of the breakpoint was not being set
> correctly.
>
> This commit fixes this issue by correctly setting the disposition in the
> post-stop hook of the breakpoint. It also adds a test to ensure this feature
> isn't regressed in the future.
>
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=18655
> ---
>   gdb/python/py-finishbreakpoint.c              |  2 +-
>   .../py-finish-breakpoint-deletion.c           | 31 ++++++++++++++
>   .../py-finish-breakpoint-deletion.exp         | 41 +++++++++++++++++++
>   .../py-finish-breakpoint-deletion.py          | 32 +++++++++++++++
>   4 files changed, 105 insertions(+), 1 deletion(-)
>   create mode 100644 gdb/testsuite/gdb.python/py-finish-breakpoint-deletion.c
>   create mode 100644 gdb/testsuite/gdb.python/py-finish-breakpoint-deletion.exp
>   create mode 100644 gdb/testsuite/gdb.python/py-finish-breakpoint-deletion.py
>
> diff --git a/gdb/python/py-finishbreakpoint.c b/gdb/python/py-finishbreakpoint.c
> index c80096f6810..d8b1aff0e2b 100644
> --- a/gdb/python/py-finishbreakpoint.c
> +++ b/gdb/python/py-finishbreakpoint.c
> @@ -145,7 +145,7 @@ bpfinishpy_post_stop_hook (struct gdbpy_breakpoint_object *bp_obj)
>       {
>         /* Can't delete it here, but it will be removed at the next stop.  */
>         disable_breakpoint (bp_obj->bp);
> -      gdb_assert (bp_obj->bp->disposition == disp_del);
> +      bp_obj->bp->disposition = disp_del_at_next_stop;
>       }
>     catch (const gdb_exception &except)
>       {
> diff --git a/gdb/testsuite/gdb.python/py-finish-breakpoint-deletion.c b/gdb/testsuite/gdb.python/py-finish-breakpoint-deletion.c
> new file mode 100644
> index 00000000000..b6e71513373
> --- /dev/null
> +++ b/gdb/testsuite/gdb.python/py-finish-breakpoint-deletion.c
> @@ -0,0 +1,31 @@
> +/* 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/>.  */
> +
> +static int
> +subroutine (int a)
> +{
> +  return a;
> +}
> +
> +int
> +main (void)
> +{
> +  int i;
> +  for (i = 0; i < 5; i++)
> +    subroutine (i);
> +  return 0;
> +}
> diff --git a/gdb/testsuite/gdb.python/py-finish-breakpoint-deletion.exp b/gdb/testsuite/gdb.python/py-finish-breakpoint-deletion.exp
> new file mode 100644
> index 00000000000..cd882cee91d
> --- /dev/null
> +++ b/gdb/testsuite/gdb.python/py-finish-breakpoint-deletion.exp
> @@ -0,0 +1,41 @@
> +# 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 FinishBreakpoints 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 "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" "Breakpoint.*at.*" "run until FinishBreakpoint stops"
> +gdb_test "python print(len(gdb.breakpoints()))" "2" "check BP count"
> diff --git a/gdb/testsuite/gdb.python/py-finish-breakpoint-deletion.py b/gdb/testsuite/gdb.python/py-finish-breakpoint-deletion.py
> new file mode 100644
> index 00000000000..ab05b0c35a3
> --- /dev/null
> +++ b/gdb/testsuite/gdb.python/py-finish-breakpoint-deletion.py
> @@ -0,0 +1,32 @@
> +# 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 MyFinishBreakpoint(gdb.FinishBreakpoint):
> +    def stop(self):
> +        print("stopped at MyFinishBreakpoint")
> +        return self.return_value == 4
> +
> +
> +class MyBreakpoint(gdb.Breakpoint):
> +    def stop(self):
> +        print("stopped at MyBreakpoint")
> +        MyFinishBreakpoint()
> +        return False
> +
> +
> +MyBreakpoint("subroutine", internal=True)
> +
> +print("Python script imported")

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

* Re: [PATCH v4] [PR python/18655] Fix deletion of FinishBreakpoints
  2022-10-20 17:49         ` [PATCH v4] " Johnson Sun
  2022-11-18 12:16           ` [PING] " Johnson Sun
@ 2022-11-18 15:56           ` Simon Marchi
  1 sibling, 0 replies; 24+ messages in thread
From: Simon Marchi @ 2022-11-18 15:56 UTC (permalink / raw)
  To: Johnson Sun, BrunoLarsen, gdb-patches



On 10/20/22 13:49, Johnson Sun wrote:
> Currently, FinishBreakpoints are set at the return address of a frame based on
> the `finish' command, and are meant to be temporary breakpoints. However, they
> are not being cleaned up after use, as reported in PR python/18655. This was
> happening because the disposition of the breakpoint was not being set
> correctly.
> 
> This commit fixes this issue by correctly setting the disposition in the
> post-stop hook of the breakpoint. It also adds a test to ensure this feature
> isn't regressed in the future.
> 
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=18655

Thanks, I see your copyright assignment is complete.  I pushed the
patch.

Simon

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

* [PING^3] [PATCH v2] [gdb/python] Replace gdbpy_should_stop with gdbpy_breakpoint_cond_says_stop
  2022-11-18 12:14     ` [PING^2] " Johnson Sun
@ 2022-11-25 15:10       ` Johnson Sun
  2022-12-04 16:45         ` [PING^4] " Johnson Sun
  0 siblings, 1 reply; 24+ messages in thread
From: Johnson Sun @ 2022-11-25 15:10 UTC (permalink / raw)
  To: BrunoLarsen, gdb-patches

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

Johnson

On 11/18/2022 8:14 PM, JohnsonSun wrote:
> Ping for: 
> <https://sourceware.org/pipermail/gdb-patches/2022-October/192910.html>.
>
> Johnson
>
> On 10/21/2022 2:24 AM, Johnson Sun wrote:
>> In 2014, the function `gdbpy_should_stop' has been replaced with
>> `gdbpy_breakpoint_cond_says_stop'
>>
>> This replaces `gdbpy_should_stop' with 
>> `gdbpy_breakpoint_cond_says_stop' in the
>> comments.
>>
>> Since `gdbpy_should_stop' has been renamed as noted in 
>> `gdb/ChangeLog-2014':
>>
>>     * python/py-breakpoint.c (gdbpy_breakpoint_cond_says_stop): Renamed
>>     from gdbpy_should_stop.  Change result type to enum scr_bp_stop.
>> ---
>>   gdb/python/py-finishbreakpoint.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/gdb/python/py-finishbreakpoint.c 
>> b/gdb/python/py-finishbreakpoint.c
>> index c80096f6810..dcf86317779 100644
>> --- a/gdb/python/py-finishbreakpoint.c
>> +++ b/gdb/python/py-finishbreakpoint.c
>> @@ -90,7 +90,7 @@ bpfinishpy_dealloc (PyObject *self)
>>     Py_TYPE (self)->tp_free (self);
>>   }
>>   -/* Triggered when gdbpy_should_stop is about to execute the `stop' 
>> callback
>> +/* Triggered when gdbpy_breakpoint_cond_says_stop is about to 
>> execute the `stop' callback
>>      of the gdb.FinishBreakpoint object BP_OBJ.  Will compute and 
>> cache the
>>      `return_value', if possible.  */
>>   @@ -134,7 +134,7 @@ bpfinishpy_pre_stop_hook (struct 
>> gdbpy_breakpoint_object *bp_obj)
>>       }
>>   }
>>   -/* Triggered when gdbpy_should_stop has triggered the `stop' callback
>> +/* Triggered when gdbpy_breakpoint_cond_says_stop has triggered the 
>> `stop' callback
>>      of the gdb.FinishBreakpoint object BP_OBJ.  */
>>     void
>

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

* [PING^4] [PATCH v2] [gdb/python] Replace gdbpy_should_stop with gdbpy_breakpoint_cond_says_stop
  2022-11-25 15:10       ` [PING^3] " Johnson Sun
@ 2022-12-04 16:45         ` Johnson Sun
  2022-12-12 21:44           ` [PING^5] " Johnson Sun
  0 siblings, 1 reply; 24+ messages in thread
From: Johnson Sun @ 2022-12-04 16:45 UTC (permalink / raw)
  To: BrunoLarsen, gdb-patches

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

Johnson

On 11/25/2022 11:10 PM, Johnson Sun wrote:
> Ping for: 
> <https://sourceware.org/pipermail/gdb-patches/2022-October/192910.html>.
>
> Johnson
>
> On 11/18/2022 8:14 PM, JohnsonSun wrote:
>> Ping for: 
>> <https://sourceware.org/pipermail/gdb-patches/2022-October/192910.html>.
>>
>> Johnson
>>
>> On 10/21/2022 2:24 AM, Johnson Sun wrote:
>>> In 2014, the function `gdbpy_should_stop' has been replaced with
>>> `gdbpy_breakpoint_cond_says_stop'
>>>
>>> This replaces `gdbpy_should_stop' with 
>>> `gdbpy_breakpoint_cond_says_stop' in the
>>> comments.
>>>
>>> Since `gdbpy_should_stop' has been renamed as noted in 
>>> `gdb/ChangeLog-2014':
>>>
>>>     * python/py-breakpoint.c (gdbpy_breakpoint_cond_says_stop): Renamed
>>>     from gdbpy_should_stop.  Change result type to enum scr_bp_stop.
>>> ---
>>>   gdb/python/py-finishbreakpoint.c | 4 ++--
>>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/gdb/python/py-finishbreakpoint.c 
>>> b/gdb/python/py-finishbreakpoint.c
>>> index c80096f6810..dcf86317779 100644
>>> --- a/gdb/python/py-finishbreakpoint.c
>>> +++ b/gdb/python/py-finishbreakpoint.c
>>> @@ -90,7 +90,7 @@ bpfinishpy_dealloc (PyObject *self)
>>>     Py_TYPE (self)->tp_free (self);
>>>   }
>>>   -/* Triggered when gdbpy_should_stop is about to execute the 
>>> `stop' callback
>>> +/* Triggered when gdbpy_breakpoint_cond_says_stop is about to 
>>> execute the `stop' callback
>>>      of the gdb.FinishBreakpoint object BP_OBJ.  Will compute and 
>>> cache the
>>>      `return_value', if possible.  */
>>>   @@ -134,7 +134,7 @@ bpfinishpy_pre_stop_hook (struct 
>>> gdbpy_breakpoint_object *bp_obj)
>>>       }
>>>   }
>>>   -/* Triggered when gdbpy_should_stop has triggered the `stop' 
>>> callback
>>> +/* Triggered when gdbpy_breakpoint_cond_says_stop has triggered the 
>>> `stop' callback
>>>      of the gdb.FinishBreakpoint object BP_OBJ.  */
>>>     void
>>

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

* Re: [PING^5] [PATCH v2] [gdb/python] Replace gdbpy_should_stop with gdbpy_breakpoint_cond_says_stop
  2022-12-04 16:45         ` [PING^4] " Johnson Sun
@ 2022-12-12 21:44           ` Johnson Sun
  2022-12-13  2:34             ` Simon Marchi
  0 siblings, 1 reply; 24+ messages in thread
From: Johnson Sun @ 2022-12-12 21:44 UTC (permalink / raw)
  To: BrunoLarsen, gdb-patches

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

Johnson

On 12/5/2022 12:45 AM, Johnson Sun wrote:
> Ping for: 
> <https://sourceware.org/pipermail/gdb-patches/2022-October/192910.html>.
>
> Johnson
>
> On 11/25/2022 11:10 PM, Johnson Sun wrote:
>> Ping for: 
>> <https://sourceware.org/pipermail/gdb-patches/2022-October/192910.html>.
>>
>> Johnson
>>
>> On 11/18/2022 8:14 PM, JohnsonSun wrote:
>>> Ping for: 
>>> <https://sourceware.org/pipermail/gdb-patches/2022-October/192910.html>.
>>>
>>> Johnson
>>>
>>> On 10/21/2022 2:24 AM, Johnson Sun wrote:
>>>> In 2014, the function `gdbpy_should_stop' has been replaced with
>>>> `gdbpy_breakpoint_cond_says_stop'
>>>>
>>>> This replaces `gdbpy_should_stop' with 
>>>> `gdbpy_breakpoint_cond_says_stop' in the
>>>> comments.
>>>>
>>>> Since `gdbpy_should_stop' has been renamed as noted in 
>>>> `gdb/ChangeLog-2014':
>>>>
>>>>     * python/py-breakpoint.c (gdbpy_breakpoint_cond_says_stop): 
>>>> Renamed
>>>>     from gdbpy_should_stop.  Change result type to enum scr_bp_stop.
>>>> ---
>>>>   gdb/python/py-finishbreakpoint.c | 4 ++--
>>>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/gdb/python/py-finishbreakpoint.c 
>>>> b/gdb/python/py-finishbreakpoint.c
>>>> index c80096f6810..dcf86317779 100644
>>>> --- a/gdb/python/py-finishbreakpoint.c
>>>> +++ b/gdb/python/py-finishbreakpoint.c
>>>> @@ -90,7 +90,7 @@ bpfinishpy_dealloc (PyObject *self)
>>>>     Py_TYPE (self)->tp_free (self);
>>>>   }
>>>>   -/* Triggered when gdbpy_should_stop is about to execute the 
>>>> `stop' callback
>>>> +/* Triggered when gdbpy_breakpoint_cond_says_stop is about to 
>>>> execute the `stop' callback
>>>>      of the gdb.FinishBreakpoint object BP_OBJ.  Will compute and 
>>>> cache the
>>>>      `return_value', if possible.  */
>>>>   @@ -134,7 +134,7 @@ bpfinishpy_pre_stop_hook (struct 
>>>> gdbpy_breakpoint_object *bp_obj)
>>>>       }
>>>>   }
>>>>   -/* Triggered when gdbpy_should_stop has triggered the `stop' 
>>>> callback
>>>> +/* Triggered when gdbpy_breakpoint_cond_says_stop has triggered 
>>>> the `stop' callback
>>>>      of the gdb.FinishBreakpoint object BP_OBJ.  */
>>>>     void
>>>

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

* Re: [PING^5] [PATCH v2] [gdb/python] Replace gdbpy_should_stop with gdbpy_breakpoint_cond_says_stop
  2022-12-12 21:44           ` [PING^5] " Johnson Sun
@ 2022-12-13  2:34             ` Simon Marchi
  0 siblings, 0 replies; 24+ messages in thread
From: Simon Marchi @ 2022-12-13  2:34 UTC (permalink / raw)
  To: Johnson Sun, BrunoLarsen, gdb-patches



On 12/12/22 16:44, Johnson Sun via Gdb-patches wrote:
> Ping for: <https://sourceware.org/pipermail/gdb-patches/2022-October/192910.html>.
> 
> Johnson

Sorry for the delay.

I pushed the patch, after rewrapping the lines so they don't exceed the line length limit.

Simon

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

end of thread, other threads:[~2022-12-13  2:34 UTC | newest]

Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-09-20 17:24 [PATCH 0/3] [PR gdb/18655] Fix Python FinishBreakpoints not deleted bug Johnson Sun
2022-09-20 17:24 ` [PATCH 1/3] [gdb/testsuite] Add gdb.python/py-finish-breakpoint-deletion.exp for Bug 18655 Johnson Sun
2022-09-21 13:44   ` Bruno Larsen
2022-09-23  5:16     ` Johnson Sun
2022-09-20 17:24 ` [PATCH 2/3] [gdb/python] Fix " Johnson Sun
2022-09-21 14:02   ` Bruno Larsen
2022-09-20 17:24 ` [PATCH 3/3] [gdb/python] Replace gdbpy_should_stop with gdbpy_breakpoint_cond_says_stop Johnson Sun
2022-09-21 14:09   ` Bruno Larsen
2022-09-23  5:41 ` [PATCH v2] [PR python/18655] Fix deletion of FinishBreakpoints Johnson Sun
2022-09-23  8:32   ` Bruno Larsen
2022-09-23 20:16     ` Johnson Sun
2022-09-23 20:27   ` [PATCH v3] " Johnson Sun
2022-10-12  1:53     ` Simon Marchi
2022-10-20 17:34       ` Johnson Sun
2022-10-20 17:49         ` [PATCH v4] " Johnson Sun
2022-11-18 12:16           ` [PING] " Johnson Sun
2022-11-18 15:56           ` Simon Marchi
2022-09-23  6:00 ` [PATCH v2] [gdb/python] Replace gdbpy_should_stop with gdbpy_breakpoint_cond_says_stop Johnson Sun
2022-10-20 18:24   ` [PING] " Johnson Sun
2022-11-18 12:14     ` [PING^2] " Johnson Sun
2022-11-25 15:10       ` [PING^3] " Johnson Sun
2022-12-04 16:45         ` [PING^4] " Johnson Sun
2022-12-12 21:44           ` [PING^5] " Johnson Sun
2022-12-13  2:34             ` Simon Marchi

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