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

Jose E. Marchesi
2014-10-10T16:38:37Z

Hi Sergio.

> +/* 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.

> +
> + /* 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... :)

> + 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 :)

> + 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? :?

> +/* Implementation of the get_probes method. */
> +
> +static void
> +dtrace_get_probes (VEC (probe_p) **probesp, struct objfile *objfile)
> +{
> + bfd *abfd = objfile->obfd;
> + asection *sect = NULL;
> +
> + /* Do nothing in case this is a .debug file, instead of the objfile
> + itself. */
> + if (objfile->separate_debug_objfile_backlink != NULL)
> + return;
> +
> + /* Iterate over the sections in OBJFILE looking for DTrace
> + information. */
> + for (sect = abfd->sections; sect != NULL; sect = sect->next)
> + {
> + if (elf_section_data (sect)->this_hdr.sh_type == SHT_SUNW_dof)
> + {
> + struct dtrace_dof_hdr *dof;
> +
> + /* Read the contents of the DOF section and then process it to
> + extract the information of any probe defined into it. */
> + if (!bfd_malloc_and_get_section (abfd, sect, (bfd_byte **) &dof))
> + {
> + complaint (&symfile_complaints,
> + _("could not obtain the contents of"
> + "section '%s' in objfile `%s'."),
> + sect->name, abfd->filename);
> + return;

Why return here? Is there only one section whose type is SHT_SUNW_dof?
If no, then I guess the loop should keep rolling. Otherwise, then
besides calling return here you should call return after the "xfree"
below. Am I getting it right?

Yeah, in principle there can be more than one sections of type
SHT_SUNW_dof. I changed the code as suggested.

> + }
> +
> + 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.

> +/* 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.