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

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

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,

-- 
Sergio
GPG key ID: 0x65FC5E36
Please send encrypted e-mail if possible
http://sergiodj.net/