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

[PATCH v4 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 adds a source highlighting self test.

The 2nd patch allows source highlighting to be interrupted when
running a list command, either using SIGINT or SIGTERM, or a run
command using SIGTERM.

The 3rd patch allows source highlighting to be interrupted when
running a run command, using SIGINT, fixing PR cli/30934.

The 4th patch changes the mode in which source highlighting is interrupted:
asking the user for confirmation that source highlighting should indeed be
interrupted, and if so only interrupting highlighting, and not the command.

Changes in v4:
- committed the first 3 patches of v3
- added handling of SIGTERM (after comment by Pedro)
- added self test
- broadened scope of quit_handler = default_quit_handler fix, and
  moved it into a patch of its own
- moved the question into a patch of its own

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
- v3:
  https://sourceware.org/pipermail/gdb-patches/2023-October/203270.html

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

Tom de Vries (4):
  [gdb/cli] Add gnu-source-highlight selftest
  [gdb/cli] Allow source highlighting to be interrupted
  [gdb/cli] Allow source highlighting to be interrupted (continued)
  [gdb/cli] Ask if source highlighting should be interrupted

 gdb/infrun.c       |  23 ++++--
 gdb/source-cache.c | 186 ++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 202 insertions(+), 7 deletions(-)


base-commit: 29736fc507c7a9c6e797b7f83e8df4be73d37767
-- 
2.35.3

[PATCH v4 1/4] [gdb/cli] Add gnu-source-highlight selftest

[Tom de Vries]

Add a selftest gnu-source-highlight:
...
$ gdb -q -batch -ex "maint selftest gnu-source-highlight"
Running selftest gnu-source-highlight.
Ran 1 unit tests, 0 failed
...

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

diff --git a/gdb/source-cache.c b/gdb/source-cache.c
index 99be3334803..7d8339a16df 100644
--- a/gdb/source-cache.c
+++ b/gdb/source-cache.c
@@ -39,6 +39,10 @@
 #include <srchilite/langmap.h>
 #endif
 
+#if GDB_SELF_TEST
+#include "gdbsupport/block-signals.h"
+#endif
+
 /* The number of source files we'll cache.  */
 
 #define MAX_ENTRIES 5
@@ -244,6 +248,62 @@ try_source_highlight (std::string &contents ATTRIBUTE_UNUSED,
 #endif /* HAVE_SOURCE_HIGHLIGHT */
 }
 
+#ifdef HAVE_SOURCE_HIGHLIGHT
+#if GDB_SELF_TEST
+namespace selftests
+{
+static void gnu_source_highlight_test ()
+{
+  const std::string prog
+    = ("int\n"
+       "foo (void)\n"
+       "{\n"
+       "  return 0;\n"
+       "}\n");
+  const std::string fullname = "test.c";
+  std::string styled_prog;
+
+  /* Make sure that the user can't raise SIGTERM and SIGINT.  */
+  gdb::block_signals blocker;
+
+  /* If SIGINT or SIGTERM is pending, handle it as usual.  */
+  QUIT;
+  gdb_assert (!sync_quit_force_run);
+
+  /* In some cases, QUIT doesn't handle SIGINT.  If so, bail out.  */
+  if (check_quit_flag ())
+    {
+      set_quit_flag ();
+      return;
+    }
+  gdb_assert (!check_quit_flag ());
+
+  bool res = false;
+  bool saw_exception = false;
+  styled_prog = prog;
+  try
+    {
+      res = try_source_highlight (styled_prog, language_c, fullname);
+    }
+  catch (...)
+    {
+      saw_exception = true;
+    }
+
+  SELF_CHECK (!saw_exception);
+  if (res)
+    SELF_CHECK (prog.size () < styled_prog.size ());
+  else
+    SELF_CHECK (prog == styled_prog);
+
+  /* Make sure there are no SIGINT or SIGTERM pending.  */
+  sync_quit_force_run = false;
+  check_quit_flag ();
+}
+}
+#endif /* GDB_SELF_TEST */
+#endif /* HAVE_SOURCE_HIGHLIGHT */
+
 /* See source-cache.h.  */
 
 bool
@@ -475,5 +535,9 @@ styling to source code lines that are shown."),
 
 #if GDB_SELF_TEST
   selftests::register_test ("source-cache", selftests::extract_lines_test);
+#ifdef HAVE_SOURCE_HIGHLIGHT
+  selftests::register_test ("gnu-source-highlight",
+			    selftests::gnu_source_highlight_test);
+#endif
 #endif
 }
-- 
2.35.3

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

[Tom de Vries]

