From patchwork Mon Feb 5 09:56:59 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Joel Brobecker X-Patchwork-Id: 25792 Received: (qmail 53086 invoked by alias); 5 Feb 2018 09:57:17 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 53077 invoked by uid 89); 5 Feb 2018 09:57:16 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RCVD_IN_DNSWL_NONE, SPF_PASS, TIME_LIMIT_EXCEEDED autolearn=unavailable version=3.3.2 spammy=gos, spoke, resembles, lie X-HELO: rock.gnat.com Received: from rock.gnat.com (HELO rock.gnat.com) (205.232.38.15) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 05 Feb 2018 09:57:06 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id CB61656104; Mon, 5 Feb 2018 04:57:04 -0500 (EST) Received: from rock.gnat.com ([127.0.0.1]) by localhost (rock.gnat.com [127.0.0.1]) (amavisd-new, port 10024) with LMTP id e89gNACGxH2O; Mon, 5 Feb 2018 04:57:04 -0500 (EST) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id 136521171DB; Mon, 5 Feb 2018 04:57:04 -0500 (EST) Received: by joel.gnat.com (Postfix, from userid 1000) id 5DD2F8304F; Mon, 5 Feb 2018 13:56:59 +0400 (+04) Date: Mon, 5 Feb 2018 13:56:59 +0400 From: Joel Brobecker To: Pedro Alves Cc: gdb-patches@sourceware.org Subject: Re: [RFC] regresssion(internal-error) printing subprogram argument Message-ID: <20180205095659.y5jzjj2e5bx6pjyf@adacore.com> References: <07a154ef-b6f5-ad86-1410-a73620de4b5b@redhat.com> <20180103043345.n6blge377ybsdx6q@adacore.com> <8df5cf8b-6e4e-e310-fcbd-2615334fe5b9@redhat.com> <832dbb30-7c2b-40ed-c03c-654bd1e2ea32@redhat.com> <20180117091332.z7bqu4aljudq33sw@adacore.com> <20180126035055.vbjtowj6q5ftbwiz@adacore.com> <21bfbb6a-bb10-812b-c34a-d367321e8d5e@redhat.com> <20180129103841.kdkomcjbuwiat5b4@adacore.com> <250976c6-6e7a-6a8e-b9f2-a57f5b92b965@redhat.com> <20180130035612.xghiskhiweftijxi@adacore.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20180130035612.xghiskhiweftijxi@adacore.com> User-Agent: NeoMutt/20170113 (1.7.2) Here is the latest update for this thread. I'm pretty much ready to work on an official patch, but I'd like some guidance. A quick summary: We noticed that, with Ada programs, "maintainance check-psymtabs" reports consistency check error for symbols such as "interfaces__cS". This happens for symbols whose linkage name resembles a C++ mangled name. In that situation, dwarf2physname finds the linkage name, and then processes it through gdb_demangle, regardless of the unit's language. As a result, we end up storing the symbol linkage name using the demangled name, which makes no sense for Ada, and thus triggers the consistency check failure. The fix for the various testcases (one in C, one in the existing Ada testcase) is to patch dwarf2_physname to avoid calling gdb_demangle for languages where it doesn't make sense. The big question is: which languages should we exclude? For now, I've decided to only touch languages where I am sure: Ada, of course, but also C, Asm, and "minimal". There was the question of Go, but I'm not sure what go does in terms of mangling. If you remember, symbols from Go units do not process symbols' linkage name through gdb_demangle, but for reasons that are unclear. Here is the comment: if (cu->language == language_go) { /* This is a lie, but we already lie to the caller new_symbol. new_symbol assumes we return the mangled name. This just undoes that lie until things are cleaned up. */ I went looking for the origin of this comment, and unfortunately, it was introduced as part of the large patch that introduced GO support (circa Apr 2012). Not much information there. To play it safe, I decided to fix dwarf2_physname independently of go, so that go's comment doesn't bleed into this fix. I also spent some time investigating all the calls to gdb_demangle. The vast majority are obviously in a situation where we're dealing with C++ types, or at least language-specific types where the language is known. So the assumption is that we know what we're doing and the call is OK. As a result, there were only the 3 instances in dwarf2read.c. One of them is the dwarf2_physname function, which is fixed here. The other two are: (a) fixup_partial_die: | /* GCC might emit a nameless struct or union that has a linkage | name. See http://gcc.gnu.org/bugzilla/show_bug.cgi?id=47510. */ | if (part_die->name == NULL | && (part_die->tag == DW_TAG_class_type | || part_die->tag == DW_TAG_interface_type | || part_die->tag == DW_TAG_structure_type | || part_die->tag == DW_TAG_union_type) | && part_die->linkage_name != NULL) (b) dwarf2_name: | case DW_TAG_class_type: | case DW_TAG_interface_type: | case DW_TAG_structure_type: | case DW_TAG_union_type: | [...] | /* GCC might emit a nameless typedef that has a linkage name. See | http://gcc.gnu.org/bugzilla/show_bug.cgi?id=47510. */ | if (!attr || DW_STRING (attr) == NULL) Theoretically, we might have an issue where we might be calling gdb_demangle improperly for those types, and we could try to fix that. However, the fix in those two situations is not clearly obvious. And to top it all off, the equivalent situation might not exist outside of certain versions of C++. So I would like to suggest we leave those areas alone for now. In terms of the fix, we have several alternatives. I discarded the idea of hard-coding the list of languages we want to exclude in dwarf2_physname as we might need that list elsewhere. We have the option of a function (like in the attached patch), probably to be moved to symtab.h/symtab.c. I chose a name that spoke to me, but I'm completely familiar with the new lookup system entirely yet, so better name suggestions are welcome. For instance, instead of talking of symbol name storage format, we could have something like... symbol_lookup_uses_demanged_name_p (enum language lang); All in all, I think a better solution would be to put that information directly in the language_defn itself, via a new "attribute". Something like a... /* True if symbol search names should be stored in demangled format. False otherwise. */ const bool symbol_stored_in_demangle_form_p; Then, I'd set it to "true" for all languages, except the languages we selected (Ada, C, Asm, minimal). I didn't implement that just yet, as this requires a larger number of files being modified, so I thought I'd seek opinions before going ahead. My vote: a new language_defn attribute. Thoughts? gdb/ChangeLog: PR gdb/22670: * dwarf2read.c (symbols_stored_in_demangled_form_p): New function. (dwarf2_physname): Do not call gdb_demangle if symbols_stored_in_demangled_form_p returns false for the cu's language. gdb/testsuite/ChangeLog: * gdb.ada/notcplusplus: New testcase. * gdb.ada/maint_with_ada.exp: Remove setup_kfail. * gdb.base/c-linkage-name.c: New file. * gdb.base/c-linkage-name.exp: New testcase. Tested on x86_64-linux, no regression. Thank you! From 4b3af7739001893595ec2b81b5cfb504b0b58859 Mon Sep 17 00:00:00 2001 From: Joel Brobecker Date: Thu, 25 Jan 2018 23:23:54 -0500 Subject: [PATCH] WIPv4: dwarf2read.c::dwarf2_physname: conditionalize call to gdb_demangle This patch avoids the call to gdb_demangle of the DW_AT_linkage_name for languages where either: demangling does not make sense, or where the language implementation chose to store symbol names in mangled form. --- gdb/dwarf2read.c | 26 ++++++++++++++++- gdb/testsuite/gdb.ada/maint_with_ada.exp | 1 - gdb/testsuite/gdb.ada/notcplusplus.exp | 45 ++++++++++++++++++++++++++++ gdb/testsuite/gdb.ada/notcplusplus/foo.adb | 21 +++++++++++++ gdb/testsuite/gdb.ada/notcplusplus/pck.adb | 21 +++++++++++++ gdb/testsuite/gdb.ada/notcplusplus/pck.ads | 19 ++++++++++++ gdb/testsuite/gdb.ada/notcplusplus/ver.ads | 22 ++++++++++++++ gdb/testsuite/gdb.base/c-linkage-name.c | 44 ++++++++++++++++++++++++++++ gdb/testsuite/gdb.base/c-linkage-name.exp | 47 ++++++++++++++++++++++++++++++ 9 files changed, 244 insertions(+), 2 deletions(-) create mode 100644 gdb/testsuite/gdb.ada/notcplusplus.exp create mode 100644 gdb/testsuite/gdb.ada/notcplusplus/foo.adb create mode 100644 gdb/testsuite/gdb.ada/notcplusplus/pck.adb create mode 100644 gdb/testsuite/gdb.ada/notcplusplus/pck.ads create mode 100644 gdb/testsuite/gdb.ada/notcplusplus/ver.ads create mode 100644 gdb/testsuite/gdb.base/c-linkage-name.c create mode 100644 gdb/testsuite/gdb.base/c-linkage-name.exp diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c index d651725..91e7862 100644 --- a/gdb/dwarf2read.c +++ b/gdb/dwarf2read.c @@ -11109,6 +11109,26 @@ dwarf2_full_name (const char *name, struct die_info *die, struct dwarf2_cu *cu) return dwarf2_compute_name (name, die, cu, 0); } +static bool +symbols_stored_in_demangled_form_p (enum language lang) +{ + if (lang == language_asm + || lang == language_minimal + || lang == language_c) + { + /* No mangling for those languages. */ + return false; + } + if (lang == language_ada) + { + /* For Ada, we store the symbol names in encoded form + and then perform the lookups using that form. */ + return false; + } + + return true; +} + /* Construct a physname for the given DIE in CU. NAME may either be from a previous call to dwarf2_name or NULL. The result will be allocated on the objfile_objstack or NULL if the DIE does not have a @@ -11142,7 +11162,11 @@ dwarf2_physname (const char *name, struct die_info *die, struct dwarf2_cu *cu) if (mangled != NULL) { - if (cu->language == language_go) + if (! symbols_stored_in_demangled_form_p (cu->language)) + { + /* Do nothing (do not demangle the symbol name). */ + } + else if (cu->language == language_go) { /* This is a lie, but we already lie to the caller new_symbol. new_symbol assumes we return the mangled name. diff --git a/gdb/testsuite/gdb.ada/maint_with_ada.exp b/gdb/testsuite/gdb.ada/maint_with_ada.exp index 73da613..9ede035 100644 --- a/gdb/testsuite/gdb.ada/maint_with_ada.exp +++ b/gdb/testsuite/gdb.ada/maint_with_ada.exp @@ -32,7 +32,6 @@ gdb_breakpoint "adainit" gdb_breakpoint "Var_Arr_Typedef" gdb_breakpoint "Do_Nothing" -setup_kfail gdb/22670 "*-*-*" gdb_test_no_output "maintenance check-psymtabs" gdb_test_no_output "maintenance check-symtabs" diff --git a/gdb/testsuite/gdb.ada/notcplusplus.exp b/gdb/testsuite/gdb.ada/notcplusplus.exp new file mode 100644 index 0000000..b2a24e8 --- /dev/null +++ b/gdb/testsuite/gdb.ada/notcplusplus.exp @@ -0,0 +1,45 @@ +# Copyright 2018 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 . + +load_lib "ada.exp" + +if { [skip_ada_tests] } { return -1 } + +standard_ada_testfile foo + +if {[gdb_compile_ada "${srcfile}" "${binfile}" executable {debug}] != ""} { + return -1 +} + +clean_restart ${testfile} + +gdb_test "print /x " \ + "= \\(a => 0x60287af\\)" \ + "print before loading symbols from ver.ads" + +# Force the partial symbosl from ver.ads to be expanded into full symbols. + +gdb_test \ + "list ver.ads:16" \ + [multi_line ".*" \ + "16\\s+package Ver is" \ + "17\\s+type Wrapper is record" \ + "18\\s+A : Integer;" \ + "19\\s+end record;" \ + "20\\s+u00045 : constant Wrapper := \\(A => 16#060287af#\\);"] + +gdb_test "print /x " \ + "= \\(a => 0x60287af\\)" \ + "print after loading symbols from ver.ads" diff --git a/gdb/testsuite/gdb.ada/notcplusplus/foo.adb b/gdb/testsuite/gdb.ada/notcplusplus/foo.adb new file mode 100644 index 0000000..89e42f9 --- /dev/null +++ b/gdb/testsuite/gdb.ada/notcplusplus/foo.adb @@ -0,0 +1,21 @@ +-- Copyright 2018 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 . + +with Pck; use Pck; +with Ver; use Ver; +procedure Foo is +begin + Do_Nothing (u00045'Address); +end Foo; diff --git a/gdb/testsuite/gdb.ada/notcplusplus/pck.adb b/gdb/testsuite/gdb.ada/notcplusplus/pck.adb new file mode 100644 index 0000000..dcfb306 --- /dev/null +++ b/gdb/testsuite/gdb.ada/notcplusplus/pck.adb @@ -0,0 +1,21 @@ +-- Copyright 2018 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 . + +package body Pck is + procedure Do_Nothing (A : System.Address) is + begin + null; + end Do_Nothing; +end Pck; diff --git a/gdb/testsuite/gdb.ada/notcplusplus/pck.ads b/gdb/testsuite/gdb.ada/notcplusplus/pck.ads new file mode 100644 index 0000000..33e369e --- /dev/null +++ b/gdb/testsuite/gdb.ada/notcplusplus/pck.ads @@ -0,0 +1,19 @@ +-- Copyright 2018 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 . + +with System; +package Pck is + procedure Do_Nothing (A : System.Address); +end Pck; diff --git a/gdb/testsuite/gdb.ada/notcplusplus/ver.ads b/gdb/testsuite/gdb.ada/notcplusplus/ver.ads new file mode 100644 index 0000000..8f264d0 --- /dev/null +++ b/gdb/testsuite/gdb.ada/notcplusplus/ver.ads @@ -0,0 +1,22 @@ +-- Copyright 2018 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 . + +package Ver is + type Wrapper is record + A : Integer; + end record; + u00045 : constant Wrapper := (A => 16#060287af#); + pragma Export (C, u00045, "symada__cS"); +end Ver; diff --git a/gdb/testsuite/gdb.base/c-linkage-name.c b/gdb/testsuite/gdb.base/c-linkage-name.c new file mode 100644 index 0000000..925004c --- /dev/null +++ b/gdb/testsuite/gdb.base/c-linkage-name.c @@ -0,0 +1,44 @@ +/* This testcase is part of GDB, the GNU debugger. + + Copyright 2018 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 . */ + +struct wrapper +{ + int a; +}; + +/* Create a global variable whose name in the assembly code + (aka the "linkage name") is different from the name in + the source code. The goal is to create a symbol described + in DWARF using a DW_AT_linkage_name attribute, with a name + which follows the C++ mangling. + + In this particular case, we chose "symada__cS" which, if it were + demangled, would translate to "symada (char, signed)". */ +struct wrapper mundane asm ("symada__cS") = {0x060287af}; + +void +do_something (struct wrapper *w) +{ + w->a++; +} + +int +main (void) +{ + do_something (&mundane); + return 0; +} diff --git a/gdb/testsuite/gdb.base/c-linkage-name.exp b/gdb/testsuite/gdb.base/c-linkage-name.exp new file mode 100644 index 0000000..c80a530 --- /dev/null +++ b/gdb/testsuite/gdb.base/c-linkage-name.exp @@ -0,0 +1,47 @@ +# Copyright 2018 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 is part of the gdb testsuite. It is intended to test that +# gdb can correctly print arrays with indexes for each element of the +# array. + +standard_testfile .c + +if { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {debug}] != "" } { + untested "failed to compile" + return -1 +} + +clean_restart ${binfile} + +# Try to print MUNDANE, but using its linkage name. + +gdb_test "print symada__cS" \ + " = {a = 100829103}" \ + "print symada__cS before partial symtab expansion" + +# Force the symbols to be expanded for the unit that contains +# our symada__cS symbol by, e.g. inserting a breakpoint on one +# of the founction also provided by the same using. + +gdb_test "break main" \ + "Breakpoint $decimal at $hex: file .*$srcfile, line $decimal\\." + +# Try to print MUNDANE using its linkage name again, after partial +# symtab expansion. + +gdb_test "print symada__cS" \ + " = {a = 100829103}" \ + "print symada__cS after partial symtab expansion" -- 2.1.4