Re: Static tracepoints support

Eli Zaretskii
2010-06-26T10:34:38Z

> From: Pedro Alves <pedro@codesourcery.com>
> Date: Fri, 25 Jun 2010 19:31:56 +0100
>
> This patch adds static tracepoints support to GDB, and adds GDBserver
> support for LTTng/UST based static tracepoints <http://lttng.org/ust>.
> We worked activelly with the LTTng/UST maintainers to design
> and implement this integration.

Thanks. However, does it mean we will only support LTTng/UST on
remote targets, not natively? If so, what would it take to add native
support as well?

> (NEWS and manual changes included. Are those okay?)

See below.

> (gdb) info static-tracepoint-markers
> Enb Address What
> n 0x00007ffff7996692
> String ID: metadata/core_marker_format
> Extra: channel %s name %s format %s

I'd suggest to have Address before Enb.

The "String" part in "String ID" sounds redundant; why not just "ID"?.

I cannot say I like the "Extra" thing. How about "Data" or "Format"
instead?

Also, what will be the actual layout of "String ID" and "Extra" lines?
What I see in your mail breaks the table structure too much. Did your
mailer fold the long lines here? If so, could you show what it will
look like on the screen in GDB? If this is the actual layout, can we
do better in terms of readability?

> The "What" column is similar to the "info breakpoints" output, and
> shows the source location of the marker, if possible.

Why wouldn't it be possible? Why some of the entries above don't have
this data?

> (gdb) info breakpoints
> Num Type Disp Enb Address What
> 1 breakpoint keep y 0x0000000000400c30 in main at stexample.c:13
> 2 static tracepoint keep y 0x0000000000400ddc in main at stexample.c:20
> static tracepoint id is ust/bar33
> collect $registers
> collect $_sdata

Suggest to use "stracepoint" instead of "static tracepoint", because
the latter makes the table appear misaligned. Correspondingly, "info
stracepoint-markers" instead of "info static-tracepoint-markers".

