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

[PATCH v3 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.

Changes in v3:
- dropped the "#if __cplusplus >= 202002L" in the first patch.
- added a reset of the highlighter's EventListener to prevent a dangling
  pointer.

Changes in v2:
- 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
- v2:
  https://sourceware.org/pipermail/gdb-patches/2023-October/203199.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 | 175 +++++++++++++++++++++++++++++++++++----------
 gdb/source-cache.h |   4 ++
 2 files changed, 143 insertions(+), 36 deletions(-)


base-commit: 6674b23fe6409e08de9c36f640bd58127eff9dda
-- 
2.35.3

[PATCH v3 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 solution.

Tested on x86_64-linux.
---
 gdb/source-cache.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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

[PATCH v3 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 part 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 | 91 ++++++++++++++++++++++++++++------------------
 1 file changed, 55 insertions(+), 36 deletions(-)

diff --git a/gdb/source-cache.c b/gdb/source-cache.c
index ae02d2516d9..191d0043e66 100644
--- a/gdb/source-cache.c
+++ b/gdb/source-cache.c
@@ -191,6 +191,59 @@ 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);
+      contents = std::move (output).str();
+      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,44 +283,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);
-	      contents = std::move (output).str ();
-	      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 v3 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 191d0043e66..aa8e40425c2 100644
--- a/gdb/source-cache.c
+++ b/gdb/source-cache.c
@@ -281,7 +281,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);
@@ -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 v3 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.

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
+  {
+    /* 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

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

[Lancelot SIX]

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
> 

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

[Tom de Vries]

On 10/16/23 12:21, Lancelot SIX wrote:
> 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.
> 

Hi,

thanks for the review.

It looks like I need to start using gcc's check_GNU_style.sh / 
check_GNU_style.py again.

I fixed this, as well as a few other style issues in the series that I 
found by running the scripts, but I don't think it requires reposting.

Thanks,
- Tom

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

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

[Pedro Alves]

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

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

[Tom de Vries]

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

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

[Lancelot SIX]

Hi,

I went through the series, and except for one minor formatting detail in
the last patch, it LGTM.

Reviewed-By: Lancelot Six <lancelot.six@amd.com>

Best,
Lancelot.

On Mon, Oct 16, 2023 at 11:17:44AM +0200, Tom de Vries wrote:
> 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.
> 
> Changes in v3:
> - dropped the "#if __cplusplus >= 202002L" in the first patch.
> - added a reset of the highlighter's EventListener to prevent a dangling
>   pointer.
> 
> Changes in v2:
> - 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
> - v2:
>   https://sourceware.org/pipermail/gdb-patches/2023-October/203199.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 | 175 +++++++++++++++++++++++++++++++++++----------
>  gdb/source-cache.h |   4 ++
>  2 files changed, 143 insertions(+), 36 deletions(-)
> 
> 
> base-commit: 6674b23fe6409e08de9c36f640bd58127eff9dda
> -- 
> 2.35.3
> 

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

[Tom de Vries]

On 10/16/23 12:23, Lancelot SIX wrote:
> Hi,
> 
> I went through the series, and except for one minor formatting detail in
> the last patch, it LGTM.
> 
> Reviewed-By: Lancelot Six <lancelot.six@amd.com>
> 

Great, thanks for the reviews.

I'll wait for feedback from Pedro on the ^C handling.

Thanks,
- Tom

> Best,
> Lancelot.
> 
> On Mon, Oct 16, 2023 at 11:17:44AM +0200, Tom de Vries wrote:
>> 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.
>>
>> Changes in v3:
>> - dropped the "#if __cplusplus >= 202002L" in the first patch.
>> - added a reset of the highlighter's EventListener to prevent a dangling
>>    pointer.
>>
>> Changes in v2:
>> - 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
>> - v2:
>>    https://sourceware.org/pipermail/gdb-patches/2023-October/203199.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 | 175 +++++++++++++++++++++++++++++++++++----------
>>   gdb/source-cache.h |   4 ++
>>   2 files changed, 143 insertions(+), 36 deletions(-)
>>
>>
>> base-commit: 6674b23fe6409e08de9c36f640bd58127eff9dda
>> -- 
>> 2.35.3
>>