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

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

On 11/22/23 13:44, Tom de Vries wrote:
> On 11/21/23 17:52, Tom de Vries wrote:
>> On 11/21/23 17:11, Simon Marchi wrote:
>>> 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?
>>
>>
>> It's the QUIT in target_write_with_progress.
>>
>> Backtrace:
>> ...
>> (gdb) bt
>> #0  0xf75906d2 in __cxa_throw () from 
>> /usr/lib/arm-linux-gnueabihf/libstdc++.so.6
>> #1  0xab1ce512 in throw_it (reason=RETURN_FORCED_QUIT, 
>> error=GDB_NO_ERROR, fmt=0xab303268 "SIGTERM", ap=...)
>>      at /home/rock/gdb/src/gdbsupport/common-exceptions.cc:202
>> #2  0xab1ce622 in throw_forced_quit (fmt=0xab303268 "SIGTERM")
>>      at /home/rock/gdb/src/gdbsupport/common-exceptions.cc:247
>> #3  0xab022576 in quit () at /home/rock/gdb/src/gdb/utils.c:639
>> #4  0xab0225f0 in maybe_quit () at /home/rock/gdb/src/gdb/utils.c:666
>> #5  0xaaf867e4 in target_write_with_progress (ops=0xab43d7b0 
>> <the_arm_linux_nat_target>,
>>      object=TARGET_OBJECT_RAW_MEMORY, annex=0x0, buf=0xab51f4dc 
>> "\376\347", offset=2863310110, len=2, progress=0x0,
>>      baton=0x0) at /home/rock/gdb/src/gdb/target.c:2213
>> #6  0xaaf86828 in target_write (ops=0xab43d7b0 
>> <the_arm_linux_nat_target>, object=TARGET_OBJECT_RAW_MEMORY,
>>      annex=0x0, buf=0xab51f4dc "\376\347", offset=2863310110, len=2) 
>> at /home/rock/gdb/src/gdb/target.c:2226
>> #7  0xaaf85d30 in target_write_raw_memory (memaddr=2863310110, 
>> myaddr=0xab51f4dc "\376\347", len=2)
>>      at /home/rock/gdb/src/gdb/target.c:1849
>> #8  0xaae48d64 in default_memory_remove_breakpoint 
>> (gdbarch=0xab519290, bp_tgt=0xab51f4c0)
>>      at /home/rock/gdb/src/gdb/mem-break.c:83
>> #9  0xaab57bee in gdbarch_memory_remove_breakpoint 
>> (gdbarch=0xab519290, bp_tgt=0xab51f4c0)
>>      at /home/rock/gdb/src/gdb/gdbarch.c:2892
>> #10 0xaae48da2 in memory_remove_breakpoint (ops=0xab43d7b0 
>> <the_arm_linux_nat_target>, gdbarch=0xab519290,
>>      bp_tgt=0xab51f4c0, reason=REMOVE_BREAKPOINT) at 
>> /home/rock/gdb/src/gdb/mem-break.c:100
>> #11 0xaab6a9b8 in 
>> memory_breakpoint_target<process_stratum_target>::remove_breakpoint (
>>      this=0xab43d7b0 <the_arm_linux_nat_target>, gdbarch=0xab519290, 
>> bp_tgt=0xab51f4c0, reason=REMOVE_BREAKPOINT)
>>      at /home/rock/gdb/src/gdb/target.h:2440
>> #12 0xaaf86b70 in target_remove_breakpoint (gdbarch=0xab519290, 
>> bp_tgt=0xab51f4c0, reason=REMOVE_BREAKPOINT)
>>      at /home/rock/gdb/src/gdb/target.c:2381
>> #13 0xaabc6db8 in code_breakpoint::remove_location (this=0xab51f3a8, 
>> bl=0xab51f450, reason=REMOVE_BREAKPOINT)
>>      at /home/rock/gdb/src/gdb/breakpoint.c:12002
>> #14 0xaabb65f8 in remove_breakpoint_1 (bl=0xab51f450, 
>> reason=REMOVE_BREAKPOINT)
>>      at /home/rock/gdb/src/gdb/breakpoint.c:4109
>> #15 0xaabb688a in remove_breakpoint (bl=0xab51f450) at 
>> /home/rock/gdb/src/gdb/breakpoint.c:4214
>> #16 0xaabc6136 in update_global_location_list 
>> (insert_mode=UGLL_DONT_INSERT)
>>      at /home/rock/gdb/src/gdb/breakpoint.c:11554
>> #17 0xaabc7ff8 in delete_breakpoint (bpt=0xab51f3a8) at 
>> /home/rock/gdb/src/gdb/breakpoint.c:12657
>> #18 0xaaf9d556 in delete_thread_breakpoint (bp_p=0xab4d3650) at 
>> /home/rock/gdb/src/gdb/thread.c:98
>> #19 0xaaf9d5bc in delete_single_step_breakpoints (tp=0xab4d3618) at 
>> /home/rock/gdb/src/gdb/thread.c:123
>> #20 0xaadce6ee in for_each_just_stopped_thread (func=0xaaf9d5a5 
>> <delete_single_step_breakpoints(thread_info*)>)
>>      at /home/rock/gdb/src/gdb/infrun.c:3920
>> #21 0xaadce7a6 in delete_just_stopped_threads_single_step_breakpoints 
>> () at /home/rock/gdb/src/gdb/infrun.c:3945
>> #22 0xaadd4a52 in handle_signal_stop (ecs=0xfffeeda0) at 
>> /home/rock/gdb/src/gdb/infrun.c:6908
>> #23 0xaadd39b2 in handle_inferior_event (ecs=0xfffeeda0) at 
>> /home/rock/gdb/src/gdb/infrun.c:6494
>> #24 0xaadd008e in fetch_inferior_event () at 
>> /home/rock/gdb/src/gdb/infrun.c:4654
>> #25 0xaadb3554 in inferior_event_handler (event_type=INF_REG_EVENT) at 
>> /home/rock/gdb/src/gdb/inf-loop.c:42
>> #26 0xaae0c700 in handle_target_event (error=0, client_data=0x0) at 
>> /home/rock/gdb/src/gdb/linux-nat.c:4316
>> #27 0xab1d2b80 in handle_file_event (file_ptr=0xab5e3348, ready_mask=1)
>>      at /home/rock/gdb/src/gdbsupport/event-loop.cc:573
>> #28 0xab1d2e74 in gdb_wait_for_event (block=0) at 
>> /home/rock/gdb/src/gdbsupport/event-loop.cc:694
>> #29 0xab1d205e in gdb_do_one_event (mstimeout=-1) at 
>> /home/rock/gdb/src/gdbsupport/event-loop.cc:217
>> #30 0xaafa6f8e in wait_sync_command_done () at 
>> /home/rock/gdb/src/gdb/top.c:427
>> #31 0xaafa7012 in maybe_wait_sync_command_done (was_sync=0) at 
>> /home/rock/gdb/src/gdb/top.c:444
>> #32 0xaafa757a in execute_command (p=0xfffef148 "", from_tty=0) at 
>> /home/rock/gdb/src/gdb/top.c:577
>> #33 0xaad5cb96 in command_handler (command=0xfffef144 "step") at 
>> /home/rock/gdb/src/gdb/event-top.c:566
>> #34 0xaafa6da4 in read_command_file (stream=0xab4ba590) at 
>> /home/rock/gdb/src/gdb/top.c:342
>> #35 0xaac3657e in script_from_file (stream=0xab4ba590, file=0xfffef7e2 
>> "outputs/gdb.base/gdb-sigterm/gdb.in.1")
>>      at /home/rock/gdb/src/gdb/cli/cli-script.c:1642
>> #36 0xaac203ca in source_script_from_stream (stream=0xab4ba590,
>>      file=0xfffef7e2 "outputs/gdb.base/gdb-sigterm/gdb.in.1",
>>      file_to_open=0xab466da8 "outputs/gdb.base/gdb-sigterm/gdb.in.1") 
>> at /home/rock/gdb/src/gdb/cli/cli-cmds.c:730
>> #37 0xaac204d0 in source_script_with_search (file=0xfffef7e2 
>> "outputs/gdb.base/gdb-sigterm/gdb.in.1", from_tty=0,
>>      search_path=0) at /home/rock/gdb/src/gdb/cli/cli-cmds.c:775
>> #38 0xaac20524 in source_script (file=0xfffef7e2 
>> "outputs/gdb.base/gdb-sigterm/gdb.in.1", from_tty=0)
>>      at /home/rock/gdb/src/gdb/cli/cli-cmds.c:784
>> #39 0xaae33760 in catch_command_errors (command=0xaac20511 
>> <source_script(char const*, int)>,
>>      arg=0xfffef7e2 "outputs/gdb.base/gdb-sigterm/gdb.in.1", 
>> from_tty=0, do_bp_actions=false)
>>      at /home/rock/gdb/src/gdb/main.c:513
>> #40 0xaae338da in execute_cmdargs (cmdarg_vec=0xfffef3d4, 
>> file_type=CMDARG_FILE, cmd_type=CMDARG_COMMAND,
>>      ret=0xfffef3c4) at /home/rock/gdb/src/gdb/main.c:609
>> #41 0xaae34c9a in captured_main_1 (context=0xfffef514) at 
>> /home/rock/gdb/src/gdb/main.c:1293
>> #42 0xaae34e52 in captured_main (data=0xfffef514) at 
>> /home/rock/gdb/src/gdb/main.c:1314
>> #43 0xaae34ebc in gdb_main (args=0xfffef514) at 
>> /home/rock/gdb/src/gdb/main.c:1343
>> #44 0xaaaee67a in main (argc=5, argv=0xfffef684) at 
>> /home/rock/gdb/src/gdb/gdb.c:39
>> (gdb)
>> ...
> 
> Hmm, looking at this backtrace, it seems that the forced_quit is thrown 
> during update_global_location_list.
> 
> Removing the QUIT from target_write_with_progress makes the test-case 
> pass, but the impact of that is likely to broad.
> 
> This patch removes the QUIT from default_memory_remove_breakpoint only, 
> which also fixes the test-case.
> 

Update: I've tested it on x86_64-linux and pinebook, no issues found.

Thanks,
- Tom

> Is this an acceptible approach?
> 
> Thanks,
> - Tom