[PATCH v5 0/6] [gdb/cli] Allow source highlighting to be interrupted

[PATCH v5 0/6] [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 harness to allow selftests to call signal handlers.

The 2nd patch uses that infrastructure in the pre-existing python selftest.

The 3rd patch adds a source highlighting selftest.

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

The 5th patch allows source highlighting to be interrupted when
running a run command, using SIGINT, fixing PR cli/30934.  It also
changes the behaviour in case of SIGINT into asking a question whether
highlighting needs to be interrupted or not.

The 6th patch is an RFC for simplifying gdb_highlight_event_listener::notify,
at the cost of more complicated selftest infrastructure.

Changes in v5:
- factored out a signal_handler_selftest harness, and put it in a separate
  patch
- applied it in the pre-existing python selftest
- simplified the initial selftest patch to not deal with SIGTERM/SIGINT
- dropped the patch "[gdb/cli] Allow source highlighting to be interrupted
  (continued)" that fiddles with the current quit_handler, after comment
  by Pedro
- updated the "[gdb/cli] Ask if source highlighting should be interrupted"
  as suggested by Pedro, added as co-author.
- made the unit test emulate the signal handler more completely, by ...
  calling the signal handler.
- made the signal_handler_selftest harness handle async_sigterm_token and
  sigint_token (now required because of the previous point).
- added the final RFC patch to try to address the problem that
  gdb_highlight_event_listener::notify is working around a limitation of the
  selftest.

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 as suggested by Lancelot, added as co-author.

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

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

Tom de Vries (6):
  [gdb/cli] Add signal_handler_selftest
  [gdb/python] Make python selftest more robust
  [gdb/cli] Add gnu-source-highlight selftest
  [gdb/cli] Allow source highlighting to be interrupted
  [gdb/cli] Ask if source highlighting should be interrupted
  [gdb/cli] Simplify gdb_highlight_event_listener::notify

 gdb/event-top.c     |  95 ++++++++++++++++++++++++
 gdb/event-top.h     |  36 +++++++++
 gdb/python/python.c |   7 +-
 gdb/source-cache.c  | 177 +++++++++++++++++++++++++++++++++++++++++++-
 gdb/top.c           |   7 ++
 gdb/top.h           |   7 ++
 6 files changed, 326 insertions(+), 3 deletions(-)


base-commit: f87cf663af71e5d78c8d647fa48562102f3b0615
-- 
2.35.3

[PATCH v5 1/6] [gdb/cli] Add signal_handler_selftest

[Tom de Vries]

Add a harness to run signal_handler selftests.

It can be used by adding this in a selftest:
...
  {
    signal_handler_selftest a_signal_handler_selftest;
    if (!a_signal_handler_selftest.valid ())
      <bail out>;

    ...
  }
...

The harness does the following:
- blocks some signals, including SIGINT and SIGTERM
- handles pending SIGINT and SIGTERM before the selftest
- clears stray pending SIGINT and SIGTERM after the selftest
- saves and restores the SIGINT and SIGTERM handlers, allowing
  the unittest to call handlers which re-install themselves.

No user yet.

Note that it's not applicable for selftest scoped_ignore_sigpipe, where the
signal is raised externally to gdb.

Tested on x86_64-linux.
---
 gdb/event-top.c | 91 +++++++++++++++++++++++++++++++++++++++++++++++++
 gdb/event-top.h | 36 +++++++++++++++++++
 2 files changed, 127 insertions(+)

diff --git a/gdb/event-top.c b/gdb/event-top.c
index 3d6fa896a9c..8c01c0902ea 100644
--- a/gdb/event-top.c
+++ b/gdb/event-top.c
@@ -1164,6 +1164,97 @@ handle_sigterm (int sig)
   mark_async_signal_handler (async_sigterm_token);
 }
 
