This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH 2/3] Implement new features needed for handling SystemTap probes


> From: Sergio Durigan Junior <sergiodj@redhat.com>
> Cc: Tom Tromey <tromey@redhat.com>
> Date: Fri, 09 Mar 2012 17:33:21 -0300
> 
> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -3,6 +3,10 @@
>  
>  *** Changes since GDB 7.4
>  
> +* GDB now has support for SystemTap <sys/sdt.h> probes.  You can set a
> +  breakpoint using the new "-p" or "-probe" options and inspect the probe
> +  arguments using the new $_probe_arg family of convenience variables.

I'd suggest to add here a URL where SystemTap is described.

> +@cindex sdt-probe

Not sure what this index entry is about.  If we want or plan to add to
GDB support for SDT probes, there should be some general text here
explaining what SDT probes are and at least a cursory reference to a
couple of such existing facilities in various development
environments.  In contrast, the text in this section is entirely
Linux- and SystemTap-centric.  So having an "sdt-probe" index entry
pointing to here would be misleading, I think.

My preference would be to make this section more general, by adding
some minimal text explaining what is an SDT probe, and then mention
SystemTap as the SDT variety we support on GNU/Linux.  Something like
"currently, SystemTap (http://....) probes are supported on
GNU/Linux".

> +You can examine the available @code{SystemTap} static probes using
> +@code{info probes}:

See my other mail where I suggest a more specific name for this
command.  I could also go with "info sdt-probes", if we include in
this section the general explanation of SDT probes.

> +@table @code
> +@kindex info probes
> +@item info probes [@var{provider} [@var{name} [@var{objfile}]]]

We are using @r{[} and @r{]} in the @item lines that are part of
"@table @code", to have the brackets typeset in the "normal" Roman
typeface, instead of the @code typeface.

Also, the way you wrote this implies that "name" cannot be given
unless "provider" is also given, and similarly with "objfile".  Is
this the intent?

> +If given, @var{provider} is a regular expression used to select which
> +providers to list.  If omitted, all providers are listed.

Not "all providers", but "probes by all providers".  (You don't list
the providers, you list the probes.)

Btw, I suggest to use @var{providers}, in plural here.

> +If given, @var{name} is a regular expression used to select which
> +probes to list.  If omitted, all probes are listed.

This doesn't make clear what is tested for matching the regexp.  I
suggest

  If given, @var{names} is a regular expression to match against probe
  names when selecting which probes to list.  If omitted, probe names
  are not considered when deciding whether to display them.

(I modified the second sentence because it's not really accurate to
say "all probes", due to filtering by "providers".)

> +These variables are always available, but attempts to access them at
> +any location other than a probe point will cause @value{GDBN} to give
> +an error.

"give an error message".

> +@cindex SystemTap static probe point

You already have a literally identical index entry in the section
where you describe "info probes".  Here, I would suggest something
different, like

  @cindex breakpoint at static probe point

> +@item -p|-probe @r{[}@var{objfile}:@r{]}@r{[}@var{provider}:@r{]}@var{name}
> +The @sc{gnu}/Linux tool @code{SystemTap} provides a way for
> +applications to embed static probes.

A cross-reference to "Static Probe Points" here is essential.

>                                        This form of linespec specifies
> +the location of such a static probe.  See
> +@uref{http://sourceware.org/systemtap/wiki/AddingUserSpaceProbingToApps}
> +for more information on static probes.

This URL should be in "Static Probe Points" instead.

In general, when you describe a feature, you should decide which of
the sections will be the main one describing it, and have all the info
there.  The other places in the manual should only mention the name of
the feature and provide a cross-reference to that main section.

> +If @var{objfile} is given, only probes coming from that shared library
> +or executable are considered.  If @var{provider} is given, then only
> +probes from that provider are considered.

Is this really different from "info probes", where these are regular
expressions?

Also, I think this should tell what happens if several probes match
the spec.

> +Some probes have an associated semaphore variable; for instance, this
> +happens automatically if you defined your probe using a DTrace-style
> +@file{.d} file.  If your probe has a semaphore, @value{GDBN} will
> +automatically enable it when you specify a breakpoint using the
> +@samp{-p} notation.  But, if you put a breakpoint at a probe's
> +location by some other method (e.g., @code{break file:line}), then
> +@value{GDBN} will not automatically set the semaphore.

This text should have an appropriate @cindex entry.  Imagine that you
are a user of this facility, and you want to quickly find in the GDB
manual whether the semaphore is enabled or not.  Whatever phrase you'd
think in that situation as something to look for in the index should
be here.

> +@item $_probe_argc
> +@itemx $_probe_arg0@dots{}$_probe_arg11
> +Arguments to a SystemTap static probe.  @xref{Static Probe Points}.

If you accept my view to make the description of probe support more
general, this should not specifically mention SystemTap.

Also, you seem to be inconsistent wrt whether you use @code{SystemTap}
or just "SystemTap".  Please pick one and stick to it.

> +@item $_probe_argc
> +Collects the number of arguments from the @code{SystemTap} probe at
> +which the tracepoint is located.

Same here.

> +@xref{Static Probe Points,,Static Probe Points}.

Not sure why you need the 3rd arg in this @xref, since it's identical
to the first one.

> +@item $_probe_arg@var{N}
> +Where @var{N} varies from 0 to 11.  Collects the @var{N}th argument

The first sentence is not a complete sentence.  I would simply drop
the "where" part.

Also, Please use @var{n}, not @var{N}, it looks nicer in print.

> +from the @code{SystemTap} probe at which the tracepoint is located.
> +@xref{Static Probe Points,,Static Probe Points}.

Same comment about this @xref.

> +  add_info ("probes", info_probes_command, _("\
> +Show available static probes.\n\
> +Usage: info probes [PROVIDER [NAME [OBJECT]]]\n\
> +Each argument is a regular expression, used to select probes.\n\
> +PROVIDER matches probe provider names.\n\
> +NAME matches the probe names.\n\
> +OBJECT match the executable or shared library name."));
          ^^^^^
"mathes", no?


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]