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

[Pedro Alves <pedro@palves.net>]

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

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