+#if GDB_SELF_TEST
+namespace selftests {
+
+/* Returns true if SIGTERM is pending.  */
+static bool
+pending_sigterm ()
+{
+  bool saved_quit_flag = check_quit_flag ();
+
+  /* If needed, restore quit_flag.  */
+  if (saved_quit_flag)
+    set_quit_flag ();
+
+  return ((saved_quit_flag && sync_quit_force_run)
+	  || async_signal_handler_is_marked (async_sigterm_token));
+}
+
+/* Clear pending SIGTERM.  */
+static void
+clear_pending_sigterm ()
+{
+  clear_async_signal_handler (async_sigterm_token);
+  sync_quit_force_run = false;
+  check_quit_flag ();
+}
+
+/* Returns true if SIGINT is pending.  */
+static bool
+pending_sigint ()
+{
+  bool saved_quit_flag = check_quit_flag ();
+
+  /* If needed, restore quit_flag.  */
+  if (saved_quit_flag)
+    set_quit_flag ();
+
+  return ((saved_quit_flag && !sync_quit_force_run)
+	  || async_signal_handler_is_marked (sigint_token));
+}
+
+/* Clear pending SIGINT.  */
+static void
+clear_pending_sigint ()
+{
+  clear_async_signal_handler (sigint_token);
+  check_quit_flag ();
+}
+
+signal_handler_selftest::signal_handler_selftest ()
+{
+  /* If SIGINT or SIGTERM is pending, try to handle it as usual.  */
+  QUIT;
+
+  /* Assert SIGTERM is not pending.  */
+  gdb_assert (!pending_sigterm ());
+
+  if (check_quit_flag ())
+    {
+      /* SIGINT is still pending.  This can happen because QUIT doesn't
+	 handle pending SIGINT in all cases.  Bail out.  */
+      set_quit_flag ();
+      return;
+    }
+
+  /* Assert SIGINT is not pending.  */
+  gdb_assert (!pending_sigint ());
+
+  /* Save signal handlers.  */
+  m_saved_sigint_handler = signal (SIGINT, SIG_IGN);
+  m_saved_sigterm_handler = signal (SIGTERM, SIG_IGN);
+
+  /* Only code from the selftest itself can cause the SIGINT or SIGTERM signal
+     handlers to run.  */
+  m_valid = true;
+}
+
+signal_handler_selftest::~signal_handler_selftest ()
+{
+  /* Make sure that no pending SIGINT or SIGTERM escapes from the
+     selftest.  */
+  clear_pending_sigint ();
+  clear_pending_sigterm ();
+
+  /* Restore saved signal handlers.  */
+  signal (SIGINT, m_saved_sigint_handler);
+  signal (SIGTERM, m_saved_sigterm_handler);
+}
+
+}
+#endif /* GDB_SELF_TEST */
+
 /* Do the quit.  All the checks have been done by the caller.  */
 void
 async_request_quit (gdb_client_data arg)
diff --git a/gdb/event-top.h b/gdb/event-top.h
index f7247f5c4f2..1cc93642dd8 100644
--- a/gdb/event-top.h
+++ b/gdb/event-top.h
@@ -24,6 +24,10 @@
 
 #include <signal.h>
 
+#if GDB_SELF_TEST
+#include "gdbsupport/block-signals.h"
+#endif
+
 struct cmd_list_element;
 
 /* Exported functions from event-top.c.
@@ -90,4 +94,36 @@ class scoped_segv_handler_restore
   segv_handler_t m_old_handler;
 };
 
+#if GDB_SELF_TEST
+namespace selftests {
+
+class signal_handler_selftest
+{
+public:
+  signal_handler_selftest ();
+  ~signal_handler_selftest ();
+
+  bool valid ()
+  {
+    return m_valid;
+  }
+
+private:
+  bool m_valid = false;
+
+  /* SIGINT Signal handler at construction, restored at destruction.  */
+  sighandler_t m_saved_sigint_handler;
+
+  /* SIGTERM Signal handler at construction, restored at destruction.  */
+  sighandler_t m_saved_sigterm_handler;
+
+  /* Make sure that SIGTERM and SIGINT are blocked during the selftest.  This
+     ensures that the user cannot send a signal that interferes with the
+     selftest.  */
+  gdb::block_signals m_blocker;
+};
+
+}
+#endif /* GDB_SELF_TEST */
+
 #endif
-- 
2.35.3

[PATCH v5 2/6] [gdb/python] Make python selftest more robust

[Tom de Vries]

When running the python selftest, we call raise directly:
...
    scoped_restore save_hook
      = make_scoped_restore (&hook_set_active_ext_lang,
			     []() { raise (SIGINT); });
...

While this tests the signal handling in its natural form, it runs the risk of
interference from other signals.

Instead:
- use signal_handler_selftest to isolate the selftest from user signals, and
- use "handle_sigint (SIGINT)" to deliver the signal.

Tested on x86_64-linux.
---
 gdb/python/python.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/gdb/python/python.c b/gdb/python/python.c
