Re: [PATCH 2/2] Catching errors on probes-based dynamic linker interface

[Gary Benson <gbenson@redhat.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/