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

[Bruno Larsen <blarsen@redhat.com>]

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"