index d569fb5a3e4..00e8968d3ed 100644
--- a/gdb/python/python.c
+++ b/gdb/python/python.c
@@ -39,6 +39,7 @@
 
 #if GDB_SELF_TEST
 #include "gdbsupport/selftest.h"
+#include "event-top.h"
 #endif
 
 /* Declared constants and enum for python stack printing.  */
@@ -2246,11 +2247,15 @@ test_python ()
     SELF_CHECK (output.empty ());
   }
 
+  signal_handler_selftest a_signal_handler_selftest;
+  if (!a_signal_handler_selftest.valid ())
+    return;
+
   saw_exception = false;
   {
     scoped_restore save_hook
       = make_scoped_restore (&hook_set_active_ext_lang,
-			     []() { raise (SIGINT); });
+			     []() { handle_sigint (SIGINT); });
     try
       {
 	CMD (output);
-- 
2.35.3

[PATCH v5 3/6] [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 | 46 +++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 45 insertions(+), 1 deletion(-)

diff --git a/gdb/source-cache.c b/gdb/source-cache.c
index 92acb100901..c955929b543 100644
--- a/gdb/source-cache.c
+++ b/gdb/source-cache.c
@@ -22,7 +22,6 @@
 #include "source.h"
 #include "cli/cli-style.h"
 #include "symtab.h"
-#include "gdbsupport/selftest.h"
 #include "objfiles.h"
 #include "exec.h"
 #include "cli/cli-cmds.h"
@@ -40,6 +39,10 @@
 #include <srchilite/settings.h>
 #endif
 
+#if GDB_SELF_TEST
+#include "gdbsupport/selftest.h"
+#endif
+
 /* The number of source files we'll cache.  */
 
 #define MAX_ENTRIES 5
@@ -258,6 +261,43 @@ 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;
+
+  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);
+}
+}
+#endif /* GDB_SELF_TEST */
+#endif /* HAVE_SOURCE_HIGHLIGHT */
+
 /* See source-cache.h.  */
 
 bool
@@ -489,5 +529,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 v5 4/6] [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 | 81 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 81 insertions(+)

diff --git a/gdb/source-cache.c b/gdb/source-cache.c
index c955929b543..2d59957f42c 100644
--- a/gdb/source-cache.c
+++ b/gdb/source-cache.c
@@ -37,10 +37,12 @@
 #include <srchilite/sourcehighlight.h>
 #include <srchilite/langmap.h>
 #include <srchilite/settings.h>
+#include <srchilite/highlighteventlistener.h>
 #endif
 
 #if GDB_SELF_TEST
 #include "gdbsupport/selftest.h"
+#include "event-top.h"
 #endif
 
 /* The number of source files we'll cache.  */
@@ -193,6 +195,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
@@ -229,6 +242,9 @@ try_source_highlight (std::string &contents ATTRIBUTE_UNUSED,
 
 	  const std::string &datadir = srchilite::Settings::retrieveDataDir ();
 	  langmap = new srchilite::LangMap (datadir, "lang.map");
+
+	  static gdb_highlight_event_listener event_listener;
+	  highlighter->setHighlightEventListener (&event_listener);
 	}
 
       std::string detected_lang;
@@ -246,6 +262,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
@@ -293,6 +319,61 @@ static void gnu_source_highlight_test ()
     SELF_CHECK (prog.size () < styled_prog.size ());
   else
     SELF_CHECK (prog == styled_prog);
+
+  if (!res)
+    {
+      /* No styling happened, bail out for the rest of the test.  */
+      return;
+    }
+
+  {
+    selftests::signal_handler_selftest a_signal_handler_selftest;
+    if (!a_signal_handler_selftest.valid ())
+      return;
+
+    /* Pretend user sent SIGTERM by calling the signal handler.  */
+    handle_sigterm (SIGTERM);
+
+    bool saw_forced_quit = false;
+    res = false;
+    styled_prog = prog;
+    try
+      {
+	res = try_source_highlight (styled_prog, language_c, fullname);
+      }
+    catch (const gdb_exception_forced_quit &ex)
+      {
+	saw_forced_quit = true;
+      }
+
+    SELF_CHECK (saw_forced_quit);
+    SELF_CHECK (!res);
+    SELF_CHECK (prog == styled_prog);
+  }
+
+  {
+    selftests::signal_handler_selftest a_signal_handler_selftest;
+    if (!a_signal_handler_selftest.valid ())
+      return;
+
+    /* Pretend user sent SIGINT by calling the signal handler.  */
+    handle_sigint (SIGINT);
+
+    bool saw_quit = false;
+    res = false;
+    styled_prog = prog;
+    try
+      {
+	res = try_source_highlight (styled_prog, language_c, fullname);
+      }
+    catch (const gdb_exception_quit &ex)
+      {
+	saw_quit = true;
+      }
+    SELF_CHECK (saw_quit);
+    SELF_CHECK (!res);
+    SELF_CHECK (prog == styled_prog);
+  }
 }
 }
 #endif /* GDB_SELF_TEST */
