[PATCH 1/2] Improve error reporting when handling SystemTap SDT probes

[PATCH 1/2] Improve error reporting when handling SystemTap SDT probes

[Sergio Durigan Junior]

This patch improves the error reporting when handling SystemTap SDT
probes.  "Handling", in this case, mostly means "parsing".

On gdb/probe.h, only trivial changes on functions' comments in order
to explicitly mention that some of them can throw exceptions.  This is
just to make the API a bit more clear.

On gdb/stap-probe.c, I have s/internal_error/error/ on two functions
that are responsible for parsing specific bits of the probes'
arguments: stap_get_opcode and stap_get_expected_argument_type.  It is
not correct to call internal_error on such situations because it is
not really GDB's fault if the probes have malformed arguments.  I also
improved the error reported on stap_get_expected_argument_type by also
including the probe name on it.

Aside from that, and perhaps most importantly, I added a check on
stap_get_arg to make sure that we don't try to extract an argument
from a probe that has no arguments.  This check issues an
internal_error, because it really means that GDB is doing something it
shouldn't.

Although it can be considered almost trivial, and despite the fact
that I am the maintainer for this part of the code, I am posting this
patch for review.  I will wait a few days, and if nobody has anything
to say, I will go ahead and push it.

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

	* probe.h (struct probe_ops) <get_probe_argument_count,
	evaluate_probe_argument, enable_probe, disable_probe>: Mention in
	the comment that the function can throw an exception.
	(get_probe_argument_count): Likewise.
	(evaluate_probe_argument): Likewise.
	* stap-probe.c (stap_get_opcode): Call error instead of
	internal_error.
	(stap_get_expected_argument_type): Likewise.  Add argument
	'probe'.  Improve error message by mentioning the probe's name.
	(stap_parse_probe_arguments): Adjust call to
	stap_get_expected_argument_type.
	(stap_get_arg): Add comment.  Assert that 'probe->args_parsed' is
	not zero.  Call internal_error if GDB requests an argument but the
	probe has no arguments.
---
 gdb/probe.h      | 20 ++++++++++++++------
 gdb/stap-probe.c | 29 ++++++++++++++++++++++-------
 2 files changed, 36 insertions(+), 13 deletions(-)

diff --git a/gdb/probe.h b/gdb/probe.h
index c058a38..317aaab 100644
--- a/gdb/probe.h
+++ b/gdb/probe.h
@@ -72,7 +72,8 @@ struct probe_ops
     CORE_ADDR (*get_probe_address) (struct probe *probe,
 				    struct objfile *objfile);
 
-    /* Return the number of arguments of PROBE.  */
+    /* Return the number of arguments of PROBE.  This function can
+       throw an exception.  */
 
     unsigned (*get_probe_argument_count) (struct probe *probe,
 					  struct frame_info *frame);
@@ -84,7 +85,8 @@ struct probe_ops
     int (*can_evaluate_probe_arguments) (struct probe *probe);
 
     /* Evaluate the Nth argument from the PROBE, returning a value
-       corresponding to it.  The argument number is represented N.  */
+       corresponding to it.  The argument number is represented N.
+       This function can throw an exception.  */
 
     struct value *(*evaluate_probe_argument) (struct probe *probe,
 					      unsigned n,
@@ -143,13 +145,15 @@ struct probe_ops
 
     /* Enable a probe.  The semantics of "enabling" a probe depend on
        the specific backend and the field can be NULL in case enabling
-       probes is not supported.  */
+       probes is not supported.  This function can throw an
+       exception.  */
 
     void (*enable_probe) (struct probe *probe);
 
     /* Disable a probe.  The semantics of "disabling" a probe depend
        on the specific backend and the field can be NULL in case
-       disabling probes is not supported.  */
+       disabling probes is not supported.  This function can throw an
+       exception.  */
 
     void (*disable_probe) (struct probe *probe);
   };
@@ -266,7 +270,9 @@ extern struct cmd_list_element **info_probes_cmdlist_get (void);
 extern CORE_ADDR get_probe_address (struct probe *probe,
 				    struct objfile *objfile);
 
-/* Return the argument count of the specified probe.  */
+/* Return the argument count of the specified probe.
+
+   This function can throw an exception.  */
 
 extern unsigned get_probe_argument_count (struct probe *probe,
 					  struct frame_info *frame);