> + if (!in_process_agent_loaded ())
> + {
> + warning ("In-process agent not loaded");
> + return 0;

There are many calls to `warning' in the patch where the message
string is not in _().

> +#define SOCK_DIR "/tmp"

This is unnecessarily platform-specific. How about using P_tmpdir
instead?

> +/* When symbols change, it probably means the sources changed as well,
> + and it might mean the static tracepoint markers are no longer at
> + the same address or line numbers they used to be at last we
> + checked. Losing your static tracepoints whenever you rebuild is
> + undesirable. This function tries to resync/rematch gdb static
> + tracepoints with the markers on the target. The heuristic is:
> +
> + 1) look for a marker at the old PC. If one is found there, assume
> + to be the same marker. If the name / string id of the marker found
> + is different from the previous known name, assume that means the
> + user renamed the marker in the sources, and output a warning.
> +
> + 2) If a marker is no longer found at the same address, it may mean
> + the marker no longer exists. But it may also just mean the code
> + changed a bit. Maybe the user added a few lines of code that made
> + the marker move up or down (in line number terms). Ask the target
> + for info about the marker with the string id as we knew it. If
> + found, update line number and address in the matching static
> + tracepoint. */

I would suggest to reverse the order of the steps: first to query the
target about the marker with the old string ID, and only if it is not
found, use the heuristics in step 1. The rationale is that if the
target can provide the info, it is always more reliable than any
heuristics.

> + init_sal (&sal); /* initialize to zeroes */
^^^^^^
"zeros"

> +If a line number is specified, probe the marker at start of code \n\
> +for that line. If a function is specified, probe the marker at start \n\

The blank before "\n\" is redundant (here and elsewhere), its only
effect is to bloat the image of GDB.

Also, please use two spaces between sentences consistently.

> +with that name. With no LOCATION, uses current execution address of \n\
> +selected stack frame.\n\

"of the selected stack frame"

> +Static tracepoints accept an extra collect action --- ``collect $_sdata''.\n\

No need for 3 dashes in a row: this isn't Texinfo. 2 dashes will do.

> +tracing library. You can inspect it when analysing the trace buffer, \n\
^^^^^^^^^
"analyzing" (US spelling).

> +Multiple tracepoints at one place are permitted, and useful if conditional.\n

This sentence is incomplete, you probably meant "if their conditionals
are different" or some such.

> @@ -4223,6 +4488,7 @@ Also accepts the following special argum
> $regs -- all registers.\n\
> $args -- all function arguments.\n\
> $locals -- all variables local to the block/function scope.\n\
> + $_sdata -- static tracepoint data (ignored for non-static tracepoints).\n\

The last line is misaligned.

> +* Static tracepoints
> +
> + Static tracepoints are calls in the user program into a tracing
> + library. One such library is a port of the LTTng kernel tracer to
> + userspace --- UST (LTTng Userspace Tracer).

A URL would be useful here.

> GDB, when debugging
> + with GDBserver, now supports combining the GDB tracepoint machinery

"When debugging with GDBserver, GDB now supports ..."

> + information, see the "Tracepoints" chapter in GDB user manual. New

"in the GDB User Manual"

> +strace FN / FILE:LINE / *ADDR / -m MARKER_ID

I think alternatives are separated by "|", not by "/". (Yes, I know
that the entry for "ftrace" used "/"; it also needs to be fixed.)

The patch for NEWS is okay with these changes.

> +@vindex $_sdata@r{, inspect, convenience variable}
> +The variable @code{$_sdata} contains extra collected static tracepoint
> +data. (@pxref{Tracepoint Actions,,Tracepoint Action Lists}). Note

@pxref is not appropriate here. I would simply use @xref and drop the
parentheses.

> +that @code{$_sdata} could be empty, if not inspecting a trace buffer,
> +or extra static tracepoint data has not been collected.

"or if extra static tracepoint data has not been collected yet."

> +in the target. Some targets may yet support controlling @dfn{static
> +tracepoints} from @value{GDBN}.

Please replace "yet" with "also".

> With static tracing, a set of
> +instrumentation points, also known as @dfn{markers}, are embedded in

This section should have @cindex entries for every phrase you have in
@dfn.

> +@cindex set static tracepoint

This index entry would be much more efficient if it did not start with
"set". For example,

@cindex static tracepoint, setting

> +@kindex strace
> +The @code{strace} command sets a static tracepoint. For targets that
> +support it, setting a static tracepoint probes a static
> +instrumentation point, or marker, found at @var{location}.

The meaning of "probe an instrumentation point" was never explained.
I think it should be; without it, much of the following material
doesn't make much sense.

> +@value{GDBN} handles arguments to @code{strace} exactly as for
> +@code{trace}, with the addition that the user can also specify
> +@code{-m @var{marker}} as @var{location}. This probes the marker
> +identified by the @var{marker} string identifier.
> +
> +Static tracepoints accept an extra collect action --- @code{collect
> +$_sdata}. This collects arbitrary user data passed in the probe point
> +call to the tracing library.

The "string identifier" and "user data" parts should also be
explained. Perhaps show a sample call to `trace_mark' in the
instrumented source and explain the relationships between its
arguments and the terminology you use in this section.

> You can inspect this data when analysing

"analyzing"

> +@item $_sdata
> +@vindex $_sdata@r{, collect}
> +Collect static tracepoint marker specific data. Only available for
> +static tracepoints.

An xref to where static tracepoints are described is missing here.

> This usually consists of data formatted to a
> +character string using the format provided by the programmer that
> +instrumented the program. E.g., on some systems, an instrumentation
> +point resembles a printf function call:
> +
> +@smallexample
> + TRACE(tp1, "hello %s", master_name)
> +@end smallexample

How do you mean ``on some systems''? This support is inherently
platform-specific, and is extremely unlikely to be extended to
any platform but GNU/Linux. It doesn't seem a good idea to be vague
about this issue here.

> +Collecting @code{$_sdata} in this case collects the string @samp{hello
> +$yourname}.

What is "$yourname"? Did you mean "master_name"?

> +@cindex information about static tracepoint markers in the target program.

This entry is too long. Perhaps just

@cindex information about static tracepoint markers

is enough. (You don't need the period, either.)

> +libraries. The most simple way to do that is to run the program to
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
"The simplest way to do that ..."

> +@item qTfSTM
> +@itemx qTsSTM

The error responses don't seem to be documented here. Should they be?

> +query), until the target responds with @samp{l} (lower-case el, for
> +@dfn{last}). ^^

"ell"

Thanks.