[PATCH 2/2] Catching errors on probes-based dynamic linker interface
Gary Benson
gbenson@redhat.com
Tue Sep 1 09:24:00 GMT 2015
Sergio Durigan Junior wrote:
> On Tuesday, August 25 2015, Sergio Durigan Junior wrote:
> > Thanks for the review, Gary.
>
> Any more comments (from Gary or anyone else) before I go ahead and
> apply this? I will wait until the end of tomorrow (Tuesday), and
> then I'll go ahead.
Sorry for the delay, I've been on PTO.
> > On Tuesday, August 25 2015, Gary Benson wrote:
> > > Sergio Durigan Junior wrote:
> > > > On Monday, August 24 2015, Gary Benson wrote:
> > > > > 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
> >
> > Yes, and both are covered by the proposed patch. It is not really
> > important to distinguish between these failures today: what really
> > matters is that GDB recognizes both as failures and acts
> > accordingly.
That matters. It also matters that future maintainers do not trip
over this.
I am ok with doing this:
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 you put a big fat comment above the following block, e.g.:
/* Note that failure of get_probe_argument_count will
set probe_argc == 0. This must result in returning
action = PROBES_INTERFACE_FAILED. */
if (probe_argc == 2)
action = FULL_RELOAD;
else if (probe_argc < 2)
action = PROBES_INTERFACE_FAILED;
But I would prefer it looked like this:
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;
That's my preference because what is happening is documented by code
(which is less likely to rot than comments).
Either way is fine, but having one block of code setting probe_argc
to zero and relying on a subsequent block of code then returning
PROBES_INTERFACE_FAILED without anything to indicate that this is
happening is not fine.
Thanks,
Gary
--
http://gbenson.net/
More information about the Gdb-patches
mailing list