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

Sergio Durigan Junior
2014-10-10T18:13:36Z

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