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

[Lancelot SIX <lsix@lancelotsix.com>]

Hi Tom,

I have one minor nit below.

On Mon, Oct 16, 2023 at 11:17:48AM +0200, Tom de Vries wrote:
> In PR cli/30934, a problem is reported where gdb becomes unresponsive for
> 2m13s because the GNU source-highlight library is taking a lot of time to
> annotate the source.
> 
> This is due to a problem in the library [1], for which I've posted a
> patch [2], which brings down the annotation time to 3s.
> 
> However, GDB should not become unresponsive due to such a problem.
> 
> Fix this by installing a srchilite::HighlightEventListener that:
> - checks whether ^C was pressed, and if so
> - asks the user whether it should interrupt highlighting, and if so
> - abandons the highlighting by throwing an exception.
> 
> Interrupting the highlighting looks like this to the user:
> ...
> $ gdb -q a.out -ex "b 56"
> Reading symbols from a.out...
> Breakpoint 1 at 0x400e2a: file test.cpp, line 56.
> (gdb) r
> Starting program: /data/vries/gdb/a.out
> 
> Breakpoint 1, Solution::numOfSubarrays () at test.cpp:56
> ^CCancel source styling using GNU source highlight of /data/vries/gdb/test.cpp?
> ([y] or n) y
> 56	        return (int) t;
> (gdb)
> ...
> Note that line 56 still can be highlighted, just by pygments instead of
> source-highlight.
> 
> Tested on x86_64-linux.
> 
> Co-Authored-By: Lancelot Six <lancelot.six@amd.com>
> 
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30934
> 
> [1] https://savannah.gnu.org/bugs/?64747
> [2] https://savannah.gnu.org/patch/index.php?10400
> ---
>  gdb/source-cache.c | 68 ++++++++++++++++++++++++++++++++++++++++++++--
>  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)
> +    : m_fullname (fullname)
> +  {
> +  }
> +
> +  void notify(const srchilite::HighlightEvent &event) override
                ^
A space is missing here before the "(".

Otherwise, and FWIW, this LGTM.

Best, Lancelot.

> +  {
> +    /* If the terminal is ours, we can ask the user a question and get an
> +       answer.  */
> +    gdb_assert (target_terminal::is_ours ());
> +
> +    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 ());
> +    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;
> +  }
> +
> +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);
> +      });
> +
>        std::istringstream input (contents);
>        std::ostringstream output;
> -      highlighter->highlight (input, output, lang_name, fullname);
> +      {
> +	/* We want to be able to interrupt the call to source-highlight.  If
> +	   the current quit_handler is infrun_quit_handler, pressing ^C is
> +	   ignored by QUIT (assuming target_terminal::is_ours), so install the
> +	   default quit handler.  */
> +	scoped_restore restore_quit_handler
> +	  = make_scoped_restore (&quit_handler, default_quit_handler);
> +
> +	highlighter->highlight (input, output, lang_name, fullname);
> +      }
>        contents = std::move (output).str();
>        styled = true;
>      }
> @@ -304,13 +365,16 @@ source_cache::ensure (struct symtab *s)
>  	     reasons:
>  	     - the language is not supported.
>  	     - the language cannot not be auto-detected from the file name.
> +	     - styling took too long and was interrupted by the user.
>  	     - no stylers available.
>  
>  	     Since styling failed, don't try styling the file again after it
>  	     drops from the cache.
>  
>  	     Note that clearing the source cache also clears
> -	     m_no_styling_files.  */
> +	     m_no_styling_files, so if styling took too long, and the user
> +	     interrupted it, and the source cache gets cleared, the user will
> +	     need to interrupt styling again.  */
>  	  m_no_styling_files.insert (fullname);
>  	}
>      }
> -- 
> 2.35.3
>