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

[Sergio Durigan Junior <sergiodj@redhat.com>]

On Tuesday, September 01 2015, Gary Benson wrote:

>> > 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;

Great, that works for me as well.  I will update the patch here to
address this.

Thanks, Gary.

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/