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

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

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.

Thanks,
- Tom