From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 31825 invoked by alias); 22 Jul 2013 21:55:17 -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 31727 invoked by uid 89); 22 Jul 2013 21:55:17 -0000 X-Spam-SWARE-Status: No, score=-5.1 required=5.0 tests=AWL,BAYES_50,KHOP_THREADED,RCVD_IN_HOSTKARMA_W,RCVD_IN_HOSTKARMA_WL,RDNS_NONE,SPF_HELO_PASS,SPF_PASS,TW_BT autolearn=no version=3.3.1 Received: from Unknown (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.84/v0.84-167-ge50287c) with ESMTP; Mon, 22 Jul 2013 21:55:16 +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 r6MLt8QY016976 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Mon, 22 Jul 2013 17:55:08 -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 r6MLt6hx020916 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Mon, 22 Jul 2013 17:55:07 -0400 Message-ID: <51EDAA3A.5090504@redhat.com> Date: Mon, 22 Jul 2013 21:55:00 -0000 From: Keith Seitz User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130625 Thunderbird/17.0.7 MIME-Version: 1.0 To: Michael Eager CC: "gdb-patches@sourceware.org ml" Subject: Re: [PATCH] Revised display-linkage-name References: <519D086A.50105@eagerm.com> <51BF47DB.6070709@eagerm.com> <51DD891D.7090009@eagerm.com> <51DF3F97.90805@redhat.com> <51E07263.6080605@eagerm.com> <51E6E797.30709@eagerm.com> <51ED705A.5000601@redhat.com> <51ED90E3.30801@eagerm.com> In-Reply-To: <51ED90E3.30801@eagerm.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-SW-Source: 2013-07/txt/msg00519.txt.bz2 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. > + > +# 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