-- 
2.35.3

[PATCH v5 5/6] [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>
Co-Authored-By: Pedro Alves <pedro@palves.net>

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

diff --git a/gdb/source-cache.c b/gdb/source-cache.c
index 2d59957f42c..6f514cbba3e 100644
--- a/gdb/source-cache.c
+++ b/gdb/source-cache.c
@@ -43,6 +43,7 @@
 #if GDB_SELF_TEST
 #include "gdbsupport/selftest.h"
 #include "event-top.h"
+#include "top.h"
 #endif
 
 /* The number of source files we'll cache.  */
@@ -200,10 +201,56 @@ 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
   {
-    QUIT;
+    /* 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 issuing
+       a SIGINT or SIGTERM to detect this.  */
+    gdb_assert (target_terminal::is_ours ());
+
+    if (!check_quit_flag ())
+      {
+	/* No SIGTERM or SIGINT pending, nothing to do.  */
+	return;
+      }
+
+    /* 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)
+      {
+	/* 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 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:
+  const std::string &m_fullname;
 };
 
 #endif /* HAVE_SOURCE_HIGHLIGHT */
@@ -242,9 +289,6 @@ try_source_highlight (std::string &contents ATTRIBUTE_UNUSED,
 
 	  const std::string &datadir = srchilite::Settings::retrieveDataDir ();
 	  langmap = new srchilite::LangMap (datadir, "lang.map");
-
-	  static gdb_highlight_event_listener event_listener;
-	  highlighter->setHighlightEventListener (&event_listener);
 	}
 
       std::string detected_lang;
@@ -256,6 +300,12 @@ try_source_highlight (std::string &contents ATTRIBUTE_UNUSED,
 	  lang_name = detected_lang.c_str ();
 	}
 
+      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);
@@ -269,8 +319,7 @@ try_source_highlight (std::string &contents ATTRIBUTE_UNUSED,
     }
   catch (const gdb_exception_quit &)
     {
-      /* SIGINT, rethrow.  */
-      throw;
+      /* SIGINT, ignore.  */
     }
   catch (...)
     {
@@ -356,6 +405,9 @@ static void gnu_source_highlight_test ()
     if (!a_signal_handler_selftest.valid ())
       return;
 
+    scoped_restore save_confirm
+      = make_scoped_restore (&confirm, false);
+
     /* Pretend user sent SIGINT by calling the signal handler.  */
     handle_sigint (SIGINT);
 
@@ -370,7 +422,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);
   }
@@ -439,13 +491,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

[PATCH v5 6/6] [gdb/cli] Simplify gdb_highlight_event_listener::notify

[Tom de Vries]

Currently gdb_highlight_event_listener::notify does things more complicated
than necessary because of the unittest.

In more detail, the unit test cannot handle it if force_quit is called.

Fix this by making signal_handler_selftest capture force_quit.

This causes the question also to be asked for the SIGTERM case, so it leaves a
less attractive command line after terminating:
...
$ gdb -q a.out -ex "list"
Reading symbols from a.out...
$ ncel source styling using GNU source highlight of test.cpp? ([y] or n)
...

This patch currently makes gdb.gdb/unittest.exp fail:
...
FAIL: gdb.gdb/unittest.exp: no executable loaded: \
  maintenance selftest (got interactive prompt)
FAIL: gdb.gdb/unittest.exp: reversed initialization: \
  selftest not dependent on initialization order
FAIL: gdb.gdb/unittest.exp: executable loaded: \
  maintenance selftest (got interactive prompt)
...
because the testsuite triggers on the highlighting question.

Since this patch is an RFC at this point, I haven't invested time in addressing
this yet.

Tested on x86_64-linux.
---
 gdb/event-top.c    |  4 ++++
 gdb/source-cache.c | 29 +++++++++++------------------
 gdb/top.c          |  7 +++++++
 gdb/top.h          |  7 +++++++
 4 files changed, 29 insertions(+), 18 deletions(-)

