From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca (simark.ca [158.69.221.121]) by sourceware.org (Postfix) with ESMTPS id DBC493858C35 for ; Mon, 27 Nov 2023 16:29:05 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org DBC493858C35 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=simark.ca Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=simark.ca ARC-Filter: OpenARC Filter v1.0.0 sourceware.org DBC493858C35 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=158.69.221.121 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1701102547; cv=none; b=ugtOO1dnUabxXBk2NRUe5L6iuRRNNVLkjidUTsemyN8OAAFbYXf3f6LlXIiHj+dYuSq5MBDWPz+vuSCZ/Mhe7NADv4T+/ufj+tbHvY2gAfP1UobZ8MOX+jGEVS+sXL0lhYKEkGR4vpjsfi6OxSrEg7/7I7EIzMhxsjFMZixGeXs= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1701102547; c=relaxed/simple; bh=QasZItppvzAHlhItuEbsXAUvDRh7ROPBDpMXIm2/yS0=; h=DKIM-Signature:Message-ID:Date:MIME-Version:Subject:To:From; b=Zt30LHOWI9ya6WBYGmgtnyJ9/AJU+5g+EIi3+f+Liz1ForAfbM0LqZrTeeY65YxaDuawPp11sdG2Tgd0GZfgPzkDZSHO4U0yM3Hh5FLGwhgGvjQyT0pFxYDXkTvZTxbllU8yr94Hlw8bH4vSngq0bdcuj782pbB6tTTkBBc4yLE= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=simark.ca; s=mail; t=1701102545; bh=QasZItppvzAHlhItuEbsXAUvDRh7ROPBDpMXIm2/yS0=; h=Date:Subject:To:References:From:In-Reply-To:From; b=C/TgEX4NYGTQ2/q8WAG778YnMCYVvxPXWH6pbpe4CAgZYlyK6xavTNrSrTQb3MGVk L7yl5nLmg4if5KJiUOsv0b3sFlIyXQzPOq7jBo0k4X0gj167BUiNFd2URxgOvu7DBS lgG6ZGfjhXYdcpP/kTT+Urt6nvAWwNHJ1vvJ7TOI= Received: from [172.16.0.146] (192-222-143-198.qc.cable.ebox.net [192.222.143.198]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature ECDSA (prime256v1) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id 24A601E091; Mon, 27 Nov 2023 11:29:05 -0500 (EST) Message-ID: <8653ed15-aaf7-4734-b673-5ec6f15cd4c1@simark.ca> Date: Mon, 27 Nov 2023 11:29:04 -0500 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH] [gdb] Fix assert in delete_breakpoint Content-Language: fr To: Tom de Vries , 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> From: Simon Marchi In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-10.8 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,SPF_HELO_PASS,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/27/23 05:19, Tom de Vries wrote: > On 11/22/23 13:44, Tom de Vries wrote: >> 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; > > I believe that something like this (suppress_quit) would be good to have. As you said, removing some QUITs from functions that do memory reads and writes would be too broad. In some cases, you want those to be interruptible, and in other cases you don't. For instance, you ask GDB to print a lot of memory, it's taking too long, you want to stop. There's not risk in stopping that operation in the middle. Then let's say you ask GDB to write a big object in memory, it's taking too long, you want to interrupt it. It might leave you with a half-written object, but you know the risks of interrupting your write operation, that's fine. On the other hand, I suspect that any memory read / write used to implement GDB's internal logic should be considered by default unsafe to interrupt. Otherwise, that code should be written in a way that ensures that should exceptions get thrown, the internal state is left in a coherent... state, either by rolling back changes or otherwise. That sounds difficult, if not impossible. So in those cases, I think the only safe thing to do is suppress QUITs. The question is, how to decide where QUITs should be suppressed. Here's a random backtrace I just grabbed from a maybe_quit call under an update_global_location_list call: #0 maybe_quit () at /home/smarchi/src/binutils-gdb/gdb/utils.c:662 #1 0x000056366ecaa8ee in target_read (ops=0x56367c2f4240 , object=TARGET_OBJECT_MEMORY, annex=0x0, buf=0x7ffe481b40e0 "HA\033H\376\177", offset=93824992235889, len=1) at /home/smarchi/src/binutils-gdb/gdb/target.c:1972 #2 0x000056366eca95a0 in target_read_memory (memaddr=0x555555555171, myaddr=0x7ffe481b40e0 "HA\033H\376\177", len=1) at /home/smarchi/src/binutils-gdb/gdb/target.c:1755 #3 0x000056366dc0f6be in default_memory_insert_breakpoint (gdbarch=0x61c00001d080, bp_tgt=0x616000073f28) at /home/smarchi/src/binutils-gdb/gdb/mem-break.c:54 #4 0x000056366c17574a in gdbarch_memory_insert_breakpoint (gdbarch=0x61c00001d080, bp_tgt=0x616000073f28) at /home/smarchi/src/binutils-gdb/gdb/gdbarch.c:2875 #5 0x000056366dc0fb94 in memory_insert_breakpoint (ops=0x56367c2f4240 , gdbarch=0x61c00001d080, bp_tgt=0x616000073f28) at /home/smarchi/src/binutils-gdb/gdb/mem-break.c:92 #6 0x000056366c0067d5 in memory_breakpoint_target::insert_breakpoint (this=0x56367c2f4240 , gdbarch=0x61c00001d080, bp_tgt=0x616000073f28) at /home/smarchi/src/binutils-gdb/gdb/target.h:2435 #7 0x000056366ecacf34 in target_insert_breakpoint (gdbarch=0x61c00001d080, bp_tgt=0x616000073f28) at /home/smarchi/src/binutils-gdb/gdb/target.c:2359 #8 0x000056366c52ebd5 in code_breakpoint::insert_location (this=0x611000045dc0, bl=0x616000073e80) at /home/smarchi/src/binutils-gdb/gdb/breakpoint.c:11977 #9 0x000056366c477a64 in insert_bp_location (bl=0x616000073e80, tmp_error_stream=0x7ffe481b48f0, disabled_breaks=0x7ffe481b4700, hw_breakpoint_error=0x7ffe481b4710, hw_bp_error_explained_already=0x7ffe481b4720) at /home/smarchi/src/binutils-gdb/gdb/breakpoint.c:2901 #10 0x000056366c481f9c in insert_breakpoint_locations () at /home/smarchi/src/binutils-gdb/gdb/breakpoint.c:3291 #11 0x000056366c529ae4 in update_global_location_list (insert_mode=UGLL_MAY_INSERT) at /home/smarchi/src/binutils-gdb/gdb/breakpoint.c:11720 #12 0x000056366c54998d in breakpoint_re_set () at /home/smarchi/src/binutils-gdb/gdb/breakpoint.c:13271 #13 0x000056366deccb27 in objfile_relocate (objfile=0x614000005440, new_offsets=std::__debug::vector of length 38, capacity 38 = {...}) at /home/smarchi/src/binutils-gdb/gdb/objfiles.c:707 #14 0x000056366e8aaa6b in svr4_relocate_main_executable () at /home/smarchi/src/binutils-gdb/gdb/solib-svr4.c:3004 #15 0x000056366e8aaefe in svr4_solib_create_inferior_hook (from_tty=0) at /home/smarchi/src/binutils-gdb/gdb/solib-svr4.c:3048 #16 0x000056366e904f3c in solib_create_inferior_hook (from_tty=0) at /home/smarchi/src/binutils-gdb/gdb/solib.c:1217 #17 0x000056366d717f57 in post_create_inferior (from_tty=0) at /home/smarchi/src/binutils-gdb/gdb/infcmd.c:275 #18 0x000056366d71a14e in run_command_1 (args=0x0, from_tty=1, run_how=RUN_NORMAL) at /home/smarchi/src/binutils-gdb/gdb/infcmd.c:486 #19 0x000056366d71a5b2 in run_command (args=0x0, from_tty=1) at /home/smarchi/src/binutils-gdb/gdb/infcmd.c:512 #20 0x000056366c8d66d5 in do_simple_func (During symbol reading: macro `CTRL' redefined at /usr/include/x86_64-linux-gnu/sys/ttydefaults.h:55; original definition at /usr/include/readline/chardefs.h:51 args=0x0, from_tty=1, c=0x61200005bcc0) at /home/smarchi/src/binutils-gdb/gdb/cli/cli-decode.c:95 #21 0x000056366c8ebe3b in cmd_func (cmd=0x61200005bcc0, args=0x0, from_tty=1) at /home/smarchi/src/binutils-gdb/gdb/cli/cli-decode.c:2735 #22 0x000056366ee07837 in execute_command (p=0x6020000393d1 "", from_tty=1) at /home/smarchi/src/binutils-gdb/gdb/top.c:575 #23 0x000056366d2e9cdb in command_handler (command=0x6020000393d0 "") at /home/smarchi/src/binutils-gdb/gdb/event-top.c:566 #24 0x000056366d2eafd1 in command_line_handler (rl=...) at /home/smarchi/src/binutils-gdb/gdb/event-top.c:802 #25 0x000056366ef64bdb in tui_command_line_handler (rl=...) at /home/smarchi/src/binutils-gdb/gdb/tui/tui-interp.c:104 #26 0x000056366d2e7d40 in gdb_rl_callback_handler (rl=0x602000039370 "r") at /home/smarchi/src/binutils-gdb/gdb/event-top.c:259 #27 0x00007f3ad551246d in rl_callback_read_char () from /lib/x86_64-linux-gnu/libreadline.so.8 #28 0x000056366d2e7764 in gdb_rl_callback_read_char_wrapper_noexcept () at /home/smarchi/src/binutils-gdb/gdb/event-top.c:195 #29 0x000056366d2e7990 in gdb_rl_callback_read_char_wrapper (client_data=0x611000001440) at /home/smarchi/src/binutils-gdb/gdb/event-top.c:234 #30 0x000056366f0b730c in stdin_event_handler (error=0, client_data=0x611000001440) at /home/smarchi/src/binutils-gdb/gdb/ui.c:155 #31 0x0000563670708345 in handle_file_event (file_ptr=0x60700000dd10, ready_mask=1) at /home/smarchi/src/binutils-gdb/gdbsupport/event-loop.cc:573 #32 0x0000563670708c58 in gdb_wait_for_event (block=1) at /home/smarchi/src/binutils-gdb/gdbsupport/event-loop.cc:694 #33 0x00005636707069ce in gdb_do_one_event (mstimeout=-1) at /home/smarchi/src/binutils-gdb/gdbsupport/event-loop.cc:264 #34 0x000056366db776a8 in start_event_loop () at /home/smarchi/src/binutils-gdb/gdb/main.c:407 #35 0x000056366db77e4b in captured_command_loop () at /home/smarchi/src/binutils-gdb/gdb/main.c:471 #36 0x000056366db7d3d4 in captured_main (data=0x7ffe481b6780) at /home/smarchi/src/binutils-gdb/gdb/main.c:1324 #37 0x000056366db7d511 in gdb_main (During symbol reading: const value length mismatch for 'sigall_set', got 8, expected 0 args=0x7ffe481b6780) at /home/smarchi/src/binutils-gdb/gdb/main.c:1343 #38 0x000056366bbf0703 in main (argc=5, argv=0x7ffe481b68f8) at /home/smarchi/src/binutils-gdb/gdb/gdb.c:39 If an exception gets thrown at that maybe_quit, I guess it goes directly through that stack to start_event_loop (ignoring the special readline handling)? So, we end up with an half-started inferior? So, should run_command_1 suppress QUITs during all its execution? Simon