Currently highlighting by the GNU source-highlight library cannot be
interrupted by either SIGINT or SIGTERM.

Fix this by installing a srchilite::HighlightEventListener that:
- checks whether SIGINT or SIGTERM was issued, and if so
- interrupts the command.

Interrupting the highlighting with SIGINT (using ^C) looks like this to the
user:
...
$ gdb -q a.out
Reading symbols from a.out...
(gdb) list
^CQuit
(gdb)
...
and with SIGTERM (using kill -SIGTERM $(pidof gdb)):
...
$ gdb -q a.out
Reading symbols from a.out...
(gdb) list
$
...

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

diff --git a/gdb/source-cache.c b/gdb/source-cache.c
index 7d8339a16df..f04a9d42e0d 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
 
 #if GDB_SELF_TEST
@@ -193,6 +194,17 @@ 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
+  {
+    QUIT;
+  }
+};
+
 #endif /* HAVE_SOURCE_HIGHLIGHT */
 
 /* Try to highlight CONTENTS from file FULLNAME in language LANG using
@@ -225,6 +237,9 @@ try_source_highlight (std::string &contents ATTRIBUTE_UNUSED,
 	{
 	  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);
@@ -233,6 +248,16 @@ try_source_highlight (std::string &contents ATTRIBUTE_UNUSED,
       contents = std::move (output).str ();
       styled = true;
     }
+  catch (const gdb_exception_forced_quit &)
+    {
+      /* SIGTERM, rethrow.  */
+      throw;
+    }
+  catch (const gdb_exception_quit &)
+    {
+      /* SIGINT, rethrow.  */
+      throw;
+    }
   catch (...)
     {
       /* Source Highlight will throw an exception if
@@ -299,6 +324,51 @@ static void gnu_source_highlight_test ()
   /* Make sure there are no SIGINT or SIGTERM pending.  */
   sync_quit_force_run = false;
   check_quit_flag ();
+
+  /* Pretend user send SIGTERM.  */
+  set_force_quit_flag ();
+
+  bool saw_forced_quit = false;
+  res = false;
+  styled_prog = prog;
+  try
+    {
+      res = try_source_highlight (styled_prog, language_c, "test.c");
+    }
+  catch (const gdb_exception_forced_quit &ex)
+    {
+      saw_forced_quit = true;
+    }
+
+  SELF_CHECK (saw_forced_quit);
+  SELF_CHECK (!res);
+  SELF_CHECK (prog == styled_prog);
+
+  /* Make sure there are no SIGINT or SIGTERM pending.  */
+  sync_quit_force_run = false;
+  check_quit_flag ();
+
+  /* Pretend user send SIGINT.  */
+  set_quit_flag ();
+
+  bool saw_quit = false;
+  res = false;
+  styled_prog = prog;
+  try
+    {
+      res = try_source_highlight (styled_prog, language_c, "test.c");
+    }
+  catch (const gdb_exception_quit &ex)
+    {
+      saw_quit = true;
+    }
+  SELF_CHECK (saw_quit);
+  SELF_CHECK (!res);
+  SELF_CHECK (prog == styled_prog);
+
+  /* Make sure there are no SIGINT or SIGTERM pending.  */
+  sync_quit_force_run = false;
+  check_quit_flag ();
 }
 }
 #endif /* GDB_SELF_TEST */
-- 
2.35.3

[PATCH v4 3/4] [gdb/cli] Allow source highlighting to be interrupted (continued)

