From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp-out2.suse.de (smtp-out2.suse.de [195.135.223.131]) by sourceware.org (Postfix) with ESMTPS id 7F6FA3858D33 for ; Mon, 27 Nov 2023 10:19:08 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 7F6FA3858D33 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=suse.de Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=suse.de ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 7F6FA3858D33 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=195.135.223.131 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1701080350; cv=none; b=Rd9paeevXA0HNaBEivWI3SYfnnECRpmBku2nNDNRG2h2PBt9onXn7fWpKKBl9P1P6nyfn6ls7tP4YQuy/Bp6POJ+3GToIC0Y63qz75ckHq6KysZVeaJSO/qSoohpfn92JElWWhGg8Fw97rDPL8c6WS/ZUfgwk1/MB8LSiDGHQ00= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1701080350; c=relaxed/simple; bh=O0R1n7tiM5SltuCFYkHoJCYq2uMxEYvN/sblvH7c94g=; h=Message-ID:Date:MIME-Version:Subject:From:To; b=dcNWVA9/CDLEibaeNyodmX7LN/WMVwafTO4CyhVxspN2Hion+TewC2GLUPdMZzR1t+cZaDnH3TBmiW23PvAS28Wqs6uT3kpvxFC5Tnm1JxsRt9BiJWqszspvHKHM+DMxZ3UrHNYf7//hrRdVz7ZQ0GI1DFeCqEim0QVHnyxNgHU= ARC-Authentication-Results: i=1; server2.sourceware.org Received: from imap1.dmz-prg2.suse.org (imap1.dmz-prg2.suse.org [IPv6:2a07:de40:b281:104:10:150:64:97]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) by smtp-out2.suse.de (Postfix) with ESMTPS id 70968202CB; Mon, 27 Nov 2023 10:19:06 +0000 (UTC) Received: from imap1.dmz-prg2.suse.org (localhost [127.0.0.1]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) by imap1.dmz-prg2.suse.org (Postfix) with ESMTPS id 3DA2B1379A; Mon, 27 Nov 2023 10:19:06 +0000 (UTC) Received: from dovecot-director2.suse.de ([2a07:de40:b281:106:10:150:64:167]) by imap1.dmz-prg2.suse.org with ESMTPSA id FdVtDBptZGX7UQAAD6G6ig (envelope-from ); Mon, 27 Nov 2023 10:19:06 +0000 Message-ID: Date: Mon, 27 Nov 2023 11:19:15 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH] [gdb] Fix assert in delete_breakpoint Content-Language: en-US From: Tom de Vries To: Simon Marchi , gdb-patches@sourceware.org References: <20231113152609.32726-1-tdevries@suse.de> <601592c1-4e19-42c8-bfd7-5671ef951ab3@simark.ca> <114c9994-ed81-4a14-bcc3-bd8086e46340@simark.ca> <7794354b-0b08-48f2-8cde-499787b5850c@suse.de> <4f0b3108-0601-421a-9c6b-9c1a79ae1c17@suse.de> In-Reply-To: <4f0b3108-0601-421a-9c6b-9c1a79ae1c17@suse.de> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Spamd-Bar: +++++++++++ X-Spam-Score: 11.14 X-Rspamd-Server: rspamd1 X-Rspamd-Queue-Id: 70968202CB Authentication-Results: smtp-out2.suse.de; dkim=none; dmarc=fail reason="No valid SPF, No valid DKIM" header.from=suse.de (policy=none); spf=softfail (smtp-out2.suse.de: 2a07:de40:b281:104:10:150:64:97 is neither permitted nor denied by domain of tdevries@suse.de) smtp.mailfrom=tdevries@suse.de X-Spamd-Result: default: False [11.14 / 50.00]; RCVD_VIA_SMTP_AUTH(0.00)[]; BAYES_SPAM(5.10)[100.00%]; XM_UA_NO_VERSION(0.01)[]; SPAMHAUS_XBL(0.00)[2a07:de40:b281:104:10:150:64:97:from]; TO_DN_SOME(0.00)[]; R_SPF_SOFTFAIL(4.60)[~all:c]; RCVD_COUNT_THREE(0.00)[3]; MX_GOOD(-0.01)[]; RCPT_COUNT_TWO(0.00)[2]; NEURAL_HAM_SHORT(-0.20)[-1.000]; FROM_EQ_ENVFROM(0.00)[]; R_DKIM_NA(2.20)[]; MIME_TRACE(0.00)[0:+]; MID_RHS_MATCH_FROM(0.00)[]; ARC_NA(0.00)[]; FROM_HAS_DN(0.00)[]; TO_MATCH_ENVRCPT_ALL(0.00)[]; NEURAL_HAM_LONG(-0.56)[-0.558]; MIME_GOOD(-0.10)[text/plain]; FUZZY_BLOCKED(0.00)[rspamd.com]; RCVD_TLS_ALL(0.00)[]; DMARC_POLICY_SOFTFAIL(0.10)[suse.de : No valid SPF, No valid DKIM,none] X-Spam-Status: No, score=-11.1 required=5.0 tests=BAYES_00,BODY_8BITS,GIT_PATCH_0,KAM_DMARC_STATUS,SPF_HELO_NONE,SPF_PASS,TXREP,T_SCC_BODY_TEXT_LINE,WEIRD_PORT autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: 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 >> , >>      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 >> , 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 >> , gdbarch=0xab519290, >>      bp_tgt=0xab51f4c0, reason=REMOVE_BREAKPOINT) at >> /home/rock/gdb/src/gdb/mem-break.c:100 >> #11 0xaab6a9b8 in >> memory_breakpoint_target::remove_breakpoint ( >>      this=0xab43d7b0 , 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 >> ) >>      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 >> , >>      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? This uses the same approach, but implements it in a less intrusive way, by adding a suppress_quit variable. Any comments about this approach? Thanks, - Tom diff --git a/gdb/mem-break.c b/gdb/mem-break.c index abff4b47081..ce61e055945 100644 --- a/gdb/mem-break.c +++ b/gdb/mem-break.c @@ -71,6 +71,9 @@ default_memory_insert_breakpoint (struct gdbarch *gdbarch, return val; } +extern bool suppress_quit; +#define SCOPED_SUPPRESS_QUIT \ + scoped_restore save_suppress_quit = make_scoped_restore (&suppress_quit, true) int default_memory_remove_breakpoint (struct gdbarch *gdbarch, @@ -80,6 +83,7 @@ default_memory_remove_breakpoint (struct gdbarch *gdbarch, gdbarch_sw_breakpoint_from_kind (gdbarch, bp_tgt->kind, &bplen); + SCOPED_SUPPRESS_QUIT; return target_write_raw_memory (bp_tgt->placed_address, bp_tgt->shadow_contents, bplen); } diff --git a/gdb/utils.c b/gdb/utils.c index 7a1841ba21e..b5e6d792383 100644 --- a/gdb/utils.c +++ b/gdb/utils.c @@ -654,11 +654,17 @@ quit (void) #endif } +extern bool suppress_quit; +bool suppress_quit; + /* See defs.h. */ void maybe_quit (void) { + if (suppress_quit) + return; + if (!is_main_thread ()) return;