Re: [PATCH v3 4/4] [gdb/cli] Allow source highlighting to be interrupted

Tom de Vries
2023-10-18T17:21:36Z

On 10/17/23 02:20, Pedro Alves wrote:
> 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 <sstream>
>> #include <srchilite/sourcehighlight.h>
>> #include <srchilite/langmap.h>
>> +#include <srchilite/highlighteventlistener.h>
>> #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".
>

Hi,

thanks for the review.

Done.

>> + : 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'm not too sure.

> 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.
>

I've gone back and forth a bit about this, I don't particularly like the
assert, but I like the alternatives less I guess. I've thought about
adding a way to enable the assert in development builds (to detect it if
the situation happens) and disable it in releases (to give end users a
stable debugger). But I've left it as is for now.

> 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?
>

Done.

> 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_.
>
>

Thanks, good point, I overlooked that. I've restructured the patch
into a v4 patch series, and added handling of SIGTERM.

>> + 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.
>

OK :)

>> + }
>> +
>> +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); };
>

Done.

I've posted the v4 here (
https://sourceware.org/pipermail/gdb-patches/2023-October/203391.html ).

Thanks,
- Tom