[v2,PR,python/18655] Fix deletion of FinishBreakpoints
Commit Message
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
Comments
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
Hi Bruno,
Thanks for reviewing this! I totally agree with you.
I revised the patch and will send version 3 shortly.
Cheers,
Johnson
@@ -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)
{
new file mode 100644
@@ -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;
+}
new file mode 100644
@@ -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"
new file mode 100644
@@ -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")