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

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

[-- Attachment #1: Type: text/plain, Size: 9210 bytes --]

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.

Is this an acceptible approach?

Thanks,
- Tom

[-- Attachment #2: 0001-gdb-Don-t-allow-quit-during-default_memory_remove_br.patch --]
[-- Type: text/x-patch, Size: 4034 bytes --]

From 87dc179a11ea3ffd97f6d993c42eb06ff6c4408b Mon Sep 17 00:00:00 2001
From: Tom de Vries <tdevries@suse.de>
Date: Wed, 22 Nov 2023 13:29:08 +0100
Subject: [PATCH] [gdb] Don't allow quit during
 default_memory_remove_breakpoint

---
 gdb/mem-break.c |  2 +-
 gdb/target.c    | 14 ++++++++------
 gdb/target.h    |  6 +++---
 3 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/gdb/mem-break.c b/gdb/mem-break.c
index abff4b47081..b505979a6b9 100644
--- a/gdb/mem-break.c
+++ b/gdb/mem-break.c
@@ -81,7 +81,7 @@ default_memory_remove_breakpoint (struct gdbarch *gdbarch,
   gdbarch_sw_breakpoint_from_kind (gdbarch, bp_tgt->kind, &bplen);
 
   return target_write_raw_memory (bp_tgt->placed_address, bp_tgt->shadow_contents,
-				  bplen);
+				  bplen, false);
 }
 
 
diff --git a/gdb/target.c b/gdb/target.c
index c3dad38f317..ebcbbec48ae 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -1844,11 +1844,11 @@ target_write_memory (CORE_ADDR memaddr, const gdb_byte *myaddr, ssize_t len)
    target_write.  */
 
 int
-target_write_raw_memory (CORE_ADDR memaddr, const gdb_byte *myaddr, ssize_t len)
+target_write_raw_memory (CORE_ADDR memaddr, const gdb_byte *myaddr, ssize_t len, bool with_quit)
 {
   if (target_write (current_inferior ()->top_target (),
 		    TARGET_OBJECT_RAW_MEMORY, NULL,
-		    myaddr, memaddr, len) == len)
+		    myaddr, memaddr, len, with_quit) == len)
     return 0;
   else
     return -1;
@@ -2175,7 +2175,8 @@ target_write_with_progress (struct target_ops *ops,
 			    enum target_object object,
 			    const char *annex, const gdb_byte *buf,
 			    ULONGEST offset, LONGEST len,
-			    void (*progress) (ULONGEST, void *), void *baton)
+			    void (*progress) (ULONGEST, void *), void *baton,
+			    bool with_quit)
 {
   LONGEST xfered_total = 0;
   int unit_size = 1;
@@ -2210,7 +2211,8 @@ target_write_with_progress (struct target_ops *ops,
 	(*progress) (xfered_partial, baton);
 
       xfered_total += xfered_partial;
-      QUIT;
+      if (with_quit)
+	QUIT;
     }
   return len;
 }
@@ -2221,10 +2223,10 @@ LONGEST
 target_write (struct target_ops *ops,
 	      enum target_object object,
 	      const char *annex, const gdb_byte *buf,
-	      ULONGEST offset, LONGEST len)
+	      ULONGEST offset, LONGEST len, bool with_quit)
 {
   return target_write_with_progress (ops, object, annex, buf, offset, len,
-				     NULL, NULL);
+				     NULL, NULL, with_quit);
 }
 
 /* Help for target_read_alloc and target_read_stralloc.  See their comments
diff --git a/gdb/target.h b/gdb/target.h
index c54bd28c88c..b28b6a351bb 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -320,7 +320,7 @@ extern std::vector<memory_read_result> read_memory_robust
 extern LONGEST target_write (struct target_ops *ops,
 			     enum target_object object,
 			     const char *annex, const gdb_byte *buf,
-			     ULONGEST offset, LONGEST len);
+			     ULONGEST offset, LONGEST len, bool with_quit = true);
 
 /* Similar to target_write, except that it also calls PROGRESS with
    the number of bytes written and the opaque BATON after every
@@ -334,7 +334,7 @@ LONGEST target_write_with_progress (struct target_ops *ops,
 				    const char *annex, const gdb_byte *buf,
 				    ULONGEST offset, LONGEST len,
 				    void (*progress) (ULONGEST, void *),
-				    void *baton);
+				    void *baton, bool with_quit = true);
 
 /* Wrapper to perform a full read of unknown size.  OBJECT/ANNEX will be read
    using OPS.  The return value will be uninstantiated if the transfer fails or
@@ -1588,7 +1588,7 @@ extern int target_read_code (CORE_ADDR memaddr, gdb_byte *myaddr, ssize_t len);
 /* For target_write_memory see target/target.h.  */
 
 extern int target_write_raw_memory (CORE_ADDR memaddr, const gdb_byte *myaddr,
-				    ssize_t len);
+				    ssize_t len, bool with_quit = true);
 
 /* Fetches the target's memory map.  If one is found it is sorted
    and returned, after some consistency checking.  Otherwise, NULL

base-commit: df4ffdd8c87b32357f929fb4a861760038f3bbb8
-- 
2.35.3