[V4,5/9] New probe type: DTrace USDT probes.

Message ID 20150326184350.GB13867@adacore.com
State New, archived
Headers

Commit Message

Joel Brobecker March 26, 2015, 6:43 p.m. UTC
  [Sergio - a change in dtrace-probe.c needs your approval, I think.
Also, if you have a chance to double-check the rest of the patch,
that'd be a much appreciated second pair of eyes!]

> > Well, the TRY..CATCH in your prototype would catch any error that may be
> > thrown in parse_expression, and the `set_language' must take care of
> > changing the language, so I would say that is enough...
> 
> OK - I will send an updated patch that makes things a little cleaner.
> I didn't know whether it was OK to default to type long makes much
> sense when the probe says the parameter should be using type "mutex_t".

Here it is.

gdb/ChangeLog:

        * dtrace-probe.c (dtrace_process_dof_probe): Contain any
        exception raised while parsing the probe arguments.
        Force parsing to be done using the C language parser.
        * expression.h (parse_expression_with_language): Declare.
        * parse.c (parse_expression_with_language): New function.

Tested on sparc-solaris.
  

Comments

Sergio Durigan Junior March 26, 2015, 6:53 p.m. UTC | #1
On Thursday, March 26 2015, Joel Brobecker wrote:

> [Sergio - a change in dtrace-probe.c needs your approval, I think.
> Also, if you have a chance to double-check the rest of the patch,
> that'd be a much appreciated second pair of eyes!]

Thanks, Joel.  I saw the messages yesterday but did not get a chance to
review until today.

>> > Well, the TRY..CATCH in your prototype would catch any error that may be
>> > thrown in parse_expression, and the `set_language' must take care of
>> > changing the language, so I would say that is enough...
>> 
>> OK - I will send an updated patch that makes things a little cleaner.
>> I didn't know whether it was OK to default to type long makes much
>> sense when the probe says the parameter should be using type "mutex_t".

I remember we had the same problem (i.e., not parsing using language_c)
on SystemTap SDT probes as well.  Coincidentally or not, you triggered
the problem twice!

> Here it is.
>
> gdb/ChangeLog:
>
>         * dtrace-probe.c (dtrace_process_dof_probe): Contain any
>         exception raised while parsing the probe arguments.
>         Force parsing to be done using the C language parser.
>         * expression.h (parse_expression_with_language): Declare.
>         * parse.c (parse_expression_with_language): New function.
>
> Tested on sparc-solaris.
>
> -- 
> Joel
>
> From ba8991270e994cd96f92316932f65f96e47bf329 Mon Sep 17 00:00:00 2001
> From: Joel Brobecker <brobecker@adacore.com>
> Date: Thu, 26 Mar 2015 19:14:03 +0100
> Subject: [PATCH] dtrace-probe: Handle error while parsing probe argument.
>
> The debugger on Solaris has been broken since the introduction of
> DTrace probe support:
>
>     (gdb) start
>     Temporary breakpoint 1 at 0x80593bc: file simple_main.adb, line 4.
>     Starting program: /[...]/simple_main
>     [Thread debugging using libthread_db enabled]
>     No definition of "mutex_t" in current context.
>
> The problem occurs while trying to parse a probe's arguument,

"argument"

> and the exception propagates all the way to the top. This patch
> fixes the issue by containing the exception and falling back on
> using the "long" builtin type if the argument's type could not
> be determined.
>
> Also, the parsing should be done using the C language parser.
>
> gdb/ChangeLog:
>
>         * dtrace-probe.c (dtrace_process_dof_probe): Contain any
>         exception raised while parsing the probe arguments.
>         Force parsing to be done using the C language parser.
>         * expression.h (parse_expression_with_language): Declare.
>         * parse.c (parse_expression_with_language): New function.
> ---
>  gdb/dtrace-probe.c |   14 ++++++++++++--
>  gdb/expression.h   |    3 +++
>  gdb/parse.c        |   22 ++++++++++++++++++++++
>  3 files changed, 37 insertions(+), 2 deletions(-)
>
> diff --git a/gdb/dtrace-probe.c b/gdb/dtrace-probe.c
> index 491d853..ff7ce7d 100644
> --- a/gdb/dtrace-probe.c
> +++ b/gdb/dtrace-probe.c
> @@ -427,8 +427,18 @@ dtrace_process_dof_probe (struct objfile *objfile,
>  	     this does not work then we set the type to `long
>  	     int'.  */
>            arg.type = builtin_type (gdbarch)->builtin_long;
> -	  expr = parse_expression (arg.type_str);
> -	  if (expr->elts[0].opcode == OP_TYPE)
> +
> +	  TRY
> +	    {
> +	      expr = parse_expression_with_language (arg.type_str, language_c);
> +	    }
> +	  CATCH (ex, RETURN_MASK_ERROR)
> +	    {
> +	      expr = NULL;
> +	    }
> +	  END_CATCH
> +
> +	  if (expr != NULL && expr->elts[0].opcode == OP_TYPE)
>  	    arg.type = expr->elts[1].type;
>  
>  	  VEC_safe_push (dtrace_probe_arg_s, ret->args, &arg);

This part looks OK.

> diff --git a/gdb/expression.h b/gdb/expression.h
> index f5cd0e5..d08ce05 100644
> --- a/gdb/expression.h
> +++ b/gdb/expression.h
> @@ -97,6 +97,9 @@ struct expression
>  
>  extern struct expression *parse_expression (const char *);
>  
> +extern struct expression *parse_expression_with_language (const char *string,
> +							  enum language lang);
> +
>  extern struct expression *parse_expression_in_context (const char *, int);
>  
>  extern struct type *parse_field_expression (char *, char **);
> diff --git a/gdb/parse.c b/gdb/parse.c
> index 2ffe52e..bc95cf1 100644
> --- a/gdb/parse.c
> +++ b/gdb/parse.c
> @@ -1268,6 +1268,28 @@ parse_expression (const char *string)
>    return exp;
>  }
>  
> +/* Same as parse_expression, but using the given language (LANG)
> +   to parse the expression.  */
> +
> +struct expression *
> +parse_expression_with_language (const char *string, enum language lang)
> +{
> +  struct cleanup *old_chain = NULL;
> +  struct expression *expr;
> +
> +  if (current_language->la_language != language_c)
> +    {
> +      old_chain = make_cleanup_restore_current_language ();
> +      set_language (language_c);
> +    }

Here I think you meant to use "lang" instead of "language_c", right?

> +
> +  expr = parse_expression (string);
> +
> +  if (old_chain != NULL)
> +    do_cleanups (old_chain);
> +  return expr;
> +}
> +
>  /* As for parse_expression, except that if VOID_CONTEXT_P, then
>     no value is expected from the expression.  */
>  
> -- 
> 1.7.10.4
>

OK with the fix above.

Thanks,
  

Patch

From ba8991270e994cd96f92316932f65f96e47bf329 Mon Sep 17 00:00:00 2001
From: Joel Brobecker <brobecker@adacore.com>
Date: Thu, 26 Mar 2015 19:14:03 +0100
Subject: [PATCH] dtrace-probe: Handle error while parsing probe argument.

The debugger on Solaris has been broken since the introduction of
DTrace probe support:

    (gdb) start
    Temporary breakpoint 1 at 0x80593bc: file simple_main.adb, line 4.
    Starting program: /[...]/simple_main
    [Thread debugging using libthread_db enabled]
    No definition of "mutex_t" in current context.

The problem occurs while trying to parse a probe's arguument,
and the exception propagates all the way to the top. This patch
fixes the issue by containing the exception and falling back on
using the "long" builtin type if the argument's type could not
be determined.

Also, the parsing should be done using the C language parser.

gdb/ChangeLog:

        * dtrace-probe.c (dtrace_process_dof_probe): Contain any
        exception raised while parsing the probe arguments.
        Force parsing to be done using the C language parser.
        * expression.h (parse_expression_with_language): Declare.
        * parse.c (parse_expression_with_language): New function.
---
 gdb/dtrace-probe.c |   14 ++++++++++++--
 gdb/expression.h   |    3 +++
 gdb/parse.c        |   22 ++++++++++++++++++++++
 3 files changed, 37 insertions(+), 2 deletions(-)

diff --git a/gdb/dtrace-probe.c b/gdb/dtrace-probe.c
index 491d853..ff7ce7d 100644
--- a/gdb/dtrace-probe.c
+++ b/gdb/dtrace-probe.c
@@ -427,8 +427,18 @@  dtrace_process_dof_probe (struct objfile *objfile,
 	     this does not work then we set the type to `long
 	     int'.  */
           arg.type = builtin_type (gdbarch)->builtin_long;
-	  expr = parse_expression (arg.type_str);
-	  if (expr->elts[0].opcode == OP_TYPE)
+
+	  TRY
+	    {
+	      expr = parse_expression_with_language (arg.type_str, language_c);
+	    }
+	  CATCH (ex, RETURN_MASK_ERROR)
+	    {
+	      expr = NULL;
+	    }
+	  END_CATCH
+
+	  if (expr != NULL && expr->elts[0].opcode == OP_TYPE)
 	    arg.type = expr->elts[1].type;
 
 	  VEC_safe_push (dtrace_probe_arg_s, ret->args, &arg);
diff --git a/gdb/expression.h b/gdb/expression.h
index f5cd0e5..d08ce05 100644
--- a/gdb/expression.h
+++ b/gdb/expression.h
@@ -97,6 +97,9 @@  struct expression
 
 extern struct expression *parse_expression (const char *);
 
+extern struct expression *parse_expression_with_language (const char *string,
+							  enum language lang);
+
 extern struct expression *parse_expression_in_context (const char *, int);
 
 extern struct type *parse_field_expression (char *, char **);
diff --git a/gdb/parse.c b/gdb/parse.c
index 2ffe52e..bc95cf1 100644
--- a/gdb/parse.c
+++ b/gdb/parse.c
@@ -1268,6 +1268,28 @@  parse_expression (const char *string)
   return exp;
 }
 
+/* Same as parse_expression, but using the given language (LANG)
+   to parse the expression.  */
+
+struct expression *
+parse_expression_with_language (const char *string, enum language lang)
+{
+  struct cleanup *old_chain = NULL;
+  struct expression *expr;
+
+  if (current_language->la_language != language_c)
+    {
+      old_chain = make_cleanup_restore_current_language ();
+      set_language (language_c);
+    }
+
+  expr = parse_expression (string);
+
+  if (old_chain != NULL)
+    do_cleanups (old_chain);
+  return expr;
+}
+
 /* As for parse_expression, except that if VOID_CONTEXT_P, then
    no value is expected from the expression.  */
 
-- 
1.7.10.4