[2/3,gdb/python] Fix gdb.python/py-finish-breakpoint-deletion.exp for Bug 18655

Message ID 20220920172426.90659-3-j3.soon777@gmail.com
State Superseded
Headers
Series Fix Python FinishBreakpoints not deleted bug |

Commit Message

Johnson Sun Sept. 20, 2022, 5:24 p.m. UTC
  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(-)
  

Comments

Guinevere Larsen Sept. 21, 2022, 2:02 p.m. UTC | #1
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"
  

Patch

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"