* [PATCH v5 1/6] [gdb/cli] Add signal_handler_selftest
2023-10-24 9:49 [PATCH v5 0/6] [gdb/cli] Allow source highlighting to be interrupted Tom de Vries
@ 2023-10-24 9:49 ` Tom de Vries
2023-11-22 16:26 ` Tom de Vries
2023-10-24 9:49 ` [PATCH v5 2/6] [gdb/python] Make python selftest more robust Tom de Vries
` (4 subsequent siblings)
5 siblings, 1 reply; 9+ messages in thread
From: Tom de Vries @ 2023-10-24 9:49 UTC (permalink / raw)
To: gdb-patches
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
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v5 1/6] [gdb/cli] Add signal_handler_selftest
2023-10-24 9:49 ` [PATCH v5 1/6] [gdb/cli] Add signal_handler_selftest Tom de Vries
@ 2023-11-22 16:26 ` Tom de Vries
0 siblings, 0 replies; 9+ messages in thread
From: Tom de Vries @ 2023-11-22 16:26 UTC (permalink / raw)
To: gdb-patches
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v5 2/6] [gdb/python] Make python selftest more robust
2023-10-24 9:49 [PATCH v5 0/6] [gdb/cli] Allow source highlighting to be interrupted Tom de Vries
2023-10-24 9:49 ` [PATCH v5 1/6] [gdb/cli] Add signal_handler_selftest Tom de Vries
@ 2023-10-24 9:49 ` Tom de Vries
2023-10-24 9:49 ` [PATCH v5 3/6] [gdb/cli] Add gnu-source-highlight selftest Tom de Vries
` (3 subsequent siblings)
5 siblings, 0 replies; 9+ messages in thread
From: Tom de Vries @ 2023-10-24 9:49 UTC (permalink / raw)
To: gdb-patches
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
^ permalink raw reply [flat|nested] 9+ messages in thread* [PATCH v5 3/6] [gdb/cli] Add gnu-source-highlight selftest
2023-10-24 9:49 [PATCH v5 0/6] [gdb/cli] Allow source highlighting to be interrupted Tom de Vries
2023-10-24 9:49 ` [PATCH v5 1/6] [gdb/cli] Add signal_handler_selftest Tom de Vries
2023-10-24 9:49 ` [PATCH v5 2/6] [gdb/python] Make python selftest more robust Tom de Vries
@ 2023-10-24 9:49 ` Tom de Vries
2023-10-24 12:33 ` Tom de Vries
2023-10-24 9:49 ` [PATCH v5 4/6] [gdb/cli] Allow source highlighting to be interrupted Tom de Vries
` (2 subsequent siblings)
5 siblings, 1 reply; 9+ messages in thread
From: Tom de Vries @ 2023-10-24 9:49 UTC (permalink / raw)
To: gdb-patches
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
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v5 3/6] [gdb/cli] Add gnu-source-highlight selftest
2023-10-24 9:49 ` [PATCH v5 3/6] [gdb/cli] Add gnu-source-highlight selftest Tom de Vries
@ 2023-10-24 12:33 ` Tom de Vries
0 siblings, 0 replies; 9+ messages in thread
From: Tom de Vries @ 2023-10-24 12:33 UTC (permalink / raw)
To: gdb-patches
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
> }
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v5 4/6] [gdb/cli] Allow source highlighting to be interrupted
2023-10-24 9:49 [PATCH v5 0/6] [gdb/cli] Allow source highlighting to be interrupted Tom de Vries
` (2 preceding siblings ...)
2023-10-24 9:49 ` [PATCH v5 3/6] [gdb/cli] Add gnu-source-highlight selftest Tom de Vries
@ 2023-10-24 9:49 ` Tom de Vries
2023-10-24 9:49 ` [PATCH v5 5/6] [gdb/cli] Ask if source highlighting should " Tom de Vries
2023-10-24 9:49 ` [PATCH v5 6/6] [gdb/cli] Simplify gdb_highlight_event_listener::notify Tom de Vries
5 siblings, 0 replies; 9+ messages in thread
From: Tom de Vries @ 2023-10-24 9:49 UTC (permalink / raw)
To: gdb-patches
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
^ permalink raw reply [flat|nested] 9+ messages in thread* [PATCH v5 5/6] [gdb/cli] Ask if source highlighting should be interrupted
2023-10-24 9:49 [PATCH v5 0/6] [gdb/cli] Allow source highlighting to be interrupted Tom de Vries
` (3 preceding siblings ...)
2023-10-24 9:49 ` [PATCH v5 4/6] [gdb/cli] Allow source highlighting to be interrupted Tom de Vries
@ 2023-10-24 9:49 ` Tom de Vries
2023-10-24 9:49 ` [PATCH v5 6/6] [gdb/cli] Simplify gdb_highlight_event_listener::notify Tom de Vries
5 siblings, 0 replies; 9+ messages in thread
From: Tom de Vries @ 2023-10-24 9:49 UTC (permalink / raw)
To: gdb-patches
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
^ permalink raw reply [flat|nested] 9+ messages in thread* [PATCH v5 6/6] [gdb/cli] Simplify gdb_highlight_event_listener::notify
2023-10-24 9:49 [PATCH v5 0/6] [gdb/cli] Allow source highlighting to be interrupted Tom de Vries
` (4 preceding siblings ...)
2023-10-24 9:49 ` [PATCH v5 5/6] [gdb/cli] Ask if source highlighting should " Tom de Vries
@ 2023-10-24 9:49 ` Tom de Vries
5 siblings, 0 replies; 9+ messages in thread
From: Tom de Vries @ 2023-10-24 9:49 UTC (permalink / raw)
To: gdb-patches
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
^ permalink raw reply [flat|nested] 9+ messages in thread