diff --git a/gdb/event-top.c b/gdb/event-top.c
index 8c01c0902ea..b4b3fec5cdc 100644
--- a/gdb/event-top.c
+++ b/gdb/event-top.c
@@ -1235,6 +1235,8 @@ signal_handler_selftest::signal_handler_selftest ()
   m_saved_sigint_handler = signal (SIGINT, SIG_IGN);
   m_saved_sigterm_handler = signal (SIGTERM, SIG_IGN);
 
+  capture_quit_force = true;
+
   /* Only code from the selftest itself can cause the SIGINT or SIGTERM signal
      handlers to run.  */
   m_valid = true;
@@ -1242,6 +1244,8 @@ signal_handler_selftest::signal_handler_selftest ()
 
 signal_handler_selftest::~signal_handler_selftest ()
 {
+  capture_quit_force = false;
+
   /* Make sure that no pending SIGINT or SIGTERM escapes from the
      selftest.  */
   clear_pending_sigint ();
diff --git a/gdb/source-cache.c b/gdb/source-cache.c
index 6f514cbba3e..032a5d5b234 100644
--- a/gdb/source-cache.c
+++ b/gdb/source-cache.c
@@ -221,25 +221,18 @@ class gdb_highlight_event_listener : public srchilite::HighlightEventListener
 	return;
       }
 
-    /* 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)
+    /* Ask the user what to do.  If we got a SIGTERM, don't skip querying.
+       Skipping isn't necessary as yquery runs a nested event loop for the
+       secondary prompt, which runs pending async signal handlers.  Note we
+       may receive a SIGTERM in between the query and the quit call.  */
+    int resp
+      = (yquery
+	 (_("Cancel source styling using GNU source highlight of %s? "),
+	  m_fullname.c_str ()));
+    if (!sync_quit_force_run && !resp)
       {
-	/* 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;
-	  }
+	/* Continue highlighting.  */
+	return;
       }
 
     /* Interrupt highlighting.  Note we don't abort via the QUIT macro
diff --git a/gdb/top.c b/gdb/top.c
index 5028440b671..03b9ec5e1ae 100644
--- a/gdb/top.c
+++ b/gdb/top.c
@@ -1725,12 +1725,19 @@ undo_terminal_modifications_before_exit (void)
   current_ui = saved_top_level;
 }
 
+#if GDB_SELF_TEST
+bool capture_quit_force;
+#endif
 
 /* Quit without asking for confirmation.  */
 
 void
 quit_force (int *exit_arg, int from_tty)
 {
+#if GDB_SELF_TEST
+  if (capture_quit_force)
+    throw_forced_quit ("capture_quit_force");
+#endif
   int exit_code = 0;
 
   /* Clear the quit flag and sync_quit_force_run so that a
diff --git a/gdb/top.h b/gdb/top.h
index 47e16ca104e..d63396e6598 100644
--- a/gdb/top.h
+++ b/gdb/top.h
@@ -109,4 +109,11 @@ extern bool check_quiet_mode ();
 
 extern void unbuffer_stream (FILE *stream);
 
+#if GDB_SELF_TEST
+/* Rather than forcibly quitting when quit_force is called, note that it was
+   called by throwing a forced_quit.  This allows selftests to handle this
+   situation.  */
+extern bool capture_quit_force;
+#endif
+
 #endif
-- 
2.35.3

Re: [PATCH v5 3/6] [gdb/cli] Add gnu-source-highlight selftest

[Tom de Vries]

On 10/24/23 11:49, Tom de Vries wrote:
> 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
> ...
> 

I realized that this simplified version doesn't really depend on 
anything from the series, so ... pushed.

Thanks,
- Tom

> Tested on x86_64-linux.
> ---
>   gdb/source-cache.c | 46 +++++++++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 45 insertions(+), 1 deletion(-)
> 
> diff --git a/gdb/source-cache.c b/gdb/source-cache.c
> index 92acb100901..c955929b543 100644
> --- a/gdb/source-cache.c
> +++ b/gdb/source-cache.c
> @@ -22,7 +22,6 @@
>   #include "source.h"
>   #include "cli/cli-style.h"
>   #include "symtab.h"
> -#include "gdbsupport/selftest.h"
>   #include "objfiles.h"
>   #include "exec.h"
>   #include "cli/cli-cmds.h"
> @@ -40,6 +39,10 @@
>   #include <srchilite/settings.h>
>   #endif
>   
> +#if GDB_SELF_TEST
> +#include "gdbsupport/selftest.h"
> +#endif
> +
>   /* The number of source files we'll cache.  */
>   
>   #define MAX_ENTRIES 5
> @@ -258,6 +261,43 @@ 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;
> +
> +  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);
> +}
> +}
> +#endif /* GDB_SELF_TEST */
> +#endif /* HAVE_SOURCE_HIGHLIGHT */
> +
>   /* See source-cache.h.  */
>   
>   bool
> @@ -489,5 +529,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
>   }

