From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 25628 invoked by alias); 23 May 2011 19:52:37 -0000 Received: (qmail 25614 invoked by uid 22791); 23 May 2011 19:52:35 -0000 X-SWARE-Spam-Status: No, hits=-6.1 required=5.0 tests=AWL,BAYES_00,KAM_STOCKGEN,RCVD_IN_DNSWL_HI,SPF_HELO_PASS,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Mon, 23 May 2011 19:52:21 +0000 Received: from int-mx12.intmail.prod.int.phx2.redhat.com (int-mx12.intmail.prod.int.phx2.redhat.com [10.5.11.25]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id p4NJqLQJ021253 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Mon, 23 May 2011 15:52:21 -0400 Received: from ns3.rdu.redhat.com (ns3.rdu.redhat.com [10.11.255.199]) by int-mx12.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id p4NJqKN7012774; Mon, 23 May 2011 15:52:20 -0400 Received: from opsy.redhat.com (ovpn01.gateway.prod.ext.phx2.redhat.com [10.5.9.1]) by ns3.rdu.redhat.com (8.13.8/8.13.8) with ESMTP id p4NJqJvF027454; Mon, 23 May 2011 15:52:19 -0400 Received: by opsy.redhat.com (Postfix, from userid 500) id 346113791A0; Mon, 23 May 2011 13:52:18 -0600 (MDT) From: Tom Tromey To: Jan Kratochvil Cc: Keith Seitz , gdb-patches@sourceware.org Subject: Re: The future of dwarf2_physname References: <4DD44983.7060406@redhat.com> <20110523131659.GA30344@host1.jankratochvil.net> Date: Mon, 23 May 2011 19:52:00 -0000 In-Reply-To: <20110523131659.GA30344@host1.jankratochvil.net> (Jan Kratochvil's message of "Mon, 23 May 2011 15:16:59 +0200") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/23.2 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii 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: 2011-05/txt/msg00541.txt.bz2 Jan> I have concerns about regressions for gdb-7.3, I would like to fix Jan> regressions gdb-7.1 (pre-physname) -> gdb-7.2 (physname) but Jan> possibly not introduce regressions gdb-7.2 (physname) -> gdb-7.3 Jan> (?). I don't see how that is really possible. I am willing to accept some minor regressions for 7.3 in exchange for doing better in the long run. Jan> I was trying to propose DMGL_RET_POSTFIX but it has other drawbacks Jan> and it is in fact a new feature (and not a regression fixup) Jan> because the trailing type was not present in gdb-7.2 anyway. I think we should not take this approach, for the simple reason that it will be weird to C++ users. I think it would be better to just require quotes in this case for 7.3, like "break 'int f ()'", then see about fixing up linespec, or whatever, in 7.4+. Also for later would be making some kind of symbol alias for this, so that in the normal case users can omit the return type. Jan> (gdb) complete p 'f( Jan> p 'f(int) Jan> p 'f(t) Jan> - I do not find it clear this is right. There should be Jan> definitely the `f(int)' - demangled linkage name - variant. But Jan> whether to show the "convenient" aliases as SYMBOL_SEARCH_NAME I Jan> do not find clear, it may be better but it also complicates the Jan> list of all the available overloads for a function name, one Jan> should ask practical C++ programmer for it IMO. I think this one is ok, but I can see how that could go either way. I find it a side issue at present. Jan> Regarding the proposals to fix dwarf2_physname computation instead Jan> of preferring DW_AT_linkage_name. ISTM we have never found the Jan> agreement with Keith, when DW_AT_linkage_name is present it is Jan> guaranteed to be correct and I do not know why not to use it. I think we should move forward with it, on the basis that it is more correct and probably has better performance to boot. I suggest something like the appended, on top of Keith's patches and your patch. The idea here is to prefer the fast path, but have a debug setting so we can easily do physname checking. WDYT? I didn't run it through the test suite or anything yet. Jan> Keith proposes to fix dwarf2_physname instead of using Jan> DW_AT_linkage_name. We must do both in the medium term. Tom diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c index 7530e3e..fff8e3b 100644 --- a/gdb/dwarf2read.c +++ b/gdb/dwarf2read.c @@ -119,6 +119,9 @@ _STATEMENT_PROLOGUE; /* When non-zero, dump DIEs after they are read in. */ static int dwarf2_die_debug = 0; +/* When non-zero, cross-check physname against demangler. */ +static int check_physname = 0; + static int pagesize; /* When set, the file that we're processing is known to have debugging @@ -5142,71 +5145,71 @@ dwarf2_full_name (char *name, struct die_info *die, struct dwarf2_cu *cu) static const char * dwarf2_physname (char *name, struct die_info *die, struct dwarf2_cu *cu) { - const char *physname = dwarf2_compute_name (name, die, cu, NAME_KIND_PHYS); + const char *physname; struct attribute *attr; - const char *mangled, *retval; - char *demangled, *canon; - struct cleanup *back_to; - - /* Do not check PHYSNAME if it never will be needed by GDB. GDB does not - need to support something it has no use for. */ - if (!die_needs_namespace (die, cu)) - return physname; + const char *retval, *mangled = NULL; + char *canon = NULL; + struct cleanup *back_to = make_cleanup (null_cleanup, NULL); + int need_copy = 1; attr = dwarf2_attr (die, DW_AT_linkage_name, cu); if (!attr) attr = dwarf2_attr (die, DW_AT_MIPS_linkage_name, cu); - if (!attr || !DW_STRING (attr)) + /* DW_AT_linkage_name is missing in some cases - depend on what GDB + has computed. */ + if (attr && DW_STRING (attr)) { - /* DW_AT_linkage_name is missing in some cases - depend on what GDB has - computed. */ - - return physname; - } - mangled = DW_STRING (attr); - - /* As both DW_AT_linkage_name (MANGLED) and computed PHYSNAME are present - cross-check them. */ - - back_to = make_cleanup (null_cleanup, 0); + char *demangled; - demangled = cplus_demangle (mangled, DMGL_PARAMS | DMGL_ANSI); - if (demangled) - make_cleanup (xfree, demangled); - else - demangled = (char *) mangled; + mangled = DW_STRING (attr); - if (cu->language == language_cplus) - { - canon = cp_canonicalize_string (demangled); - if (canon != NULL) - make_cleanup (xfree, canon); + demangled = cplus_demangle (mangled, (DMGL_PARAMS | DMGL_ANSI + | DMGL_VERBOSE)); + if (demangled) + { + make_cleanup (xfree, demangled); + canon = demangled; + } else - canon = demangled; + { + canon = (char *) mangled; + need_copy = 0; + } } - else - canon = demangled; - if (strcmp (physname, canon) != 0) + if (canon == NULL || check_physname) { - /* It may not mean a bug in GDB. The compiler could also compute - DW_AT_linkage_name incorrectly. But in such case GDB would need to be - bug-to-bug compatible. */ + physname = dwarf2_compute_name (name, die, cu, NAME_KIND_PHYS); - complaint (&symfile_complaints, - _("Computed physname <%s> does not match demangled <%s> " - "(from linkage <%s>) - DIE at 0x%x [in module %s]"), - physname, canon, mangled, die->offset, cu->objfile->name); + if (canon != NULL && strcmp (physname, canon) != 0) + { + /* It may not mean a bug in GDB. The compiler could also + compute DW_AT_linkage_name incorrectly. But in such case + GDB would need to be bug-to-bug compatible. */ + + complaint (&symfile_complaints, + _("Computed physname <%s> does not match demangled <%s> " + "(from linkage <%s>) - DIE at 0x%x [in module %s]"), + physname, canon, mangled, die->offset, cu->objfile->name); - /* Prefer DW_AT_linkage_name (in the CANON form) - when it is available - here - over computed PHYSNAME. It is safer against both buggy GDB and - buggy compilers. */ + /* Prefer DW_AT_linkage_name (in the CANON form) - when it + is available here - over computed PHYSNAME. It is safer + against both buggy GDB and buggy compilers. */ - retval = obsavestring (canon, strlen (canon), - &cu->objfile->objfile_obstack); + retval = canon; + } + else + { + retval = physname; + need_copy = 0; + } } else - retval = physname; + retval = canon; + + if (need_copy) + retval = obsavestring (retval, strlen (retval), + &cu->objfile->objfile_obstack); do_cleanups (back_to); return retval; @@ -16247,6 +16250,15 @@ show_dwarf2_always_disassemble (struct ui_file *file, int from_tty, value); } +static void +show_check_physname (struct ui_file *file, int from_tty, + struct cmd_list_element *c, const char *value) +{ + fprintf_filtered (file, + _("Whether to check \"physname\" is %s.\n"), + value); +} + void _initialize_dwarf2_read (void); void @@ -16302,6 +16314,14 @@ The value is the maximum depth to print."), NULL, &setdebuglist, &showdebuglist); + add_setshow_boolean_cmd ("check-physname", no_class, &check_physname, _("\ +Set cross-checking of \"physname\" code against demangler."), _("\ +Show cross-checking of \"physname\" code against demangler."), _("\ +When enabled, GDB's internal \"physname\" code is checked against\n\ +the demangler."), + NULL, show_check_physname, + &setdebuglist, &showdebuglist); + c = add_cmd ("gdb-index", class_files, save_gdb_index_command, _("\ Save a gdb-index file.\n\