From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 3910 invoked by alias); 16 Apr 2010 09:38:45 -0000 Received: (qmail 3895 invoked by uid 22791); 16 Apr 2010 09:38:43 -0000 X-SWARE-Spam-Status: No, hits=1.6 required=5.0 tests=BAYES_50,SPF_SOFTFAIL,TW_BJ,TW_CP X-Spam-Check-By: sourceware.org Received: from mtaout20.012.net.il (HELO mtaout20.012.net.il) (80.179.55.166) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Fri, 16 Apr 2010 09:38:34 +0000 Received: from conversion-daemon.a-mtaout20.012.net.il by a-mtaout20.012.net.il (HyperSendmail v2007.08) id <0L0Y00C00QMDME00@a-mtaout20.012.net.il> for gdb-patches@sourceware.org; Fri, 16 Apr 2010 12:37:26 +0300 (IDT) Received: from HOME-C4E4A596F7 ([77.127.69.249]) by a-mtaout20.012.net.il (HyperSendmail v2007.08) with ESMTPA id <0L0Y00CM3QQCLI00@a-mtaout20.012.net.il>; Fri, 16 Apr 2010 12:37:26 +0300 (IDT) Date: Fri, 16 Apr 2010 09:38:00 -0000 From: Eli Zaretskii Subject: Re: [RFA] [1/2] auto-loading scripts from .debug_gdb_scripts section In-reply-to: <20100416070540.4205E84396@ruffy.mtv.corp.google.com> To: dje@google.com (Doug Evans) Cc: gdb-patches@sourceware.org Reply-to: Eli Zaretskii Message-id: <83tyrbwxex.fsf@gnu.org> References: <20100416070540.4205E84396@ruffy.mtv.corp.google.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-04/txt/msg00506.txt.bz2 > Date: Fri, 16 Apr 2010 00:05:40 -0700 (PDT) > From: dje@google.com (Doug Evans) > > This patch adds support for auto-loading scripts mentioned in > section .debug_gdb_scripts. Thanks. I reviewed the documentation part of the patch. There's also a single comment to the code part. > I moved the Auto-loading section of the docs out of the Python API section > because I like it better that way, and because it solves a technical > problem: there's no @subsubsubsection. :-) The existing Python Auto-loading section is not too long, so how about simply adding the description of this new feature to that section? I think ripping it out of "Python API" is not a good idea. Btw, is this auto-loading feature supposed to be used only for Python scripts, or does it support scripts written in the GDB scripting language as well? If the former, I think we should use "Python scripts" more often than we do now, to avoid duping the user into thinking otherwise. > +** GDB now looks for scripts to auto-load in the .debug_gdb_scripts section. This is rather cryptic, especially since this section is our own invention. How about this text instead: ** GDB now looks for names of Python scripts to auto-load in a special section named `.debug_gdb_scripts', in addition to looking for a OBJFILE-gdb.py script when OBJFILE is read by the debugger. > +When a new object file is read (for example, due to the @code{file} > +command, or because the inferior has loaded a shared library), > +@value{GDBN} will look for support scripts in up to two ways: ^^^^^^^^^^^^^^^^^ I suggest "in several ways", because 2 is just what we currently support, and we may extend that to more ways in the future. > +@file{@var{objfile}-gdb.py} and @file{.debug_gdb_scripts} section. `.debug_gdb_scripts' is a name of a section, not a file name, so it should have the @code markup (here and elsewhere). > +The auto-loading feature is useful for supplying application-specific > +debugging commands and scripts. Application-specific or "specific to an objfile"? I think it's the latter, because the former is adequately supported by a .gdbinit file in the source tree. > +When reading an auto-loaded file, @value{GDBN} sets the ``current > +objfile''. This is available via the @code{gdb.current_objfile} > +function (@pxref{Objfiles In Python}). This can be useful for > +registering objfile-specific pretty-printers. I think this paragraph should be moved to after you describe the "maint set python auto-load" commands, because it's a minor aspect of the features. In addition, please use @dfn{current objfile} instead of the explicit quotes, since you are introducing new terminology here. Btw, why are these "maint" commands? Aren't they useful to Joe Random Hacker? > +@node @file{@var{objfile}-gdb.py} file Using @-commands in node names is a no-no, see the node "Node Line Requirements" in the Texinfo manual. > +@subsubsection @file{@var{objfile}-gdb.py} file Chapter, section, and subsubsection names should follow the English grammar (node names are exempt from this requirement), so please use "The @file{@var{objfile}-gdb.py} file" instead (assuming you leave this as a separate node, which I advise against). > +If this file does not exist, and if the parameter > +@code{debug-file-directory} is set (@pxref{Separate Debug Files}), > +then @value{GDBN} will use for its each separated directory component > +@code{component} the file named @file{@code{component}/@var{real-name}}, where > +@var{real-name} is the object file's real name, as described above. I know you just copied this from the original text, but this is confusingly complicated. How about this rewording: If this file does not exist, and if the parameter @code{debug-file-directory} is set (@pxref{Separate Debug Files}), then @value{GDBN} will look for the file in that directory and in all of its parents. Btw, what does the text mean when it says "by ensuring that the file name is absolute"? Is this a requirement to the form of OBJFILE as it is recorded in the binary? Or does it mean GDB converts it to an absolute file name if it is not? In the former case, I presume that @var{real-name} in the original text corresponds to the basename of the script's name, not to its full absolute file name; is that correct? If the latter, then what does GDB do if the file name is _not_ absolute? > +Finally, if this file does not exist, then @value{GDBN} will look for > +a file named @file{@var{data-directory}/python/auto-load/@var{real-name}}, where > +@var{data-directory} is @value{GDBN}'s data directory (available via > +@code{show data-directory}, @pxref{Data Files}), Doesn't @pxref produce a capitalized "See" for this @pxref? If so, please use "see @ref" instead, or move "available via ..." part out of the parentheses. > and @var{real-name} > +is the object file's real name, as described above. Again, what about the leading directories in @var{real-name}? > +So, your @samp{-gdb.py} file should take care to ensure that it may be > +evaluated multiple times without error. So your @samp{-gdb.py} file should take care to ensure that, if it is evaluated multiple times, it does that without errors. Or, better yet So your @file{-gdb.py} file should be careful to avoid errors if it is evaluated more than once. > +@node @file{.debug_gdb_scripts} section > +@subsubsection @file{.debug_gdb_scripts} section Again, don't use @-commands in node names and use "The" in the subsubsection name. > +@cindex .debug_gdb_scripts section Please give `.debug_gdb_scripts' a @code markup here (there's no problem doing that in @cindex entries, which by default are typeset in normal Roman typeface, unlike @findex etc.). > +For systems using the ELF file format, and other formats supporting > +multiple sections, It would be good to state which platforms support multiple sections. (I think all modern platforms do, in native debugging, but maybe I'm wrong.) The reader should not be required to be an expert in binary formats, in order to understand whether this can be used on her platform. > when @value{GDBN} loads an @var{objfile} > +it will look for scripts in the @file{.debug_gdb_scripts} section > +when an objfile is loaded. > +This section contains a list of names of scripts to load. when @value{GDBN} loads a new object file @var{objfile}, it will look for a special section named @samp{.debug_gdb_scripts}. If this section exists, its contents should be a string specifying the name of the script file to load. IOW, first say that GDB look for the section, then what its contents are, and only then that the specified file will be auto-loaded. > +File names are recorded instead of the file's contents so that > +the user doesn't have to relink if a script changes, > +and so that the plethora of random scripts are better managed, If this is important, it should be at most in a @footnote. We don't generally explain our design decisions in the user manual. > +The scripts are searched for first in the current directory, and then > +in the source search path @value{GDBN} will look for the specified script file first in the current directory and then along the source search path IOW, avoid unnecessary passive tense. > +with the exception that @file{$cdir} is not searched, the compilation > +directory is not relevant to scripts. except that @file{$cdir} is not searched, since the compilation directory is not relevant to scripts. > +@example > +#define DEFINE_GDB_SCRIPT(script_name) \ > + asm("\ > +.pushsection \".debug_gdb_scripts\", \"MS\",@@progbits,1\n\ > +.byte 1\n\ > +.asciz \"" script_name "\"\n\ > +.popsection \n\ > +"); > +@end example I'm far from being an expert on inline assembly, but doesn't the ".asciz" line have too many quotes, the script_name argument being itself in quotes? > +Then one can reference the macro in a header or source file like this: Please put @noindent before this line, because it does not start a new paragraph. It is an explanation for the @example. > +@example > +DEFINE_GDB_SCRIPT ("my-app-scripts.py") > +@end example We should tell if the file name can be absolute, or whether it can include any leading directories. Btw, does the format of sections impose any limitations on characters that can appear in this file name? If it does, we should state them. > +Unlike loading of @file{@var{objfile}-gdb.py} scripts, > +@value{GDBN} keeps track of scripts loaded this way. > +If the reference to the script is coming from a header file, > +the script can be referenced by multiple shared libraries. > +As an optimization to prevent loading the script 100's or even 1000's > +of times, @value{GDBN} will only auto-load a script once in each > +program space. > +However, you should still write your scripts to ensure > +that it may be evaluated multiple times without error. The last sentence means, IMO, that this paragraph should be removed. The user should not need to think differently about the script depending on how it is loaded, because it's quite reasonable to expect the same script to be loaded using both methods in the same application. This text just adds confusion, IMO. > +@node Which flavor to choose? > +@subsubsection Which flavor to choose? > + > +Given the two ways of auto-loading scripts, it might not always be clear > +which one to choose. This section provides some guidance. Btw, WIBNI the places where GDB searches for the two kinds of auto-loaded scripts were not as dissimilar as they are now? Why do we need them to be so different? After all, there's a reasonable chance that both these methods will be used in the same project, so why force the users to spread the same scripts in several different places? > +For publicly installed libraries, e.g. libstdc++, there typically isn't a ^^^^^^^^^^^^^^ e.g., @file{libstdc++} (Note the comma after "e.g.", without it TeX will typeset the "e.g." as if it ends a sentence.) > +@kindex maint print section-scripts > +@cindex info for known .debug_gdb_scripts-loaded scripts > +@item maint print section-scripts [@var{regexp}] > +Print a dump of scripts specified in the @file{.debug_gdb_section} section. > +If @var{regexp} is specified, only print scripts loaded by object files > +matching @var{regexp}. > +For each script, this command prints its name as specified in the objfile, > +and the full path if known. This should have an @xref to the node where this feature is described. > + if (!input && debug_file_directory) > + { > + /* Also try the same file in the separate debug info directory. */ > + debugfile = xmalloc (strlen (filename) > + + strlen (debug_file_directory) + 1); > + strcpy (debugfile, debug_file_directory); > + /* FILENAME is absolute, so we don't need a "/" here. */ > + strcat (debugfile, filename); What will that last strcat do if FILENAME has a drive letter? > + strcpy (debugfile, gdb_datadir); > + strcat (debugfile, "/auto-load"); > + /* FILENAME is absolute, so we don't need a "/" here. */ > + strcat (debugfile, filename); Ditto. Thanks.