Re: [PATCH v5 1/6] [gdb/cli] Add signal_handler_selftest

[Tom de Vries]

On 10/24/23 11:49, Tom de Vries wrote:
> +signal_handler_selftest::signal_handler_selftest () +{ + /* If SIGINT 
> or SIGTERM is pending, try to handle it as usual. */ + QUIT; + + /* 
> Assert SIGTERM is not pending. */ + gdb_assert (!pending_sigterm ()); + 
> + if (check_quit_flag ()) + { + /* SIGINT is still pending. This can 
> happen because QUIT doesn't + handle pending SIGINT in all cases. Bail 
> out. */ + set_quit_flag (); + return; + } + + /* Assert SIGINT is not 
> pending. */ + gdb_assert (!pending_sigint ()); +

I was about to check in this patch, and the following that uses it in 
the python selftest, but I decided to play a bit more with it, and ran 
the selftests while hitting control-C, and ran into:
...
Running selftest python.
/data/vries/gdb/src/gdb/event-top.c:1246: internal-error: 
signal_handler_selftest: Assertion `!pending_sigint ()' failed.
A problem internal to GDB has been detected,
further debugging may prove unreliable.
----- Backtrace -----
0x5a1ac2 gdb_internal_backtrace_1
	/data/vries/gdb/src/gdb/bt-utils.c:122
0x5a1b65 _Z22gdb_internal_backtracev
	/data/vries/gdb/src/gdb/bt-utils.c:168
0xcf93d6 internal_vproblem
	/data/vries/gdb/src/gdb/utils.c:396
0xcf97a5 _Z15internal_verrorPKciS0_P13__va_list_tag
	/data/vries/gdb/src/gdb/utils.c:476
0x14b002e _Z18internal_error_locPKciS0_z
	/data/vries/gdb/src/gdbsupport/errors.cc:58
0x788092 _ZN9selftests23signal_handler_selftestC2Ev
	/data/vries/gdb/src/gdb/event-top.c:1246
0xa84b53 test_python
	/data/vries/gdb/src/gdb/python/python.c:2331
0x42258f ???
0x6104af _ZNKSt8functionIFvvEEclEv
	/usr/include/c++/9/bits/std_function.h:688
0x14c4632 _ZN9selftests9run_testsEN3gdb10array_viewIKPKcEEb
	/data/vries/gdb/src/gdbsupport/selftest.cc:101
0x91cbaa maintenance_selftest
	/data/vries/gdb/src/gdb/maint.c:1164
0x5f60ee do_simple_func
	/data/vries/gdb/src/gdb/cli/cli-decode.c:95
0x5fb0e1 _Z8cmd_funcP16cmd_list_elementPKci
	/data/vries/gdb/src/gdb/cli/cli-decode.c:2735
0xc605b2 _Z15execute_commandPKci
	/data/vries/gdb/src/gdb/top.c:575
0x912551 catch_command_errors
	/data/vries/gdb/src/gdb/main.c:513
0x91275c execute_cmdargs
	/data/vries/gdb/src/gdb/main.c:612
0x913aac captured_main_1
	/data/vries/gdb/src/gdb/main.c:1293
0x913caf captured_main
	/data/vries/gdb/src/gdb/main.c:1314
0x913d4e _Z8gdb_mainP18captured_main_args
	/data/vries/gdb/src/gdb/main.c:1343
0x4188bd main
	/data/vries/gdb/src/gdb/gdb.c:39
---------------------
/data/vries/gdb/src/gdb/event-top.c:1246: internal-error: 
signal_handler_selftest: Assertion `!pending_sigint ()' failed.
A problem internal to GDB has been detected,
further debugging may prove unreliable.
Quit this debugging session? (y or n) [answered Y; input not from terminal]
...

I'm not sure yet how this happens, but it's clear this needs further work.

Thanks,
- Tom