Re: [PATCH v4 3/8] Catch gdb_exception_error instead of gdb_exception (in many places)

Pedro Alves
2023-01-30T19:00:53Z

Hi Kevin,

This patch took me a bit to convince myself is good as is. See below why.

On 2023-01-12 1:56 a.m., Kevin Buettner wrote:
> 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
> ---
> 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 40f85914a56..241dd1b9f24 100644
> --- a/gdb/ada-lang.c
> +++ b/gdb/ada-lang.c
> @@ -12320,7 +12320,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 8cfc46e0bed..af8b62e4880 100644
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -2833,7 +2833,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);
> @@ -5295,7 +5295,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 "
> @@ -5539,7 +5539,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");
> @@ -13476,7 +13476,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 0497ad05091..895d84025cf 100644
> --- a/gdb/infcmd.c
> +++ b/gdb/infcmd.c
> @@ -1588,7 +1588,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);
> }

Most of this patch makes a little nervous, as in several cases it doesn't look like
just letting an exception propagate will do the right thing, particularly
if it's a ctrl-c. For example, in the code touched by the hunk above we have:

void
print_return_value (struct ui_out *uiout, struct return_value_info *rv)
{
if (rv->type == nullptr
|| check_typedef (rv->type)->code () == TYPE_CODE_VOID)
return;

try
{
/* print_return_value_1 can throw an exception in some
circumstances. We need to catch this so that we still
delete the breakpoint. */
print_return_value_1 (uiout, rv);
}
catch (const gdb_exception &ex)
{
exception_print (gdb_stdout, ex);
}
}

Note:

"We need to catch this so that we still delete the breakpoint."

That's referring to the internal breakpoint inserted by the
"finish" command.

Actually, it's not that easy to think about how to best handle a Ctrl-C exception
thrown while handling an event, within fetch_inferior_event & friends.

When the inferior is running in the foreground, then a Ctrl-C typed while
handling some event will end up translated into a call to target_pass_ctrlc(),
so the quit-exception now escaping case doesn't happen then:

void
default_quit_handler (void)
{
if (check_quit_flag ())
{
if (target_terminal::is_ours ())
quit ();
else
target_pass_ctrlc ();
}
}

It's when the inferior is running on the background, say with "c&" that is
more concerning (the is_ours() case above). In that case a Ctrl-C really results in a Quit,
which if called within fetch_inferior_event & friends can abort run control handling, and most
probably leave run control in an inconsistent state. It should be possible to contruct e.g. a
testcase that sets a thread constantly hitting a breakpoint with a condition that evals
false, a watchpoint etc. to maximize chances of a QUIT macro call done while an event is being
handled, and then do "c &" and then periodically press Ctrl-C. Since the run control handling,
and breakpoint and watchpoint evaluation, etc. are running in the background from the perspective
of the CLI, when users type ctrl-c in this situation, they're thinking of aborting whatever
other command they were typing or running at the prompt, not the run control side, not the previous
"c&" command.

So I'm thinking that we should probably install a custom quit_handler while inside
fetch_inferior_event, like we disable pagination and other things, one that just does nothing,
or at least, does nothing until the user to tries ctrl-c a number of times before querying the
user what to do, in case GDB really gets stuck.

And if we agree with that direction, then I guess my concern about making all these try/catch that can
be run during run control, the ones in infrun.c, breakpoint.c, etc., escape gdb_exception_quit mostly
goes away.

So I think with that direction this patch is OK.

Pedro Alves

> diff --git a/gdb/infrun.c b/gdb/infrun.c
> index 181d961d80d..9cfafda5ace 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -8730,7 +8730,7 @@ normal_stop (void)
> {
> 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 48bfef0a026..7f6b289cdd3 100644
> --- a/gdb/jit.c
> +++ b/gdb/jit.c
> @@ -715,7 +715,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 cf3d738edce..ba58044f868 100644
> --- a/gdb/mi/mi-interp.c
> +++ b/gdb/mi/mi-interp.c
> @@ -847,7 +847,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 e17a4c406c0..054895c02e3 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 2f7d58061ab..9aa080ff974 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 c3c2e5ad612..fe26771abe4 100644
> --- a/gdb/printcmd.c
> +++ b/gdb/printcmd.c
> @@ -2109,7 +2109,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);
> }
>