Re: [PATCH v2] [PR python/18655] Fix deletion of FinishBreakpoints

[Bruno Larsen <blarsen@redhat.com>]

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