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

[jose.marchesi@oracle.com (Jose E. Marchesi)]

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.