[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:
...
$ gdb -q a.out -ex "b 56"
Reading symbols from a.out...
Breakpoint 1 at 0x400e2a: file test.cpp, line 56.
(gdb) run
Starting program: /data/vries/gdb/a.out

Breakpoint 1, Solution::numOfSubarrays () at test.cpp:56
<... wait for it ...>
56	        return (int) t;
(gdb)
...

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.

The previous patch allows us to interrupt a list command during highlighting
by either SIGTERM or SIGINT.

In this case, we can use SIGTERM to interrupt the run command, but not SIGINT.

This is due to the fact that the infrun_quit_handler is still active.

The purpose of infrun_quit_handler is to:
- if !target_terminal::is_ours, pass ^C to the inferior, and
- if target_terminal::is_ours, ignore ^C.  If gdb is executing say a continue
  in the background, it generates events and we don't want to interrupt the
  handling of these events.

In this case however, the reason target_terminal::is_ours is not because
there's background execution, but because normal_stop has decided that we have
stopped execution, and it's preparing to hand back control to GDB.

Fix this by switching back to the default_quit_handler during notify_normal_stop.

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/infrun.c | 23 +++++++++++++++++------
 1 file changed, 17 insertions(+), 6 deletions(-)

diff --git a/gdb/infrun.c b/gdb/infrun.c
index 4fde96800fb..816a76a1b2c 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -9063,12 +9063,23 @@ normal_stop ()
   if (saved_context.changed ())
     return true;
 
-  /* Notify observers about the stop.  This is where the interpreters
-     print the stop event.  */
-  notify_normal_stop ((inferior_ptid != null_ptid
-		       ? inferior_thread ()->control.stop_bpstat
-		       : nullptr),
-		      stop_print_frame);
+  {
+    /* 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
+       no need for this behaviour anymore, and we want to be able to interrupt
+       notify_normal_stop, so install the default_quit_handler.  */
+    scoped_restore restore_quit_handler
+      = make_scoped_restore (&quit_handler, default_quit_handler);
+
+    /* Notify observers about the stop.  This is where the interpreters
+       print the stop event.  */
+    notify_normal_stop ((inferior_ptid != null_ptid
+			 ? inferior_thread ()->control.stop_bpstat
+			 : nullptr),
+			stop_print_frame);
+  }
+
   annotate_stopped ();
 
   if (target_has_execution ())
-- 
2.35.3

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

[Tom de Vries]

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

diff --git a/gdb/source-cache.c b/gdb/source-cache.c
index f04a9d42e0d..b742478c4c8 100644
--- a/gdb/source-cache.c
+++ b/gdb/source-cache.c
@@ -42,6 +42,7 @@
 
 #if GDB_SELF_TEST
 #include "gdbsupport/block-signals.h"
+#include "top.h"
 #endif
 
 /* 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 ("");
+      }
+
+    /* 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
+       checking the quit flag, such that we don't rely on the user pressing
+       ^C so detect this.  */
+    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?"),
+		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 */
@@ -237,11 +279,14 @@ try_source_highlight (std::string &contents ATTRIBUTE_UNUSED,
 	{
 	  highlighter = new srchilite::SourceHighlight ("esc.outlang");
 	  highlighter->setStyleFile ("esc.style");
-
-	  static gdb_highlight_event_listener event_listener;
-	  highlighter->setHighlightEventListener (&event_listener);
 	}
 
+      gdb_highlight_event_listener event_listener (fullname);
+      highlighter->setHighlightEventListener (&event_listener);
+      /* Make sure that the highlighter's EventListener doesn't become a
+	 dangling pointer.  */
+      SCOPE_EXIT { highlighter->setHighlightEventListener (nullptr); };
+
       std::istringstream input (contents);
       std::ostringstream output;
       highlighter->highlight (input, output, lang_name, fullname);
@@ -255,8 +300,7 @@ try_source_highlight (std::string &contents ATTRIBUTE_UNUSED,
     }
   catch (const gdb_exception_quit &)
     {
-      /* SIGINT, rethrow.  */
-      throw;
+      /* SIGINT, ignore.  */
     }
   catch (...)
     {
@@ -348,6 +392,9 @@ static void gnu_source_highlight_test ()
   sync_quit_force_run = false;
   check_quit_flag ();
 
+  scoped_restore save_confirm
+    = make_scoped_restore (&confirm, false);
+
   /* Pretend user send SIGINT.  */
   set_quit_flag ();
 
@@ -362,7 +409,7 @@ static void gnu_source_highlight_test ()
     {
       saw_quit = true;
     }
-  SELF_CHECK (saw_quit);
+  SELF_CHECK (!saw_quit);
   SELF_CHECK (!res);
   SELF_CHECK (prog == styled_prog);
 
@@ -434,13 +481,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 v4 3/4] [gdb/cli] Allow source highlighting to be interrupted (continued)

[Pedro Alves]

On 2023-10-18 18:11, 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:
> ...
> $ gdb -q a.out -ex "b 56"
> Reading symbols from a.out...
> Breakpoint 1 at 0x400e2a: file test.cpp, line 56.
> (gdb) run
> Starting program: /data/vries/gdb/a.out
> 
> Breakpoint 1, Solution::numOfSubarrays () at test.cpp:56
> <... wait for it ...>
> 56	        return (int) t;
> (gdb)
> ...
> 
> 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.
> 
> The previous patch allows us to interrupt a list command during highlighting
> by either SIGTERM or SIGINT.
> 
> In this case, we can use SIGTERM to interrupt the run command, but not SIGINT.
> 
> This is due to the fact that the infrun_quit_handler is still active.
> 
> The purpose of infrun_quit_handler is to:
> - if !target_terminal::is_ours, pass ^C to the inferior, and
> - if target_terminal::is_ours, ignore ^C.  If gdb is executing say a continue
>   in the background, it generates events and we don't want to interrupt the
>   handling of these events.
> 
> In this case however, the reason target_terminal::is_ours is not because
> there's background execution, but because normal_stop has decided that we have
> stopped execution, and it's preparing to hand back control to GDB.
> 
> Fix this by switching back to the default_quit_handler during notify_normal_stop.

Hmm.  This worries me.  GDB is preparing to hand back control, but it isn't
yet done doing that.  E.g., at the bottom of normal_stop, after notifying the
interpreters, we handle breakpoint auto delete.  And then the caller of normal_stop
still has run control work to do which we better not skip.  By switching to
the default_quit_handler in normal_stop, we risk a Ctrl-C running into a QUIT
and resulting in skipping all that code.  Similarly, imagine if the MI "normal_stop"
observer runs into a QUIT and throws due to a Ctrl-C, and that makes GDB not print
the *stopped async record or some other bit of important info for the frontend.
That would leave the frontend out of sync.

Also, overriding the overridden quit_handler is a bit of code smell.  We can't
be sure that the right handler is the default_quit_handler.  Better would be to force
the infrun_quit_handler override scoped_restore dtor (e.g., by wrapping that
scoped_restore in an optional, and passing down a reference to that so we could
clear the optional.)  But that would likely look not-so-pretty.

It just seems way safer to do this, to drop this patch.  This means not using QUIT in
the next patch after all...  I will reply to that patch next.

Pedro Alves

Re: [PATCH v4 4/4] [gdb/cli] Ask if source highlighting should be interrupted

[Pedro Alves]

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

Re: [PATCH v4 3/4] [gdb/cli] Allow source highlighting to be interrupted (continued)

[Tom de Vries]

On 10/20/23 21:16, Pedro Alves wrote:
> On 2023-10-18 18:11, 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:
>> ...
>> $ gdb -q a.out -ex "b 56"
>> Reading symbols from a.out...
>> Breakpoint 1 at 0x400e2a: file test.cpp, line 56.
>> (gdb) run
>> Starting program: /data/vries/gdb/a.out
>>
>> Breakpoint 1, Solution::numOfSubarrays () at test.cpp:56
>> <... wait for it ...>
>> 56	        return (int) t;
>> (gdb)
>> ...
>>
>> 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.
>>
>> The previous patch allows us to interrupt a list command during highlighting
>> by either SIGTERM or SIGINT.
>>
>> In this case, we can use SIGTERM to interrupt the run command, but not SIGINT.
>>
>> This is due to the fact that the infrun_quit_handler is still active.
>>
>> The purpose of infrun_quit_handler is to:
>> - if !target_terminal::is_ours, pass ^C to the inferior, and
>> - if target_terminal::is_ours, ignore ^C.  If gdb is executing say a continue
>>    in the background, it generates events and we don't want to interrupt the
>>    handling of these events.
>>
>> In this case however, the reason target_terminal::is_ours is not because
>> there's background execution, but because normal_stop has decided that we have
>> stopped execution, and it's preparing to hand back control to GDB.
>>
>> Fix this by switching back to the default_quit_handler during notify_normal_stop.
> 
> Hmm.  This worries me.  GDB is preparing to hand back control, but it isn't
> yet done doing that.  E.g., at the bottom of normal_stop, after notifying the
> interpreters, we handle breakpoint auto delete.  And then the caller of normal_stop
> still has run control work to do which we better not skip.  By switching to
> the default_quit_handler in normal_stop, we risk a Ctrl-C running into a QUIT
> and resulting in skipping all that code.  Similarly, imagine if the MI "normal_stop"
> observer runs into a QUIT and throws due to a Ctrl-C, and that makes GDB not print
> the *stopped async record or some other bit of important info for the frontend.
> That would leave the frontend out of sync.
> 
> Also, overriding the overridden quit_handler is a bit of code smell.  We can't
> be sure that the right handler is the default_quit_handler.  Better would be to force
> the infrun_quit_handler override scoped_restore dtor (e.g., by wrapping that
> scoped_restore in an optional, and passing down a reference to that so we could
> clear the optional.)  But that would likely look not-so-pretty.
> 
> It just seems way safer to do this, to drop this patch.  This means not using QUIT in
> the next patch after all...  I will reply to that patch next.

Ack, dropped in v5 ( 
https://sourceware.org/pipermail/gdb-patches/2023-October/203505.html ).

Thanks,
- Tom

Re: [PATCH v4 4/4] [gdb/cli] Ask if source highlighting should be interrupted

[Tom de Vries]

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