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

[Gary Benson <gbenson@redhat.com>]

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/