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

  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.

The idea for this patch came from
<https://bugzilla.redhat.com/show_bug.cgi?id=1196181>, which is a bug
initially filed against Fedora GDB (but now under Fedora GLIBC).  This
bug happens on armhfp (although it could happen on other targets as
well), and is triggered because GCC generates a strange argument for
one of the probes used by GDB in the dynamic linker interface.  As can
be seen in the bug, this argument is "-4@.L1052".

I don't want to discuss the reasons for this argument to be there
(this discussion belongs to the bug, or to another thread), but GDB
could definitely do a better error handling here.  Currently, one sees
the following message when there is an error in the probes-based
dynamic linker interface:

  (gdb) run
  Starting program: /bin/inferior
  warning: Probes-based dynamic linker interface failed.
  Reverting to original interface.

  Cannot parse expression `.L976 4@r4'.
  (gdb)

Which means that one needs to explicitly issue a "continue" command to
make GDB continue running the inferior, even though this error is not
fatal and GDB will fallback to the old interface automatically.

This is where this patch helps: it makes GDB still print the necessary
warnings or error messages, but it *also* does not stop the inferior
unnecessarily.

I have tested this patch on the systems where this error happens, but
I could not come up with a way to create a testcase for it.
Nevertheless, it should be straightforward to see that this patch does
improve the current situation.

OK to apply?

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.
---
 gdb/solib-svr4.c | 36 +++++++++++++++++++++++++++++++++---
 1 file changed, 33 insertions(+), 3 deletions(-)
  

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