@@ -278,7 +284,9 @@ extern unsigned get_probe_argument_count (struct probe *probe,
 extern int can_evaluate_probe_arguments (struct probe *probe);
 
 /* Evaluate argument N of the specified probe.  N must be between 0
-   inclusive and get_probe_argument_count exclusive.  */
+   inclusive and get_probe_argument_count exclusive.
+
+   This function can throw an exception.  */
 
 extern struct value *evaluate_probe_argument (struct probe *probe,
 					      unsigned n,
diff --git a/gdb/stap-probe.c b/gdb/stap-probe.c
index 9ee9767..2462acc 100644
--- a/gdb/stap-probe.c
+++ b/gdb/stap-probe.c
@@ -313,9 +313,8 @@ stap_get_opcode (const char **s)
       break;
 
     default:
-      internal_error (__FILE__, __LINE__,
-		      _("Invalid opcode in expression `%s' for SystemTap"
-			"probe"), *s);
+      error (_("Invalid opcode in expression `%s' for SystemTap"
+	       "probe"), *s);
     }
 
   return op;
@@ -326,7 +325,8 @@ stap_get_opcode (const char **s)
 
 static struct type *
 stap_get_expected_argument_type (struct gdbarch *gdbarch,
-				 enum stap_arg_bitness b)
+				 enum stap_arg_bitness b,
+				 const struct stap_probe *probe)
 {
   switch (b)
     {
@@ -361,8 +361,8 @@ stap_get_expected_argument_type (struct gdbarch *gdbarch,
       return builtin_type (gdbarch)->builtin_uint64;
 
     default:
-      internal_error (__FILE__, __LINE__,
-		      _("Undefined bitness for probe."));
+      error (_("Undefined bitness for probe '%s'."),
+	     probe->p.name);
       break;
     }
 }
@@ -1172,7 +1172,8 @@ stap_parse_probe_arguments (struct stap_probe *probe, struct gdbarch *gdbarch)
       else
 	arg.bitness = STAP_ARG_BITNESS_UNDEFINED;
 
-      arg.atype = stap_get_expected_argument_type (gdbarch, arg.bitness);
+      arg.atype = stap_get_expected_argument_type (gdbarch, arg.bitness,
+						   probe);
 
       expr = stap_parse_argument (&cur, arg.atype, gdbarch);
 
@@ -1278,12 +1279,26 @@ stap_is_operator (const char *op)
   return ret;
 }
 
+/* Return argument N of probe PROBE.
+
+   If the probe's arguments have not been parsed yet, parse them.  If
+   there are no arguments, throw an exception (error).  Otherwise,
+   return the requested argument.  */
+
 static struct stap_probe_arg *
 stap_get_arg (struct stap_probe *probe, unsigned n, struct gdbarch *gdbarch)
 {
   if (!probe->args_parsed)
     stap_parse_probe_arguments (probe, gdbarch);
 
+  gdb_assert (probe->args_parsed);
+  if (probe->args_u.vec == NULL)
+    internal_error (__FILE__, __LINE__,
+		    _("Probe '%s' apparently does not have arguments, but \n"
+		      "GDB is requesting its argument number %u anyway.  "
+		      "This should not happen.  Please report this bug."),
+		    probe->p.name, n);
+
   return VEC_index (stap_probe_arg_s, probe->args_u.vec, n);
 }
 
-- 
2.4.3

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

[Sergio Durigan Junior]

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

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

[PATCH 0/2] Improve error management on probes-based dynamic linker interface (and workaround RH BZ 1196181)

[Sergio Durigan Junior]

Hi,

This series improves the error reporting done by the SystemTap SDT
probe support code, and addresses the problem reported on
<https://bugzilla.redhat.com/show_bug.cgi?id=1196181> by making the
error handling on the probes-based dynamic linker interface more
robust.

Further comments on the patches themselves.  I intend to push the
first patch soon if there are no comments.

Thanks,

Sergio.

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

[Gary Benson]

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/

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

[Sergio Durigan Junior]

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

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

[Gary Benson]

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

-- 
http://gbenson.net/

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

[Sergio Durigan Junior]

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

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

[Sergio Durigan Junior]

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/

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

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

[Gary Benson]

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

-- 
http://gbenson.net/

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

[Sergio Durigan Junior]

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/

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

[Sergio Durigan Junior]

On Tuesday, September 01 2015, I wrote:

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

I took the liberty to modify and expand the comment; I hope you still
find it OK.  Here's what I pushed.

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/

From eb7f0ec56321645e1999ddbac9db4e4cd14d03e4 Mon Sep 17 00:00:00 2001
From: Sergio Durigan Junior <sergiodj@redhat.com>
Date: Fri, 21 Aug 2015 18:28:07 -0400
Subject: [PATCH 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 | 43 ++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 40 insertions(+), 3 deletions(-)

diff --git a/gdb/solib-svr4.c b/gdb/solib-svr4.c
index 36b6c59..5d2b9dd 100644
--- a/gdb/solib-svr4.c
+++ b/gdb/solib-svr4.c
@@ -1786,7 +1786,23 @@ 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 get_probe_argument_count throws an exception, probe_argc will
+     be set to zero.  However, if pa->probe does not have arguments,
+     then get_probe_argument_count will succeed but probe_argc will
+     also be zero.  Both cases happen because of different things, but
+     they are treated equally here: action will be set to
+     PROBES_INTERFACE_FAILED.  */
   if (probe_argc == 2)
     action = FULL_RELOAD;
   else if (probe_argc < 2)
@@ -1940,7 +1956,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 +1997,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);
 
-- 
2.4.3

Re: [PATCH 1/2] Improve error reporting when handling SystemTap SDT probes

[Sergio Durigan Junior]

On Friday, August 21 2015, I wrote:

> This patch improves the error reporting when handling SystemTap SDT
> probes.  "Handling", in this case, mostly means "parsing".

