[RFC] [gdb/cli] Allow source highlighting to be interrupted

[RFC] [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
^Cwarning: Couldn't highlight source file /data/vries/gdb/test.cpp: Interrupted
56	        return (int) t;
(gdb)
...

RFC: is the warning a good idea, or overkill?

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

diff --git a/gdb/source-cache.c b/gdb/source-cache.c
index 77b357cb42b..bd466869276 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,25 @@ 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");
+      }
+  }
+};
+
+/* Object with notify function called during highlighting.  */
+
+static gdb_highlight_event_listener the_gdb_highlight_event_listener;
+
 #endif /* HAVE_SOURCE_HIGHLIGHT */
 
 /* See source-cache.h.  */
@@ -247,6 +267,8 @@ source_cache::ensure (struct symtab *s)
 		{
 		  highlighter = new srchilite::SourceHighlight ("esc.outlang");
 		  highlighter->setStyleFile ("esc.style");
+		  highlighter->setHighlightEventListener
+		    (&the_gdb_highlight_event_listener);
 		}
 
 	      std::istringstream input (contents);
@@ -255,6 +277,11 @@ source_cache::ensure (struct symtab *s)
 	      contents = output.str ();
 	      already_styled = true;
 	    }
+	  catch (const gdb_exception_quit &ex)
+	    {
+	      warning (_("Failed to highlight source file %s: %s"),
+		       fullname.c_str (), ex.what ());
+	    }
 	  catch (...)
 	    {
 	      /* Source Highlight will throw an exception if

base-commit: f6ca448ab70c52e923b7010aecdf7be9c0d4d4fc
-- 
2.35.3

Re: [RFC] [gdb/cli] Allow source highlighting to be interrupted

[Tom Tromey]

>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:

Tom> Breakpoint 1, Solution::numOfSubarrays (this=0x7fffffffdbcf, arr=...) at test.cpp:56
Tom> ^Cwarning: Couldn't highlight source file /data/vries/gdb/test.cpp: Interrupted
Tom> 56	        return (int) t;
Tom> (gdb)
Tom> ...

Tom> RFC: is the warning a good idea, or overkill?

I'm on the fence, though I guess if it can't be done it might be nice if
gdb kept track of this and just didn't try highlighting that file again.

Tom> +class gdb_highlight_event_listener : public srchilite::HighlightEventListener
Tom> +{
Tom> +public:
Tom> +  void notify(const srchilite::HighlightEvent &event) override
Tom> +  {
Tom> +    if (check_quit_flag ())
Tom> +      {
Tom> +	/* Got SIGINT, interrupt highlighting, it may take too long.  */
Tom> +	throw_quit ("Interrupted");
Tom> +      }

Can the whole block just be "QUIT"?

Tom

Re: [RFC] [gdb/cli] Allow source highlighting to be interrupted

[Konstantin Kharlamov]

> RFC: is the warning a good idea, or overkill?

Yes, I think it is a good idea. I think a user pressing ^C too quickly
has to know they interrupted some ongoing process.

Re: [RFC] [gdb/cli] Allow source highlighting to be interrupted

[Tom de Vries]

