From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 13327 invoked by alias); 11 Jul 2013 23:28:29 -0000 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 Received: (qmail 13317 invoked by uid 89); 11 Jul 2013 23:28:28 -0000 X-Spam-SWARE-Status: No, score=-5.3 required=5.0 tests=AWL,BAYES_20,KAM_STOCKGEN,KHOP_THREADED,RCVD_IN_HOSTKARMA_W,RCVD_IN_HOSTKARMA_WL,RP_MATCHES_RCVD,SPF_HELO_PASS,SPF_PASS,TW_BT,TW_CP autolearn=no version=3.3.1 Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.84/v0.84-167-ge50287c) with ESMTP; Thu, 11 Jul 2013 23:28:26 +0000 Received: from int-mx01.intmail.prod.int.phx2.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r6BNSOjB027368 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Thu, 11 Jul 2013 19:28:24 -0400 Received: from valrhona.uglyboxes.com (ovpn01.gateway.prod.ext.phx2.redhat.com [10.5.9.1]) by int-mx01.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id r6BNSNL3013466 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Thu, 11 Jul 2013 19:28:24 -0400 Message-ID: <51DF3F97.90805@redhat.com> Date: Thu, 11 Jul 2013 23:28:00 -0000 From: Keith Seitz User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130514 Thunderbird/17.0.6 MIME-Version: 1.0 To: Michael Eager CC: gdb-patches@sourceware.org Subject: Re: [PATCH] Revised display-linkage-name References: <519D086A.50105@eagerm.com> <51BF47DB.6070709@eagerm.com> <51DD891D.7090009@eagerm.com> In-Reply-To: <51DD891D.7090009@eagerm.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-SW-Source: 2013-07/txt/msg00329.txt.bz2 On 07/10/2013 09:17 AM, Michael Eager wrote: > Can someone review and approve this patch? Tom's been a little busy of late, so I thought I would try to help out here a little. You'll be one step closer to approval! Your patches no longer apply cleanly to HEAD, so I fixed them up to play with them. I'll be commenting on this version. > diff --git a/gdb/NEWS b/gdb/NEWS > index e469f1e..40addb1 100644 > --- a/gdb/NEWS > +++ b/gdb/NEWS > @@ -56,6 +56,20 @@ show range-stepping > --configuration > Display the details of GDB configure-time options. > > +* New commands have been added to select whether to display the > + linker symbol name for functions in addition to the name used in the > + source. This may be useful when debugging programs where the compiler > + prepends characters to the source symbol, such as a leading underscore: "prepends" or "prefixes"? > + > +set|show display-linkage-name [off|on] > + > + The default is "off", to not display the linkage name. > + > +set|show display-linkage-name-len > + > + Set the maximum number of characters to display in the linkage name, > + if display-linkage-name is on. The default is 20. > + > * The command 'tsave' can now support new option '-ctf' to save trace > buffer in Common Trace Format. > > diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c > index 8240fee..e9e0002 100644 > --- a/gdb/ada-lang.c > +++ b/gdb/ada-lang.c > @@ -11114,6 +11114,7 @@ is_known_support_routine (struct frame_info *frame) > { > struct symtab_and_line sal; > char *func_name; > + const char *linkname; > enum language func_lang; > int i; > const char *fullname; > @@ -11152,7 +11153,7 @@ is_known_support_routine (struct frame_info *frame) > > /* Check whether the function is a GNAT-generated entity. */ > > - find_frame_funname (frame, &func_name, &func_lang, NULL); > + find_frame_funname (frame, &func_name, &linkname, &func_lang, NULL); > if (func_name == NULL) > return 1; > `linkname' is unused here -- more about this later. > @@ -11225,9 +11226,10 @@ ada_unhandled_exception_name_addr_from_raise (void) > while (fi != NULL) > { > char *func_name; > + const char *linkname; > enum language func_lang; > > - find_frame_funname (fi, &func_name, &func_lang, NULL); > + find_frame_funname (fi, &func_name, &linkname, &func_lang, NULL); > if (func_name != NULL) > { > make_cleanup (xfree, func_name); Likewise. > diff --git a/gdb/annotate.c b/gdb/annotate.c > index ccba5fe..84edeac 100644 > --- a/gdb/annotate.c > +++ b/gdb/annotate.c > @@ -582,6 +582,13 @@ breakpoint_changed (struct breakpoint *b) > } > > void > +annotate_linkage_name (void) > +{ > + if (annotation_level == 2) > + printf_filtered (("\n\032\032linkage_name\n")); > +} > + This is a new function and will require a (trivial) comment. > +void > _initialize_annotate (void) > { > observer_attach_breakpoint_created (breakpoint_changed); > diff --git a/gdb/annotate.h b/gdb/annotate.h > index 72c4f19..33a9a0e 100644 > --- a/gdb/annotate.h > +++ b/gdb/annotate.h > @@ -98,5 +98,7 @@ extern void annotate_elt_rep_end (void); > extern void annotate_elt (void); > extern void annotate_array_section_end (void); > > +extern void annotate_linkage_name (void); > + Likewise. > extern void (*deprecated_annotate_signalled_hook) (void); > extern void (*deprecated_annotate_signal_hook) (void); > diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c > index 4d09b30..b717021 100644 > --- a/gdb/breakpoint.c > +++ b/gdb/breakpoint.c > @@ -5704,6 +5704,33 @@ print_breakpoint_location (struct breakpoint *b, > ui_out_text (uiout, "in "); > ui_out_field_string (uiout, "func", > SYMBOL_PRINT_NAME (sym)); > + if (display_linkage_name) > + { > + struct bound_minimal_symbol msymbol = > + lookup_minimal_symbol_by_pc (loc->address); > + if (msymbol.minsym != NULL > + && strcmp (SYMBOL_LINKAGE_NAME (msymbol.minsym), > + SYMBOL_PRINT_NAME (sym)) != 0) > + { > + ui_out_text (uiout, " ["); > + > + if (strlen (SYMBOL_LINKAGE_NAME (msymbol.minsym)) > > + display_linkage_name_len) Operators should never end a line -- move '>' to the beginning of the next. > + { > + char *lname = alloca (display_linkage_name_len + 4); I believe we are now requiring a blank line after variable declarations. > + strncpy (lname, SYMBOL_LINKAGE_NAME (msymbol.minsym), > + display_linkage_name_len); > + lname[display_linkage_name_len] = '\0'; > + strcat (lname, "..."); > + ui_out_field_string (uiout, "linkage-name", lname); > + } > + else > + ui_out_field_string (uiout, "linkage-name", > + SYMBOL_LINKAGE_NAME (msymbol.minsym)); > + > + ui_out_text (uiout, "]"); > + } > + } > ui_out_text (uiout, " "); > ui_out_wrap_hint (uiout, wrap_indent_at_field (uiout, "what")); > ui_out_text (uiout, "at "); There's no mention of this [optional] MI parameter in the manual. There probably should be. > diff --git a/gdb/defs.h b/gdb/defs.h > index 014d7d4..de5f785 100644 > --- a/gdb/defs.h > +++ b/gdb/defs.h > @@ -87,7 +87,7 @@ > /* The O_BINARY flag is defined in fcntl.h on some non-Posix platforms. > It is used as an access modifier in calls to open(), where it acts > similarly to the "b" character in fopen()'s MODE argument. On Posix > - platforms it should be a no-op, so it is defined as 0 here. This > + platforms it should be a no-op, so it is defined as 0 here. This > ensures that the symbol may be used freely elsewhere in gdb. */ Superfluous whitespace change? > > #ifndef O_BINARY > @@ -161,7 +161,7 @@ extern char *debug_file_directory; > handler. Otherwise, SIGINT simply sets a flag; code that might > take a long time, and which ought to be interruptible, checks this > flag using the QUIT macro. > - > + Likewise? > If GDB is built with Python support, it uses Python's low-level > interface to implement the flag. This approach makes it possible > for Python and GDB SIGINT handling to coexist seamlessly. > @@ -334,10 +334,11 @@ extern int print_address_symbolic (struct gdbarch *, CORE_ADDR, > > extern int build_address_symbolic (struct gdbarch *, > CORE_ADDR addr, > - int do_demangle, > - char **name, > - int *offset, > - char **filename, > + int do_demangle, > + char **name, > + char **linkname, > + int *offset, > + char **filename, > int *line, > int *unmapped); More here? [I see that there is one actual change here, but it is buried inside several unnecessary changes.] > > @@ -717,9 +718,9 @@ extern int (*deprecated_ui_loop_hook) (int signo); > extern void (*deprecated_init_ui_hook) (char *argv0); > extern void (*deprecated_command_loop_hook) (void); > extern void (*deprecated_show_load_progress) (const char *section, > - unsigned long section_sent, > - unsigned long section_size, > - unsigned long total_sent, > + unsigned long section_sent, > + unsigned long section_size, > + unsigned long total_sent, > unsigned long total_size); More superfluous whitespace changes? > extern void (*deprecated_print_frame_info_listing_hook) (struct symtab * s, > int line, > diff --git a/gdb/disasm.c b/gdb/disasm.c > index e643c2d..62fa34d 100644 > --- a/gdb/disasm.c > +++ b/gdb/disasm.c > @@ -112,6 +112,7 @@ dump_insns (struct gdbarch *gdbarch, struct ui_out *uiout, > { > char *filename = NULL; > char *name = NULL; > + char *linkname = NULL; > > QUIT; > if (how_many >= 0) > @@ -127,8 +128,8 @@ dump_insns (struct gdbarch *gdbarch, struct ui_out *uiout, > ui_out_text (uiout, pc_prefix (pc)); > ui_out_field_core_addr (uiout, "address", gdbarch, pc); > > - if (!build_address_symbolic (gdbarch, pc, 0, &name, &offset, &filename, > - &line, &unmapped)) > + if (!build_address_symbolic (gdbarch, pc, 0, &name, &linkname, &offset, > + &filename, &line, &unmapped)) > { > /* We don't care now about line, filename and > unmapped. But we might in the future. */ > @@ -146,6 +147,7 @@ dump_insns (struct gdbarch *gdbarch, struct ui_out *uiout, > xfree (filename); > if (name != NULL) > xfree (name); > + xfree (linkname); > > ui_file_rewind (stb); > if (flags & DISASSEMBLY_RAW_INSN) > diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo > index fae54e4..5d7b249 100644 > --- a/gdb/doc/gdb.texinfo > +++ b/gdb/doc/gdb.texinfo > @@ -15933,8 +15933,31 @@ line 1574. > @} > (@value{GDBP}) > @end smallexample > -@end table > > +@kindex set display-linkage-name @code{on}|@code{off} > +@kindex show display-linkage-name > +@cindex list linker symbol names > +@item set display-linkage-name > +@itemx show display-linkage-name > +Control displaying linker symbol names for functions. > + > +The default is @code{off}, which means that @value{GDBN} will only > +display the function name used in the source. When @code{on}, @value{GDBN} > +will also display the symbol name used by the linker within brackets if it is > +different from the name in the source. This can be useful with compilers > +which may prepend characters to a source name, for example, an underscore. "prefix" or "prepend"? [Yeah, that's a nit (again): I don't want to see slang creep into the manual. Eli to have final word on the use of that "word."] > + > +This is different from "set print asm-demangle on" which only displays > +the linkage name for C++ symbols and does not display the source name. Should that be "C@t{++}"? > + > +@kindex set display-linkage-name-len > +@kindex show display-linkage-name-len > +@cindex list linker symbol names > +@item set display-linkage-name-len @var{len} > +@itemx show display-linkage-name-len @var{len} > +Set the maximum number of characters of linkage name to display. The > +@code{show} command displays the current setting. The default is @code{20}. > +@end table > > @node Altering > @chapter Altering Execution > diff --git a/gdb/mt-tdep.c b/gdb/mt-tdep.c > index a863cee..22e1cbc 100644 > --- a/gdb/mt-tdep.c > +++ b/gdb/mt-tdep.c > @@ -718,7 +718,7 @@ mt_registers_info (struct gdbarch *gdbarch, > print_spaces_filtered (15 - strlen (gdbarch_register_name > (gdbarch, regnum)), > file); > - get_raw_print_options (&opts); > + get_no_prettyformat_print_options (&opts); > opts.deref_ref = 1; > val_print (register_type (gdbarch, regnum), buf, > 0, 0, file, 0, NULL, This is not mentioned in the ChangeLog, and it is superfluous anyway. I'm guessing to workaround the recent build breakage? > diff --git a/gdb/printcmd.c b/gdb/printcmd.c > index 99d4dba..66dc6a5 100644 > --- a/gdb/printcmd.c > +++ b/gdb/printcmd.c > @@ -51,6 +51,7 @@ > #include "cli/cli-utils.h" > #include "format.h" > #include "source.h" > +#include "top.h" > > #ifdef TUI > #include "tui/tui.h" /* For tui_active et al. */ > @@ -569,17 +570,19 @@ print_address_symbolic (struct gdbarch *gdbarch, CORE_ADDR addr, > int do_demangle, char *leadin) > { > char *name = NULL; > + char *linkname = NULL; > char *filename = NULL; > int unmapped = 0; > int offset = 0; > int line = 0; > > - /* Throw away both name and filename. */ > + /* Throw away both name, linkname, and filename. */ s/both// > struct cleanup *cleanup_chain = make_cleanup (free_current_contents, &name); > make_cleanup (free_current_contents, &filename); > + make_cleanup (free_current_contents, &linkname); > > - if (build_address_symbolic (gdbarch, addr, do_demangle, &name, &offset, > - &filename, &line, &unmapped)) > + if (build_address_symbolic (gdbarch, addr, do_demangle, &name, &linkname, > + &offset, &filename, &line, &unmapped)) > { > do_cleanups (cleanup_chain); > return 0; > @@ -591,6 +594,27 @@ print_address_symbolic (struct gdbarch *gdbarch, CORE_ADDR addr, > else > fputs_filtered ("<", stream); > fputs_filtered (name, stream); > + > + /* Print linkage name after source name if requested and different. */ > + if (display_linkage_name > + && linkname != NULL && strcmp (name, linkname) != 0) > + { > + fputs_filtered (" [", stream); > + > + if (strlen (linkname) > display_linkage_name_len) > + { > + char *lname = alloca (display_linkage_name_len + 4); A blank line needed here. > + strncpy (lname, linkname, display_linkage_name_len); > + lname[display_linkage_name_len] = '\0'; > + strcat (lname, "..."); > + fputs_filtered (lname, stream); > + } > + else > + fputs_filtered (linkname, stream); Could you double-check the indentation of this block? It doesn't look right. > + > + fputs_filtered ("]", stream); > + } > + > if (offset != 0) > fprintf_filtered (stream, "+%u", (unsigned int) offset); > > @@ -623,6 +647,7 @@ build_address_symbolic (struct gdbarch *gdbarch, > CORE_ADDR addr, /* IN */ > int do_demangle, /* IN */ > char **name, /* OUT */ > + char **linkname, /* OUT */ > int *offset, /* OUT */ > char **filename, /* OUT */ > int *line, /* OUT */ > @@ -637,6 +662,9 @@ build_address_symbolic (struct gdbarch *gdbarch, > /* Let's say it is mapped (not unmapped). */ > *unmapped = 0; > > + /* Let's say the link name is the same as the symbol name. */ > + *linkname = 0; > + > /* Determine if the address is in an overlay, and whether it is > mapped. */ > if (overlay_debugging) > @@ -740,6 +768,12 @@ build_address_symbolic (struct gdbarch *gdbarch, > *line = sal.line; > } > } > + > + /* If we have both symbol names and they are different, let caller know. */ > + if (msymbol != NULL && symbol != NULL > + && strcmp (SYMBOL_LINKAGE_NAME (msymbol), SYMBOL_LINKAGE_NAME (symbol)) != 0) > + *linkname = xstrdup (SYMBOL_LINKAGE_NAME (msymbol)); > + > return 0; > } > > diff --git a/gdb/python/py-frame.c b/gdb/python/py-frame.c > index f960b08..0b0dd8b 100644 > --- a/gdb/python/py-frame.c > +++ b/gdb/python/py-frame.c > @@ -123,6 +123,7 @@ frapy_name (PyObject *self, PyObject *args) > { > struct frame_info *frame; > char *name = NULL; > + const char *linkname; > enum language lang; > PyObject *result; > volatile struct gdb_exception except; > @@ -131,7 +132,7 @@ frapy_name (PyObject *self, PyObject *args) > { > FRAPY_REQUIRE_VALID (self, frame); > > - find_frame_funname (frame, &name, &lang, NULL); > + find_frame_funname (frame, &name, &linkname, &lang, NULL); > } > `linkname' is also not used here (see below). > if (except.reason < 0) > diff --git a/gdb/stack.c b/gdb/stack.c > index 313d57f..67e230d 100644 > --- a/gdb/stack.c > +++ b/gdb/stack.c > @@ -55,6 +55,7 @@ > #include "psymtab.h" > #include "symfile.h" > #include "python/python.h" > +#include "top.h" > > void (*deprecated_selected_frame_level_changed_hook) (int); > > @@ -1000,15 +1001,20 @@ get_last_displayed_sal (struct symtab_and_line *sal) > > > /* Attempt to obtain the FUNNAME, FUNLANG and optionally FUNCP of the function > - corresponding to FRAME. FUNNAME needs to be freed by the caller. */ > + corresponding to FRAME. > + Set linkname to linkage name (symbol used by linker) if linkname is non-null > + and linkage name differs from source name. > + FUNNAME needs to be freed by the caller. */ You want to use linkname here in caps, since it is the value of `linkname' that is being referred to (just as for FUNNAME, FUNLANG, FUNCP). > > void > find_frame_funname (struct frame_info *frame, char **funname, > - enum language *funlang, struct symbol **funcp) > + const char **linkname, enum language *funlang, > + struct symbol **funcp) > { > struct symbol *func; > > *funname = NULL; > + *linkname = NULL; > *funlang = language_unknown; > if (funcp) > *funcp = NULL; It seems that a lot of callers of find_frame_funname don't actually use/care about LINKNAME. Why not treat it like FUNCP and make it optional, allowing callers to explicitly pass NULL for this (optional) parameter? What do you think? > @@ -1045,6 +1051,11 @@ find_frame_funname (struct frame_info *frame, char **funname, > memset (&msymbol, 0, sizeof (msymbol)); > > if (msymbol.minsym != NULL > + && strcmp (SYMBOL_LINKAGE_NAME (msymbol.minsym), > + SYMBOL_LINKAGE_NAME (func)) != 0) > + *linkname = SYMBOL_LINKAGE_NAME (msymbol.minsym); > + > + if (msymbol.minsym != NULL > && (SYMBOL_VALUE_ADDRESS (msymbol.minsym) > > BLOCK_START (SYMBOL_BLOCK_VALUE (func)))) > { > @@ -1102,6 +1113,7 @@ print_frame (struct frame_info *frame, int print_level, > struct gdbarch *gdbarch = get_frame_arch (frame); > struct ui_out *uiout = current_uiout; > char *funname = NULL; > + const char *linkname = NULL; > enum language funlang = language_unknown; > struct ui_file *stb; > struct cleanup *old_chain, *list_chain; > @@ -1115,7 +1127,7 @@ print_frame (struct frame_info *frame, int print_level, > stb = mem_fileopen (); > old_chain = make_cleanup_ui_file_delete (stb); > > - find_frame_funname (frame, &funname, &funlang, &func); > + find_frame_funname (frame, &funname, &linkname, &funlang, &func); > make_cleanup (xfree, funname); > > annotate_frame_begin (print_level ? frame_relative_level (frame) : 0, > @@ -1147,9 +1159,32 @@ print_frame (struct frame_info *frame, int print_level, > fprintf_symbol_filtered (stb, funname ? funname : "??", > funlang, DMGL_ANSI); > ui_out_field_stream (uiout, "func", stb); > + > + /* Print linkage name after source name if requested and different. */ > + if ((display_linkage_name || ui_out_is_mi_like_p (uiout)) > + && linkname != NULL && strcmp (funname, linkname) != 0) > + { > + annotate_linkage_name (); > + ui_out_text (uiout, " ["); > + > + if (strlen (linkname) > display_linkage_name_len) > + { > + char *lname = alloca (display_linkage_name_len + 4); A blank line is needed here. > + strncpy (lname, linkname, display_linkage_name_len); > + lname[display_linkage_name_len] = '\0'; > + strcat (lname, "..."); > + ui_out_text (uiout, lname); > + } > + else > + ui_out_text (uiout, linkname); > + > + ui_out_text (uiout, "]"); > + ui_out_field_stream (uiout, "linkage_name", stb); > + } > + > ui_out_wrap_hint (uiout, " "); > annotate_frame_args (); > - > + Unnecessary whitespace change? > ui_out_text (uiout, " ("); > if (print_args) > { > diff --git a/gdb/stack.h b/gdb/stack.h > index 4badf19..9248c85 100644 > --- a/gdb/stack.h > +++ b/gdb/stack.h > @@ -23,7 +23,8 @@ > void select_frame_command (char *level_exp, int from_tty); > > void find_frame_funname (struct frame_info *frame, char **funname, > - enum language *funlang, struct symbol **funcp); > + const char **linkname, enum language *funlang, > + struct symbol **funcp); > > typedef void (*iterate_over_block_arg_local_vars_cb) (const char *print_name, > struct symbol *sym, > diff --git a/gdb/testsuite/gdb.cp/display-linkage-name.cc b/gdb/testsuite/gdb.cp/display-linkage-name.cc > new file mode 100644 > index 0000000..93b2ba2 > --- /dev/null > +++ b/gdb/testsuite/gdb.cp/display-linkage-name.cc > @@ -0,0 +1,19 @@ You should probably add a copyright header to this file. It might be optional (one of the maintainers to verify), but IMO better safe than sorry. FWIW, I *always* add a copyright header to any new file I commit. > +void foo (const char *msg) > +{ > +} /* set breakpoint 1 here */ > + > +void fun_with_a_long_name (int i, const char *s, double d) > +{ > + foo ("Hello"); /* set breakpoint 2 here */ > +} > + > +void goo (void) > +{ > + fun_with_a_long_name (1, "abc", 3.14); > +} > + > +int main (void) > +{ > + goo(); > + return 0; > +} > diff --git a/gdb/testsuite/gdb.cp/display-linkage-name.exp b/gdb/testsuite/gdb.cp/display-linkage-name.exp > new file mode 100644 > index 0000000..aa077c4 > --- /dev/null > +++ b/gdb/testsuite/gdb.cp/display-linkage-name.exp > @@ -0,0 +1,153 @@ > +# Copyright 2013 Free Software Foundation, Inc. > + > +# This program is free software; you can redistribute it and/or modify > +# it under the terms of the GNU General Public License as published by > +# the Free Software Foundation; either version 3 of the License, or > +# (at your option) any later version. > +# > +# This program is distributed in the hope that it will be useful, > +# but WITHOUT ANY WARRANTY; without even the implied warranty of > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > +# GNU General Public License for more details. > +# > +# You should have received a copy of the GNU General Public License > +# along with this program. If not, see . > + > +# This file was written by Michael Eager (eager@eagercon.com). I guess some are now discouraging adding any attribution, but I don't necessarily agree with that. Just a preemptive warning that another maintainer might ask you to remove this. [But *I* am not. :-)] > + > +if { [skip_cplus_tests] } { continue } > + > +standard_testfile .cc > + > +if [get_compiler_info "c++"] { [My personal nit] Please add '{'/'}' around all expressions. [I realize you probably just cut-n-paste much of this like many of us do...] > + return -1 > +} > + > +if { [prepare_for_testing display-linkage-name.exp display-linkage-name display-linkage-name.cc {debug c++}] } { Eek. That's a really long line. While I don't think we have any hard rules about wrapping in the test suite, could you split this up a bit? [You can insert '\' somewhere in that.] Actually, I think you can shorten this using the globals set by standard_testfile: if {[prepare_for_testing $testfile.exp $testfile $srcfile {debug c++}} { ... } > + return -1 > +} > +set srcfile display-linkage-name.cc This should not be necessary -- standard_testfile should set that for you. > + > +set bp1 [gdb_get_line_number "set breakpoint 1 here"] > +set bp2 [gdb_get_line_number "set breakpoint 2 here"] > + > +# > +# test display-linkage-name commands. > +# > + > +gdb_test "set display-linkage-name off" "" You can use gdb_test_no_output to do this. > + > +# > +# set break at functions > +# > +gdb_test "break foo" \ > + "Breakpoint.*at.* file .*$srcfile, line $bp1.*" \ > + "breakpoint function" > + > +gdb_test "break fun_with_a_long_name" \ > + "Breakpoint.*at.* file .*$srcfile, line $bp2.*" \ > + "breakpoint function" You can use the convenience function gdb_breakpoint to set these breakpoints. These two tests also have the same name; they should be different. > + > +gdb_test "info break" \ > + "Num Type\[ \]+Disp Enb Address\[\t \]+What.* > +1\[\t \]+breakpoint keep y.* in foo(.*) at .*$srcfile:$bp1.* > +2\[\t \]+breakpoint keep y.* in fun_with_a_long_name(.*) at .*$srcfile:$bp2.*" \ > + "info breakpoint - display off" > + > +gdb_run_cmd > +gdb_expect { > + -re "Breakpoint 2, fun_with_a_long_name (.*) at .*$srcfile:$bp2.*$gdb_prompt $" { > + pass "run until breakpoint - display off" > + } > + -re "$gdb_prompt $" { > + fail "run until breakpoint - display off" > + } > + timeout { > + fail "run until breakpoint - display off (timeout)" > + } > +} > + > +gdb_test continue "Continuing\\..*Breakpoint 1, foo (.*) at .*$srcfile:$bp1.*\[\t \]+\\}.*" \ > + "continue until function breakpoint - display off" > + You can replace most of this with a call (or two) to gdb_continue_to_breakpoint. [And you would want gdb_test_multiple instead of gdb_expect here anyway.] > +set bttable "#0 foo (.*) at.*\[\r\n\]" > +append bttable "#1 \[0-9a-fx\]+ in fun_with_a_long_name (.*) at .*$srcfile:.*\[\r\n\]" > +append bttable "#2 \[0-9a-fx\]+ in goo (.*) at .*$srcfile:.*\[\r\n\]" > +append bttable "#3 \[0-9a-fx\]+ in main (.*) at .*$srcfile:.*" We have the global "hex" that you can use. > + > +gdb_test "backtrace" $bttable "backtrace - display off" > + > +######################## > +# Test with display-linkage-name on > + > +gdb_test "set display-linkage-name on" "" > + > +gdb_test "info break" \ > + "Num Type\[ \]+Disp Enb Address\[\t \]+What.* > +1\[\t \]+breakpoint keep y.* in foo(.*) \\\[_Z3fooPKc\\\] at .*$srcfile:$bp1.* > +2\[\t \]+breakpoint keep y.* in fun_with_a_long_name(.*) \\\[_Z20fun_with_a_long_\.\.\.\\\] at .*$srcfile:$bp2.*" \ > + "info breakpoint - display on" > + > +gdb_run_cmd > +gdb_expect { > + -re "Breakpoint 2, fun_with_a_long_name \\\[_Z20fun_with_a_long_\.\.\.\\\] (.*) at .*$srcfile:$bp2.*$gdb_prompt $" { > + pass "run until function breakpoint - display on" > + } > + -re "$gdb_prompt $" { > + fail "run until function breakpoint - display on" > + } > + timeout { > + fail "run until function breakpoint (timeout) - display on" > + } > +} > + > +gdb_test continue "Continuing\\..*Breakpoint 1, foo \\\[_Z3fooPKc\\\] (.*) at .*$srcfile:$bp1.*\[\t \]+\\}.*" \ > + "continue until function breakpoint - display on" > + Same here with gdb_continue_to_breakpoint. > +set bttable "#0 foo \\\[_Z3fooPKc\\\] (.*) at.*\[\r\n\]" > +append bttable "#1 \[0-9a-fx\]+ in fun_with_a_long_name \\\[_Z20fun_with_a_long_\.\.\.\\\] (.*) at .*$srcfile:.*\[\r\n\]" > +append bttable "#2 \[0-9a-fx\]+ in goo \\\[_Z3goov\\\] (.*) at .*$srcfile:.*\[\r\n\]" > +append bttable "#3 \[0-9a-fx\]+ in main (.*) at .*$srcfile:.*" > + and $hex. > +gdb_test "backtrace" $bttable "backtrace - display on" > + In fact, all these tests are so similar, you could probably stick all this in a proc and run it multiple times: proc test_expected_frame {with_linkage_name} { # ... set bttable ... if {$with_linkage_name} { set linkage_name "\\\[_Z20fun_with_a_long\.\.\.\\\]" } else { set linkage_name "" } append bttable "#1 $hex+ in fun_with_a_long_name $linkage_name (.*) ..." # ... if {$with_linkage_name} { set with "on" } else { set with "off" } gdb_test "backtrace" $bttable "backtrace - display $with" } Then: test_expected_frame 0 gdb_test_no_output "set display_linkage_name on" test_expected_frame 1 You can probably also fold in the remaining test, too: > +######################## > +# Test set/show display-linkage-name-len > + > +gdb_test "show display-linkage-name-len" \ > + "Length of linkage name \\(symbol used by linker\\) to be displayed is 20." > + > +gdb_test "set display-linkage-name-len 10" "" > + > +gdb_test "show display-linkage-name-len" \ > + "Length of linkage name \\(symbol used by linker\\) to be displayed is 10." > + > +gdb_test "info break" \ > + "Num Type\[ \]+Disp Enb Address\[\t \]+What.* > +1\[\t \]+breakpoint keep y.* in foo(.*) \\\[_Z3fooPKc\\\] at .*$srcfile:$bp1.* > +2\[\t \]+breakpoint keep y.* in fun_with_a_long_name(.*) \\\[_Z20fun_wi\.\.\.\\\] at .*$srcfile:$bp2.*" \ > + "info breakpoint - display 10" > + > +gdb_run_cmd > +gdb_expect { > + -re "Breakpoint 2, fun_with_a_long_name \\\[_Z20fun_wi\.\.\.\\\] (.*) at .*$srcfile:$bp2.*$gdb_prompt $" { > + pass "run until function breakpoint - display 10" > + } > + -re "$gdb_prompt $" { > + fail "run until function breakpoint - display 10" > + } > + timeout { > + fail "run until function breakpoint (timeout) - display 10" > + } > +} > + > +gdb_test continue "Continuing\\..*Breakpoint 1, foo \\\[_Z3fooPKc\\\] (.*) at .*$srcfile:$bp1.*\[\t \]+\\}.*" \ > + "continue until function breakpoint - display 10" > + > +set bttable "#0 foo \\\[_Z3fooPKc\\\] (.*) at.*\[\r\n\]" > +append bttable "#1 \[0-9a-fx\]+ in fun_with_a_long_name \\\[_Z20fun_wi\.\.\.\\\] (.*) at .*$srcfile:.*\[\r\n\]" > +append bttable "#2 \[0-9a-fx\]+ in goo \\\[_Z3goov\\\] (.*) at .*$srcfile:.*\[\r\n\]" > +append bttable "#3 \[0-9a-fx\]+ in main (.*) at .*$srcfile:.*" > + > +gdb_test "backtrace" $bttable "backtrace - display 10" by adding another parameter for the length of the linkage name to the proc. [The tcl command "string range" will be useful to automate this.] But this is starting to get a little elaborate (and I suspect beyond your familiarity with Tcl/expect). So if you can't do it, don't sweat it; I'm not going to stop this patch from going in because of test suite minutia like this. I'm happy to see someone so thorough with testing! > + > diff --git a/gdb/top.c b/gdb/top.c > index 46faaa7..e9476ab 100644 > --- a/gdb/top.c > +++ b/gdb/top.c > @@ -287,6 +287,29 @@ quit_cover (void) > quit_command ((char *) 0, 0); > } > #endif /* defined SIGHUP */ > + > +/* Flag for whether we want to print linkage name for functions. > + Length of linkage name to print. */ > + > +int display_linkage_name = 1; /* Default is yes. */ > +int display_linkage_name_len = 20; /* Default is first 20 chars. */ NEWS and gdb.texinfo say the default is "no". [Keeping it on by default also causes 250+ test regressions!] > + > +static void > +show_display_linkage_name (struct ui_file *file, int from_tty, > + struct cmd_list_element *c, const char *value) > +{ > + fprintf_filtered (file, _("\ > +Whether to display linkage name (symbol used by linker) for functions is %s.\n"), > + value); Not all functions have the same linkage name, so it should be "Whether to display linkage names for functions". Do we really need to explain what a linkage name is here? I don't think so, but perhaps others will disagree. Also, I believe it is more correct to use "of" instead of "for," the linkage names of functions. > +} > +static void > +show_display_linkage_name_len (struct ui_file *file, int from_tty, > + struct cmd_list_element *c, const char *value) > +{ > + fprintf_filtered (file, _("\ > +Length of linkage name (symbol used by linker) to be displayed is %s.\n"), > + value); > +} "names" > > /* Line number we are currently in, in a file which is being sourced. */ > /* NOTE 1999-04-29: This variable will be static again, once we modify > @@ -1809,7 +1832,22 @@ Use \"on\" to enable the notification, and \"off\" to disable it."), > When set, GDB uses the specified path to search for data files."), > set_gdb_datadir, NULL, > &setlist, > - &showlist); > + &showlist); > + > + add_setshow_boolean_cmd ("display-linkage-name", class_support, &display_linkage_name, _("\ > +Set whether to display linkage name (symbol used by linker) for functions."), _("\ > +Show whether to display linkage name (symbol used by linker) for functions."), NULL, > + NULL, > + show_display_linkage_name, > + &setlist, &showlist); "names"/"of" > + > + add_setshow_zinteger_cmd ("display-linkage-name-len", class_support, &display_linkage_name_len, _("\ > +Set number of characters of linkage name to display."), _("\ > +Show number of characters of linkage name to display."), NULL, "names". Missing "the": Set/Show the number of characters... > + NULL, > + show_display_linkage_name_len, > + &setlist, &showlist); > + > } > > void > diff --git a/gdb/top.h b/gdb/top.h > index 2f70539..5acb311 100644 > --- a/gdb/top.h > +++ b/gdb/top.h > @@ -26,6 +26,8 @@ extern int saved_command_line_size; > extern FILE *instream; > extern int in_user_command; > extern int confirm; > +extern int display_linkage_name; > +extern int display_linkage_name_len; > extern char gdb_dirbuf[1024]; > extern int inhibit_gdbinit; > extern const char gdbinit[]; Keith