This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH 2/2] Catching errors on probes-based dynamic linker interface
- From: Gary Benson <gbenson at redhat dot com>
- To: Sergio Durigan Junior <sergiodj at redhat dot com>
- Cc: GDB Patches <gdb-patches at sourceware dot org>
- Date: Mon, 24 Aug 2015 09:42:55 +0100
- Subject: Re: [PATCH 2/2] Catching errors on probes-based dynamic linker interface
- Authentication-results: sourceware.org; auth=none
- References: <1440200253-28603-1-git-send-email-sergiodj at redhat dot com> <1440200253-28603-3-git-send-email-sergiodj at redhat dot com>
Sergio Durigan Junior wrote:
> This patch is intended to make the interaction between the
> probes-based dynamic linker interface and the SystemTap SDT probe
> code on GDB more robust. It does that by wrapping the calls to the
> probe API with TRY...CATCH'es, so that any exception thrown will be
> caught and handled properly.
Thanks for doing this!
> diff --git a/gdb/solib-svr4.c b/gdb/solib-svr4.c
> index 1fb07d5..028c3d0 100644
> --- a/gdb/solib-svr4.c
> +++ b/gdb/solib-svr4.c
> @@ -1786,7 +1786,17 @@ solib_event_probe_action (struct probe_and_action *pa)
> arg0: Lmid_t lmid (mandatory)
> arg1: struct r_debug *debug_base (mandatory)
> arg2: struct link_map *new (optional, for incremental updates) */
> - probe_argc = get_probe_argument_count (pa->probe, frame);
> + TRY
> + {
> + probe_argc = get_probe_argument_count (pa->probe, frame);
> + }
> + CATCH (ex, RETURN_MASK_ERROR)
> + {
> + exception_print (gdb_stderr, ex);
> + probe_argc = 0;
> + }
> + END_CATCH
> +
> if (probe_argc == 2)
> action = FULL_RELOAD;
> else if (probe_argc < 2)
Maybe this would be clearer and more robust:
TRY
{
unsigned probe_argc;
probe_argc = get_probe_argument_count (pa->probe, frame);
if (probe_argc == 2)
action = FULL_RELOAD;
else if (probe_argc < 2)
action = PROBES_INTERFACE_FAILED;
}
CATCH (ex, RETURN_MASK_ERROR)
{
exception_print (gdb_stderr, ex);
action = PROBES_INTERFACE_FAILED;
}
END_CATCH
> @@ -1940,7 +1950,17 @@ svr4_handle_solib_event (void)
> usm_chain = make_cleanup (resume_section_map_updates_cleanup,
> current_program_space);
>
> - val = evaluate_probe_argument (pa->probe, 1, frame);
> + TRY
> + {
> + val = evaluate_probe_argument (pa->probe, 1, frame);
> + }
> + CATCH (ex, RETURN_MASK_ERROR)
> + {
> + exception_print (gdb_stderr, ex);
> + val = NULL;
> + }
> + END_CATCH
> +
> if (val == NULL)
> {
> do_cleanups (old_chain);
This is ok.
> @@ -1971,7 +1991,17 @@ svr4_handle_solib_event (void)
>
> if (action == UPDATE_OR_RELOAD)
> {
> - val = evaluate_probe_argument (pa->probe, 2, frame);
> + TRY
> + {
> + val = evaluate_probe_argument (pa->probe, 2, frame);
> + }
> + CATCH (ex, RETURN_MASK_ERROR)
> + {
> + exception_print (gdb_stderr, ex);
> + val = NULL;
> + }
> + END_CATCH
> +
> if (val != NULL)
> lm = value_as_address (val);
>
This failure will not cause the probes interface to be disabled.
FULL_RELOAD is an ok thing to do here, but it could be difficult
to debug in future (if this one probe argument is broken GDB will
be very much slower than it could be.) So maybe this should be:
CATCH (ex, RETURN_MASK_ERROR)
{
exception_print (gdb_stderr, ex);
do_cleanups (old_chain);
return;
}
END_CATCH
As an aside it would clarify this code greatly if "old_chain"
were renamed "disable_probes_interface" or similar. It took
me a while to figure out what the code was doing, and I wrote
it!
Cheers,
Gary
--
http://gbenson.net/