[v4,4/4,gdb/cli] Ask if source highlighting should be interrupted

  When source highlighting is done using the GNU source-highlight library,
and it takes a long time, it can be interrupted using ^C, but that cancels the
rest of the command:
...
$ 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
^C
(gdb)
...

That means in this case that the user doesn't get to see the line gdb stopped
at.

This is especially problematic if the user actually meant to interrupt the
inferior execution.

Instead, ask the user whether highlighting needs to be interrupted, and if so,
interrupt only the highlighting part, and continue with the command:
...
$ 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 after cancelling, line 56 still can be highlighted, just by pygments
instead of source-highlight.

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

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

Hi,

So I've stared and played with this for a few hours today, and I'm convinced that avoiding
the QUIT macro/quit_handler mechanism here is best.  It avoids the troubles I mentioned in the
previous patch.  More below.

On 2023-10-18 18:11, Tom de Vries wrote:

>  /* The number of source files we'll cache.  */
> @@ -199,10 +200,51 @@ get_language_name (enum language lang)
>  class gdb_highlight_event_listener : public srchilite::HighlightEventListener
>  {
>  public:
> +  explicit gdb_highlight_event_listener (const std::string &fullname)
> +    : m_fullname (fullname)
> +  {
> +  }
> +
>    void notify (const srchilite::HighlightEvent &event) override
>    {
> +    if (sync_quit_force_run)
> +      {
> +	/* Handle SIGTERM.  */
> +	QUIT;
> +	gdb_assert_not_reached ("");
> +      }

I missed this before, but it turns out that if we don't check for SIGTERM explicitly,
it'll still work out OK, because yquery internally starts a secondary prompt, and runs
a nested event loop to handle newline.  That runs the pending async signal handlers.
I.e., if you get a SIGTERM before the query, the query call ends up quitting GDB.

That doesn't happen during the unit test, because the unit test doesn't emulate
the SIGTERM handler completely.  The SIGTERM handler calls:

  mark_async_signal_handler (async_sigterm_token);

We can't do that in the unit tests, because that would cause the SIGTERM unit test
to exit GDB...  Ask me how I know.  :-)

+    int resp
+      = yquery (_("Cancel source styling using GNU source highlight of %s?"),
+		m_fullname.c_str ());

I noticed this printed:

 Cancel source styling using GNU source highlight of /tmp/test.cpp?([y] or n)

Queries normally put that "(y or no)" in the same line _and_ add a space in between.
Like so:

 Cancel source styling using GNU source highlight of /tmp/test.cpp? ([y] or n)
                                                                   ^

So...  here's a patch on top of yours showing what I'm thinking we should do.

WDYT?

From 22bb476d59dac611f38ce57fcada552e9a1ed2ce Mon Sep 17 00:00:00 2001
From: Pedro Alves <pedro@palves.net>
Date: Fri, 20 Oct 2023 16:56:49 +0100
Subject: [PATCH] no QUIT

Change-Id: I42e35273a66c2d6658be4a53e84e5c609bf60c1f
---
 gdb/infrun.c       |  2 ++
 gdb/source-cache.c | 43 ++++++++++++++++++++++++-------------------
 2 files changed, 26 insertions(+), 19 deletions(-)

diff --git a/gdb/infrun.c b/gdb/infrun.c
index 816a76a1b2c..537fccf5ffc 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -9064,6 +9064,7 @@ normal_stop ()
     return true;
 
   {
+#if 0
     /* If the current quit_handler is infrun_quit_handler, and
        target_terminal::is_ours, pressing ^C is ignored by QUIT.  See
        infrun_quit_handler for an explanation.  At this point, there's
@@ -9071,6 +9072,7 @@ normal_stop ()
        notify_normal_stop, so install the default_quit_handler.  */
     scoped_restore restore_quit_handler
       = make_scoped_restore (&quit_handler, default_quit_handler);
+#endif
 
     /* Notify observers about the stop.  This is where the interpreters
        print the stop event.  */
diff --git a/gdb/source-cache.c b/gdb/source-cache.c
index b742478c4c8..b51282431f3 100644
--- a/gdb/source-cache.c
+++ b/gdb/source-cache.c
@@ -207,13 +207,6 @@ class gdb_highlight_event_listener : public srchilite::HighlightEventListener
 
   void notify (const srchilite::HighlightEvent &event) override
   {
-    if (sync_quit_force_run)
-      {
-	/* Handle SIGTERM.  */
-	QUIT;
-	gdb_assert_not_reached ("");
-      }
-
     /* If target_terminal::is_ours, we can ask the user a question and get an
        answer.  We're not sure if or how !target_terminal::is_ours can happen.
        Assert to catch it happening.  Note that we're asserting before
@@ -227,20 +220,32 @@ class gdb_highlight_event_listener : public srchilite::HighlightEventListener
 	return;
       }
 
-    /* Ask the user what to do.  */
-    int resp
-      = yquery (_("Cancel source styling using GNU source highlight of %s?"),
-		m_fullname.c_str ());
-    if (!resp)
+    /* If we got a SIGTERM, skip querying.  This isn't strictly
+       necessary as yquery runs a nested event loop for the secondary
+       prompt, which runs pending async signal handlers.  However,
+       this helps with the unit test, which makes sure that the quit
+       call at the bottom throws a gdb_exception_forced_quit exception
+       and that our caller doesn't swallow it.  Note we may receive a
+       SIGTERM in between the query and the quit call.  */
+    if (!sync_quit_force_run)
       {
-	/* Continue highlighting.  */
-	return;
+	/* Ask the user what to do.  */
+	int resp
+	  = (yquery
+	     (_("Cancel source styling using GNU source highlight of %s? "),
+	      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;
+    /* Interrupt highlighting.  Note we don't abort via the QUIT macro
+       as that may do nothing.  E.g. if the current quit_handler is
+       infrun_quit_handler, and target_terminal::is_ours, pressing ^C
+       is ignored by QUIT.  */
+    quit ();
   }
 
 private:
@@ -369,7 +374,7 @@ static void gnu_source_highlight_test ()
   sync_quit_force_run = false;
   check_quit_flag ();
 
-  /* Pretend user send SIGTERM.  */
+  /* Pretend user sent SIGTERM.  */
   set_force_quit_flag ();
 
   bool saw_forced_quit = false;

base-commit: d605374748fef3d3b1dea713e78bbef9c8b0fb65
prerequisite-patch-id: 60f8451db717747d5edc39816e0ede52491f4b7a
prerequisite-patch-id: 83b3e8420ad3803d15be15e2b98aba59030c90df
prerequisite-patch-id: 51ff9fec1b39153b7b8c47fc52be9581d8da56bc
prerequisite-patch-id: df7879fe8ab505311863915e5aca0081684e97b6
  

On 10/20/23 21:17, Pedro Alves wrote:
> Hi,
> 
> So I've stared and played with this for a few hours today, and I'm convinced that avoiding
> the QUIT macro/quit_handler mechanism here is best.  It avoids the troubles I mentioned in the
> previous patch.  More below.
> 
> On 2023-10-18 18:11, Tom de Vries wrote:
> 
>>   /* The number of source files we'll cache.  */
>> @@ -199,10 +200,51 @@ get_language_name (enum language lang)
>>   class gdb_highlight_event_listener : public srchilite::HighlightEventListener
>>   {
>>   public:
>> +  explicit gdb_highlight_event_listener (const std::string &fullname)
>> +    : m_fullname (fullname)
>> +  {
>> +  }
>> +
>>     void notify (const srchilite::HighlightEvent &event) override
>>     {
>> +    if (sync_quit_force_run)
>> +      {
>> +	/* Handle SIGTERM.  */
>> +	QUIT;
>> +	gdb_assert_not_reached ("");
>> +      }
> 
> I missed this before, but it turns out that if we don't check for SIGTERM explicitly,
> it'll still work out OK, because yquery internally starts a secondary prompt, and runs
> a nested event loop to handle newline.  That runs the pending async signal handlers.
> I.e., if you get a SIGTERM before the query, the query call ends up quitting GDB.
> 
> That doesn't happen during the unit test, because the unit test doesn't emulate
> the SIGTERM handler completely.  The SIGTERM handler calls:
> 
>    mark_async_signal_handler (async_sigterm_token);
> 
> We can't do that in the unit tests, because that would cause the SIGTERM unit test
> to exit GDB...  Ask me how I know.  :-)

Heh :)

> +    int resp
> +      = yquery (_("Cancel source styling using GNU source highlight of %s?"),
> +		m_fullname.c_str ());
> 
> I noticed this printed:
> 
>   Cancel source styling using GNU source highlight of /tmp/test.cpp?([y] or n)
> 
> Queries normally put that "(y or no)" in the same line _and_ add a space in between.
> Like so:
> 
>   Cancel source styling using GNU source highlight of /tmp/test.cpp? ([y] or n)
>                                                                     ^
> 
> So...  here's a patch on top of yours showing what I'm thinking we should do.
> 
> WDYT?
> 
>  From 22bb476d59dac611f38ce57fcada552e9a1ed2ce Mon Sep 17 00:00:00 2001
> From: Pedro Alves <pedro@palves.net>
> Date: Fri, 20 Oct 2023 16:56:49 +0100
> Subject: [PATCH] no QUIT
> 
> Change-Id: I42e35273a66c2d6658be4a53e84e5c609bf60c1f
> ---
>   gdb/infrun.c       |  2 ++
>   gdb/source-cache.c | 43 ++++++++++++++++++++++++-------------------
>   2 files changed, 26 insertions(+), 19 deletions(-)
> 
> diff --git a/gdb/infrun.c b/gdb/infrun.c
> index 816a76a1b2c..537fccf5ffc 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -9064,6 +9064,7 @@ normal_stop ()
>       return true;
>   
>     {
> +#if 0
>       /* If the current quit_handler is infrun_quit_handler, and
>          target_terminal::is_ours, pressing ^C is ignored by QUIT.  See
>          infrun_quit_handler for an explanation.  At this point, there's
> @@ -9071,6 +9072,7 @@ normal_stop ()
>          notify_normal_stop, so install the default_quit_handler.  */
>       scoped_restore restore_quit_handler
>         = make_scoped_restore (&quit_handler, default_quit_handler);
> +#endif
>   
>       /* Notify observers about the stop.  This is where the interpreters
>          print the stop event.  */

I've dropped the patch in v5 ( 
https://sourceware.org/pipermail/gdb-patches/2023-October/203505.html ) .

> diff --git a/gdb/source-cache.c b/gdb/source-cache.c
> index b742478c4c8..b51282431f3 100644
> --- a/gdb/source-cache.c
> +++ b/gdb/source-cache.c
> @@ -207,13 +207,6 @@ class gdb_highlight_event_listener : public srchilite::HighlightEventListener
>   
>     void notify (const srchilite::HighlightEvent &event) override
>     {
> -    if (sync_quit_force_run)
> -      {
> -	/* Handle SIGTERM.  */
> -	QUIT;
> -	gdb_assert_not_reached ("");
> -      }
> -
>       /* If target_terminal::is_ours, we can ask the user a question and get an
>          answer.  We're not sure if or how !target_terminal::is_ours can happen.
>          Assert to catch it happening.  Note that we're asserting before
> @@ -227,20 +220,32 @@ class gdb_highlight_event_listener : public srchilite::HighlightEventListener
>   	return;
>         }
>   
> -    /* Ask the user what to do.  */
> -    int resp
> -      = yquery (_("Cancel source styling using GNU source highlight of %s?"),
> -		m_fullname.c_str ());
> -    if (!resp)
> +    /* If we got a SIGTERM, skip querying.  This isn't strictly
> +       necessary as yquery runs a nested event loop for the secondary
> +       prompt, which runs pending async signal handlers.  However,
> +       this helps with the unit test, which makes sure that the quit
> +       call at the bottom throws a gdb_exception_forced_quit exception
> +       and that our caller doesn't swallow it.  Note we may receive a
> +       SIGTERM in between the query and the quit call.  */
> +    if (!sync_quit_force_run)
>         {
> -	/* Continue highlighting.  */
> -	return;
> +	/* Ask the user what to do.  */
> +	int resp
> +	  = (yquery
> +	     (_("Cancel source styling using GNU source highlight of %s? "),
> +	      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;
> +    /* Interrupt highlighting.  Note we don't abort via the QUIT macro
> +       as that may do nothing.  E.g. if the current quit_handler is
> +       infrun_quit_handler, and target_terminal::is_ours, pressing ^C
> +       is ignored by QUIT.  */
> +    quit ();
>     }
>

Merged into the patch in full.

I've tried to fix the fact that we're working around a selftest issue by 
capturing the forced_quit call in the selftest.  This is an RFC for now, 
but AFAIU it allows the simplification you're proposing in the comment 
"If we got a SIGTERM, skip querying.  This isn't strictly necessary ...".

>   private:
> @@ -369,7 +374,7 @@ static void gnu_source_highlight_test ()
>     sync_quit_force_run = false;
>     check_quit_flag ();
>   
> -  /* Pretend user send SIGTERM.  */
> +  /* Pretend user sent SIGTERM.  */
>     set_force_quit_flag ();
>   
>     bool saw_forced_quit = false;
> 

Merged into an earlier patch.

Thanks,
- Tom