Re: [PATCH] Revised display-linkage-name

[Keith Seitz <keiths at redhat dot com>]

On 07/22/2013 01:06 PM, Michael Eager wrote:
> On 07/22/13 10:48, Keith Seitz wrote:
> > On 07/17/2013 11:51 AM, Michael Eager wrote:
>
> > >   void
> > > +annotate_linkage_name (void)
> > > +{
> > > +  if (annotation_level == 2)
> > > +    printf_filtered (("\n\032\032linkage_name\n"));
> > > +}
> > > +
> >
> > This is still missing a (trivial) comment. [IIRC, we require comments
> > for *all* functions, even if
> > they are pretty trivial.]
>
> The Changelog contains
>        * annotate.c (annotate_linkage_name): New.
>        * annotate.h (annotate_linkage_name): New decl.
>
> Is something else needed?

Yes, all functions need a comment, e.g.,

/* Emit an annotation for a symbol's linkage name.  */

void
annotate_linkage_name (void)
{
   /* ... */
}

> I believe that the attached updated patch addresses all of your comments.

Pretty much. Just two really minor nits:

> 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"));
> +}

You know about this one already. :-)
> 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..19c6191
> --- /dev/null
> +++ b/gdb/testsuite/gdb.cp/display-linkage-name.exp
> @@ -0,0 +1,114 @@
> +#   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).
> +
> +if { [skip_cplus_tests] } { continue }
> +
> +standard_testfile .cc
> +
> +if {[get_compiler_info "c++"]} {
> +  return -1
> +}
> +
> +if { [prepare_for_testing $testfile.exp $testfile $srcfile {debug c++}] } {
> +    return -1
> +}
> +
> +# set break at functions
> +
> +set bp1 [gdb_get_line_number "set breakpoint 1 here"]
> +set bp2 [gdb_get_line_number "set breakpoint 2 here"]
> +
> +if {![gdb_breakpoint "foo"]} {
> +    fail "set breakpoint foo"
> +}
> +
> +if {![gdb_breakpoint "fun_with_a_long_name"]} {
> +    fail "set breakpoint fun_with_a_long_name"
> +}
> +
> +########################
> +# Test with display-linkage-name off
> +
> +gdb_test_no_output "set display-linkage-name off" ""

You can omit the trailing quotes. gdb_test_no_output will then use the 
command name. Fortunately, this is only used once so the test's name is 
unique.

> +
> +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
> +set test "break at fun_with_a_long_name"
> +gdb_continue_to_breakpoint "fun_with_a_long_name (.*) at.*$srcfile.$bp2"
> +
> +set bttable "#0  foo (.*) at.*\[\r\n\]"
> +append bttable "#1  $hex in fun_with_a_long_name (.*) at .*$srcfile:.*\[\r\n\]"
> +append bttable "#2  $hex in goo (.*) at .*$srcfile:.*\[\r\n\]"
> +append bttable "#3  $hex in main (.*) at .*$srcfile:.*"
> +
> +gdb_test "backtrace" $bttable "backtrace - display off"
> +
> +########################
> +# Test with display-linkage-name on
> +
> +gdb_test_no_output "set display-linkage-name on" ""

Likewise.

> +
> +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
> +set test "break at fun_with_a_long_name - display on"
> +gdb_continue_to_breakpoint "fun_with_a_long_name \\\[_Z20fun_with_a_long_...\\\] (.*) at.*$srcfile.$bp2"
> +
> +set bttable "#0  foo \\\[_Z3fooPKc\\\] (.*) at.*\[\r\n\]"
> +append bttable "#1  $hex in fun_with_a_long_name \\\[_Z20fun_with_a_long_\.\.\.\\\] (.*) at .*$srcfile:.*\[\r\n\]"
> +append bttable "#2  $hex in goo \\\[_Z3goov\\\] (.*) at .*$srcfile:.*\[\r\n\]"
> +append bttable "#3  $hex in main (.*) at .*$srcfile:.*"
> +
> +gdb_test "backtrace" $bttable "backtrace - display on"
> +
> +########################
> +# Test set/show display-linkage-name-len
> +
> +gdb_test "show display-linkage-name-len" \
> +    "Length of linkage names \\(symbol used by linker\\) to be displayed is 20."

NOTE 1 (see below)

> +
> +gdb_test_no_output "set display-linkage-name-len 10" ""
> +

Likewise.

> +gdb_test "show display-linkage-name-len" \
> +    "Length of linkage names \\(symbol used by linker\\) to be displayed is 10."

NOTE 2: These two tests output the same test name. You should supply the 
(optional) third parameter to gdb_test (MESSAGE) to uniquely identify 
this test. [You could simply use "show display-linkage-name-len 20" and 
"show display-linkage-name-len 10".]

For future reference, you can use:

$ make check RUNTESTFLAGS="my-test.exp"
$ cat testsuite/gdb.sum | grep "PASS" | sort | uniq -c | sort -n

to determine whether all the test names in your test file are unique. I 
apologize, I missed this earlier.

> +
> +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
> +set test "break at fun_with_a_long_name - display 10"
> +gdb_continue_to_breakpoint "fun_with_a_long_name \\\[_Z20fun_wi\.\.\.\\\] (.*) at .*$srcfile:$bp2"
> +
> +set bttable "#0  foo \\\[_Z3fooPKc\\\] (.*) at.*\[\r\n\]"
> +append bttable "#1  $hex in fun_with_a_long_name \\\[_Z20fun_wi\.\.\.\\\] (.*) at .*$srcfile:.*\[\r\n\]"
> +append bttable "#2  $hex in goo \\\[_Z3goov\\\] (.*) at .*$srcfile:.*\[\r\n\]"
> +append bttable "#3  $hex in main (.*) at .*$srcfile:.*"
> +
> +gdb_test "backtrace" $bttable "backtrace - display 10"
> +
> diff --git a/gdb/top.c b/gdb/top.c
> index 46faaa7..c0f6f25 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 = 0;		/* Default is no.  */
> +int display_linkage_name_len = 20; 	/* Default is first 20 chars.  */
> +
> +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) of functions is %s.\n"),
> +		    value);
> +}
> +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 names (symbol used by linker) to be displayed is %s.\n"),
> +		    value);
> +}

As silly as it seems, these two new functions need a comment.

>   
>   /* 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);
> +
> +  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,
> +			   NULL,
> +			   show_display_linkage_name_len,
> +			   &setlist, &showlist);
> +
>   }
>
>   void

After you fix those trivial things, I don't think there is anything more 
for me to review. You've already got documentation approval from Eli, so 
all that's left is a global maintainer's stamp of approval.

Keith