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

[Joel Brobecker <brobecker@adacore.com>]

[-- Attachment #1: Type: text/plain, Size: 4984 bytes --]

Jose,

> > 2015-02-02  Jose E. Marchesi  <jose.marchesi@oracle.com>
> >
> > 	* breakpoint.c (BREAK_ARGS_HELP): help string updated to mention
> > 	the -probe-dtrace new vpossible value for PROBE_MODIFIER.
> > 	* configure.ac (CONFIG_OBS): dtrace-probe.o added if BFD can
> > 	handle ELF files.
> > 	* Makefile.in (SFILES): dtrace-probe.c added.
> > 	* configure: Regenerate.
> > 	* dtrace-probe.c: New file.
> > 	(SHT_SUNW_dof): New constant.
> > 	(dtrace_probe_type): New enum.
> > 	(dtrace_probe_arg): New struct.
> > 	(dtrace_probe_arg_s): New typedef.
> > 	(struct dtrace_probe_enabler): New struct.
> > 	(dtrace_probe_enabler_s): New typedef.
> > 	(dtrace_probe): New struct.
> > 	(dtrace_probe_is_linespec): New function.
> > 	(dtrace_dof_sect_type): New enum.
> > 	(dtrace_dof_dofh_ident): Likewise.
> > 	(dtrace_dof_encoding): Likewise.
> > 	(DTRACE_DOF_ENCODE_LSB): Likewise.
> > 	(DTRACE_DOF_ENCODE_MSB): Likewise.
> > 	(dtrace_dof_hdr): New struct.
> > 	(dtrace_dof_sect): Likewise.
> > 	(dtrace_dof_provider): Likewise.
> > 	(dtrace_dof_probe): Likewise.
> > 	(DOF_UINT): New macro.
> > 	(DTRACE_DOF_PTR): Likewise.
> > 	(DTRACE_DOF_SECT): Likewise.
> > 	(dtrace_process_dof_probe): New function.
> > 	(dtrace_process_dof): Likewise.
> > 	(dtrace_build_arg_exprs): Likewise.
> > 	(dtrace_get_arg): Likewise.
> > 	(dtrace_get_probes): Likewise.
> > 	(dtrace_get_probe_argument_count): Likewise.
> > 	(dtrace_can_evaluate_probe_arguments): Likewise.
> > 	(dtrace_evaluate_probe_argument): Likewise.
> > 	(dtrace_compile_to_ax): Likewise.
> > 	(dtrace_probe_destroy): Likewise.
> > 	(dtrace_gen_info_probes_table_header): Likewise.
> > 	(dtrace_gen_info_probes_table_values): Likewise.
> > 	(dtrace_probe_is_enabled): Likewise.
> > 	(dtrace_probe_ops): New variable.
> > 	(info_probes_dtrace_command): New function.
> > 	(_initialize_dtrace_probe): Likewise.
> > 	(dtrace_type_name): Likewise.

Unfortunately, this patch is breaking GDB completely on Solaris.
The first issue I started investigating is the following, which
happens with nearly any unthreaded program:

    (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 error happens in...

> > +static void
> > +dtrace_process_dof_probe (struct objfile *objfile,

... and more precisely...

> > +      /* Store argument type descriptions.  A description of the type
> > +         of the argument is in the (J+1)th null-terminated string
> > +         starting at 'strtab' + 'probe->dofpr_nargv'.  */
> > +      ret->args = NULL;
> > +      p = strtab + DOF_UINT (dof, probe->dofpr_nargv);
> > +      for (j = 0; j < ret->probe_argc; j++)
> > +	{
> > +	  struct dtrace_probe_arg arg;
> > +	  struct expression *expr;
> > +
> > +	  arg.type_str = xstrdup (p);
> > +
> > +	  /* Use strtab_size as a sentinel.  */
> > +	  while (*p++ != '\0' && p - strtab < strtab_size);
> > +
> > +	  /* Try to parse a type expression from the type string.  If
> > +	     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);
                 ^^^^^^^^^^^^^^^^
                 there

The most obvious issue is that type "mutex_t" does not exist in
my program, so I think that the code should handle that case.

The second, less obvious issue, is that the parsing is done using
the current language, which I don't think is a good idea. Attached
is small prototype that attempts to do something like that.
I think we need a function that parses an expression using
a specific language, but I went for short prototype to test
my theory first.

That solves most of the issues, even if I don't really know if
it's not just sweeping them under the carpet instead.

And once I had that fixed, the next issue that I looked at was:

    (gdb) b adainit
    Breakpoint 1 at 0x8051f03
    (gdb) run
    Starting program: /[...]/a
    [Thread debugging using libthread_db enabled]
    zsh: 12378 segmentation fault (core dumped)  /[...]/gdb -q a

This is where I'm getting even more out of my league, here.
The SEGV happens on the following line:

377           uint32_t enabler_offset
378             = ((uint32_t *) eofftab)[DOF_UINT (dof, probe->dofpr_enoffidx) + i];

... and eofftab seems to have a bogus value, because:

    (gdb) p eofftab
    $6 = 0x9479fb2 <error: Cannot access memory at address 0x9479fb2>

For me to investigate further, I think I'd have to investigate how
probes work and what's where, and I don't really have the time or
interest at the moment. So, I'm hoping that you have access to some
Solaris systems and you could look into this?

I can send you reproducers, if you need. I investigated the issue on
x86-solaris 2.10, but the same happens on sparc-solaris 2.10 and
sparc64-solaris 2.10.

Thank you,
-- 
Joel

[-- Attachment #2: dtrace.diff --]
[-- Type: text/x-diff, Size: 1261 bytes --]

diff --git a/gdb/dtrace-probe.c b/gdb/dtrace-probe.c
index 491d853..5d5c5b2 100644
--- a/gdb/dtrace-probe.c
+++ b/gdb/dtrace-probe.c
@@ -414,6 +414,7 @@ dtrace_process_dof_probe (struct objfile *objfile,
 	{
 	  struct dtrace_probe_arg arg;
 	  struct expression *expr;
+	  struct cleanup *old_chain = NULL;
 
 	  /* Set arg.expr to ensure all fields in expr are initialized and
 	     the compiler will not warn when arg is used.  */
@@ -427,8 +428,24 @@ 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)
+
+	  if (current_language->la_language != language_c)
+	    {
+	      old_chain = make_cleanup_restore_current_language ();
+	      set_language (language_c);
+	    }
+	  TRY
+	    {
+	      expr = parse_expression (arg.type_str);
+	    }
+	  CATCH (ex, RETURN_MASK_ERROR)
+	    {
+	      expr = NULL;
+	    }
+	  END_CATCH
+	  if (old_chain != NULL)
+	    do_cleanups (old_chain);
+	  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);