From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 1697 invoked by alias); 26 Jun 2010 10:34:38 -0000 Received: (qmail 1669 invoked by uid 22791); 26 Jun 2010 10:34:35 -0000 X-SWARE-Spam-Status: No, hits=0.7 required=5.0 tests=AWL,BAYES_50,RCVD_IN_DNSWL_NONE,SPF_SOFTFAIL X-Spam-Check-By: sourceware.org Received: from mtaout22.012.net.il (HELO mtaout22.012.net.il) (80.179.55.172) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Sat, 26 Jun 2010 10:34:27 +0000 Received: from conversion-daemon.a-mtaout22.012.net.il by a-mtaout22.012.net.il (HyperSendmail v2007.08) id <0L4M00F00AN0MD00@a-mtaout22.012.net.il> for gdb-patches@sourceware.org; Sat, 26 Jun 2010 13:33:55 +0300 (IDT) Received: from HOME-C4E4A596F7 ([77.127.155.52]) by a-mtaout22.012.net.il (HyperSendmail v2007.08) with ESMTPA id <0L4M004H9AOIZZX0@a-mtaout22.012.net.il>; Sat, 26 Jun 2010 13:33:55 +0300 (IDT) Date: Sat, 26 Jun 2010 10:34:00 -0000 From: Eli Zaretskii Subject: Re: Static tracepoints support In-reply-to: <201006251931.57860.pedro@codesourcery.com> To: Pedro Alves Cc: gdb-patches@sourceware.org Reply-to: Eli Zaretskii Message-id: <83tyoqcc8i.fsf@gnu.org> References: <201006251931.57860.pedro@codesourcery.com> X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2010-06/txt/msg00611.txt.bz2 > From: Pedro Alves > 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 . > 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.