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

[Sergio Durigan Junior <sergiodj@redhat.com>]

On Friday, October 10 2014, Jose E. Marchesi wrote:

>     > +/* The type of the ELF sections where we will find the DOF programs
>     > +   with information about probes.  */
>     > +
>     > +#ifndef SHT_SUNW_dof
>     > +# define SHT_SUNW_dof	0x6ffffff4
>     > +#endif
>     
>     Can this macro exist in another header file that you are including?
>
> That macro is defined in elf.h in Solaris, Minix, and probably other
> systems too.  I would not be surprised if it is eventually added to the
> elf headers in GNU/Linux, and also in binutils.  I strongly recommend to
> keep that sentinel in place to avoid potential problems with indirect
> includes in the future.

Sure thing :-).  I was mostly curious.

>     > +
>     > +      /* Number of arguments in the probe.  */
>     > +      ret->probe_argc = DOF_UINT (dof, probe->dofpr_nargc);
>     > +
>     > +      /* 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'.  */
>     
>     We're not using `' anymore; instead, we're using '' (GNU Coding Style
>     has been updated).
>
> A hard-to-die habit after so many years... :)

Haha, yeah, I can imagine :-).

>     > +      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);
>     > +	  while (((p - strtab) < strtab_size) /* sentinel.  */
>     > +		 && *p++);
>     
>     Again a matter of style, but for readability I prefer to write this loop
>     as:
>     
>       /* Use strtab_size as a sentinel.  */
>       while (*p != '\0' && p - strtab < strtab_size)
>         ++p;
>
> What you are suggesting is not exactly equivalent: it leaves `p' at the
> blank character, while the idea is to leave `p' at the character next ot
> the blank character.  I changed the loop to:
>
> /* Use strtab_size as a sentinel.  */
> while (*p++ != '\0' && p - strtab < strtab_size);
>
> Which makes the comparison explicit and thus may be more palatable for
> you :)

Ops, indeed, thanks for catching this :-).  No wonder I proposed to make
the loop clearer :-P.

>     > +	  VEC_safe_push (dtrace_probe_arg_s, ret->args, &arg);
>     > +	}
>     > +
>     > +      /* Add the vector of enablers to this probe, if any.  */
>     > +      ret->enablers = VEC_copy (dtrace_probe_enabler_s, enablers);
>     
>     You should free the enablers VEC in the end of the function.  You could
>     probably make a cleanup and call it later.
>
> Hmm, I don't see the need of doing a deep copy of the vector, nor I
> remember why I felt it was necessary to do it when I wrote the original
> code.
>
> I changed that to:
>
> /* Add the vector of enablers to this probe, if any.  */
> ret->enablers = enablers;
>
> But maybe(probably) I am missing something? :?

Hm, right.  But if you do that, you will have to adjust
dtrace_probe_destroy, because it will be freeing the same 'enablers'
over and over...

>     > +	    }
>     > +	  
>     > +	  dtrace_process_dof (sect, objfile, probesp, dof);
>     > +	  xfree (dof);
>     > +	}
>     > +    }
>     
>     What about using bfd_map_over_sections instead of this for loop?  I know
>     there is precedence of iterating over BFD sections by hand on GDB code,
>     but bfd_map_over_sections exists for this very purpose.
>
> I considered that, but the need to define a new structure type for
> passing `objfile' and `probesp' to the handler (not to mention the
> handler itself) makes it a bit overkill to use bfd_map_over_sections in
> this specific case IMO...  especially considering that
> dtrace_process_dof is only called by this function.

OK, fair enough.

>     > +/* Implementation of the clear_semaphore method.  */
>     > +
>     > +static void
>     > +dtrace_clear_semaphore (struct probe *probe_generic, struct objfile *objfile,
>     > +			struct gdbarch *gdbarch)
>     > +{
>     > +  gdb_assert (probe_generic->pops == &dtrace_probe_ops);
>     > +}
>     
>     This shouldn't be needed, because USDT probes don't have the concept of
>     a semaphore, right?  I will submit a patch soon to fix the fact that the
>     set/clear_semaphore functions are being called inconditionally.
>
> Correct, that should not be needed and can go away as soon as you do
> that change.

I should be able to post something today.  Will put you on the loop.

Cheers,

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