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

[PATCH 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 (...)

base-commit: 4b41a55fe53ce2da4823ffa3a5b94bd09bf2ab0d
-- 
2.35.3

[PATCH 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 | 93 +++++++++++++++++++++++++++-------------------
 1 file changed, 54 insertions(+), 39 deletions(-)

diff --git a/gdb/source-cache.c b/gdb/source-cache.c
index 7ef54411c69..186a4f73256 100644
--- a/gdb/source-cache.c
+++ b/gdb/source-cache.c
@@ -191,6 +191,58 @@ get_language_name (enum language lang)
 
 #endif /* HAVE_SOURCE_HIGHLIGHT */
 
+/* Try to highlight CONTENTS from file FULLNAME in language LANG_NAME using
+   the GNU source-higlight library.  Return true if highlighting
+   succeeded.  */
+
+static bool
+try_source_highlight (std::string &contents, const char *lang_name,
+		      const std::string &fullname)
+{
+#ifdef HAVE_SOURCE_HIGHLIGHT
+  if (lang_name == nullptr || !use_gnu_source_highlight)
+    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 +282,11 @@ 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, lang_name, 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 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 186a4f73256..c29e5f4a210 100644
--- a/gdb/source-cache.c
+++ b/gdb/source-cache.c
@@ -280,7 +280,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)
     {
       const char *lang_name = get_language_name (s->language ());
       bool already_styled
@@ -291,7 +292,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 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 abandons the highlighting by throwing an exception.

Abandoning 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 (this=0x7fffffffdbcf, arr=...) at \
  test.cpp:56
^C
56	        return (int) t;
(gdb)
...
Note that line 56 still can be highlighted, just by pygments instead of
source-highlight.

It may or may not be a good idea to issue a warning that highlighting by
source-highlight was interrupted, I've left it out for now.

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 | 24 +++++++++++++++++++++++-
 1 file changed, 23 insertions(+), 1 deletion(-)

diff --git a/gdb/source-cache.c b/gdb/source-cache.c
index c29e5f4a210..3ce2ea9d779 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,21 @@ get_language_name (enum language lang)
   return nullptr;
 }
 
+/* Class with notify function called during highlighting.  */
+
+class gdb_highlight_event_listener : public srchilite::HighlightEventListener
+{
+public:
+  void notify(const srchilite::HighlightEvent &event) override
+  {
+    if (check_quit_flag ())
+      {
+	/* Got SIGINT, interrupt highlighting, it may take too long.  */
+	throw_quit ("Interrupted");
+      }
+  }
+};
+
 #endif /* HAVE_SOURCE_HIGHLIGHT */
 
 /* Try to highlight CONTENTS from file FULLNAME in language LANG_NAME using
@@ -216,6 +232,9 @@ try_source_highlight (std::string &contents, const char *lang_name,
 	{
 	  highlighter = new srchilite::SourceHighlight ("esc.outlang");
 	  highlighter->setStyleFile ("esc.style");
+
+	  static gdb_highlight_event_listener event_listener;
+	  highlighter->setHighlightEventListener (&event_listener);
 	}
 
       std::istringstream input (contents);
@@ -304,13 +323,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 1/4] [gdb/cli] Skip string copy in source_cache::ensure

[Tom de Vries]

I've submitted a v2 series here ( 
https://sourceware.org/pipermail/gdb-patches/2023-October/203199.html ).

Thanks,
- Tom