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

[Tom de Vries <tdevries@suse.de>]

On 11/15/23 12:12, Tom de Vries wrote:
> On 11/14/23 16:09, Simon Marchi wrote:
>> On 11/13/23 10:26, Tom de Vries wrote:
>>> On a pinebook (aarch64 with 64-bit kernel and 32-bit userland) with 
>>> test-case
>>> gdb.base/gdb-sigterm.exp I run into:
>>> ...
>>> intrusive_list.h:458: internal-error: erase_element: \
>>>    Assertion `elem_node->prev != INTRUSIVE_LIST_UNLINKED_VALUE' 
>>> failed.^M
>>> ...
>>>
>>> Fix this similar to:
>>> - commit c0afd99439f ("[gdb/tui] Fix assert in 
>>> ~gdbpy_tui_window_maker"), and
>>> - commit 995a34b1772 ("Guard against frame.c destructors running before
>>>    frame-info.c's"
>>> by checking is_linked () before attempting to remove from 
>>> breakpoint_chain.
>>>
>>> Tested on the pinebook and x86_64-linux.
>>>
>>> PR gdb/31061
>>> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31061
>>
>> Hi Tom,
>>
>> Did you dig a little bit to understand why this happened specifically
>> with this setup, 
> 
> No, I just applied the usual recipe and observed that the problem was 
> fixed.
> 
> But now that you asked, I did (and should have done of course).
> 
>> and that it's indeed expected that the breakpoint is
>> not in the breakpoint chain?  The stack trace you showed contains (with
>> names demangled):
>>
>>      0xaae1d42b delete_breakpoint(breakpoint*)^M
>>              /home/rock/gdb/src/gdb/breakpoint.c:12636^M
>>      0xab1ed045 delete_thread_breakpoint^M
>>              /home/rock/gdb/src/gdb/thread.c:98^M
>>      0xab1ed0ab delete_single_step_breakpoints(thread_info*)^M
>>              /home/rock/gdb/src/gdb/thread.c:123^M
>>      0xab021e81 delete_thread_infrun_breakpoints^M
>>              /home/rock/gdb/src/gdb/infrun.c:3733^M
>>      0xab021edd for_each_just_stopped_thread^M
>>              /home/rock/gdb/src/gdb/infrun.c:3752^M
>>      0xab021f79 delete_just_stopped_threads_infrun_breakpoints^M
>>              /home/rock/gdb/src/gdb/infrun.c:3768^M
>>
>> It looks like this is specific to architectures that use software
>> single stepping?  Does "32-bit userland on 64-bit kernel on aarch64" use
>> software single stepping, like 32-bit ARM?
>>
> 
> System gdb is configured with --host=arm-linux-gnueabihf 
> --target=arm-linux-gnueabihf (and I've copied that for my gdb build) so 
> I suppose it's similar to 32-bit arm.
> 
>> Software single step breakpoints are inserted with:
>>
>>    if (tp->control.single_step_breakpoints == NULL)
>>      {
>>        std::unique_ptr<breakpoint> b
>>     (new momentary_breakpoint (gdbarch, bp_single_step,
>>                    current_program_space,
>>                    null_frame_id,
>>                    tp->global_num));
>>
>>        tp->control.single_step_breakpoints
>>     = add_to_breakpoint_chain (std::move (b));
>>      }
>>
>> They are added to the global breakpoint_chain by
>> add_to_breakpoint_chain.  So how come the breakpoint is not linked when
>> comes the time to delete it?
> 
> 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.

Hi,

did you have any further comments?

I'm inclined to apply the patch as is, but I'm not sure if I addressed 
your questions sufficiently.

Thanks,
- Tom