[PATCH v2 0/4] [gdb/cli] Allow source highlighting to be interrupted

[PATCH v2 0/4] [gdb/cli] Allow source highlighting to be interrupted

[Tom de Vries]

I wrote this patch series to fix PR cli/30934.

The 1st patch is an unrelated optimization, which I added to this series
because it touches the same code.

The 2nd patch factors out a function.

The 3rd patch adds a means to keep track of styling failures in the
source cache, as suggested here (
https://sourceware.org/pipermail/gdb-patches/2023-October/203164.html ).

The 4th patch fixes the PR cli/30934.

This is v2.  Changes since v1:
- fixed a build problem with --disable-source-highlight, reported by the linaro
  CI.
- temporarily installs the default_quit_handler to be able to use QUIT
- added a question whether to interrupt highlighting or not
  (in the RFC, I had a warning, in v1 I dropped it)
- added "gdb_assert (target_terminal::is_ours ())"

Adding the question was inspired by the v3 patch "gdb/debuginfod: Ctrl-C ask
to cancel further downloads" (
https://sourceware.org/pipermail/gdb-patches/2023-March/197679.html ).

Submission history:
- RFC:
  https://sourceware.org/pipermail/gdb-patches/2023-October/203157.html
- v1:
  https://sourceware.org/pipermail/gdb-patches/2023-October/203179.html

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30934

Tom de Vries (4):
  [gdb/cli] Skip string copy in source_cache::ensure
  [gdb/cli] Factor out try_source_highlight
  [gdb/cli] Keep track of styling failures in source_cache
  [gdb/cli] Allow source highlighting to be interrupted

 gdb/source-cache.c | 173 +++++++++++++++++++++++++++++++++++----------
 gdb/source-cache.h |   4 ++
 2 files changed, 141 insertions(+), 36 deletions(-)


base-commit: 9326300e4d3dbe943380b30766ed474d98df07ba
-- 
2.35.3

[PATCH v2 1/4] [gdb/cli] Skip string copy in source_cache::ensure

[Tom de Vries]

In function source_cache::ensure we have:
...
 	      std::ostringstream output;
	      ...
	      contents = output.str();
...
The last line causes an unnecessary string copy.

C++20 allows us to skip it, like this:
...
	      contents = std::move (output).str();
...

Use the more efficient version if compiling with -std=c++20.

Tested on x86_64-linux.
---
 gdb/source-cache.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/gdb/source-cache.c b/gdb/source-cache.c
index 77b357cb42b..7ef54411c69 100644
--- a/gdb/source-cache.c
+++ b/gdb/source-cache.c
@@ -252,7 +252,11 @@ source_cache::ensure (struct symtab *s)
 	      std::istringstream input (contents);
 	      std::ostringstream output;
 	      highlighter->highlight (input, output, lang_name, fullname);
+#if __cplusplus >= 202002L
+	      contents = std::move (output).str();
+#else
 	      contents = output.str ();
+#endif
 	      already_styled = true;
 	    }
 	  catch (...)
-- 
2.35.3

[PATCH v2 2/4] [gdb/cli] Factor out try_source_highlight

[Tom de Vries]

Function source_cache::ensure contains some code using the GNU
source-highlight library.

