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

[Sergio Durigan Junior <sergiodj at redhat dot com>]

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,

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

gdb/ChangeLog:
2015-08-21  Sergio Durigan Junior  <sergiodj@redhat.com>

	* solib-svr4.c (solib_event_probe_action): Call
	get_probe_argument_count using TRY...CATCH.
	(svr4_handle_solib_event): Likewise, for evaluate_probe_argument.

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