Re: [PATCH] Revised display-linkage-name

[Keith Seitz <keiths@redhat.com>]

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 <http://www.gnu.org/licenses/>.
> +
> +# 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"
>  \f
>  /* 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