diff mbox

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

Message ID 87r3msd5xr.fsf@redhat.com
State New, archived
Headers

Commit Message

Sergio Durigan Junior Aug. 24, 2015, 4:09 p.m. UTC 
  Thanks for the review, Gary.

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.

I am using this idiom in the other places as well, so I also think it is
good to keep things similar across the code.

>> @@ -1971,7 +1991,17 @@ svr4_handle_solib_event (void)
>>  
>>    if (action == UPDATE_OR_RELOAD)
>>      {
>> -      val = evaluate_probe_argument (pa->probe, 2, frame);
>> +      TRY
>> +	{
>> +	  val = evaluate_probe_argument (pa->probe, 2, frame);
>> +	}
>> +      CATCH (ex, RETURN_MASK_ERROR)
>> +	{
>> +	  exception_print (gdb_stderr, ex);
>> +	  val = NULL;
>> +	}
>> +      END_CATCH
>> +
>>        if (val != NULL)
>>  	lm = value_as_address (val);
>>  
>
> This failure will not cause the probes interface to be disabled.
> FULL_RELOAD is an ok thing to do here, but it could be difficult
> to debug in future (if this one probe argument is broken GDB will
> be very much slower than it could be.)  So maybe this should be:
>
>   CATCH (ex, RETURN_MASK_ERROR)
>     {
>       exception_print (gdb_stderr, ex);
>       do_cleanups (old_chain);
>       return;
>     }
>   END_CATCH

You're right, thanks for... catching... this.

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

Here's the updated version of the current patch.

Thanks,

Comments

Gary Benson Aug. 25, 2015, 12:47 p.m. UTC | #1 
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
Sergio Durigan Junior Aug. 25, 2015, 6:16 p.m. UTC | #2 
Thanks for the review, Gary.

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.

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

I think we should cross this bridge when we come to it.  Plus, the
version you proposed does not take that scenario into account as well:
if probe_argc is zero, action will be PROBES_INTERFACE_FAILED;
therefore, this code would have to be rewritten anyway (in the scenario
you're proposing).

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

This has crossed my mind when I was writing this part, but probe_argc is
unsigned int and therefore is never < 0.

Moreover, as I said above, we are not really interested in
differentiating between the errors here; what we really want to know is
if there was an error.

>> > 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).

Sure, no problem.

Cheers,
Sergio Durigan Junior Sept. 1, 2015, 3:27 a.m. UTC | #3 
On Tuesday, August 25 2015, I 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.

Thanks,

> 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.
>
>> 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.
>
> I think we should cross this bridge when we come to it.  Plus, the
> version you proposed does not take that scenario into account as well:
> if probe_argc is zero, action will be PROBES_INTERFACE_FAILED;
> therefore, this code would have to be rewritten anyway (in the scenario
> you're proposing).
>
>> 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.
>
> This has crossed my mind when I was writing this part, but probe_argc is
> unsigned int and therefore is never < 0.
>
> Moreover, as I said above, we are not really interested in
> differentiating between the errors here; what we really want to know is
> if there was an error.
>
>>> > 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).
>
> Sure, no problem.
>
> Cheers,
>
> -- 
> Sergio
> GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
> Please send encrypted e-mail if possible
> http://sergiodj.net/
Gary Benson Sept. 1, 2015, 9:24 a.m. UTC | #4 
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
Sergio Durigan Junior Sept. 1, 2015, 4:26 p.m. UTC | #5 
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.
diff mbox

Patch

diff --git a/gdb/solib-svr4.c b/gdb/solib-svr4.c
index 1fb07d5..fd90fc8 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)
@@ -1940,7 +1950,17 @@  svr4_handle_solib_event (void)
   usm_chain = make_cleanup (resume_section_map_updates_cleanup,
 			    current_program_space);
 
-  val = evaluate_probe_argument (pa->probe, 1, frame);
+  TRY
+    {
+      val = evaluate_probe_argument (pa->probe, 1, frame);
+    }
+  CATCH (ex, RETURN_MASK_ERROR)
+    {
+      exception_print (gdb_stderr, ex);
+      val = NULL;
+    }
+  END_CATCH
+
   if (val == NULL)
     {
       do_cleanups (old_chain);
@@ -1971,7 +1991,18 @@  svr4_handle_solib_event (void)
 
   if (action == UPDATE_OR_RELOAD)
     {
-      val = evaluate_probe_argument (pa->probe, 2, frame);
+      TRY
+	{
+	  val = evaluate_probe_argument (pa->probe, 2, frame);
+	}
+      CATCH (ex, RETURN_MASK_ERROR)
+	{
+	  exception_print (gdb_stderr, ex);
+	  do_cleanups (old_chain);
+	  return;
+	}
+      END_CATCH
+
       if (val != NULL)
 	lm = value_as_address (val);