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: Tue, 25 Aug 2015 13:47:48 +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> <20150824084255 dot GA16508 at blade dot nx> <87r3msd5xr dot fsf at redhat dot com>
Sergio Durigan Junior wrote:
> On Monday, August 24 2015, Gary Benson wrote:
> > > 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
>
> Maybe it's a matter of preference, but I don't like this (and I
> don't see why it is more robust). I prefer to have as little code
> as possible running on the TRY block, and handle everything else
> outside of it. I think it also makes things a bit more confuse
> because you have two places where action can be
> PROBES_INTERFACE_FAILED.
Well, there are two different failures:
1) get_probe_argument_count failed
2) get_probe_argument_count returned < 2
I think it's more robust because, imagine a future where someone adds
a zero-argument probe to glibc. They update the "if (probe_argc)..."
block to allow zero-argument probes through. If get_probe_argument_count
with such a GDB then it will not be treated as a failure.
FWIW I also like to keep code in TRY blocks to a minimum. Maybe you
could do it your original way, but set probe_argc to -1 in the CATCH
and have the below block like:
if (probe_argc < 0)
/* get_probe_argument_count failed */
action = PROBES_INTERFACE_FAILED
else if (probe_argc == 2)
action = FULL_RELOAD;
else if (probe_argc < 2)
/* we don't understand this probe with too few arguments */
action = PROBES_INTERFACE_FAILED;
It looks kind of silly but the compiler will optimize it out.
> > 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!
>
> Yeah. I'll leave this to another patch.
I'll do it if you like (but I'll wait til you've got this through).
Cheers,
Gary
--
http://gbenson.net/