From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id a1hXDoDTLWV6xi8AWB0awg (envelope-from ) for ; Mon, 16 Oct 2023 20:21:20 -0400 Received: by simark.ca (Postfix, from userid 112) id 31D9A1E0C1; Mon, 16 Oct 2023 20:21:20 -0400 (EDT) Received: from server2.sourceware.org (server2.sourceware.org [8.43.85.97]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (prime256v1) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPS id 252C91E091 for ; Mon, 16 Oct 2023 20:21:18 -0400 (EDT) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 986583858002 for ; Tue, 17 Oct 2023 00:21:17 +0000 (GMT) Received: from mail-wr1-f42.google.com (mail-wr1-f42.google.com [209.85.221.42]) by sourceware.org (Postfix) with ESMTPS id 0B4AE3858D33 for ; Tue, 17 Oct 2023 00:20:58 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 0B4AE3858D33 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=palves.net Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gmail.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 0B4AE3858D33 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=209.85.221.42 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1697502064; cv=none; b=az8Yck0ykhO+Mv0Z9JhhtA9VkfBofHVZr5HpaBva+WHK5ZzVp7Co1nymjN9VjMQM9ektc1pp7r+ekqRuFzE1v6/cL7uRfphzMxVICPk/L3MxPRfvCxRD43/hG9fqeSSc/4Jh1EdV9FM6l545buYgO01/xDvkojdZOe3VN5Enx3s= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1697502064; c=relaxed/simple; bh=Xfeco1F+u3qbkReRdiOf65oQ7jGAm3V7gwPn1kZHH7w=; h=Message-ID:Date:MIME-Version:Subject:To:From; b=J00TfR3UMnvYeOm131/WrDrKB/T6ARFU0zsWhUgTENvNWCBgpAYBupo+E0rPj0RiF29VJREcytnSl5Gof4471OnnxJpb6ozyUH9NWHc2FqjJeW+41m0z46wC4RF/84pYLClCfUTTtu7YMgowGwYKHRsXhYIXRdm4smP/Mcq++P8= ARC-Authentication-Results: i=1; server2.sourceware.org Received: by mail-wr1-f42.google.com with SMTP id ffacd0b85a97d-32d9effe314so2932847f8f.3 for ; Mon, 16 Oct 2023 17:20:57 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1697502056; x=1698106856; h=content-transfer-encoding:in-reply-to:from:references:to :content-language:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=vj81RMa412MohZtLlf86yRN6n4PRzKr3wuh+CpDQ3BM=; b=dwT5gx1TcZrR/a7rNK//TGhFpfNCMoXYzCL5N4nygFunOBaRXD/e8my3iVMYNPryNo /RBSCet4Yd9GngSshOYYldZKVpyNI64p9+POVSto12c79iEg7fSIIJQR7vgl/9vINnid MSVILgQ4SxR/SIU+NMg4mK0/inlJRka0w1JOeHojJBXHyk7wvXU9EtqChb5mMFpweNzs tpBvD8rVnry+8lRD13+8FZl5RySbwkRvfb5HagVn25bMaSehDwfRzSmLi0qUuZ03vfAE rBCC2n9bGlitMhly+9PMrxOyweriAqrjXxYPXmLDbX8XOnAXuMQCfjq7Y247r6nRx+ku SAdg== X-Gm-Message-State: AOJu0YyIMmr/XClJOYPdefog0Pm0oLqrA7X5au6MyJ06OsXof/i+0IpM Mn7NoY1b6w//92CpUIaRl0kBD5kifTmXjg== X-Google-Smtp-Source: AGHT+IFLjBrJ7BXzs0sZHuSEFjr1d7zh0Us1DbargH9/x4GoF4d0hs+QmSANC5GpmOIu2poOnIROiQ== X-Received: by 2002:a5d:630c:0:b0:32d:b759:a329 with SMTP id i12-20020a5d630c000000b0032db759a329mr715534wru.56.1697502056159; Mon, 16 Oct 2023 17:20:56 -0700 (PDT) Received: from ?IPV6:2001:8a0:f939:d200:120b:b0fa:905:a893? ([2001:8a0:f939:d200:120b:b0fa:905:a893]) by smtp.gmail.com with ESMTPSA id m16-20020a056000009000b0032d8034724esm387822wrx.94.2023.10.16.17.20.55 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 16 Oct 2023 17:20:55 -0700 (PDT) Message-ID: <965cd924-0980-498e-89f4-d952c812dfc0@palves.net> Date: Tue, 17 Oct 2023 01:20:54 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v3 4/4] [gdb/cli] Allow source highlighting to be interrupted Content-Language: en-US To: Tom de Vries , gdb-patches@sourceware.org References: <20231016091748.26247-1-tdevries@suse.de> <20231016091748.26247-5-tdevries@suse.de> From: Pedro Alves In-Reply-To: <20231016091748.26247-5-tdevries@suse.de> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-9.6 required=5.0 tests=BAYES_00, FREEMAIL_FORGED_FROMDOMAIN, FREEMAIL_FROM, GIT_PATCH_0, HEADER_FROM_DIFFERENT_DOMAINS, KAM_DMARC_STATUS, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H2, 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 X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.30 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: gdb-patches-bounces+public-inbox=simark.ca@sourceware.org Hi, On 2023-10-16 10:17, Tom de Vries wrote: > 1 file changed, 66 insertions(+), 2 deletions(-) > > diff --git a/gdb/source-cache.c b/gdb/source-cache.c > index aa8e40425c2..39d8bcf7aec 100644 > --- a/gdb/source-cache.c > +++ b/gdb/source-cache.c > @@ -37,6 +37,7 @@ > #include > #include > #include > +#include > #endif > > /* The number of source files we'll cache. */ > @@ -189,6 +190,48 @@ get_language_name (enum language lang) > return nullptr; > } > > +/* Class with notify function called during highlighting. */ > + > +class gdb_highlight_event_listener : public srchilite::HighlightEventListener > +{ > +public: > + gdb_highlight_event_listener (const std::string &fullname) A ctor with one parameter should usually be "explicit". > + : m_fullname (fullname) > + { > + } > + > + void notify(const srchilite::HighlightEvent &event) override > + { > + /* If the terminal is ours, we can ask the user a question and get an > + answer. */ > + gdb_assert (target_terminal::is_ours ()); How sure are we that we won't ever print source listings while the inferior has the terminal (either both for I/O, or just for input with is_ours_for_output)? I was trying to think of scenarios, but not much came out. I'm imagining maybe a Python script hooking into events, internal breakpoints, or timers, or some such and printing sources while the inferior is running? OTOH, that printing might well come out badly formatted as the Python layer doesn't expose target_terminal::{ours,ours_for_output}() to Python. Maybe we don't need to worry about it. At least we have the assertion. If we ever do manage to get here with "!target_terminal::ours ()" , then we'd want to be sure that a Ctrl-C that happens to be pressed exactly while GDB is printing the sources isn't immediately interpreted as a request to cancel source highlight, because the source highlight may well end up being quickly, and the user was just unlucky enough that Ctrl-C was pressed in that short window. In that scenario, it is better to assume that the source highlight will finish quickly, and that the Ctrl-C is meant to interrupt the inferior. The remote target handles this by only asking the user the second time the user presses Ctrl-C -- see remote.c:remote_target::remote_serial_quit_handler(). But we can worry about this if we ever come to it, assuming it isn't really expected to end up printing sources while the inferior is running. It's just unfortunate that if such case does exits, reproducing it won't be super easy, as it'll be timing dependent. Assuming we don't want to bother with that for now, the approach in the patch seems fine to me. A few nits/comments below. > + > + if (!check_quit_flag ()) > + { > + /* User didn't press ^C, nothing to do. */ > + return; > + } > + > + /* Ask the user what to do. */ > + int resp > + = yquery (_("Cancel source styling using GNU source highlight of %s?\n"), > + m_fullname.c_str ()); I don't think it's usual for secondary prompts / queries to end in newline, and then the "(y or n)" appearing on the next line? I think most (all?) other queries / secondary prompts leave the "(y or n)" on the same line as the question text? Also, I think we should skip the query if sync_quit_force_run is set, so that if we got a SIGTERM we abort quickly and end up in the top level exiting GDB ASAP. To make that work properly we'll need to tweak try_source_highlight to not swallow gdb_exception_forced_quit, though, it currently swallows _everything_. > + 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; It doesn't hurt to do it this way, but the QUIT idea looked more appealing when it seemed like you could get rid of the check_quit_flag() call too, in v1. With the query in the middle, the end result is just that default_quit_handler ends up in "quit ();", which you could call directly here: /* Interrupt highlighting. */ quit (); But OTOH, the default_quit_handler approach is more future proof, especially considering SIGTERM handling in maybe_quit and quit, which would move elsewhere someday. So yeah, hmm, better do it the default_quit_handler/QUIT way after all. > + } > + > +private: > + const std::string &m_fullname; > +}; > + > #endif /* HAVE_SOURCE_HIGHLIGHT */ > > /* Try to highlight CONTENTS from file FULLNAME in language LANG using > @@ -223,9 +266,27 @@ try_source_highlight (std::string &contents ATTRIBUTE_UNUSED, > highlighter->setStyleFile ("esc.style"); > } > > + gdb_highlight_event_listener event_listener (fullname); > + highlighter->setHighlightEventListener (&event_listener); > + /* Make sure that the highlighter's EventListener doesn't become a > + dangling pointer. */ > + auto clear_event_listener = make_scope_exit ([]() > + { > + highlighter->setHighlightEventListener (nullptr); > + }); clear_event_listener is never used, so this could instead be a simpler SCOPE_EXIT: SCOPE_EXIT { highlighter->setHighlightEventListener (nullptr); };