The code is a sizable chunk of the function, and contains conditional
compilation in a slightly convoluted way:
...
       if (!already_styled)
 #endif /* HAVE_SOURCE_HIGHLIGHT */
       {
...

Fix this by factoring out the code into new function try_source_highlight,
such that:
- source_cache::ensure is easier to read, and
- the conditional compilation is at the level of the function body.

Tested on x86_64-linux.
---
 gdb/source-cache.c | 99 +++++++++++++++++++++++++++-------------------
 1 file changed, 59 insertions(+), 40 deletions(-)

diff --git a/gdb/source-cache.c b/gdb/source-cache.c
index 7ef54411c69..9757721e932 100644
--- a/gdb/source-cache.c
+++ b/gdb/source-cache.c
@@ -191,6 +191,63 @@ get_language_name (enum language lang)
 
 #endif /* HAVE_SOURCE_HIGHLIGHT */
 
+/* Try to highlight CONTENTS from file FULLNAME in language LANG using
+   the GNU source-higlight library.  Return true if highlighting
+   succeeded.  */
+
+static bool
+try_source_highlight (std::string &contents ATTRIBUTE_UNUSED,
+		      enum language lang ATTRIBUTE_UNUSED,
+		      const std::string &fullname ATTRIBUTE_UNUSED)
+{
+#ifdef HAVE_SOURCE_HIGHLIGHT
+  if (!use_gnu_source_highlight)
+    return false;
+
+  const char *lang_name = get_language_name (lang);
+  if (lang_name == nullptr)
+    return false;
+
+  /* The global source highlight object, or null if one was
+     never constructed.  This is stored here rather than in
+     the class so that we don't need to include anything or do
+     conditional compilation in source-cache.h.  */
+  static srchilite::SourceHighlight *highlighter;
+
+  bool styled = false;
+  try
+    {
+      if (highlighter == nullptr)
+	{
+	  highlighter = new srchilite::SourceHighlight ("esc.outlang");
+	  highlighter->setStyleFile ("esc.style");
+	}
+
+      std::istringstream input (contents);
+      std::ostringstream output;
+      highlighter->highlight (input, output, lang_name, fullname);
+#if __cplusplus >= 202002L
+      contents = std::move (output).str();
+#else
+      contents = output.str ();
+#endif
+      styled = true;
+    }
+  catch (...)
+    {
+      /* Source Highlight will throw an exception if
+	 highlighting fails.  One possible reason it can fail
+	 is if the language is unknown -- which matters to gdb
+	 because Rust support wasn't added until after 3.1.8.
+	 Ignore exceptions here. */
+    }
+
+  return styled;
+#else
+  return false;
+#endif /* HAVE_SOURCE_HIGHLIGHT */
+}
+
 /* See source-cache.h.  */
 
 bool
@@ -230,48 +287,10 @@ source_cache::ensure (struct symtab *s)
 
   if (source_styling && gdb_stdout->can_emit_style_escape ())
     {
-#ifdef HAVE_SOURCE_HIGHLIGHT
-      bool already_styled = false;
-      const char *lang_name = get_language_name (s->language ());
-      if (lang_name != nullptr && use_gnu_source_highlight)
-	{
-	  /* The global source highlight object, or null if one was
-	     never constructed.  This is stored here rather than in
-	     the class so that we don't need to include anything or do
-	     conditional compilation in source-cache.h.  */
-	  static srchilite::SourceHighlight *highlighter;
-
-	  try
-	    {
-	      if (highlighter == nullptr)
-		{
-		  highlighter = new srchilite::SourceHighlight ("esc.outlang");
-		  highlighter->setStyleFile ("esc.style");
-		}
-
-	      std::istringstream input (contents);
-	      std::ostringstream output;
-	      highlighter->highlight (input, output, lang_name, fullname);
-#if __cplusplus >= 202002L
-	      contents = std::move (output).str();
-#else
-	      contents = output.str ();
-#endif
-	      already_styled = true;
-	    }
-	  catch (...)
-	    {
-	      /* Source Highlight will throw an exception if
-		 highlighting fails.  One possible reason it can fail
-		 is if the language is unknown -- which matters to gdb
-		 because Rust support wasn't added until after 3.1.8.
-		 Ignore exceptions here and fall back to
-		 un-highlighted text. */
-	    }
-	}
+      bool already_styled
+	= try_source_highlight (contents, s->language (), fullname);
 
       if (!already_styled)
-#endif /* HAVE_SOURCE_HIGHLIGHT */
 	{
 	  gdb::optional<std::string> ext_contents;
 	  ext_contents = ext_lang_colorize (fullname, contents);
-- 
2.35.3

[PATCH v2 3/4] [gdb/cli] Keep track of styling failures in source_cache

[Tom de Vries]

In source_cache::ensure, keep track of which files failed to be styled, and
don't attempt to style them again in case the file dropped out of the cache.

Tested on x86_64-linux.
---
 gdb/source-cache.c | 24 ++++++++++++++++++++++--
 gdb/source-cache.h |  4 ++++
 2 files changed, 26 insertions(+), 2 deletions(-)

diff --git a/gdb/source-cache.c b/gdb/source-cache.c
index 9757721e932..f8c98d7e23f 100644
--- a/gdb/source-cache.c
+++ b/gdb/source-cache.c
@@ -285,7 +285,8 @@ source_cache::ensure (struct symtab *s)
       return false;
     }
 
-  if (source_styling && gdb_stdout->can_emit_style_escape ())
+  if (source_styling && gdb_stdout->can_emit_style_escape ()
+      && m_no_styling_files.count (fullname) == 0)
     {
       bool already_styled
 	= try_source_highlight (contents, s->language (), fullname);
@@ -295,7 +296,26 @@ source_cache::ensure (struct symtab *s)
 	  gdb::optional<std::string> ext_contents;
 	  ext_contents = ext_lang_colorize (fullname, contents);
 	  if (ext_contents.has_value ())
-	    contents = std::move (*ext_contents);
+	    {
+	      contents = std::move (*ext_contents);
+	      already_styled = true;
+	    }
+	}
+
+      if (!already_styled)
+	{
+	  /* Styling failed.  Styling can fail for instance for these
+	     reasons:
+	     - the language is not supported.
+	     - the language cannot not be auto-detected from the file name.
+	     - 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.insert (fullname);
 	}
     }
 
diff --git a/gdb/source-cache.h b/gdb/source-cache.h
index bec6598ff9f..f1d30b43ce0 100644
--- a/gdb/source-cache.h
+++ b/gdb/source-cache.h
@@ -66,6 +66,7 @@ class source_cache
   {
     m_source_map.clear ();
     m_offset_cache.clear ();
+    m_no_styling_files.clear ();
   }
 
 private:
@@ -95,6 +96,9 @@ class source_cache
   /* The file offset cache.  The key is the full name of the source
      file.  */
   std::unordered_map<std::string, std::vector<off_t>> m_offset_cache;
+
+  /* The list of files where styling failed.  */
+  std::unordered_set<std::string> m_no_styling_files;
 };
 
 /* The global source cache.  */
-- 
2.35.3

[PATCH v2 4/4] [gdb/cli] Allow source highlighting to be interrupted

[Tom de Vries]

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.

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 | 62 ++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 60 insertions(+), 2 deletions(-)

diff --git a/gdb/source-cache.c b/gdb/source-cache.c
index f8c98d7e23f..57d9f3b8ab4 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
+  {
+    /* 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,21 @@ try_source_highlight (std::string &contents ATTRIBUTE_UNUSED,
 	  highlighter->setStyleFile ("esc.style");
 	}
 
+      gdb_highlight_event_listener event_listener (fullname);
+      highlighter->setHighlightEventListener (&event_listener);
+
       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);
+      }
 #if __cplusplus >= 202002L
       contents = std::move (output).str();
 #else
@@ -308,13 +363,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

Re: [PATCH v2 1/4] [gdb/cli] Skip string copy in source_cache::ensure

[Lancelot SIX]

Hi Tom,

On Fri, Oct 13, 2023 at 02:19:50PM +0200, Tom de Vries wrote:
> In function source_cache::ensure we have:
> ...
>  	      std::ostringstream output;
> 	      ...
> 	      contents = output.str();
> ...
> The last line causes an unnecessary string copy.
> 
> C++20 allows us to skip it, like this:
> ...
> 	      contents = std::move (output).str();
> ...

Looking at cppreference[1], it suggests that this new C++ overload for
str should be equivalent to

  content = std::move (*output.rbuf ()).str ();

Should we use this construct instead regardless of C++'s version and
avoid the #ifdef?  That being said, I find the C++20 version easier to
read, so it's up to you.

Best,
Lancelot.

[1] https://en.cppreference.com/w/cpp/io/basic_ostringstream/str

> 
> Use the more efficient version if compiling with -std=c++20.
> 
> Tested on x86_64-linux.
> ---
>  gdb/source-cache.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/gdb/source-cache.c b/gdb/source-cache.c
> index 77b357cb42b..7ef54411c69 100644
> --- a/gdb/source-cache.c
> +++ b/gdb/source-cache.c
> @@ -252,7 +252,11 @@ source_cache::ensure (struct symtab *s)
>  	      std::istringstream input (contents);
>  	      std::ostringstream output;
>  	      highlighter->highlight (input, output, lang_name, fullname);
> +#if __cplusplus >= 202002L
> +	      contents = std::move (output).str();
> +#else
>  	      contents = output.str ();
> +#endif
>  	      already_styled = true;
>  	    }
>  	  catch (...)
> -- 
> 2.35.3
> 

Re: [PATCH v2 2/4] [gdb/cli] Factor out try_source_highlight

[Lancelot SIX]

Hi Tom

On Fri, Oct 13, 2023 at 02:19:51PM +0200, Tom de Vries wrote:
> Function source_cache::ensure contains some code using the GNU
> source-highlight library.
> 
> The code is a sizable chunk of the function, and contains conditional
> compilation in a slightly convoluted way:
> ...
>        if (!already_styled)
>  #endif /* HAVE_SOURCE_HIGHLIGHT */
>        {
> ...
> 
> Fix this by factoring out the code into new function try_source_highlight,
> such that:
> - source_cache::ensure is easier to read, and
> - the conditional compilation is at the level of the function body.
> 
> Tested on x86_64-linux.
> ---
>  gdb/source-cache.c | 99 +++++++++++++++++++++++++++-------------------
>  1 file changed, 59 insertions(+), 40 deletions(-)
> 
> diff --git a/gdb/source-cache.c b/gdb/source-cache.c
> index 7ef54411c69..9757721e932 100644
> --- a/gdb/source-cache.c
> +++ b/gdb/source-cache.c
> @@ -191,6 +191,63 @@ get_language_name (enum language lang)
>  
>  #endif /* HAVE_SOURCE_HIGHLIGHT */
>  
> +/* Try to highlight CONTENTS from file FULLNAME in language LANG using
> +   the GNU source-higlight library.  Return true if highlighting
> +   succeeded.  */
> +
> +static bool
> +try_source_highlight (std::string &contents ATTRIBUTE_UNUSED,
> +		      enum language lang ATTRIBUTE_UNUSED,
> +		      const std::string &fullname ATTRIBUTE_UNUSED)
> +{
> +#ifdef HAVE_SOURCE_HIGHLIGHT
> +  if (!use_gnu_source_highlight)
> +    return false;
> +
> +  const char *lang_name = get_language_name (lang);
> +  if (lang_name == nullptr)
> +    return false;
> +
> +  /* The global source highlight object, or null if one was
> +     never constructed.  This is stored here rather than in
> +     the class so that we don't need to include anything or do
> +     conditional compilation in source-cache.h.  */
> +  static srchilite::SourceHighlight *highlighter;

Even if this is pre-existing code, I think I would be tempted to get the
initialization of HIGHLIGHTER outside of the TRY block (new can throw,
we might not want to hide it).

Using an immediately invoked lambda does the trick and works well with
static variables:

    static srchilite::SourceHighlight *highlighter
      = []()
        {
	  srchilite::SourceHighlight * h = new srchilite::SourceHighlight ("esc.outlang");
	  h->setStyleFile ("esc.style");
	  return h;
	} ();

Best,
Lancelot.

> +
> +  bool styled = false;
> +  try
> +    {
> +      if (highlighter == nullptr)
> +	{
> +	  highlighter = new srchilite::SourceHighlight ("esc.outlang");
> +	  highlighter->setStyleFile ("esc.style");
> +	}
> +
> +      std::istringstream input (contents);
> +      std::ostringstream output;
> +      highlighter->highlight (input, output, lang_name, fullname);
> +#if __cplusplus >= 202002L
> +      contents = std::move (output).str();
> +#else
> +      contents = output.str ();
> +#endif
> +      styled = true;
> +    }
> +  catch (...)
> +    {
> +      /* Source Highlight will throw an exception if
> +	 highlighting fails.  One possible reason it can fail
> +	 is if the language is unknown -- which matters to gdb
> +	 because Rust support wasn't added until after 3.1.8.
> +	 Ignore exceptions here. */
> +    }
> +
> +  return styled;
> +#else
> +  return false;
> +#endif /* HAVE_SOURCE_HIGHLIGHT */
> +}
> +
>  /* See source-cache.h.  */
>  
>  bool
> @@ -230,48 +287,10 @@ source_cache::ensure (struct symtab *s)
>  
>    if (source_styling && gdb_stdout->can_emit_style_escape ())
>      {
> -#ifdef HAVE_SOURCE_HIGHLIGHT
> -      bool already_styled = false;
> -      const char *lang_name = get_language_name (s->language ());
> -      if (lang_name != nullptr && use_gnu_source_highlight)
> -	{
> -	  /* The global source highlight object, or null if one was
> -	     never constructed.  This is stored here rather than in
> -	     the class so that we don't need to include anything or do
> -	     conditional compilation in source-cache.h.  */
> -	  static srchilite::SourceHighlight *highlighter;
> -
> -	  try
> -	    {
> -	      if (highlighter == nullptr)
> -		{
> -		  highlighter = new srchilite::SourceHighlight ("esc.outlang");
> -		  highlighter->setStyleFile ("esc.style");
> -		}
> -
> -	      std::istringstream input (contents);
> -	      std::ostringstream output;
> -	      highlighter->highlight (input, output, lang_name, fullname);
> -#if __cplusplus >= 202002L
> -	      contents = std::move (output).str();
> -#else
> -	      contents = output.str ();
> -#endif
> -	      already_styled = true;
> -	    }
> -	  catch (...)
> -	    {
> -	      /* Source Highlight will throw an exception if
> -		 highlighting fails.  One possible reason it can fail
> -		 is if the language is unknown -- which matters to gdb
> -		 because Rust support wasn't added until after 3.1.8.
> -		 Ignore exceptions here and fall back to
> -		 un-highlighted text. */
> -	    }
> -	}
> +      bool already_styled
> +	= try_source_highlight (contents, s->language (), fullname);
>  
>        if (!already_styled)
> -#endif /* HAVE_SOURCE_HIGHLIGHT */
>  	{
>  	  gdb::optional<std::string> ext_contents;
>  	  ext_contents = ext_lang_colorize (fullname, contents);
> -- 
> 2.35.3
> 

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

[Lancelot SIX]

Hi Tom,

I have a comment below.

On Fri, Oct 13, 2023 at 02:19:53PM +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.
> 
> 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 | 62 ++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 60 insertions(+), 2 deletions(-)
> 
> diff --git a/gdb/source-cache.c b/gdb/source-cache.c
> index f8c98d7e23f..57d9f3b8ab4 100644
> --- a/gdb/source-cache.c
> +++ b/gdb/source-cache.c
> @@ -223,9 +266,21 @@ try_source_highlight (std::string &contents ATTRIBUTE_UNUSED,
>  	  highlighter->setStyleFile ("esc.style");
>  	}
>  
> +      gdb_highlight_event_listener event_listener (fullname);
> +      highlighter->setHighlightEventListener (&event_listener);

In this instance, highlighter outlives event_listener but keeps a
reference to it.  Arguably, the event listener will be reset to point to
another instance before the next invocation of highlight, but I think it
would be cleaner to make sure that the event listener is cleared anyway:

    auto clear_event_listener = make_scope_exit ([highlighter]()
      {
        highlighter->setHighlightEventListener (nullptr);
      });

Who knows, maybe one day highlighter's dtor might want to notify
something.

WDYT?

Best,
Lancelot.

> +
>        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);
> +      }
>  #if __cplusplus >= 202002L
>        contents = std::move (output).str();
>  #else
> @@ -308,13 +363,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
> 

Re: [PATCH v2 2/4] [gdb/cli] Factor out try_source_highlight

[Tom de Vries]

On 10/13/23 18:52, Lancelot SIX wrote:
> Hi Tom
> 
> On Fri, Oct 13, 2023 at 02:19:51PM +0200, Tom de Vries wrote:
>> Function source_cache::ensure contains some code using the GNU
>> source-highlight library.
>>
>> The code is a sizable chunk of the function, and contains conditional
>> compilation in a slightly convoluted way:
>> ...
>>         if (!already_styled)
>>   #endif /* HAVE_SOURCE_HIGHLIGHT */
>>         {
>> ...
>>
>> Fix this by factoring out the code into new function try_source_highlight,
>> such that:
>> - source_cache::ensure is easier to read, and
>> - the conditional compilation is at the level of the function body.
>>
>> Tested on x86_64-linux.
>> ---
>>   gdb/source-cache.c | 99 +++++++++++++++++++++++++++-------------------
>>   1 file changed, 59 insertions(+), 40 deletions(-)
>>
>> diff --git a/gdb/source-cache.c b/gdb/source-cache.c
>> index 7ef54411c69..9757721e932 100644
>> --- a/gdb/source-cache.c
>> +++ b/gdb/source-cache.c
>> @@ -191,6 +191,63 @@ get_language_name (enum language lang)
>>   
>>   #endif /* HAVE_SOURCE_HIGHLIGHT */
>>   
>> +/* Try to highlight CONTENTS from file FULLNAME in language LANG using
>> +   the GNU source-higlight library.  Return true if highlighting
>> +   succeeded.  */
>> +
>> +static bool
>> +try_source_highlight (std::string &contents ATTRIBUTE_UNUSED,
>> +		      enum language lang ATTRIBUTE_UNUSED,
>> +		      const std::string &fullname ATTRIBUTE_UNUSED)
>> +{
>> +#ifdef HAVE_SOURCE_HIGHLIGHT
>> +  if (!use_gnu_source_highlight)
>> +    return false;
>> +
>> +  const char *lang_name = get_language_name (lang);
>> +  if (lang_name == nullptr)
>> +    return false;
>> +
>> +  /* The global source highlight object, or null if one was
>> +     never constructed.  This is stored here rather than in
>> +     the class so that we don't need to include anything or do
>> +     conditional compilation in source-cache.h.  */
>> +  static srchilite::SourceHighlight *highlighter;
> 
> Even if this is pre-existing code, I think I would be tempted to get the
> initialization of HIGHLIGHTER outside of the TRY block (new can throw,
> we might not want to hide it).
> 

Hi,

The pre-existing code is a result of commit f64e2f40454 ("[gdb] Catch 
exception when constructing the highlighter"), which does the opposite 
of what you propose here.

And I think the rationale given there still holds.

I suppose if we're interested in not hiding that new throws, you could 
catch the exceptions of interest and handle them appropriately, by say 
issuing a warning or rethrowing.

I will leave this as is for now.

Thanks,
- Tom

> Using an immediately invoked lambda does the trick and works well with
> static variables:
> 
>      static srchilite::SourceHighlight *highlighter
>        = []()
>          {
> 	  srchilite::SourceHighlight * h = new srchilite::SourceHighlight ("esc.outlang");
> 	  h->setStyleFile ("esc.style");
> 	  return h;
> 	} ();
> 
> Best,
> Lancelot.
> 
>> +
>> +  bool styled = false;
>> +  try
>> +    {
>> +      if (highlighter == nullptr)
>> +	{
>> +	  highlighter = new srchilite::SourceHighlight ("esc.outlang");
>> +	  highlighter->setStyleFile ("esc.style");
>> +	}
>> +
>> +      std::istringstream input (contents);
>> +      std::ostringstream output;
>> +      highlighter->highlight (input, output, lang_name, fullname);
>> +#if __cplusplus >= 202002L
>> +      contents = std::move (output).str();
>> +#else
>> +      contents = output.str ();
>> +#endif
>> +      styled = true;
>> +    }
>> +  catch (...)
>> +    {
>> +      /* Source Highlight will throw an exception if
>> +	 highlighting fails.  One possible reason it can fail
>> +	 is if the language is unknown -- which matters to gdb
>> +	 because Rust support wasn't added until after 3.1.8.
>> +	 Ignore exceptions here. */
>> +    }
>> +
>> +  return styled;
>> +#else
>> +  return false;
>> +#endif /* HAVE_SOURCE_HIGHLIGHT */
>> +}
>> +
>>   /* See source-cache.h.  */
>>   
>>   bool
>> @@ -230,48 +287,10 @@ source_cache::ensure (struct symtab *s)
>>   
>>     if (source_styling && gdb_stdout->can_emit_style_escape ())
>>       {
>> -#ifdef HAVE_SOURCE_HIGHLIGHT
>> -      bool already_styled = false;
>> -      const char *lang_name = get_language_name (s->language ());
>> -      if (lang_name != nullptr && use_gnu_source_highlight)
>> -	{
>> -	  /* The global source highlight object, or null if one was
>> -	     never constructed.  This is stored here rather than in
>> -	     the class so that we don't need to include anything or do
>> -	     conditional compilation in source-cache.h.  */
>> -	  static srchilite::SourceHighlight *highlighter;
>> -
>> -	  try
>> -	    {
>> -	      if (highlighter == nullptr)
>> -		{
>> -		  highlighter = new srchilite::SourceHighlight ("esc.outlang");
>> -		  highlighter->setStyleFile ("esc.style");
>> -		}
>> -
>> -	      std::istringstream input (contents);
>> -	      std::ostringstream output;
>> -	      highlighter->highlight (input, output, lang_name, fullname);
>> -#if __cplusplus >= 202002L
>> -	      contents = std::move (output).str();
>> -#else
>> -	      contents = output.str ();
>> -#endif
>> -	      already_styled = true;
>> -	    }
>> -	  catch (...)
>> -	    {
>> -	      /* Source Highlight will throw an exception if
>> -		 highlighting fails.  One possible reason it can fail
>> -		 is if the language is unknown -- which matters to gdb
>> -		 because Rust support wasn't added until after 3.1.8.
>> -		 Ignore exceptions here and fall back to
>> -		 un-highlighted text. */
>> -	    }
>> -	}
>> +      bool already_styled
>> +	= try_source_highlight (contents, s->language (), fullname);
>>   
>>         if (!already_styled)
>> -#endif /* HAVE_SOURCE_HIGHLIGHT */
>>   	{
>>   	  gdb::optional<std::string> ext_contents;
>>   	  ext_contents = ext_lang_colorize (fullname, contents);
>> -- 
>> 2.35.3
>>

Re: [PATCH v2 1/4] [gdb/cli] Skip string copy in source_cache::ensure

[Tom de Vries]

On 10/13/23 18:34, Lancelot SIX wrote:
> Hi Tom,
> 
> On Fri, Oct 13, 2023 at 02:19:50PM +0200, Tom de Vries wrote:
>> In function source_cache::ensure we have:
>> ...
>>   	      std::ostringstream output;
>> 	      ...
>> 	      contents = output.str();
>> ...
>> The last line causes an unnecessary string copy.
>>
>> C++20 allows us to skip it, like this:
>> ...
>> 	      contents = std::move (output).str();
>> ...
> 
> Looking at cppreference[1], it suggests that this new C++ overload for
> str should be equivalent to
> 
>    content = std::move (*output.rbuf ()).str ();
> 
> Should we use this construct instead regardless of C++'s version and
> avoid the #ifdef?  That being said, I find the C++20 version easier to
> read, so it's up to you.
> 

I've played around with this a bit, and found out that I can also use 
"contents = std::move (output).str();" in c++11.  So I've just dropped 
the ifdef.

Thanks,
- Tom

> Best,
> Lancelot.
> 
> [1] https://en.cppreference.com/w/cpp/io/basic_ostringstream/str
> 
>>
>> Use the more efficient version if compiling with -std=c++20.
>>
>> Tested on x86_64-linux.
>> ---
>>   gdb/source-cache.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/gdb/source-cache.c b/gdb/source-cache.c
>> index 77b357cb42b..7ef54411c69 100644
>> --- a/gdb/source-cache.c
>> +++ b/gdb/source-cache.c
>> @@ -252,7 +252,11 @@ source_cache::ensure (struct symtab *s)
>>   	      std::istringstream input (contents);
>>   	      std::ostringstream output;
>>   	      highlighter->highlight (input, output, lang_name, fullname);
>> +#if __cplusplus >= 202002L
>> +	      contents = std::move (output).str();
>> +#else
>>   	      contents = output.str ();
>> +#endif
>>   	      already_styled = true;
>>   	    }
>>   	  catch (...)
>> -- 
>> 2.35.3
>>

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

[Tom de Vries]

On 10/13/23 19:00, Lancelot SIX wrote:
> Hi Tom,
> 
> I have a comment below.
> 
> On Fri, Oct 13, 2023 at 02:19:53PM +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.
>>
>> 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 | 62 ++++++++++++++++++++++++++++++++++++++++++++--
>>   1 file changed, 60 insertions(+), 2 deletions(-)
>>
>> diff --git a/gdb/source-cache.c b/gdb/source-cache.c
>> index f8c98d7e23f..57d9f3b8ab4 100644
>> --- a/gdb/source-cache.c
>> +++ b/gdb/source-cache.c
>> @@ -223,9 +266,21 @@ try_source_highlight (std::string &contents ATTRIBUTE_UNUSED,
>>   	  highlighter->setStyleFile ("esc.style");
>>   	}
>>   
>> +      gdb_highlight_event_listener event_listener (fullname);
>> +      highlighter->setHighlightEventListener (&event_listener);
> 
> In this instance, highlighter outlives event_listener but keeps a
> reference to it.  Arguably, the event listener will be reset to point to
> another instance before the next invocation of highlight, but I think it
> would be cleaner to make sure that the event listener is cleared anyway:
> 
>      auto clear_event_listener = make_scope_exit ([highlighter]()
>        {
>          highlighter->setHighlightEventListener (nullptr);
>        });
> 
> Who knows, maybe one day highlighter's dtor might want to notify
> something.
> 
> WDYT?
> 

That's a good idea, thanks, I've added it to the patch.

The only change I had to make was to drop the highlighter capture, I got 
some error because of that:
...
/data/vries/gdb/src/gdb/source-cache.c: In function ‘bool 
try_source_highlight(std::__cxx11::string&, language, const string&)’:
/data/vries/gdb/src/gdb/source-cache.c:273:53: error: capture of 
variable ‘highlighter’ with non-automatic storage duration [-Werror]
...

I'll post a v3 after retesting.

Thanks,
- Tom

> Best,
> Lancelot.
> 
>> +
>>         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);
>> +      }
>>   #if __cplusplus >= 202002L
>>         contents = std::move (output).str();
>>   #else
>> @@ -308,13 +363,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
>>

Re: [PATCH v2 2/4] [gdb/cli] Factor out try_source_highlight

[Lancelot SIX]

> 
> Hi,
> 
> The pre-existing code is a result of commit f64e2f40454 ("[gdb] Catch
> exception when constructing the highlighter"), which does the opposite of
> what you propose here.
> 
> And I think the rationale given there still holds.
> 

Hi Tom,

Thanks for pointing this out, I missed this commit.

Best,
Lancelot.

> I suppose if we're interested in not hiding that new throws, you could catch
> the exceptions of interest and handle them appropriately, by say issuing a
> warning or rethrowing.
> 
> I will leave this as is for now.
> 
> Thanks,
> - Tom
>