From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp-out2.suse.de (smtp-out2.suse.de [195.135.220.29]) by sourceware.org (Postfix) with ESMTPS id 134973858CDA for ; Tue, 24 Oct 2023 12:46:26 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 134973858CDA 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 134973858CDA Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=195.135.220.29 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1698151587; cv=none; b=DbRQuOapWgja4JhqeUxQ6/4/n9lKVLsBV83bLhR1MDdyeOSS56Up07ITZbFt7skQEqDm5aiQBcfWvnAy9hMLuAemqwOWKQAFv/d61Mbg8Iuyo+HvNjzSzJrjpzPMgIQvd47EHyjWc3YVcvk+VnYBOyWPt2BiuVgOScY4t1bG3NE= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1698151587; c=relaxed/simple; bh=znR1bIK1BgWJn3xHTLfPphk4IUqGMdVrLDL/cA/dxYI=; h=DKIM-Signature:DKIM-Signature:Message-ID:Date:MIME-Version: Subject:To:From; b=g4xQC8q+pFovPOpS3paOOdLZV1nA3leUP/+siPYDmB1+YCBIZav8W2fvwTq/GC6mIBoLO7/gaqnz5rZP1tl2aw+rh4NXbzJUbgNhQU4Hb3WX7i+EHeOSI1banWLV+Agy9v6p7I66jJN6ubFhabPaHOlB8Ru7B+od0ngl8J44lqw= ARC-Authentication-Results: i=1; server2.sourceware.org Received: from imap2.suse-dmz.suse.de (imap2.suse-dmz.suse.de [192.168.254.74]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-521) server-digest SHA512) (No client certificate requested) by smtp-out2.suse.de (Postfix) with ESMTPS id DEE491FE6C; Tue, 24 Oct 2023 12:46:24 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_rsa; t=1698151584; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc: mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=ryNtyR7f3VPI60wKpvUzV0LgvgmuZKb7OaWYrHiTgJs=; b=wUeoTq2NQu1laZyb15IlDGB0O1WYbrTIGq5j96qR12Q4cFVwBwyOCF2iHYiTt6+uCXb2AE SQtJ9rs/vd+ucWQhKIMum0pokWiy/NO5RMVfs6+bK53icachA+rsSVjwUY8DJs39XPkFXZ 3bY3ZcgYqyKSil30bePfRf0fjXf1iks= DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_ed25519; t=1698151584; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc: mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=ryNtyR7f3VPI60wKpvUzV0LgvgmuZKb7OaWYrHiTgJs=; b=nq905MupO4zLMnJSxaJFiWraVZYw5PCAJ2IgexCg8XbV4QtO3HYcRXD4EK8puipaJreXWC msG8Auz4LtDHjVAA== Received: from imap2.suse-dmz.suse.de (imap2.suse-dmz.suse.de [192.168.254.74]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-521) server-digest SHA512) (No client certificate requested) by imap2.suse-dmz.suse.de (Postfix) with ESMTPS id CCE01134F5; Tue, 24 Oct 2023 12:46:24 +0000 (UTC) Received: from dovecot-director2.suse.de ([192.168.254.65]) by imap2.suse-dmz.suse.de with ESMTPSA id VAoJMaC8N2VqbwAAMHmgww (envelope-from ); Tue, 24 Oct 2023 12:46:24 +0000 Message-ID: <59c9c732-1ef6-4c93-bfce-406f535f38e3@suse.de> Date: Tue, 24 Oct 2023 14:47:13 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v4 4/4] [gdb/cli] Ask if source highlighting should be interrupted Content-Language: en-US To: Pedro Alves , gdb-patches@sourceware.org References: <20231018171151.25427-1-tdevries@suse.de> <20231018171151.25427-5-tdevries@suse.de> <718b5c39-ced7-4560-a664-2eba4555063a@palves.net> From: Tom de Vries In-Reply-To: <718b5c39-ced7-4560-a664-2eba4555063a@palves.net> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Authentication-Results: smtp-out2.suse.de; none X-Spam-Level: X-Spam-Score: -7.09 X-Spamd-Result: default: False [-7.09 / 50.00]; ARC_NA(0.00)[]; RCVD_VIA_SMTP_AUTH(0.00)[]; XM_UA_NO_VERSION(0.01)[]; FROM_HAS_DN(0.00)[]; TO_DN_SOME(0.00)[]; TO_MATCH_ENVRCPT_ALL(0.00)[]; BAYES_HAM(-3.00)[100.00%]; MIME_GOOD(-0.10)[text/plain]; NEURAL_HAM_LONG(-3.00)[-1.000]; DKIM_SIGNED(0.00)[suse.de:s=susede2_rsa,suse.de:s=susede2_ed25519]; NEURAL_HAM_SHORT(-1.00)[-1.000]; RCPT_COUNT_TWO(0.00)[2]; FROM_EQ_ENVFROM(0.00)[]; MIME_TRACE(0.00)[0:+]; RCVD_COUNT_TWO(0.00)[2]; RCVD_TLS_ALL(0.00)[]; MID_RHS_MATCH_FROM(0.00)[] X-Spam-Status: No, score=-12.2 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,SPF_HELO_NONE,SPF_PASS,TXREP 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 10/20/23 21:17, Pedro Alves wrote: > Hi, > > So I've stared and played with this for a few hours today, and I'm convinced that avoiding > the QUIT macro/quit_handler mechanism here is best. It avoids the troubles I mentioned in the > previous patch. More below. > > On 2023-10-18 18:11, Tom de Vries wrote: > >> /* The number of source files we'll cache. */ >> @@ -199,10 +200,51 @@ get_language_name (enum language lang) >> class gdb_highlight_event_listener : public srchilite::HighlightEventListener >> { >> public: >> + explicit gdb_highlight_event_listener (const std::string &fullname) >> + : m_fullname (fullname) >> + { >> + } >> + >> void notify (const srchilite::HighlightEvent &event) override >> { >> + if (sync_quit_force_run) >> + { >> + /* Handle SIGTERM. */ >> + QUIT; >> + gdb_assert_not_reached (""); >> + } > > I missed this before, but it turns out that if we don't check for SIGTERM explicitly, > it'll still work out OK, because yquery internally starts a secondary prompt, and runs > a nested event loop to handle newline. That runs the pending async signal handlers. > I.e., if you get a SIGTERM before the query, the query call ends up quitting GDB. > > That doesn't happen during the unit test, because the unit test doesn't emulate > the SIGTERM handler completely. The SIGTERM handler calls: > > mark_async_signal_handler (async_sigterm_token); > > We can't do that in the unit tests, because that would cause the SIGTERM unit test > to exit GDB... Ask me how I know. :-) Heh :) > + int resp > + = yquery (_("Cancel source styling using GNU source highlight of %s?"), > + m_fullname.c_str ()); > > I noticed this printed: > > Cancel source styling using GNU source highlight of /tmp/test.cpp?([y] or n) > > Queries normally put that "(y or no)" in the same line _and_ add a space in between. > Like so: > > Cancel source styling using GNU source highlight of /tmp/test.cpp? ([y] or n) > ^ > > So... here's a patch on top of yours showing what I'm thinking we should do. > > WDYT? > > From 22bb476d59dac611f38ce57fcada552e9a1ed2ce Mon Sep 17 00:00:00 2001 > From: Pedro Alves > Date: Fri, 20 Oct 2023 16:56:49 +0100 > Subject: [PATCH] no QUIT > > Change-Id: I42e35273a66c2d6658be4a53e84e5c609bf60c1f > --- > gdb/infrun.c | 2 ++ > gdb/source-cache.c | 43 ++++++++++++++++++++++++------------------- > 2 files changed, 26 insertions(+), 19 deletions(-) > > diff --git a/gdb/infrun.c b/gdb/infrun.c > index 816a76a1b2c..537fccf5ffc 100644 > --- a/gdb/infrun.c > +++ b/gdb/infrun.c > @@ -9064,6 +9064,7 @@ normal_stop () > return true; > > { > +#if 0 > /* If the current quit_handler is infrun_quit_handler, and > target_terminal::is_ours, pressing ^C is ignored by QUIT. See > infrun_quit_handler for an explanation. At this point, there's > @@ -9071,6 +9072,7 @@ normal_stop () > notify_normal_stop, so install the default_quit_handler. */ > scoped_restore restore_quit_handler > = make_scoped_restore (&quit_handler, default_quit_handler); > +#endif > > /* Notify observers about the stop. This is where the interpreters > print the stop event. */ I've dropped the patch in v5 ( https://sourceware.org/pipermail/gdb-patches/2023-October/203505.html ) . > diff --git a/gdb/source-cache.c b/gdb/source-cache.c > index b742478c4c8..b51282431f3 100644 > --- a/gdb/source-cache.c > +++ b/gdb/source-cache.c > @@ -207,13 +207,6 @@ class gdb_highlight_event_listener : public srchilite::HighlightEventListener > > void notify (const srchilite::HighlightEvent &event) override > { > - if (sync_quit_force_run) > - { > - /* Handle SIGTERM. */ > - QUIT; > - gdb_assert_not_reached (""); > - } > - > /* If target_terminal::is_ours, we can ask the user a question and get an > answer. We're not sure if or how !target_terminal::is_ours can happen. > Assert to catch it happening. Note that we're asserting before > @@ -227,20 +220,32 @@ class gdb_highlight_event_listener : public srchilite::HighlightEventListener > return; > } > > - /* Ask the user what to do. */ > - int resp > - = yquery (_("Cancel source styling using GNU source highlight of %s?"), > - m_fullname.c_str ()); > - if (!resp) > + /* If we got a SIGTERM, skip querying. This isn't strictly > + necessary as yquery runs a nested event loop for the secondary > + prompt, which runs pending async signal handlers. However, > + this helps with the unit test, which makes sure that the quit > + call at the bottom throws a gdb_exception_forced_quit exception > + and that our caller doesn't swallow it. Note we may receive a > + SIGTERM in between the query and the quit call. */ > + if (!sync_quit_force_run) > { > - /* Continue highlighting. */ > - return; > + /* Ask the user what to do. */ > + int resp > + = (yquery > + (_("Cancel source styling using GNU source highlight of %s? "), > + m_fullname.c_str ())); > + if (!resp) > + { > + /* Continue highlighting. */ > + return; > + } > } > > - /* Interrupt highlighting. Note that check_quit_flag clears the > - quit_flag, so we have to set it again. */ > - set_quit_flag (); > - QUIT; > + /* Interrupt highlighting. Note we don't abort via the QUIT macro > + as that may do nothing. E.g. if the current quit_handler is > + infrun_quit_handler, and target_terminal::is_ours, pressing ^C > + is ignored by QUIT. */ > + quit (); > } > Merged into the patch in full. I've tried to fix the fact that we're working around a selftest issue by capturing the forced_quit call in the selftest. This is an RFC for now, but AFAIU it allows the simplification you're proposing in the comment "If we got a SIGTERM, skip querying. This isn't strictly necessary ...". > private: > @@ -369,7 +374,7 @@ static void gnu_source_highlight_test () > sync_quit_force_run = false; > check_quit_flag (); > > - /* Pretend user send SIGTERM. */ > + /* Pretend user sent SIGTERM. */ > set_force_quit_flag (); > > bool saw_forced_quit = false; > Merged into an earlier patch. Thanks, - Tom