On 10/11/23 19:02, Tom Tromey wrote:
>>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:
> 
> Tom> Breakpoint 1, Solution::numOfSubarrays (this=0x7fffffffdbcf, arr=...) at test.cpp:56
> Tom> ^Cwarning: Couldn't highlight source file /data/vries/gdb/test.cpp: Interrupted
> Tom> 56	        return (int) t;
> Tom> (gdb)
> Tom> ...
> 
> Tom> RFC: is the warning a good idea, or overkill?
> 
> I'm on the fence, though I guess if it can't be done it might be nice if
> gdb kept track of this and just didn't try highlighting that file again.
> 
> Tom> +class gdb_highlight_event_listener : public srchilite::HighlightEventListener
> Tom> +{
> Tom> +public:
> Tom> +  void notify(const srchilite::HighlightEvent &event) override
> Tom> +  {
> Tom> +    if (check_quit_flag ())
> Tom> +      {
> Tom> +	/* Got SIGINT, interrupt highlighting, it may take too long.  */
> Tom> +	throw_quit ("Interrupted");
> Tom> +      }
> 
> Can the whole block just be "QUIT"?

I tried that, and found that it doesn't work, which is why I came up 
with this.

QUIT calls maybe_quit:
...
void
maybe_quit (void)
{
   if (!is_main_thread ())
     return;

   if (sync_quit_force_run)
     quit ();

   quit_handler ();
}
...
and since sync_quit_force_run == false, it calls quit_handler, which is 
infrun_quit_handler. That one does nothing because 
target_terminal::is_ours ():
...
static void
infrun_quit_handler ()
{
   if (target_terminal::is_ours ())
     {
       /* Do nothing. 

 

          default_quit_handler would throw a quit in this case, but if 

          we're handling an event while we have the terminal, it means 

          the target is running a background execution command, and 

          thus when users press Ctrl-C, they're wanting to interrupt 

          whatever command they were executing in the command line. 

          E.g.: 

 

           (gdb) c& 

           (gdb) foo bar whatever<ctrl-c> 

 

          That Ctrl-C should clear the input line, not interrupt event 

          handling if it happens that the user types Ctrl-C at just the 

          "wrong" time! 

 

          It's as-if background event handling was handled by a 

          separate background thread. 

 

          To be clear, the Ctrl-C is not lost -- it will be processed 

          by the next QUIT call once we're out of fetch_inferior_event 

          again.  */
     }
...

The highlight interruption scenario is not covered by the comment.  So 
maybe that means that the function needs to be extended to handle it. 
Alternatively, maybe it means that we need to have a different quit 
handler while doing the highlighting.

[ cc-ing Pedro since this is ^C related. ]

Thanks,
- Tom

Re: [RFC] [gdb/cli] Allow source highlighting to be interrupted

[Tom de Vries]

On 10/11/23 19:02, Tom Tromey wrote:
>>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:
> 
> Tom> Breakpoint 1, Solution::numOfSubarrays (this=0x7fffffffdbcf, arr=...) at test.cpp:56
> Tom> ^Cwarning: Couldn't highlight source file /data/vries/gdb/test.cpp: Interrupted
> Tom> 56	        return (int) t;
> Tom> (gdb)
> Tom> ...
> 
> Tom> RFC: is the warning a good idea, or overkill?
> 
> I'm on the fence,

Hmm, I just realized that the line can be (and in the setup I'm looking 
at actually is) highlighted, not by GNU source-hightlight but by python 
pygments, which makes the warning in the current form confusing.

> though I guess if it can't be done it might be nice if
> gdb kept track of this and just didn't try highlighting that file again.
> 

Sounds doable.

Thanks,
- Tom

Re: [RFC] [gdb/cli] Allow source highlighting to be interrupted

[Tom de Vries]

On 10/11/23 23:56, Tom de Vries wrote:
> On 10/11/23 19:02, Tom Tromey wrote:
>>>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:
>>
>> Tom> Breakpoint 1, Solution::numOfSubarrays (this=0x7fffffffdbcf, 
>> arr=...) at test.cpp:56
>> Tom> ^Cwarning: Couldn't highlight source file 
>> /data/vries/gdb/test.cpp: Interrupted
>> Tom> 56            return (int) t;
>> Tom> (gdb)
>> Tom> ...
>>
>> Tom> RFC: is the warning a good idea, or overkill?
>>
>> I'm on the fence,
> 
> Hmm, I just realized that the line can be (and in the setup I'm looking 
> at actually is) highlighted, not by GNU source-hightlight but by python 
> pygments, which makes the warning in the current form confusing.
> 
>> though I guess if it can't be done it might be nice if
>> gdb kept track of this and just didn't try highlighting that file again.
>>
> 
> Sounds doable.
> 

I've submitted a patch series ( 
https://sourceware.org/pipermail/gdb-patches/2023-October/203179.html ) 
that:
- drops the warning, and
- adds the tracking of files for which highlighting fails.

Thanks,
- Tom

Re: [RFC] [gdb/cli] Allow source highlighting to be interrupted

[Tom de Vries]

On 10/11/23 20:11, Tom de Vries wrote:
> On 10/11/23 19:02, Tom Tromey wrote:
>>>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:
>>
>> Tom> Breakpoint 1, Solution::numOfSubarrays (this=0x7fffffffdbcf, 
>> arr=...) at test.cpp:56
>> Tom> ^Cwarning: Couldn't highlight source file 
>> /data/vries/gdb/test.cpp: Interrupted
>> Tom> 56            return (int) t;
>> Tom> (gdb)
>> Tom> ...
>>
>> Tom> RFC: is the warning a good idea, or overkill?
>>
>> I'm on the fence, though I guess if it can't be done it might be nice if
>> gdb kept track of this and just didn't try highlighting that file again.
>>
>> Tom> +class gdb_highlight_event_listener : public 
>> srchilite::HighlightEventListener
>> Tom> +{
>> Tom> +public:
>> Tom> +  void notify(const srchilite::HighlightEvent &event) override
>> Tom> +  {
>> Tom> +    if (check_quit_flag ())
>> Tom> +      {
>> Tom> +    /* Got SIGINT, interrupt highlighting, it may take too 
>> long.  */
>> Tom> +    throw_quit ("Interrupted");
>> Tom> +      }
>>
>> Can the whole block just be "QUIT"?
> 
> I tried that, and found that it doesn't work, which is why I came up 
> with this.
> 
> QUIT calls maybe_quit:
> ...
> void
> maybe_quit (void)
> {
>    if (!is_main_thread ())
>      return;
> 
>    if (sync_quit_force_run)
>      quit ();
> 
>    quit_handler ();
> }
> ...
> and since sync_quit_force_run == false, it calls quit_handler, which is 
> infrun_quit_handler. That one does nothing because 
> target_terminal::is_ours ():
> ...
> static void
> infrun_quit_handler ()
> {
>    if (target_terminal::is_ours ())
>      {
>        /* Do nothing.
> 
> 
>           default_quit_handler would throw a quit in this case, but if
>           we're handling an event while we have the terminal, it means
>           the target is running a background execution command, and
>           thus when users press Ctrl-C, they're wanting to interrupt
>           whatever command they were executing in the command line.
>           E.g.:
> 
> 
>            (gdb) c&
>            (gdb) foo bar whatever<ctrl-c>
> 
> 
>           That Ctrl-C should clear the input line, not interrupt event
>           handling if it happens that the user types Ctrl-C at just the
>           "wrong" time!
> 
> 
>           It's as-if background event handling was handled by a
>           separate background thread.
> 
> 
>           To be clear, the Ctrl-C is not lost -- it will be processed
>           by the next QUIT call once we're out of fetch_inferior_event
>           again.  */
>      }
> ...
> 
> The highlight interruption scenario is not covered by the comment.  So 
> maybe that means that the function needs to be extended to handle it. 
> Alternatively, maybe it means that we need to have a different quit 
> handler while doing the highlighting.
> 
> [ cc-ing Pedro since this is ^C related. ]
> 

In the v2 ( 
https://sourceware.org/pipermail/gdb-patches/2023-October/203198.html ) 
I managed to use QUIT by temporarily installing the default_quit_handler.

I hope that that's the appropriate way of handling this.

Thanks,
- Tom