Pushed.

  https://sourceware.org/ml/gdb-cvs/2015-09/msg00001.html

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

Re: [PATCH 0/2] Improve error management on probes-based dynamic linker interface (and workaround RH BZ 1196181)

[Sergio Durigan Junior]

On Friday, August 21 2015, I wrote:

> Hi,
>
> This series improves the error reporting done by the SystemTap SDT
> probe support code, and addresses the problem reported on
> <https://bugzilla.redhat.com/show_bug.cgi?id=1196181> by making the
> error handling on the probes-based dynamic linker interface more
> robust.
>
> Further comments on the patches themselves.  I intend to push the
> first patch soon if there are no comments.

I pushed both patches now.  The first one was obvious enough that I
could push it without further comments.  I would like to thank Gary for
commenting on the second patch and help me tweak it.

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

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

[Sergio Durigan Junior]

On Wednesday, September 02 2015, I wrote:

> On Tuesday, September 01 2015, I wrote:
>
>>> 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.
>
> I took the liberty to modify and expand the comment; I hope you still
> find it OK.  Here's what I pushed.

Pushed.

  https://sourceware.org/ml/gdb-cvs/2015-09/msg00002.html

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

[PATCH] Initialize variable and silence GCC warning from last commit

[Sergio Durigan Junior]

BuildBot e-mailed me to let me know that my last commit broke GDB on
RHEL-7.1 s390x.  On solib-svr4.c:svr4_handle_solib_event, 'val' now
needs to be initialized as NULL because it is inside a TRY..CATCH
block.  This patch does that.  Pushed as obvious.

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

	* solib-svr4.c (svr4_handle_solib_event): Initialize 'val' as NULL
---
 gdb/ChangeLog    | 4 ++++
 gdb/solib-svr4.c | 2 +-
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 8ed469f..80fff7e 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,9 @@
 2015-09-01  Sergio Durigan Junior  <sergiodj@redhat.com>
 
+	* solib-svr4.c (svr4_handle_solib_event): Initialize 'val' as NULL
+
+2015-09-01  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 5d2b9dd..d3411fa 100644
--- a/gdb/solib-svr4.c
+++ b/gdb/solib-svr4.c
@@ -1908,7 +1908,7 @@ svr4_handle_solib_event (void)
   struct probe_and_action *pa;
   enum probe_action action;
   struct cleanup *old_chain, *usm_chain;
-  struct value *val;
+  struct value *val = NULL;
   CORE_ADDR pc, debug_base, lm = 0;
   int is_initial_ns;
   struct frame_info *frame = get_current_frame ();
-- 
2.4.3

[PATCH] Initialize yet another variable to silence GCC warning from last-but-one commit

[Sergio Durigan Junior]

Yet another BuildBot e-mail, yet another breakage on RHEL-7.1 s390x
(which uses an older GCC).  This time,
solib-svr4.c:solib_event_probe_action has the probe_argc variable,
which is now inside a TRY..CATCH and therefore needs to be
initialized.  Pushed as obvious.

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

	* solib-svr4.c (solib_event_probe_action): Initialize 'probe_argc'
	as zero.
---
 gdb/ChangeLog    | 5 +++++
 gdb/solib-svr4.c | 2 +-
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 80fff7e..b054438 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,10 @@
 2015-09-01  Sergio Durigan Junior  <sergiodj@redhat.com>
 
+	* solib-svr4.c (solib_event_probe_action): Initialize 'probe_argc'
+	as zero.
+
+2015-09-01  Sergio Durigan Junior  <sergiodj@redhat.com>
+
 	* solib-svr4.c (svr4_handle_solib_event): Initialize 'val' as NULL
 
 2015-09-01  Sergio Durigan Junior  <sergiodj@redhat.com>
diff --git a/gdb/solib-svr4.c b/gdb/solib-svr4.c
index d3411fa..5d3d41e 100644
--- a/gdb/solib-svr4.c
+++ b/gdb/solib-svr4.c
@@ -1772,7 +1772,7 @@ static enum probe_action
 solib_event_probe_action (struct probe_and_action *pa)
 {
   enum probe_action action;
-  unsigned probe_argc;
+  unsigned probe_argc = 0;
   struct frame_info *frame = get_current_frame ();
 
   action = pa->action;
-- 
2.4.3

Re: [PATCH 0/2] Improve error management on probes-based dynamic linker interface (and workaround RH BZ 1196181)

[Gary Benson]

Sergio Durigan Junior wrote:
> On Friday, August 21 2015, I wrote:
> > This series improves the error reporting done by the SystemTap SDT
> > probe support code, and addresses the problem reported on
> > <https://bugzilla.redhat.com/show_bug.cgi?id=1196181> by making
> > the error handling on the probes-based dynamic linker interface
> > more robust.
> >
> > Further comments on the patches themselves.  I intend to push the
> > first patch soon if there are no comments.
> 
> I pushed both patches now.  The first one was obvious enough that I
> could push it without further comments.  I would like to thank Gary
> for commenting on the second patch and help me tweak it.

Awesome, thanks Sergio.

Cheers,
Gary

-- 
http://gbenson.net/