Re: [RFC] regresssion(internal-error) printing subprogram argument

[Joel Brobecker <brobecker at adacore dot com>]

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!
-- 
Joel

>From 4b3af7739001893595ec2b81b5cfb504b0b58859 Mon Sep 17 00:00:00 2001
From: Joel Brobecker <brobecker@adacore.com>
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 <http://www.gnu.org/licenses/>.
+
+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 <symada__cS>" \
+         "= \\(a => 0x60287af\\)" \
+         "print <symada__cS> 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 <symada__cS>" \
+         "= \\(a => 0x60287af\\)" \
+         "print <symada__cS> 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 <http://www.gnu.org/licenses/>.
+
+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 <http://www.gnu.org/licenses/>.
+
+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 <http://www.gnu.org/licenses/>.
+
+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 <http://www.gnu.org/licenses/>.
+
+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 <http://www.gnu.org/licenses/>.  */
+
+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 <http://www.gnu.org/licenses/>.
+
+# 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