* [PATCH v5 1/8] Introduce gdb_exception_forced_quit
2023-02-22 23:46 [PATCH v5 0/8] Fix gdb.base/gdb-sigterm.exp failure/error Kevin Buettner
@ 2023-02-22 23:46 ` Kevin Buettner
2023-02-22 23:46 ` [PATCH v5 2/8] Handle gdb SIGTERM by throwing / catching gdb_exception_force_quit Kevin Buettner
` (7 subsequent siblings)
8 siblings, 0 replies; 12+ messages in thread
From: Kevin Buettner @ 2023-02-22 23:46 UTC (permalink / raw)
To: gdb-patches; +Cc: pedro, Kevin Buettner, Tom de Vries
This commit adds a new exception 'gdb_exception_forced_quit', reason
code 'REASON_FORCED_QUIT', return mask 'RETURN_MASK_FORCED_QUIT', and
a wrapper for throwing the exception, throw_forced_quit().
The addition of this exception plus supporting code will allow us to
recognize that a SIGTERM has been received by GDB and then propagate
recognition of that fact to the upper levels of GDB where it can be
correctly handled. At the moment, when GDB receives a SIGTERM, it
will attempt to exit via a series of calls from the QUIT checking
code. However, before it can exit, it must do various cleanups, such
as killing or detaching all inferiors. Should these cleanups be
attempted while GDB is executing very low level code, such as reading
target memory from within ps_xfer_memory(), it can happen that some of
GDB's state is out of sync with regard to the cleanup code's
expectations. In the case just mentioned, it's been observed that
inferior_ptid and the current_thread_ are not in sync; this triggers
an assert / internal error.
This commit only introduces the exception plus supporting machinery;
changes which use this new exception are in later commits in this
series.
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=26761
Tested-by: Tom de Vries <tdevries@suse.de>
Approved-by: Pedro Alves <pedro@palves.net>
---
gdbsupport/common-exceptions.cc | 14 ++++++++++++++
gdbsupport/common-exceptions.h | 22 +++++++++++++++++++++-
2 files changed, 35 insertions(+), 1 deletion(-)
diff --git a/gdbsupport/common-exceptions.cc b/gdbsupport/common-exceptions.cc
index d0dd8081c41..edc1b56181a 100644
--- a/gdbsupport/common-exceptions.cc
+++ b/gdbsupport/common-exceptions.cc
@@ -184,6 +184,8 @@ throw_exception (gdb_exception &&exception)
{
if (exception.reason == RETURN_QUIT)
throw gdb_exception_quit (std::move (exception));
+ else if (exception.reason == RETURN_FORCED_QUIT)
+ throw gdb_exception_forced_quit (std::move (exception));
else if (exception.reason == RETURN_ERROR)
throw gdb_exception_error (std::move (exception));
else
@@ -196,6 +198,8 @@ throw_it (enum return_reason reason, enum errors error, const char *fmt,
{
if (reason == RETURN_QUIT)
throw gdb_exception_quit (fmt, ap);
+ else if (reason == RETURN_FORCED_QUIT)
+ throw gdb_exception_forced_quit (fmt, ap);
else if (reason == RETURN_ERROR)
throw gdb_exception_error (error, fmt, ap);
else
@@ -233,3 +237,13 @@ throw_quit (const char *fmt, ...)
throw_vquit (fmt, args);
va_end (args);
}
+
+void
+throw_forced_quit (const char *fmt, ...)
+{
+ va_list args;
+
+ va_start (args, fmt);
+ throw_it (RETURN_FORCED_QUIT, GDB_NO_ERROR, fmt, args);
+ va_end (args);
+}
diff --git a/gdbsupport/common-exceptions.h b/gdbsupport/common-exceptions.h
index a2a4f5a3892..f9b59ece736 100644
--- a/gdbsupport/common-exceptions.h
+++ b/gdbsupport/common-exceptions.h
@@ -32,6 +32,8 @@
enum return_reason
{
+ /* SIGTERM sent to GDB. */
+ RETURN_FORCED_QUIT = -3,
/* User interrupt. */
RETURN_QUIT = -2,
/* Any other error. */
@@ -42,9 +44,10 @@ enum return_reason
typedef enum
{
+ RETURN_MASK_FORCED_QUIT = RETURN_MASK (RETURN_FORCED_QUIT),
RETURN_MASK_QUIT = RETURN_MASK (RETURN_QUIT),
RETURN_MASK_ERROR = RETURN_MASK (RETURN_ERROR),
- RETURN_MASK_ALL = (RETURN_MASK_QUIT | RETURN_MASK_ERROR)
+ RETURN_MASK_ALL = (RETURN_MASK_FORCED_QUIT | RETURN_MASK_QUIT | RETURN_MASK_ERROR)
} return_mask;
/* Describe all exceptions. */
@@ -294,6 +297,21 @@ struct gdb_exception_quit : public gdb_exception
}
};
+struct gdb_exception_forced_quit : public gdb_exception
+{
+ gdb_exception_forced_quit (const char *fmt, va_list ap)
+ ATTRIBUTE_PRINTF (2, 0)
+ : gdb_exception (RETURN_FORCED_QUIT, GDB_NO_ERROR, fmt, ap)
+ {
+ }
+
+ explicit gdb_exception_forced_quit (gdb_exception &&ex) noexcept
+ : gdb_exception (std::move (ex))
+ {
+ gdb_assert (ex.reason == RETURN_FORCED_QUIT);
+ }
+};
+
/* An exception type that inherits from both std::bad_alloc and a gdb
exception. This is necessary because operator new can only throw
std::bad_alloc, and OTOH, we want exceptions thrown due to memory
@@ -336,5 +354,7 @@ extern void throw_error (enum errors error, const char *fmt, ...)
ATTRIBUTE_NORETURN ATTRIBUTE_PRINTF (2, 3);
extern void throw_quit (const char *fmt, ...)
ATTRIBUTE_NORETURN ATTRIBUTE_PRINTF (1, 2);
+extern void throw_forced_quit (const char *fmt, ...)
+ ATTRIBUTE_NORETURN ATTRIBUTE_PRINTF (1, 2);
#endif /* COMMON_COMMON_EXCEPTIONS_H */
--
2.34.3
^ permalink raw reply [flat|nested] 12+ messages in thread* [PATCH v5 2/8] Handle gdb SIGTERM by throwing / catching gdb_exception_force_quit
2023-02-22 23:46 [PATCH v5 0/8] Fix gdb.base/gdb-sigterm.exp failure/error Kevin Buettner
2023-02-22 23:46 ` [PATCH v5 1/8] Introduce gdb_exception_forced_quit Kevin Buettner
@ 2023-02-22 23:46 ` Kevin Buettner
2023-03-02 20:14 ` Simon Marchi
2023-02-22 23:46 ` [PATCH v5 3/8] Catch gdb_exception_error instead of gdb_exception (in many places) Kevin Buettner
` (6 subsequent siblings)
8 siblings, 1 reply; 12+ messages in thread
From: Kevin Buettner @ 2023-02-22 23:46 UTC (permalink / raw)
To: gdb-patches; +Cc: pedro, Kevin Buettner, Tom de Vries
When a GDB process receives the SIGTERM signal, handle_sigterm() in
event-top.c is called. The global variable 'sync_quit_force_run' is
set by this signal handler. It does some other things too, but the
setting of this global is the important bit for the SIGTERM part of
this discussion.
GDB will periodically check to see whether a Ctrl-C or SIGTERM has
been received. This is performed via use of the QUIT macro in
GDB's code. QUIT is defined to invoke maybe_quit(), which will be
periodically called during any lengthy operation. This is supposed to
ensure that the user won't have to wait too long for a Ctrl-C or
SIGTERM to be acted upon.
When a Ctrl-C / SIGINT is received, quit_handler() will decide whether
to pass the SIGINT onto the inferior or to call quit() which causes
gdb_exception_quit to be thrown. This exception (usually) propagates
to the top level. Control is then returned to the top level event
loop.
At the moment, SIGTERM is handled very differently. Instead of
throwing an exception, quit_force() is called. This does eventually
cause GDB to exit(), but prior to that happening, the inferiors
are killed or detached and other target related cleanup occurs.
As shown in this discussion between Pedro Alves and myself...
https://sourceware.org/pipermail/gdb-patches/2021-July/180802.html
https://sourceware.org/pipermail/gdb-patches/2021-July/180902.html
https://sourceware.org/pipermail/gdb-patches/2021-July/180903.html
...we found that it is possible for inferior_ptid and current_thread_
to get out of sync. When that happens, the "current_thread_ != nullptr"
assertion in inferior_thread() can fail resulting in a GDB internal
error.
Pedro recommended that we "let the normal quit exception propagate all
the way to the top level, and then have the top level call quit_force
if sync_quit_force_run is set." However, after the v2 series for this
patch set, we tweaked that idea by introducing a new exception for
handling SIGTERM.
This commit implements the obvious part of Pedro's suggestion:
Instead of calling quit_force from quit(), throw_forced_quit() is now
called instead. This causes the new exception 'gdb_exception_forced_quit'
to be thrown.
At the top level, I changed catch_command_errors() and captured_main()
to catch gdb_exception_forced_quit and then call quit_force() from the
catch block. I also changed start_event_loop() to also catch
gdb_exception_forced_quit; while we could also call quit_force() from
that catch block, it's sufficient to simply rethrow the exception
since it'll be caught by the newly added code in captured_main().
Making these changes fixed the failure / regression that I was seeing
for gdb.base/gdb-sigterm.exp when run on a machine with glibc-2.34.
However, there are many other paths back to the top level which this
test case does not test. I did an audit of all of the try / catch
code in GDB in which calls in the try-block might (eventually) call
QUIT. I found many cases where gdb_exception_quit and the new
gdb_exception_forced_quit will be swallowed. (When using GDB, have
you ever hit Ctrl-C and not have it do anything; if so, it could be
due to a swallowed gdb_exception_quit in one of the cases I've
identified.) The rest of the patches in this series deal with this
concern.
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=26761
Tested-by: Tom de Vries <tdevries@suse.de>
Approved-by: Pedro Alves <pedro@palves.net>
---
gdb/main.c | 12 ++++++++++++
gdb/utils.c | 2 +-
2 files changed, 13 insertions(+), 1 deletion(-)
diff --git a/gdb/main.c b/gdb/main.c
index c04d37a45f9..0bf6f98a797 100644
--- a/gdb/main.c
+++ b/gdb/main.c
@@ -410,6 +410,10 @@ start_event_loop ()
{
result = gdb_do_one_event ();
}
+ catch (const gdb_exception_forced_quit &ex)
+ {
+ throw;
+ }
catch (const gdb_exception &ex)
{
exception_print (gdb_stderr, ex);
@@ -518,6 +522,10 @@ catch_command_errors (catch_command_errors_const_ftype command,
if (do_bp_actions)
bpstat_do_actions ();
}
+ catch (const gdb_exception_forced_quit &e)
+ {
+ quit_force (NULL, 0);
+ }
catch (const gdb_exception &e)
{
return handle_command_errors (e);
@@ -1309,6 +1317,10 @@ captured_main (void *data)
{
captured_command_loop ();
}
+ catch (const gdb_exception_forced_quit &ex)
+ {
+ quit_force (NULL, 0);
+ }
catch (const gdb_exception &ex)
{
exception_print (gdb_stderr, ex);
diff --git a/gdb/utils.c b/gdb/utils.c
index 91e6974b976..08cc41b4ef8 100644
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -642,7 +642,7 @@ quit (void)
if (sync_quit_force_run)
{
sync_quit_force_run = 0;
- quit_force (NULL, 0);
+ throw_forced_quit ("SIGTERM");
}
#ifdef __MSDOS__
--
2.34.3
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH v5 2/8] Handle gdb SIGTERM by throwing / catching gdb_exception_force_quit
2023-02-22 23:46 ` [PATCH v5 2/8] Handle gdb SIGTERM by throwing / catching gdb_exception_force_quit Kevin Buettner
@ 2023-03-02 20:14 ` Simon Marchi
0 siblings, 0 replies; 12+ messages in thread
From: Simon Marchi @ 2023-03-02 20:14 UTC (permalink / raw)
To: Kevin Buettner, gdb-patches; +Cc: pedro, Tom de Vries
On 2/22/23 18:46, Kevin Buettner via Gdb-patches wrote:
> When a GDB process receives the SIGTERM signal, handle_sigterm() in
> event-top.c is called. The global variable 'sync_quit_force_run' is
> set by this signal handler. It does some other things too, but the
> setting of this global is the important bit for the SIGTERM part of
> this discussion.
>
> GDB will periodically check to see whether a Ctrl-C or SIGTERM has
> been received. This is performed via use of the QUIT macro in
> GDB's code. QUIT is defined to invoke maybe_quit(), which will be
> periodically called during any lengthy operation. This is supposed to
> ensure that the user won't have to wait too long for a Ctrl-C or
> SIGTERM to be acted upon.
>
> When a Ctrl-C / SIGINT is received, quit_handler() will decide whether
> to pass the SIGINT onto the inferior or to call quit() which causes
> gdb_exception_quit to be thrown. This exception (usually) propagates
> to the top level. Control is then returned to the top level event
> loop.
>
> At the moment, SIGTERM is handled very differently. Instead of
> throwing an exception, quit_force() is called. This does eventually
> cause GDB to exit(), but prior to that happening, the inferiors
> are killed or detached and other target related cleanup occurs.
> As shown in this discussion between Pedro Alves and myself...
>
> https://sourceware.org/pipermail/gdb-patches/2021-July/180802.html
> https://sourceware.org/pipermail/gdb-patches/2021-July/180902.html
> https://sourceware.org/pipermail/gdb-patches/2021-July/180903.html
>
> ...we found that it is possible for inferior_ptid and current_thread_
> to get out of sync. When that happens, the "current_thread_ != nullptr"
> assertion in inferior_thread() can fail resulting in a GDB internal
> error.
>
> Pedro recommended that we "let the normal quit exception propagate all
> the way to the top level, and then have the top level call quit_force
> if sync_quit_force_run is set." However, after the v2 series for this
> patch set, we tweaked that idea by introducing a new exception for
> handling SIGTERM.
>
> This commit implements the obvious part of Pedro's suggestion:
> Instead of calling quit_force from quit(), throw_forced_quit() is now
> called instead. This causes the new exception 'gdb_exception_forced_quit'
> to be thrown.
>
> At the top level, I changed catch_command_errors() and captured_main()
> to catch gdb_exception_forced_quit and then call quit_force() from the
> catch block. I also changed start_event_loop() to also catch
> gdb_exception_forced_quit; while we could also call quit_force() from
> that catch block, it's sufficient to simply rethrow the exception
> since it'll be caught by the newly added code in captured_main().
>
> Making these changes fixed the failure / regression that I was seeing
> for gdb.base/gdb-sigterm.exp when run on a machine with glibc-2.34.
> However, there are many other paths back to the top level which this
> test case does not test. I did an audit of all of the try / catch
> code in GDB in which calls in the try-block might (eventually) call
> QUIT. I found many cases where gdb_exception_quit and the new
> gdb_exception_forced_quit will be swallowed. (When using GDB, have
> you ever hit Ctrl-C and not have it do anything; if so, it could be
> due to a swallowed gdb_exception_quit in one of the cases I've
> identified.) The rest of the patches in this series deal with this
> concern.
>
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=26761
> Tested-by: Tom de Vries <tdevries@suse.de>
> Approved-by: Pedro Alves <pedro@palves.net>
I see this failure following this commit:
$ make check TESTS="gdb.base/quit-live.exp" RUNTESTFLAGS="--target_board=native-gdbserver"
FAIL: gdb.base/quit-live.exp: appear_how=run: extra_inferior=0: quit_how=sigterm: quit with SIGTERM (GDB internal error)
FAIL: gdb.base/quit-live.exp: appear_how=run: extra_inferior=1: quit_how=sigterm: quit with SIGTERM (GDB internal error)
In gdb.log:
(gdb) continue
Continuing.
Breakpoint 1, main () at /home/smarchi/src/binutils-gdb/gdb/testsuite/gdb.base/quit-live.c:23
23 int secs = 30;
(gdb) Executing on host: kill -TERM 1989873 (timeout = 300)
builtin_spawn -ignore SIGHUP kill -TERM 1989873
SIGTERM
/home/smarchi/src/binutils-gdb/gdb/exceptions.c:100: internal-error: Bad switch.
A problem internal to GDB has been detected,
further debugging may prove unreliable.
----- Backtrace -----
FAIL: gdb.base/quit-live.exp: appear_how=run: extra_inferior=0: quit_how=sigterm: quit with SIGTERM (GDB internal error)
Resyncing due to internal error.
0x557cb89b93f0 gdb_internal_backtrace_1
/home/smarchi/src/binutils-gdb/gdb/bt-utils.c:122
0x557cb89b93f0 _Z22gdb_internal_backtracev
/home/smarchi/src/binutils-gdb/gdb/bt-utils.c:168
0x557cb8e118f4 internal_vproblem
/home/smarchi/src/binutils-gdb/gdb/utils.c:401
0x557cb8e11bb0 _Z15internal_verrorPKciS0_P13__va_list_tag
/home/smarchi/src/binutils-gdb/gdb/utils.c:481
0x557cb8f666c4 _Z18internal_error_locPKciS0_z
/home/smarchi/src/binutils-gdb/gdbsupport/errors.cc:58
0x557cb8aece58 print_exception
/home/smarchi/src/binutils-gdb/gdb/exceptions.c:100
0x557cb8d9f363 _Z10quit_forcePii
/home/smarchi/src/binutils-gdb/gdb/top.c:1849
0x557cb8977cdb _Z28invoke_async_signal_handlersv
/home/smarchi/src/binutils-gdb/gdb/async-event.c:233
0x557cb8f67ad7 _Z16gdb_do_one_eventi
/home/smarchi/src/binutils-gdb/gdbsupport/event-loop.cc:199
0x557cb8bed709 start_event_loop
/home/smarchi/src/binutils-gdb/gdb/main.c:411
0x557cb8bed709 captured_command_loop
/home/smarchi/src/binutils-gdb/gdb/main.c:475
0x557cb8bef2f4 captured_main
/home/smarchi/src/binutils-gdb/gdb/main.c:1318
0x557cb8bef2f4 _Z8gdb_mainP18captured_main_args
/home/smarchi/src/binutils-gdb/gdb/main.c:1337
0x557cb890d87f main
/home/smarchi/src/binutils-gdb/gdb/gdb.c:32
---------------------
/home/smarchi/src/binutils-gdb/gdb/exceptions.c:100: internal-error: Bad switch.
Simon
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v5 3/8] Catch gdb_exception_error instead of gdb_exception (in many places)
2023-02-22 23:46 [PATCH v5 0/8] Fix gdb.base/gdb-sigterm.exp failure/error Kevin Buettner
2023-02-22 23:46 ` [PATCH v5 1/8] Introduce gdb_exception_forced_quit Kevin Buettner
2023-02-22 23:46 ` [PATCH v5 2/8] Handle gdb SIGTERM by throwing / catching gdb_exception_force_quit Kevin Buettner
@ 2023-02-22 23:46 ` Kevin Buettner
2023-02-22 23:46 ` [PATCH v5 4/8] Python QUIT processing updates Kevin Buettner
` (5 subsequent siblings)
8 siblings, 0 replies; 12+ messages in thread
From: Kevin Buettner @ 2023-02-22 23:46 UTC (permalink / raw)
To: gdb-patches; +Cc: pedro, Kevin Buettner, Tom de Vries
As described in the previous commit for this series, I became
concerned that there might be instances in which a QUIT (due to either
a SIGINT or SIGTERM) might not cause execution to return to the top
level. In some (though very few) instances, it is okay to not
propagate the exception for a Ctrl-C / SIGINT, but I don't think that
it is ever okay to swallow the exception caused by a SIGTERM.
Allowing that to happen would definitely be a deviation from the
current behavior in which GDB exits upon receipt of a SIGTERM.
I looked at all cases where an exception handler catches a
gdb_exception. Handlers which did NOT need modification were those
which satisifed one or more of the following conditions:
1) There is no call path to maybe_quit() in the try block. I used a
static analysis tool to help make this determination. In
instances where the tool didn't provide an answer of "yes, this
call path can result in maybe_quit() being called", I reviewed it
by hand.
2) The catch block contains a throw for conditions that it
doesn't want to handle; these "not handled" conditions
must include the quit exception and the new "forced quit" exception.
3) There was (also) a catch for gdb_exception_quit.
Any try/catch blocks not meeting the above conditions could
potentially swallow a QUIT exception.
My first thought was to add catch blocks for gdb_exception_quit and
then rethrow the exception. But Pedro pointed out that this can be
handled without adding additional code by simply catching
gdb_exception_error instead. That's what this patch series does.
There are some oddball cases which needed to be handled differently,
plus the extension languages, but those are handled in later patches.
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=26761
Tested-by: Tom de Vries <tdevries@suse.de>
Approved-by: Pedro Alves <pedro@palves.net>
---
gdb/ada-lang.c | 2 +-
gdb/breakpoint.c | 8 ++++----
gdb/i386-linux-tdep.c | 2 +-
gdb/inf-loop.c | 2 +-
gdb/infcmd.c | 2 +-
gdb/infrun.c | 2 +-
gdb/jit.c | 2 +-
gdb/mi/mi-cmd-break.c | 2 +-
gdb/mi/mi-interp.c | 2 +-
gdb/objc-lang.c | 2 +-
gdb/parse.c | 2 +-
gdb/printcmd.c | 2 +-
gdb/record-btrace.c | 2 +-
gdb/record-full.c | 2 +-
gdb/solib.c | 2 +-
gdb/sparc64-linux-tdep.c | 2 +-
gdb/symfile-mem.c | 2 +-
17 files changed, 20 insertions(+), 20 deletions(-)
diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
index 7c528ab629f..8e87a2aef6d 100644
--- a/gdb/ada-lang.c
+++ b/gdb/ada-lang.c
@@ -12296,7 +12296,7 @@ should_stop_exception (const struct bp_location *bl)
scoped_value_mark mark;
stop = value_true (evaluate_expression (ada_loc->excep_cond_expr.get ()));
}
- catch (const gdb_exception &ex)
+ catch (const gdb_exception_error &ex)
{
exception_fprintf (gdb_stderr, ex,
_("Error in testing exception condition:\n"));
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 0db3adaf916..ebf9d8f1349 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -2840,7 +2840,7 @@ insert_bp_location (struct bp_location *bl,
if (val)
bp_excpt = gdb_exception {RETURN_ERROR, GENERIC_ERROR};
}
- catch (gdb_exception &e)
+ catch (gdb_exception_error &e)
{
rethrow_on_target_close_error (e);
bp_excpt = std::move (e);
@@ -5302,7 +5302,7 @@ bpstat_check_watchpoint (bpstat *bs)
{
e = watchpoint_check (bs);
}
- catch (const gdb_exception &ex)
+ catch (const gdb_exception_error &ex)
{
exception_fprintf (gdb_stderr, ex,
"Error evaluating expression "
@@ -5546,7 +5546,7 @@ bpstat_check_breakpoint_conditions (bpstat *bs, thread_info *thread)
{
condition_result = breakpoint_cond_eval (cond);
}
- catch (const gdb_exception &ex)
+ catch (const gdb_exception_error &ex)
{
exception_fprintf (gdb_stderr, ex,
"Error in testing breakpoint condition:\n");
@@ -13518,7 +13518,7 @@ enable_breakpoint_disp (struct breakpoint *bpt, enum bpdisp disposition,
bpt->enable_state = bp_enabled;
update_watchpoint (w, true /* reparse */);
}
- catch (const gdb_exception &e)
+ catch (const gdb_exception_error &e)
{
bpt->enable_state = orig_enable_state;
exception_fprintf (gdb_stderr, e, _("Cannot enable watchpoint %d: "),
diff --git a/gdb/i386-linux-tdep.c b/gdb/i386-linux-tdep.c
index ad0377de5a6..a6adeca1b97 100644
--- a/gdb/i386-linux-tdep.c
+++ b/gdb/i386-linux-tdep.c
@@ -415,7 +415,7 @@ i386_linux_report_signal_info (struct gdbarch *gdbarch, struct ui_out *uiout,
access
= parse_and_eval_long ("$_siginfo._sifields._sigfault.si_addr");
}
- catch (const gdb_exception &exception)
+ catch (const gdb_exception_error &exception)
{
return;
}
diff --git a/gdb/inf-loop.c b/gdb/inf-loop.c
index f49d1c3c872..b9f25008247 100644
--- a/gdb/inf-loop.c
+++ b/gdb/inf-loop.c
@@ -69,7 +69,7 @@ inferior_event_handler (enum inferior_event_type event_type)
{
bpstat_do_actions ();
}
- catch (const gdb_exception &e)
+ catch (const gdb_exception_error &e)
{
/* If the user was running a foreground execution
command, then propagate the error so that the prompt
diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index a851fe1f8c8..be8684b8016 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -1585,7 +1585,7 @@ print_return_value (struct ui_out *uiout, struct return_value_info *rv)
delete the breakpoint. */
print_return_value_1 (uiout, rv);
}
- catch (const gdb_exception &ex)
+ catch (const gdb_exception_error &ex)
{
exception_print (gdb_stdout, ex);
}
diff --git a/gdb/infrun.c b/gdb/infrun.c
index 3629db0443a..89ffd205062 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -8784,7 +8784,7 @@ normal_stop ()
{
execute_cmd_pre_hook (stop_command);
}
- catch (const gdb_exception &ex)
+ catch (const gdb_exception_error &ex)
{
exception_fprintf (gdb_stderr, ex,
"Error while running hook_stop:\n");
diff --git a/gdb/jit.c b/gdb/jit.c
index 0b089bc6a82..7f4b9f0fd37 100644
--- a/gdb/jit.c
+++ b/gdb/jit.c
@@ -716,7 +716,7 @@ jit_reader_try_read_symtab (gdbarch *gdbarch, jit_code_entry *code_entry,
code_entry->symfile_size))
status = 0;
}
- catch (const gdb_exception &e)
+ catch (const gdb_exception_error &e)
{
status = 0;
}
diff --git a/gdb/mi/mi-cmd-break.c b/gdb/mi/mi-cmd-break.c
index 8b0483803b6..75957b75bad 100644
--- a/gdb/mi/mi-cmd-break.c
+++ b/gdb/mi/mi-cmd-break.c
@@ -58,7 +58,7 @@ breakpoint_notify (struct breakpoint *b)
{
print_breakpoint (b);
}
- catch (const gdb_exception &ex)
+ catch (const gdb_exception_error &ex)
{
exception_print (gdb_stderr, ex);
}
diff --git a/gdb/mi/mi-interp.c b/gdb/mi/mi-interp.c
index 29d1aee8235..e1244f3df43 100644
--- a/gdb/mi/mi-interp.c
+++ b/gdb/mi/mi-interp.c
@@ -841,7 +841,7 @@ mi_print_breakpoint_for_event (struct mi_interp *mi, breakpoint *bp)
print_breakpoint (bp);
}
- catch (const gdb_exception &ex)
+ catch (const gdb_exception_error &ex)
{
exception_print (gdb_stderr, ex);
}
diff --git a/gdb/objc-lang.c b/gdb/objc-lang.c
index f43d158a770..4a9dee44dd7 100644
--- a/gdb/objc-lang.c
+++ b/gdb/objc-lang.c
@@ -1286,7 +1286,7 @@ find_objc_msgcall_submethod (int (*f) (CORE_ADDR, CORE_ADDR *),
if (f (pc, new_pc) == 0)
return 1;
}
- catch (const gdb_exception &ex)
+ catch (const gdb_exception_error &ex)
{
exception_fprintf (gdb_stderr, ex,
"Unable to determine target of "
diff --git a/gdb/parse.c b/gdb/parse.c
index f2917b30740..b2cc6c59fb5 100644
--- a/gdb/parse.c
+++ b/gdb/parse.c
@@ -514,7 +514,7 @@ parse_exp_in_context (const char **stringptr, CORE_ADDR pc,
{
lang->parser (&ps);
}
- catch (const gdb_exception &except)
+ catch (const gdb_exception_error &except)
{
/* If parsing for completion, allow this to succeed; but if no
expression elements have been written, then there's nothing
diff --git a/gdb/printcmd.c b/gdb/printcmd.c
index 0d3bc292d4e..341efc779fe 100644
--- a/gdb/printcmd.c
+++ b/gdb/printcmd.c
@@ -2126,7 +2126,7 @@ do_one_display (struct display *d)
d->exp = parse_expression (d->exp_string.c_str (), &tracker);
d->block = tracker.block ();
}
- catch (const gdb_exception &ex)
+ catch (const gdb_exception_error &ex)
{
/* Can't re-parse the expression. Disable this display item. */
d->enabled_p = false;
diff --git a/gdb/record-btrace.c b/gdb/record-btrace.c
index 0daba893813..61de8491bb9 100644
--- a/gdb/record-btrace.c
+++ b/gdb/record-btrace.c
@@ -2932,7 +2932,7 @@ cmd_record_btrace_start (const char *args, int from_tty)
{
execute_command ("target record-btrace", from_tty);
}
- catch (const gdb_exception &exception)
+ catch (const gdb_exception_error &exception)
{
record_btrace_conf.format = BTRACE_FORMAT_BTS;
diff --git a/gdb/record-full.c b/gdb/record-full.c
index e3cfa3f45d5..15c5b7d682e 100644
--- a/gdb/record-full.c
+++ b/gdb/record-full.c
@@ -785,7 +785,7 @@ record_full_message_wrapper_safe (struct regcache *regcache,
{
record_full_message (regcache, signal);
}
- catch (const gdb_exception &ex)
+ catch (const gdb_exception_error &ex)
{
exception_print (gdb_stderr, ex);
return false;
diff --git a/gdb/solib.c b/gdb/solib.c
index 60bdf46cb91..ec0fcfb5beb 100644
--- a/gdb/solib.c
+++ b/gdb/solib.c
@@ -785,7 +785,7 @@ update_solib_list (int from_tty)
{
ops->open_symbol_file_object (from_tty);
}
- catch (const gdb_exception &ex)
+ catch (const gdb_exception_error &ex)
{
exception_fprintf (gdb_stderr, ex,
"Error reading attached "
diff --git a/gdb/sparc64-linux-tdep.c b/gdb/sparc64-linux-tdep.c
index beff812eeef..685df066f3a 100644
--- a/gdb/sparc64-linux-tdep.c
+++ b/gdb/sparc64-linux-tdep.c
@@ -139,7 +139,7 @@ sparc64_linux_report_signal_info (struct gdbarch *gdbarch, struct ui_out *uiout,
if (si_code >= SEGV_ACCADI && si_code <= SEGV_ADIPERR)
addr = parse_and_eval_long ("$_siginfo._sifields._sigfault.si_addr");
}
- catch (const gdb_exception &exception)
+ catch (const gdb_exception_error &exception)
{
return;
}
diff --git a/gdb/symfile-mem.c b/gdb/symfile-mem.c
index d36b9090e7e..7ac3ac709ec 100644
--- a/gdb/symfile-mem.c
+++ b/gdb/symfile-mem.c
@@ -197,7 +197,7 @@ add_vsyscall_page (inferior *inf)
name.c_str (),
0 /* from_tty */);
}
- catch (const gdb_exception &ex)
+ catch (const gdb_exception_error &ex)
{
exception_print (gdb_stderr, ex);
}
--
2.34.3
^ permalink raw reply [flat|nested] 12+ messages in thread* [PATCH v5 4/8] Python QUIT processing updates
2023-02-22 23:46 [PATCH v5 0/8] Fix gdb.base/gdb-sigterm.exp failure/error Kevin Buettner
` (2 preceding siblings ...)
2023-02-22 23:46 ` [PATCH v5 3/8] Catch gdb_exception_error instead of gdb_exception (in many places) Kevin Buettner
@ 2023-02-22 23:46 ` Kevin Buettner
2023-02-22 23:46 ` [PATCH v5 5/8] Guile " Kevin Buettner
` (4 subsequent siblings)
8 siblings, 0 replies; 12+ messages in thread
From: Kevin Buettner @ 2023-02-22 23:46 UTC (permalink / raw)
To: gdb-patches; +Cc: pedro, Kevin Buettner, Tom de Vries
See the previous patches in this series for the motivation behind
these changes.
This commit contains updates to Python's QUIT handling. Ideally, we'd
like to throw gdb_exception_forced_quit through the extension
language; I made an attempt to do this for gdb_exception_quit in an
earlier version of this patch, but Pedro pointed out that it is
(almost certainly) not safe to do so.
Still, we definitely don't want to swallow the exception representing
a SIGTERM for GDB, nor do we want to force modules written in the
extension language to have to explicitly handle this case. Since the
idea is for GDB to cleanup and quit for this exception, we'll simply
call quit_force() just as if the gdb_exception_forced_quit propagation
had managed to make it back to the top level.
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=26761
Tested-by: Tom de Vries <tdevries@suse.de>
---
gdb/python/py-finishbreakpoint.c | 5 +++++
gdb/python/py-gdb-readline.c | 4 ++++
gdb/python/py-symbol.c | 5 +++++
gdb/python/py-utils.c | 3 +++
gdb/python/py-value.c | 5 +++++
5 files changed, 22 insertions(+)
diff --git a/gdb/python/py-finishbreakpoint.c b/gdb/python/py-finishbreakpoint.c
index d4d129110e9..1a224b35779 100644
--- a/gdb/python/py-finishbreakpoint.c
+++ b/gdb/python/py-finishbreakpoint.c
@@ -20,6 +20,7 @@
#include "defs.h"
+#include "top.h" /* For quit_force(). */
#include "python-internal.h"
#include "breakpoint.h"
#include "frame.h"
@@ -271,6 +272,10 @@ bpfinishpy_init (PyObject *self, PyObject *args, PyObject *kwargs)
}
}
}
+ catch (const gdb_exception_forced_quit &except)
+ {
+ quit_force (NULL, 0);
+ }
catch (const gdb_exception &except)
{
/* Just swallow. Either the return type or the function value
diff --git a/gdb/python/py-gdb-readline.c b/gdb/python/py-gdb-readline.c
index ea0f78c9ad8..b9294ad9afc 100644
--- a/gdb/python/py-gdb-readline.c
+++ b/gdb/python/py-gdb-readline.c
@@ -46,6 +46,10 @@ gdbpy_readline_wrapper (FILE *sys_stdin, FILE *sys_stdout,
p = command_line_input (buffer, prompt, "python");
}
/* Handle errors by raising Python exceptions. */
+ catch (const gdb_exception_forced_quit &e)
+ {
+ quit_force (NULL, 0);
+ }
catch (const gdb_exception &except)
{
/* Detect user interrupt (Ctrl-C). */
diff --git a/gdb/python/py-symbol.c b/gdb/python/py-symbol.c
index b8777966c47..899b0787582 100644
--- a/gdb/python/py-symbol.c
+++ b/gdb/python/py-symbol.c
@@ -18,6 +18,7 @@
along with this program. If not, see <http://www.gnu.org/licenses/>. */
#include "defs.h"
+#include "top.h" /* For force_quit (). */
#include "block.h"
#include "frame.h"
#include "symtab.h"
@@ -515,6 +516,10 @@ gdbpy_lookup_static_symbol (PyObject *self, PyObject *args, PyObject *kw)
= get_selected_frame (_("No frame selected."));
block = get_frame_block (selected_frame, NULL);
}
+ catch (const gdb_exception_forced_quit &e)
+ {
+ quit_force (NULL, 0);
+ }
catch (const gdb_exception &except)
{
/* Nothing. */
diff --git a/gdb/python/py-utils.c b/gdb/python/py-utils.c
index 624b90a827f..d5b07a80d82 100644
--- a/gdb/python/py-utils.c
+++ b/gdb/python/py-utils.c
@@ -18,6 +18,7 @@
along with this program. If not, see <http://www.gnu.org/licenses/>. */
#include "defs.h"
+#include "top.h" /* For quit_force (). */
#include "charset.h"
#include "value.h"
#include "python-internal.h"
@@ -219,6 +220,8 @@ gdbpy_convert_exception (const struct gdb_exception &exception)
if (exception.reason == RETURN_QUIT)
exc_class = PyExc_KeyboardInterrupt;
+ else if (exception.reason == RETURN_FORCED_QUIT)
+ quit_force (NULL, 0);
else if (exception.error == MEMORY_ERROR)
exc_class = gdbpy_gdb_memory_error;
else
diff --git a/gdb/python/py-value.c b/gdb/python/py-value.c
index c61de577de1..429d4f17644 100644
--- a/gdb/python/py-value.c
+++ b/gdb/python/py-value.c
@@ -18,6 +18,7 @@
along with this program. If not, see <http://www.gnu.org/licenses/>. */
#include "defs.h"
+#include "top.h" /* For quit_force (). */
#include "charset.h"
#include "value.h"
#include "language.h"
@@ -371,6 +372,10 @@ valpy_get_address (PyObject *self, void *closure)
res_val = value_addr (val_obj->value);
val_obj->address = value_to_value_object (res_val);
}
+ catch (const gdb_exception_forced_quit &except)
+ {
+ quit_force (NULL, 0);
+ }
catch (const gdb_exception &except)
{
val_obj->address = Py_None;
--
2.34.3
^ permalink raw reply [flat|nested] 12+ messages in thread* [PATCH v5 5/8] Guile QUIT processing updates
2023-02-22 23:46 [PATCH v5 0/8] Fix gdb.base/gdb-sigterm.exp failure/error Kevin Buettner
` (3 preceding siblings ...)
2023-02-22 23:46 ` [PATCH v5 4/8] Python QUIT processing updates Kevin Buettner
@ 2023-02-22 23:46 ` Kevin Buettner
2023-02-22 23:46 ` [PATCH v5 6/8] QUIT processing w/ explicit throw for gdb_exception_forced_quit Kevin Buettner
` (3 subsequent siblings)
8 siblings, 0 replies; 12+ messages in thread
From: Kevin Buettner @ 2023-02-22 23:46 UTC (permalink / raw)
To: gdb-patches; +Cc: pedro, Kevin Buettner, Tom de Vries
This commit contains QUIT processing updates for GDB's Guile support.
As with the Python updates, we don't want to permit this code to
swallow the exception, gdb_exception_forced_quit, which is associated
with GDB receiving a SIGTERM.
I've adopted the same solution that I used for Python; whereever
a gdb_exception is caught in try/catch code in the Guile extension
language support, a catch for gdb_exception_forced_quit has been
added; this catch block will simply call quit_force(), which will
cause the necessary cleanups to occur followed by GDB exiting.
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=26761
Tested-by: Tom de Vries <tdevries@suse.de>
---
gdb/guile/guile-internal.h | 5 +++++
gdb/guile/scm-pretty-print.c | 5 +++++
gdb/guile/scm-type.c | 5 +++++
gdb/guile/scm-value.c | 5 +++++
gdb/top.h | 2 +-
5 files changed, 21 insertions(+), 1 deletion(-)
diff --git a/gdb/guile/guile-internal.h b/gdb/guile/guile-internal.h
index 42ecb3c24f9..b04ef17a5ad 100644
--- a/gdb/guile/guile-internal.h
+++ b/gdb/guile/guile-internal.h
@@ -29,6 +29,7 @@
#include "symtab.h"
#include "libguile.h"
#include "objfiles.h"
+#include "top.h" /* For quit_force(). */
struct block;
struct frame_info;
@@ -704,6 +705,10 @@ gdbscm_wrap (Function &&func, Args &&... args)
{
result = func (std::forward<Args> (args)...);
}
+ catch (const gdb_exception_forced_quit &e)
+ {
+ quit_force (NULL, 0);
+ }
catch (const gdb_exception &except)
{
exc = unpack (except);
diff --git a/gdb/guile/scm-pretty-print.c b/gdb/guile/scm-pretty-print.c
index e172a14dbb3..ae56758b2bc 100644
--- a/gdb/guile/scm-pretty-print.c
+++ b/gdb/guile/scm-pretty-print.c
@@ -21,6 +21,7 @@
conventions, et.al. */
#include "defs.h"
+#include "top.h" /* For quit_force(). */
#include "charset.h"
#include "symtab.h" /* Needed by language.h. */
#include "language.h"
@@ -558,6 +559,10 @@ ppscm_pretty_print_one_value (SCM printer, struct value **out_value,
(_("invalid result from pretty-printer to-string"), result);
}
}
+ catch (const gdb_exception_forced_quit &except)
+ {
+ quit_force (NULL, 0);
+ }
catch (const gdb_exception &except)
{
}
diff --git a/gdb/guile/scm-type.c b/gdb/guile/scm-type.c
index da16d22990c..008e792cc34 100644
--- a/gdb/guile/scm-type.c
+++ b/gdb/guile/scm-type.c
@@ -21,6 +21,7 @@
conventions, et.al. */
#include "defs.h"
+#include "top.h" /* For quit_force(). */
#include "arch-utils.h"
#include "value.h"
#include "gdbtypes.h"
@@ -132,6 +133,10 @@ tyscm_type_name (struct type *type)
&type_print_raw_options);
return stb.release ();
}
+ catch (const gdb_exception_forced_quit &except)
+ {
+ quit_force (NULL, 0);
+ }
catch (const gdb_exception &except)
{
excp = gdbscm_scm_from_gdb_exception (unpack (except));
diff --git a/gdb/guile/scm-value.c b/gdb/guile/scm-value.c
index ac948dcd1a2..32a9539b3e2 100644
--- a/gdb/guile/scm-value.c
+++ b/gdb/guile/scm-value.c
@@ -21,6 +21,7 @@
conventions, et.al. */
#include "defs.h"
+#include "top.h" /* For quit_force(). */
#include "arch-utils.h"
#include "charset.h"
#include "cp-abi.h"
@@ -416,6 +417,10 @@ gdbscm_value_address (SCM self)
{
address = vlscm_scm_from_value (value_addr (value));
}
+ catch (const gdb_exception_forced_quit &except)
+ {
+ quit_force (NULL, 0);
+ }
catch (const gdb_exception &except)
{
}
diff --git a/gdb/top.h b/gdb/top.h
index 9f9a0281be4..5a1352ef81c 100644
--- a/gdb/top.h
+++ b/gdb/top.h
@@ -236,7 +236,7 @@ extern void read_command_file (FILE *);
extern void init_history (void);
extern void command_loop (void);
extern int quit_confirm (void);
-extern void quit_force (int *, int);
+extern void quit_force (int *, int) ATTRIBUTE_NORETURN;
extern void quit_command (const char *, int);
extern void quit_cover (void);
extern void execute_command (const char *, int);
--
2.34.3
^ permalink raw reply [flat|nested] 12+ messages in thread* [PATCH v5 6/8] QUIT processing w/ explicit throw for gdb_exception_forced_quit
2023-02-22 23:46 [PATCH v5 0/8] Fix gdb.base/gdb-sigterm.exp failure/error Kevin Buettner
` (4 preceding siblings ...)
2023-02-22 23:46 ` [PATCH v5 5/8] Guile " Kevin Buettner
@ 2023-02-22 23:46 ` Kevin Buettner
2023-02-22 23:46 ` [PATCH v5 7/8] Introduce set_force_quit_flag and change type of sync_quit_force_run Kevin Buettner
` (2 subsequent siblings)
8 siblings, 0 replies; 12+ messages in thread
From: Kevin Buettner @ 2023-02-22 23:46 UTC (permalink / raw)
To: gdb-patches; +Cc: pedro, Kevin Buettner, Tom de Vries
This commit contains changes which have an explicit throw for
gdb_exception_forced_quit, or, in a couple of cases for gdb_exception,
but with a throw following a check to see if 'reason' is
RETURN_FORCED_QUIT.
Most of these are straightforward - it made sense to continue to allow
an existing catch of gdb_exception to also catch gdb_exception_quit;
in these cases, a catch/throw for gdb_exception_forced_quit was added.
There are two cases, however, which deserve a more detailed
explanation.
1) remote_fileio_request in gdb/remote-fileio.c:
The try block calls do_remote_fileio_request which can (in turn)
call one of the functions in remote_fio_func_map[]. Taking the
first one, remote_fileio_func_open(), we have the following call
path to maybe_quit():
remote_fileio_func_open(remote_target*, char*)
-> target_read_memory(unsigned long, unsigned char*, long)
-> target_read(target_ops*, target_object, char const*, unsigned char*, unsigned long, long)
-> maybe_quit()
Since there is a path to maybe_quit(), we must ensure that the
catch block is not permitted to swallow a QUIT representing a
SIGTERM.
However, for this case, we must take care not to change the way that
Ctrl-C / SIGINT is handled; we want to send a suitable EINTR reply to
the remote target should that happen. That being the case, I added a
catch/throw for gdb_exception_forced_quit. I also did a bit of
rewriting here, adding a catch for gdb_exception_quit in favor of
checking the 'reason' code in the catch block for gdb_exception.
2) mi_execute_command in gdb/mi/mi-main.c:
The try block calls captured_mi_execute_command(); there exists
a call path to maybe_quit():
captured_mi_execute_command(ui_out*, mi_parse*)
-> mi_cmd_execute(mi_parse*)
-> get_current_frame()
-> get_prev_frame_always_1(frame_info*)
-> frame_register_unwind_location(frame_info*, int, int*, lval_type*, unsigned long*, int*)
-> frame_register_unwind(frame_info*, int, int*, int*, lval_type*, unsigned long*, int*, unsigned char*)
-> value_entirely_available(value*)
-> value_fetch_lazy(value*)
-> value_fetch_lazy_memory(value*)
-> read_value_memory(value*, long, int, unsigned long, unsigned char*, unsigned long)
-> maybe_quit()
That being the case, we can't allow the exception handler (catch block)
to swallow a gdb_exception_quit for SIGTERM. However, it does seem
reasonable to output the exception via the mi interface so that some
suitable message regarding SIGTERM might be printed; therefore, I
check the exception's 'reason' field for RETURN_FORCED_QUIT and
do a throw for this case.
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=26761
Tested-by: Tom de Vries <tdevries@suse.de>
---
gdb/event-top.c | 2 ++
gdb/mi/mi-main.c | 4 ++++
gdb/remote-fileio.c | 13 +++++++++----
3 files changed, 15 insertions(+), 4 deletions(-)
diff --git a/gdb/event-top.c b/gdb/event-top.c
index 14984707df1..563050a4081 100644
--- a/gdb/event-top.c
+++ b/gdb/event-top.c
@@ -1289,6 +1289,8 @@ async_disconnect (gdb_client_data arg)
gdb_puts ("Could not kill the program being debugged",
gdb_stderr);
exception_print (gdb_stderr, exception);
+ if (exception.reason == RETURN_FORCED_QUIT)
+ throw;
}
for (inferior *inf : all_inferiors ())
diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c
index 84a95a24684..0013e5dfafd 100644
--- a/gdb/mi/mi-main.c
+++ b/gdb/mi/mi-main.c
@@ -1952,6 +1952,10 @@ mi_execute_command (const char *cmd, int from_tty)
somewhere. */
mi_print_exception (command->token, result);
mi_out_rewind (current_uiout);
+
+ /* Throw to a higher level catch for SIGTERM sent to GDB. */
+ if (result.reason == RETURN_FORCED_QUIT)
+ throw;
}
bpstat_do_actions ();
diff --git a/gdb/remote-fileio.c b/gdb/remote-fileio.c
index 3ff2a65b0ec..69b409708a3 100644
--- a/gdb/remote-fileio.c
+++ b/gdb/remote-fileio.c
@@ -1191,12 +1191,17 @@ remote_fileio_request (remote_target *remote, char *buf, int ctrlc_pending_p)
{
do_remote_fileio_request (remote, buf);
}
+ catch (const gdb_exception_forced_quit &ex)
+ {
+ throw;
+ }
+ catch (const gdb_exception_quit &ex)
+ {
+ remote_fileio_reply (remote, -1, FILEIO_EINTR);
+ }
catch (const gdb_exception &ex)
{
- if (ex.reason == RETURN_QUIT)
- remote_fileio_reply (remote, -1, FILEIO_EINTR);
- else
- remote_fileio_reply (remote, -1, FILEIO_EIO);
+ remote_fileio_reply (remote, -1, FILEIO_EIO);
}
}
--
2.34.3
^ permalink raw reply [flat|nested] 12+ messages in thread* [PATCH v5 7/8] Introduce set_force_quit_flag and change type of sync_quit_force_run
2023-02-22 23:46 [PATCH v5 0/8] Fix gdb.base/gdb-sigterm.exp failure/error Kevin Buettner
` (5 preceding siblings ...)
2023-02-22 23:46 ` [PATCH v5 6/8] QUIT processing w/ explicit throw for gdb_exception_forced_quit Kevin Buettner
@ 2023-02-22 23:46 ` Kevin Buettner
2023-02-22 23:46 ` [PATCH v5 8/8] Forced quit cases handled by resetting sync_quit_force_run Kevin Buettner
2023-02-23 13:01 ` [PATCH v5 0/8] Fix gdb.base/gdb-sigterm.exp failure/error Pedro Alves
8 siblings, 0 replies; 12+ messages in thread
From: Kevin Buettner @ 2023-02-22 23:46 UTC (permalink / raw)
To: gdb-patches; +Cc: pedro, Kevin Buettner
At the moment, handle_sigterm() in event-top.c does the following:
sync_quit_force_run = 1;
set_quit_flag ();
This was used several more times in a later patch in this series, so
I'm introducing (at Pedro's suggestion) a new function named
'set_force_quit_flag'. It simply sets sync_quit_force_run and also
calls set_quit_flag(). I've revised the later patch to call
set_force_quit_flag instead.
I noticed that sync_quit_force_run is declared as an int but is being
used as a bool, so I also changed its type to bool in this commit.
---
gdb/defs.h | 5 ++++-
gdb/event-top.c | 13 ++++++++++---
gdb/utils.c | 2 +-
3 files changed, 15 insertions(+), 5 deletions(-)
diff --git a/gdb/defs.h b/gdb/defs.h
index f4fba9acf30..e7c9a39dd89 100644
--- a/gdb/defs.h
+++ b/gdb/defs.h
@@ -173,7 +173,10 @@ extern quit_handler_ftype *quit_handler;
extern void default_quit_handler (void);
/* Flag that function quit should call quit_force. */
-extern volatile int sync_quit_force_run;
+extern volatile bool sync_quit_force_run;
+
+/* Set sync_quit_force_run and also call set_quit_flag(). */
+extern void set_force_quit_flag ();
extern void quit (void);
diff --git a/gdb/event-top.c b/gdb/event-top.c
index 563050a4081..eed0ec0eb44 100644
--- a/gdb/event-top.c
+++ b/gdb/event-top.c
@@ -1217,7 +1217,15 @@ async_sigterm_handler (gdb_client_data arg)
}
/* See defs.h. */
-volatile int sync_quit_force_run;
+volatile bool sync_quit_force_run;
+
+/* See defs.h. */
+void
+set_force_quit_flag ()
+{
+ sync_quit_force_run = true;
+ set_quit_flag ();
+}
/* Quit GDB if SIGTERM is received.
GDB would quit anyway, but this way it will clean up properly. */
@@ -1226,8 +1234,7 @@ handle_sigterm (int sig)
{
signal (sig, handle_sigterm);
- sync_quit_force_run = 1;
- set_quit_flag ();
+ set_force_quit_flag ();
mark_async_signal_handler (async_sigterm_token);
}
diff --git a/gdb/utils.c b/gdb/utils.c
index 08cc41b4ef8..0138c8e9fb6 100644
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -641,7 +641,7 @@ quit (void)
{
if (sync_quit_force_run)
{
- sync_quit_force_run = 0;
+ sync_quit_force_run = false;
throw_forced_quit ("SIGTERM");
}
--
2.34.3
^ permalink raw reply [flat|nested] 12+ messages in thread* [PATCH v5 8/8] Forced quit cases handled by resetting sync_quit_force_run
2023-02-22 23:46 [PATCH v5 0/8] Fix gdb.base/gdb-sigterm.exp failure/error Kevin Buettner
` (6 preceding siblings ...)
2023-02-22 23:46 ` [PATCH v5 7/8] Introduce set_force_quit_flag and change type of sync_quit_force_run Kevin Buettner
@ 2023-02-22 23:46 ` Kevin Buettner
2023-02-23 13:01 ` [PATCH v5 0/8] Fix gdb.base/gdb-sigterm.exp failure/error Pedro Alves
8 siblings, 0 replies; 12+ messages in thread
From: Kevin Buettner @ 2023-02-22 23:46 UTC (permalink / raw)
To: gdb-patches; +Cc: pedro, Kevin Buettner, Tom de Vries
During my audit of the use of gdb_exception with regard to QUIT
processing, I found a try/catch in the scoped_switch_fork_info
destructor.
Static analysis found this call path from the destructor to
maybe_quit():
scoped_switch_fork_info::~scoped_switch_fork_info()
-> remove_breakpoints()
-> remove_breakpoint(bp_location*)
-> remove_breakpoint_1(bp_location*, remove_bp_reason)
-> memory_validate_breakpoint(gdbarch*, bp_target_info*)
-> target_read_memory(unsigned long, unsigned char*, long)
-> target_read(target_ops*, target_object, char const*, unsigned char*, unsigned long, long)
-> maybe_quit()
Since it's not safe to do a 'throw' from a destructor, we simply
call set_quit_flag and, for gdb_exception_forced_quit, also
set sync_quit_force_run. This will cause the appropriate
exception to be rethrown at the next QUIT check.
Another case is the try / catch in tui_getc() in tui-io.c. The
existing catch swallows the exception. I've added a catch for
'gdb_exception_forced_quit', which also swallows the exception,
but also sets sync_quit_force_run and calls set_quit_flag in
order to restart forced quit processing at the next QUIT check.
This is required because it isn't safe to throw into/through
readline.
Thanks to Pedro Alves for suggesting this idea.
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=26761
Tested-by: Tom de Vries <tdevries@suse.de>
---
gdb/linux-fork.c | 12 ++++++++++++
gdb/tui/tui-io.c | 8 ++++++++
gdb/tui/tui.c | 7 +++++++
3 files changed, 27 insertions(+)
diff --git a/gdb/linux-fork.c b/gdb/linux-fork.c
index 61545b859ea..a6e74c0cb52 100644
--- a/gdb/linux-fork.c
+++ b/gdb/linux-fork.c
@@ -430,6 +430,18 @@ class scoped_switch_fork_info
fork_load_infrun_state (m_oldfp);
insert_breakpoints ();
}
+ catch (const gdb_exception_quit &ex)
+ {
+ /* We can't throw from a destructor, so re-set the quit flag
+ for later QUIT checking. */
+ set_quit_flag ();
+ }
+ catch (const gdb_exception_forced_quit &ex)
+ {
+ /* Like above, but (eventually) cause GDB to terminate by
+ setting sync_quit_force_run. */
+ set_force_quit_flag ();
+ }
catch (const gdb_exception &ex)
{
warning (_("Couldn't restore checkpoint state in %s: %s"),
diff --git a/gdb/tui/tui-io.c b/gdb/tui/tui-io.c
index a17cf38a1f2..7752701378e 100644
--- a/gdb/tui/tui-io.c
+++ b/gdb/tui/tui-io.c
@@ -1271,6 +1271,14 @@ tui_getc (FILE *fp)
{
return tui_getc_1 (fp);
}
+ catch (const gdb_exception_forced_quit &ex)
+ {
+ /* As noted below, it's not safe to let an exception escape
+ to newline, so, for this case, reset the quit flag for
+ later QUIT checking. */
+ set_force_quit_flag ();
+ return 0;
+ }
catch (const gdb_exception &ex)
{
/* Just in case, don't ever let an exception escape to readline.
diff --git a/gdb/tui/tui.c b/gdb/tui/tui.c
index cdae9ffe02b..3604194a760 100644
--- a/gdb/tui/tui.c
+++ b/gdb/tui/tui.c
@@ -122,6 +122,13 @@ tui_rl_switch_mode (int notused1, int notused2)
tui_enable ();
}
}
+ catch (const gdb_exception_forced_quit &ex)
+ {
+ /* Ideally, we'd do a 'throw' here, but as noted above, we can't
+ do that, so, instead, we'll set the necessary flags so that
+ a later QUIT check will restart the forced quit. */
+ set_force_quit_flag ();
+ }
catch (const gdb_exception &ex)
{
exception_print (gdb_stderr, ex);
--
2.34.3
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH v5 0/8] Fix gdb.base/gdb-sigterm.exp failure/error
2023-02-22 23:46 [PATCH v5 0/8] Fix gdb.base/gdb-sigterm.exp failure/error Kevin Buettner
` (7 preceding siblings ...)
2023-02-22 23:46 ` [PATCH v5 8/8] Forced quit cases handled by resetting sync_quit_force_run Kevin Buettner
@ 2023-02-23 13:01 ` Pedro Alves
2023-02-27 23:22 ` Kevin Buettner
8 siblings, 1 reply; 12+ messages in thread
From: Pedro Alves @ 2023-02-23 13:01 UTC (permalink / raw)
To: Kevin Buettner, gdb-patches
Hi!
On 2023-02-22 11:46 p.m., Kevin Buettner wrote:
> This series fixes the failure in gdb.base/gdb-sigterm.exp when
> running in environments with glibc-2.34 or later.
>
> It contains only a few changes from the v4 series posted in
> January:
>
> https://sourceware.org/pipermail/gdb-patches/2023-January/195579.html
>
> This series drops part 6 from v4, "Call quit_force for
> gdb_exception_forced_quit in safe_execute_command", in favor of
> Pedros's already committed/pushed "Simplify interp::exec / interp_exec
> - let exceptions propagate".
>
> In his review of the v4 series, Pedro asked for a new function,
> set_force_quit_flag() which sets sync_quit_force_run and also calls
> set_quit_flag(). This is done in v5's part 7, "Introduce
> set_force_quit_flag and change type of sync_quit_force_run".
>
> This series also makes several other minor changes that Pedro had
> asked for.
>
> I've added Tested-by tags for Tom de Vries for all parts except
> for the new part 7 mentioned above.
>
> Pedro has approved parts 1, 2, and 3; I added Approved-by lines
> referencing Pedro to those parts.
>
Thank you!
> Pedro gave a sort of half-hearted approval for part 4, "Python QUIT
> processing updates". I haven't marked this commit with an Approved-by
> line yet. I would, however, like to commit this one as is. I think
> that implementing Pedro's suggestion of throwing a Python exception
> corresponding to the forced-quit and then calling set_force_quit_flag()
> when returning out of the Python interpreter is doable, but probably
> also somewhat tricky/involved. Rather than going a few more rounds on
> this series in an attempt to get those details correct, I'd prefer to
> push the Python and Guile portions of the current series and then
> work on a separate series implementing Pedro's suggestion. The Python
> and Guile portions of this series will cause GDB to exit on receipt of
> a SIGTERM, but might not be safe for future GDB in which Python (or
> Guile) might be called from GDB from very low level code in which
> certain data structures are not in sync. If this should happen, we
> might run into other problems anyway if those low-level invocations of
> Python from GDB want be able to call arbitrary methods in GDB's Python
> API. (We would need to make sure that all of GDB's Python API is safe
> to use with out-of-sync data structures. I think it's possible that
> some case might trigger the same assert which prompted this series.)
Agreed.
>
> Kevin Buettner (8):
> Introduce gdb_exception_forced_quit
> Handle gdb SIGTERM by throwing / catching gdb_exception_force_quit
> Catch gdb_exception_error instead of gdb_exception (in many places)
> Python QUIT processing updates
> Guile QUIT processing updates
> QUIT processing w/ explicit throw for gdb_exception_forced_quit
> Introduce set_force_quit_flag and change type of sync_quit_force_run
> Forced quit cases handled by resetting sync_quit_force_run
For the whole series:
Approved-By: Pedro Alves <pedro@palves.net>
Pedro Alves
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH v5 0/8] Fix gdb.base/gdb-sigterm.exp failure/error
2023-02-23 13:01 ` [PATCH v5 0/8] Fix gdb.base/gdb-sigterm.exp failure/error Pedro Alves
@ 2023-02-27 23:22 ` Kevin Buettner
0 siblings, 0 replies; 12+ messages in thread
From: Kevin Buettner @ 2023-02-27 23:22 UTC (permalink / raw)
To: Pedro Alves; +Cc: gdb-patches
On Thu, 23 Feb 2023 13:01:07 +0000
Pedro Alves <pedro@palves.net> wrote:
> > Kevin Buettner (8):
> > Introduce gdb_exception_forced_quit
> > Handle gdb SIGTERM by throwing / catching gdb_exception_force_quit
> > Catch gdb_exception_error instead of gdb_exception (in many places)
> > Python QUIT processing updates
> > Guile QUIT processing updates
> > QUIT processing w/ explicit throw for gdb_exception_forced_quit
> > Introduce set_force_quit_flag and change type of sync_quit_force_run
> > Forced quit cases handled by resetting sync_quit_force_run
>
> For the whole series:
>
> Approved-By: Pedro Alves <pedro@palves.net>
I've pushed this series. Thanks for all of the reviews!
Kevin
^ permalink raw reply [flat|nested] 12+ messages in thread