Re: [PATCH] [gdb] Fix assert in delete_breakpoint

[Simon Marchi <simark@simark.ca>]

On 11/15/23 06:12, Tom de Vries wrote:
> Because it's deleted twice.
> 
> The last thing we see in gdb.log before the internal error is:
> ...
>    [infrun] handle_signal_stop: [13633.13633.0] hit its single-step breakpoint^M
> ...
> 
> And the statement following the infrun_debug_printf printing that message is:
> ...
>   delete_just_stopped_threads_single_step_breakpoints ();
> ...
> 
> So, the following events happen:
> - the single-step breakpoint is hit
> - delete_just_stopped_threads_single_step_breakpoints is called
> - during delete_just_stopped_threads_single_step_breakpoints,
>   delete_breakpoint is called which removes the breakpoint from the
>   breakpoint chain
> - gdb is interrupted by SIGTERM before finish delete_breakpoint
> - the SIGTERM triggers a SCOPE_EXIT cleanup, calling
>   delete_just_stopped_threads_infrun_breakpoints which ends up
>   calling delete_breakpoint again for the same breakpoint.
> 
> The proposed fix tries to handle delete_breakpoint being interrupted, and called again.
> 
> This is an alternative fix:
> ...
> diff --git a/gdb/thread.c b/gdb/thread.c
> index 47cc5c9cd14..7f029f2414c 100644
> --- a/gdb/thread.c
> +++ b/gdb/thread.c
> @@ -93,11 +93,12 @@ inferior_thread (void)
>  static void
>  delete_thread_breakpoint (struct breakpoint **bp_p)
>  {
> -  if (*bp_p != NULL)
> -    {
> -      delete_breakpoint (*bp_p);
> -      *bp_p = NULL;
> -    }
> +  struct breakpoint *bp = *bp_p;
> +
> +  *bp_p = nullptr;
> +
> +  if (bp != nullptr)
> +    delete_breakpoint (bp);
>  }
> 
>  void
> ...
> 
> It makes sure delete_breakpoint is called only once, but I don't think this is the way to go, since it prevents the cleanup.

Err, my intuition is that it doesn't make sense for operations like
that, that update GDB's state (especially the tricky infrun / breakpoint
state) in reaction to target events, to be interruptible by SIGTERM.
It's just a recipe for ending up with half-baked state that is not
up-to-date with the reality.

I suppose that after receiving the SIGTERM, execution is interrupted by
a QUIT macro and a gdb_exception_quit is thrown?  Are you able to tell
which QUIT causes this to happen, and show what the stack looks like at
this point?

Simon