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

[Gary Benson <gbenson@redhat.com>]

Sergio Durigan Junior wrote:
> This patch is intended to make the interaction between the
> probes-based dynamic linker interface and the SystemTap SDT probe
> code on GDB more robust.  It does that by wrapping the calls to the
> probe API with TRY...CATCH'es, so that any exception thrown will be
> caught and handled properly.

Thanks for doing this!

> 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

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

This is ok.

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

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!

Cheers,
Gary

-- 
http://gbenson.net/