[PATCH] Demangle function names when disassembling

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH] Demangle function names when disassembling

Tom Tromey-4
From: Andrew Burgess <[hidden email]>

Andrew Burgess pointed out a regression, which he described in
PR symtab/26270:

================
After commit:

  commit bcfe6157ca288efed127c5efe21ad7924e0d98cf (refs/bisect/bad)
  Date:   Fri Apr 24 15:35:01 2020 -0600

      Use the linkage name if it exists

The disassembler no longer demangles function names in its output.  So
we see things like this:

  (gdb) disassemble tree_insert
  Dump of assembler code for function _Z11tree_insertP4nodei:
    ....

Instead of this:

  (gdb) disassemble tree_insert
  Dump of assembler code for function tree_insert(node*, int):
    ....

This is because find_pc_partial_function now returns the linkage name
rather than the demangled name.
================

This patch fixes the problem by introducing a new "overload" of
find_pc_partial_function, which returns the general_symbol_info rather
than simply the name.  This lets the disassemble command choose which
name to show.

Regression tested on x86-64 Fedora 32.

gdb/ChangeLog
2020-07-23  Tom Tromey  <[hidden email]>

        PR symtab/26270:
        * symtab.h (find_pc_partial_function_sym): Declare.
        * cli/cli-cmds.c (disassemble_command): Use
        find_pc_partial_function_sym.  Check asm_demangle.
        * blockframe.c (cache_pc_function_sym): New global.
        (cache_pc_function_name): Remove.
        (clear_pc_function_cache): Update.
        (find_pc_partial_function_sym): New function, from
        find_pc_partial_function.
        (find_pc_partial_function): Rewrite using
        find_pc_partial_function_sym.

gdb/testsuite/ChangeLog
2020-07-23  Andrew Burgess  <[hidden email]>

        PR symtab/26270:
        * gdb.cp/disasm-func-name.cc: New file.
        * gdb.cp/disasm-func-name.exp: New file.
---
 gdb/ChangeLog                             | 14 +++++++
 gdb/blockframe.c                          | 36 +++++++++++-----
 gdb/cli/cli-cmds.c                        |  9 +++-
 gdb/symtab.h                              |  8 ++++
 gdb/testsuite/ChangeLog                   |  6 +++
 gdb/testsuite/gdb.cp/disasm-func-name.cc  | 48 ++++++++++++++++++++++
 gdb/testsuite/gdb.cp/disasm-func-name.exp | 50 +++++++++++++++++++++++
 7 files changed, 160 insertions(+), 11 deletions(-)
 create mode 100644 gdb/testsuite/gdb.cp/disasm-func-name.cc
 create mode 100644 gdb/testsuite/gdb.cp/disasm-func-name.exp

diff --git a/gdb/blockframe.c b/gdb/blockframe.c
index 05c26bc2c2a..80b769514eb 100644
--- a/gdb/blockframe.c
+++ b/gdb/blockframe.c
@@ -191,7 +191,7 @@ find_pc_sect_containing_function (CORE_ADDR pc, struct obj_section *section)
 
 static CORE_ADDR cache_pc_function_low = 0;
 static CORE_ADDR cache_pc_function_high = 0;
-static const char *cache_pc_function_name = 0;
+static const general_symbol_info *cache_pc_function_sym = nullptr;
 static struct obj_section *cache_pc_function_section = NULL;
 static const struct block *cache_pc_function_block = nullptr;
 
@@ -202,7 +202,7 @@ clear_pc_function_cache (void)
 {
   cache_pc_function_low = 0;
   cache_pc_function_high = 0;
-  cache_pc_function_name = (char *) 0;
+  cache_pc_function_sym = nullptr;
   cache_pc_function_section = NULL;
   cache_pc_function_block = nullptr;
 }
@@ -210,8 +210,10 @@ clear_pc_function_cache (void)
 /* See symtab.h.  */
 
 bool
-find_pc_partial_function (CORE_ADDR pc, const char **name, CORE_ADDR *address,
-  CORE_ADDR *endaddr, const struct block **block)
+find_pc_partial_function_sym (CORE_ADDR pc,
+      const struct general_symbol_info **sym,
+      CORE_ADDR *address, CORE_ADDR *endaddr,
+      const struct block **block)
 {
   struct obj_section *section;
   struct symbol *f;
@@ -257,7 +259,7 @@ find_pc_partial_function (CORE_ADDR pc, const char **name, CORE_ADDR *address,
  {
   const struct block *b = SYMBOL_BLOCK_VALUE (f);
 
-  cache_pc_function_name = f->linkage_name ();
+  cache_pc_function_sym = f;
   cache_pc_function_section = section;
   cache_pc_function_block = b;
 
@@ -313,8 +315,8 @@ find_pc_partial_function (CORE_ADDR pc, const char **name, CORE_ADDR *address,
   if (msymbol.minsym == NULL)
     {
       /* No available symbol.  */
-      if (name != NULL)
- *name = 0;
+      if (sym != nullptr)
+ *sym = 0;
       if (address != NULL)
  *address = 0;
       if (endaddr != NULL)
@@ -325,7 +327,7 @@ find_pc_partial_function (CORE_ADDR pc, const char **name, CORE_ADDR *address,
     }
 
   cache_pc_function_low = BMSYMBOL_VALUE_ADDRESS (msymbol);
-  cache_pc_function_name = msymbol.minsym->linkage_name ();
+  cache_pc_function_sym = msymbol.minsym;
   cache_pc_function_section = section;
   cache_pc_function_high = minimal_symbol_upper_bound (msymbol);
   cache_pc_function_block = nullptr;
@@ -340,8 +342,8 @@ find_pc_partial_function (CORE_ADDR pc, const char **name, CORE_ADDR *address,
  *address = cache_pc_function_low;
     }
 
-  if (name)
-    *name = cache_pc_function_name;
+  if (sym != nullptr)
+    *sym = cache_pc_function_sym;
 
   if (endaddr)
     {
@@ -365,6 +367,20 @@ find_pc_partial_function (CORE_ADDR pc, const char **name, CORE_ADDR *address,
   return true;
 }
 
+/* See symtab.h.  */
+
+bool
+find_pc_partial_function (CORE_ADDR pc, const char **name, CORE_ADDR *address,
+  CORE_ADDR *endaddr, const struct block **block)
+{
+  const general_symbol_info *gsi;
+  bool r = find_pc_partial_function_sym (pc, &gsi, address, endaddr, block);
+  if (name != nullptr)
+    *name = r ? gsi->linkage_name () : nullptr;
+  return r;
+}
+
+
 /* See symtab.h.  */
 
 bool
diff --git a/gdb/cli/cli-cmds.c b/gdb/cli/cli-cmds.c
index 14718d1181a..07b119038d2 100644
--- a/gdb/cli/cli-cmds.c
+++ b/gdb/cli/cli-cmds.c
@@ -1482,6 +1482,7 @@ disassemble_command (const char *arg, int from_tty)
 {
   struct gdbarch *gdbarch = get_current_arch ();
   CORE_ADDR low, high;
+  const general_symbol_info *symbol = nullptr;
   const char *name;
   CORE_ADDR pc;
   gdb_disassembly_flags flags;
@@ -1537,8 +1538,14 @@ disassemble_command (const char *arg, int from_tty)
   if (p[0] == '\0')
     {
       /* One argument.  */
-      if (find_pc_partial_function (pc, &name, &low, &high, &block) == 0)
+      if (!find_pc_partial_function_sym (pc, &symbol, &low, &high, &block))
  error (_("No function contains specified address."));
+
+      if (asm_demangle)
+ name = symbol->print_name ();
+      else
+ name = symbol->linkage_name ();
+
 #if defined(TUI)
       /* NOTE: cagney/2003-02-13 The `tui_active' was previously
  `tui_version'.  */
diff --git a/gdb/symtab.h b/gdb/symtab.h
index 0b186554ea1..026ffcaa016 100644
--- a/gdb/symtab.h
+++ b/gdb/symtab.h
@@ -1769,6 +1769,14 @@ extern bool find_pc_partial_function (CORE_ADDR pc, const char **name,
       CORE_ADDR *address, CORE_ADDR *endaddr,
       const struct block **block = nullptr);
 
+/* Like find_pc_partial_function, above, but returns the underlying
+   general_symbol_info (rather than the name) as an out parameter.  */
+
+extern bool find_pc_partial_function_sym
+  (CORE_ADDR pc, const general_symbol_info **sym,
+   CORE_ADDR *address, CORE_ADDR *endaddr,
+   const struct block **block = nullptr);
+
 /* Like find_pc_partial_function, above, but *ADDRESS and *ENDADDR are
    set to start and end addresses of the range containing the entry pc.
 
diff --git a/gdb/testsuite/gdb.cp/disasm-func-name.cc b/gdb/testsuite/gdb.cp/disasm-func-name.cc
new file mode 100644
index 00000000000..428baf98964
--- /dev/null
+++ b/gdb/testsuite/gdb.cp/disasm-func-name.cc
@@ -0,0 +1,48 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2020 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/>.  */
+
+class A
+{
+private:
+  int m_i;
+
+public:
+  A(int i);
+
+  int get_i () const
+  { return m_i; }
+
+  void set_i (int i)
+  { m_i = i; }
+};
+
+A::A(int i)
+  : m_i (i)
+{ /* Nothing.  */ }
+
+void process (A *obj, int num)
+{
+  obj->set_i (obj->get_i () + num);
+}
+
+int
+main (void)
+{
+  A a(42);
+  process (&a, 2);
+  return a.get_i ();
+}
diff --git a/gdb/testsuite/gdb.cp/disasm-func-name.exp b/gdb/testsuite/gdb.cp/disasm-func-name.exp
new file mode 100644
index 00000000000..3fb63c89a28
--- /dev/null
+++ b/gdb/testsuite/gdb.cp/disasm-func-name.exp
@@ -0,0 +1,50 @@
+# Copyright 2020 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
+
+# Test that the disassembler correctly demangles C++ function names in
+# it's header line.
+
+if {[skip_cplus_tests]} { continue }
+
+standard_testfile .cc
+
+if [get_compiler_info "c++"] {
+    return -1
+}
+
+if {[prepare_for_testing "failed to prepare" $testfile \
+ $srcfile {debug c++}]} {
+    return -1
+}
+
+if ![runto_main] then {
+    perror "couldn't run to breakpoint"
+    continue
+}
+
+proc check_disassembly_header { request expected } {
+    gdb_test "disassemble ${request}" \
+ "Dump of assembler code for function ${expected}:\r\n.*"
+}
+
+gdb_test_no_output "set print asm-demangle on"
+
+check_disassembly_header "main" "main\\(\\)"
+check_disassembly_header "process" "process\\(A\\*, int\\)"
+check_disassembly_header "A::A" "A::A\\(int\\)"
+check_disassembly_header "A::get_i" "A::get_i\\(\\) const"
+check_disassembly_header "A::set_i" "A::set_i\\(int\\)"
--
2.26.2

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Demangle function names when disassembling

Andrew Burgess
* Tom Tromey <[hidden email]> [2020-07-23 10:10:43 -0600]:

> From: Andrew Burgess <[hidden email]>
>
> Andrew Burgess pointed out a regression, which he described in
> PR symtab/26270:
>
> ================
> After commit:
>
>   commit bcfe6157ca288efed127c5efe21ad7924e0d98cf (refs/bisect/bad)
>   Date:   Fri Apr 24 15:35:01 2020 -0600
>
>       Use the linkage name if it exists
>
> The disassembler no longer demangles function names in its output.  So
> we see things like this:
>
>   (gdb) disassemble tree_insert
>   Dump of assembler code for function _Z11tree_insertP4nodei:
>     ....
>
> Instead of this:
>
>   (gdb) disassemble tree_insert
>   Dump of assembler code for function tree_insert(node*, int):
>     ....
>
> This is because find_pc_partial_function now returns the linkage name
> rather than the demangled name.
> ================
>
> This patch fixes the problem by introducing a new "overload" of
> find_pc_partial_function, which returns the general_symbol_info rather
> than simply the name.  This lets the disassemble command choose which
> name to show.
>
> Regression tested on x86-64 Fedora 32.

LGTM.

Thanks for fixing this one.

Andrew


>
> gdb/ChangeLog
> 2020-07-23  Tom Tromey  <[hidden email]>
>
> PR symtab/26270:
> * symtab.h (find_pc_partial_function_sym): Declare.
> * cli/cli-cmds.c (disassemble_command): Use
> find_pc_partial_function_sym.  Check asm_demangle.
> * blockframe.c (cache_pc_function_sym): New global.
> (cache_pc_function_name): Remove.
> (clear_pc_function_cache): Update.
> (find_pc_partial_function_sym): New function, from
> find_pc_partial_function.
> (find_pc_partial_function): Rewrite using
> find_pc_partial_function_sym.
>
> gdb/testsuite/ChangeLog
> 2020-07-23  Andrew Burgess  <[hidden email]>
>
> PR symtab/26270:
> * gdb.cp/disasm-func-name.cc: New file.
> * gdb.cp/disasm-func-name.exp: New file.
> ---
>  gdb/ChangeLog                             | 14 +++++++
>  gdb/blockframe.c                          | 36 +++++++++++-----
>  gdb/cli/cli-cmds.c                        |  9 +++-
>  gdb/symtab.h                              |  8 ++++
>  gdb/testsuite/ChangeLog                   |  6 +++
>  gdb/testsuite/gdb.cp/disasm-func-name.cc  | 48 ++++++++++++++++++++++
>  gdb/testsuite/gdb.cp/disasm-func-name.exp | 50 +++++++++++++++++++++++
>  7 files changed, 160 insertions(+), 11 deletions(-)
>  create mode 100644 gdb/testsuite/gdb.cp/disasm-func-name.cc
>  create mode 100644 gdb/testsuite/gdb.cp/disasm-func-name.exp
>
> diff --git a/gdb/blockframe.c b/gdb/blockframe.c
> index 05c26bc2c2a..80b769514eb 100644
> --- a/gdb/blockframe.c
> +++ b/gdb/blockframe.c
> @@ -191,7 +191,7 @@ find_pc_sect_containing_function (CORE_ADDR pc, struct obj_section *section)
>  
>  static CORE_ADDR cache_pc_function_low = 0;
>  static CORE_ADDR cache_pc_function_high = 0;
> -static const char *cache_pc_function_name = 0;
> +static const general_symbol_info *cache_pc_function_sym = nullptr;
>  static struct obj_section *cache_pc_function_section = NULL;
>  static const struct block *cache_pc_function_block = nullptr;
>  
> @@ -202,7 +202,7 @@ clear_pc_function_cache (void)
>  {
>    cache_pc_function_low = 0;
>    cache_pc_function_high = 0;
> -  cache_pc_function_name = (char *) 0;
> +  cache_pc_function_sym = nullptr;
>    cache_pc_function_section = NULL;
>    cache_pc_function_block = nullptr;
>  }
> @@ -210,8 +210,10 @@ clear_pc_function_cache (void)
>  /* See symtab.h.  */
>  
>  bool
> -find_pc_partial_function (CORE_ADDR pc, const char **name, CORE_ADDR *address,
> -  CORE_ADDR *endaddr, const struct block **block)
> +find_pc_partial_function_sym (CORE_ADDR pc,
> +      const struct general_symbol_info **sym,
> +      CORE_ADDR *address, CORE_ADDR *endaddr,
> +      const struct block **block)
>  {
>    struct obj_section *section;
>    struct symbol *f;
> @@ -257,7 +259,7 @@ find_pc_partial_function (CORE_ADDR pc, const char **name, CORE_ADDR *address,
>   {
>    const struct block *b = SYMBOL_BLOCK_VALUE (f);
>  
> -  cache_pc_function_name = f->linkage_name ();
> +  cache_pc_function_sym = f;
>    cache_pc_function_section = section;
>    cache_pc_function_block = b;
>  
> @@ -313,8 +315,8 @@ find_pc_partial_function (CORE_ADDR pc, const char **name, CORE_ADDR *address,
>    if (msymbol.minsym == NULL)
>      {
>        /* No available symbol.  */
> -      if (name != NULL)
> - *name = 0;
> +      if (sym != nullptr)
> + *sym = 0;
>        if (address != NULL)
>   *address = 0;
>        if (endaddr != NULL)
> @@ -325,7 +327,7 @@ find_pc_partial_function (CORE_ADDR pc, const char **name, CORE_ADDR *address,
>      }
>  
>    cache_pc_function_low = BMSYMBOL_VALUE_ADDRESS (msymbol);
> -  cache_pc_function_name = msymbol.minsym->linkage_name ();
> +  cache_pc_function_sym = msymbol.minsym;
>    cache_pc_function_section = section;
>    cache_pc_function_high = minimal_symbol_upper_bound (msymbol);
>    cache_pc_function_block = nullptr;
> @@ -340,8 +342,8 @@ find_pc_partial_function (CORE_ADDR pc, const char **name, CORE_ADDR *address,
>   *address = cache_pc_function_low;
>      }
>  
> -  if (name)
> -    *name = cache_pc_function_name;
> +  if (sym != nullptr)
> +    *sym = cache_pc_function_sym;
>  
>    if (endaddr)
>      {
> @@ -365,6 +367,20 @@ find_pc_partial_function (CORE_ADDR pc, const char **name, CORE_ADDR *address,
>    return true;
>  }
>  
> +/* See symtab.h.  */
> +
> +bool
> +find_pc_partial_function (CORE_ADDR pc, const char **name, CORE_ADDR *address,
> +  CORE_ADDR *endaddr, const struct block **block)
> +{
> +  const general_symbol_info *gsi;
> +  bool r = find_pc_partial_function_sym (pc, &gsi, address, endaddr, block);
> +  if (name != nullptr)
> +    *name = r ? gsi->linkage_name () : nullptr;
> +  return r;
> +}
> +
> +
>  /* See symtab.h.  */
>  
>  bool
> diff --git a/gdb/cli/cli-cmds.c b/gdb/cli/cli-cmds.c
> index 14718d1181a..07b119038d2 100644
> --- a/gdb/cli/cli-cmds.c
> +++ b/gdb/cli/cli-cmds.c
> @@ -1482,6 +1482,7 @@ disassemble_command (const char *arg, int from_tty)
>  {
>    struct gdbarch *gdbarch = get_current_arch ();
>    CORE_ADDR low, high;
> +  const general_symbol_info *symbol = nullptr;
>    const char *name;
>    CORE_ADDR pc;
>    gdb_disassembly_flags flags;
> @@ -1537,8 +1538,14 @@ disassemble_command (const char *arg, int from_tty)
>    if (p[0] == '\0')
>      {
>        /* One argument.  */
> -      if (find_pc_partial_function (pc, &name, &low, &high, &block) == 0)
> +      if (!find_pc_partial_function_sym (pc, &symbol, &low, &high, &block))
>   error (_("No function contains specified address."));
> +
> +      if (asm_demangle)
> + name = symbol->print_name ();
> +      else
> + name = symbol->linkage_name ();
> +
>  #if defined(TUI)
>        /* NOTE: cagney/2003-02-13 The `tui_active' was previously
>   `tui_version'.  */
> diff --git a/gdb/symtab.h b/gdb/symtab.h
> index 0b186554ea1..026ffcaa016 100644
> --- a/gdb/symtab.h
> +++ b/gdb/symtab.h
> @@ -1769,6 +1769,14 @@ extern bool find_pc_partial_function (CORE_ADDR pc, const char **name,
>        CORE_ADDR *address, CORE_ADDR *endaddr,
>        const struct block **block = nullptr);
>  
> +/* Like find_pc_partial_function, above, but returns the underlying
> +   general_symbol_info (rather than the name) as an out parameter.  */
> +
> +extern bool find_pc_partial_function_sym
> +  (CORE_ADDR pc, const general_symbol_info **sym,
> +   CORE_ADDR *address, CORE_ADDR *endaddr,
> +   const struct block **block = nullptr);
> +
>  /* Like find_pc_partial_function, above, but *ADDRESS and *ENDADDR are
>     set to start and end addresses of the range containing the entry pc.
>  
> diff --git a/gdb/testsuite/gdb.cp/disasm-func-name.cc b/gdb/testsuite/gdb.cp/disasm-func-name.cc
> new file mode 100644
> index 00000000000..428baf98964
> --- /dev/null
> +++ b/gdb/testsuite/gdb.cp/disasm-func-name.cc
> @@ -0,0 +1,48 @@
> +/* This testcase is part of GDB, the GNU debugger.
> +
> +   Copyright 2020 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/>.  */
> +
> +class A
> +{
> +private:
> +  int m_i;
> +
> +public:
> +  A(int i);
> +
> +  int get_i () const
> +  { return m_i; }
> +
> +  void set_i (int i)
> +  { m_i = i; }
> +};
> +
> +A::A(int i)
> +  : m_i (i)
> +{ /* Nothing.  */ }
> +
> +void process (A *obj, int num)
> +{
> +  obj->set_i (obj->get_i () + num);
> +}
> +
> +int
> +main (void)
> +{
> +  A a(42);
> +  process (&a, 2);
> +  return a.get_i ();
> +}
> diff --git a/gdb/testsuite/gdb.cp/disasm-func-name.exp b/gdb/testsuite/gdb.cp/disasm-func-name.exp
> new file mode 100644
> index 00000000000..3fb63c89a28
> --- /dev/null
> +++ b/gdb/testsuite/gdb.cp/disasm-func-name.exp
> @@ -0,0 +1,50 @@
> +# Copyright 2020 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
> +
> +# Test that the disassembler correctly demangles C++ function names in
> +# it's header line.
> +
> +if {[skip_cplus_tests]} { continue }
> +
> +standard_testfile .cc
> +
> +if [get_compiler_info "c++"] {
> +    return -1
> +}
> +
> +if {[prepare_for_testing "failed to prepare" $testfile \
> + $srcfile {debug c++}]} {
> +    return -1
> +}
> +
> +if ![runto_main] then {
> +    perror "couldn't run to breakpoint"
> +    continue
> +}
> +
> +proc check_disassembly_header { request expected } {
> +    gdb_test "disassemble ${request}" \
> + "Dump of assembler code for function ${expected}:\r\n.*"
> +}
> +
> +gdb_test_no_output "set print asm-demangle on"
> +
> +check_disassembly_header "main" "main\\(\\)"
> +check_disassembly_header "process" "process\\(A\\*, int\\)"
> +check_disassembly_header "A::A" "A::A\\(int\\)"
> +check_disassembly_header "A::get_i" "A::get_i\\(\\) const"
> +check_disassembly_header "A::set_i" "A::set_i\\(int\\)"
> --
> 2.26.2
>
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Demangle function names when disassembling

Andrew Burgess
In reply to this post by Tom Tromey-4
* Tom Tromey <[hidden email]> [2020-07-23 10:10:43 -0600]:

> From: Andrew Burgess <[hidden email]>
>
> Andrew Burgess pointed out a regression, which he described in
> PR symtab/26270:
>
> ================
> After commit:
>
>   commit bcfe6157ca288efed127c5efe21ad7924e0d98cf (refs/bisect/bad)
>   Date:   Fri Apr 24 15:35:01 2020 -0600
>
>       Use the linkage name if it exists
>
> The disassembler no longer demangles function names in its output.  So
> we see things like this:
>
>   (gdb) disassemble tree_insert
>   Dump of assembler code for function _Z11tree_insertP4nodei:
>     ....
>
> Instead of this:
>
>   (gdb) disassemble tree_insert
>   Dump of assembler code for function tree_insert(node*, int):
>     ....
>
> This is because find_pc_partial_function now returns the linkage name
> rather than the demangled name.
> ================
>
> This patch fixes the problem by introducing a new "overload" of
> find_pc_partial_function, which returns the general_symbol_info rather
> than simply the name.  This lets the disassemble command choose which
> name to show.
>
> Regression tested on x86-64 Fedora 32.
>
> gdb/ChangeLog
> 2020-07-23  Tom Tromey  <[hidden email]>
>
> PR symtab/26270:
> * symtab.h (find_pc_partial_function_sym): Declare.
> * cli/cli-cmds.c (disassemble_command): Use
> find_pc_partial_function_sym.  Check asm_demangle.
> * blockframe.c (cache_pc_function_sym): New global.
> (cache_pc_function_name): Remove.
> (clear_pc_function_cache): Update.
> (find_pc_partial_function_sym): New function, from
> find_pc_partial_function.
> (find_pc_partial_function): Rewrite using
> find_pc_partial_function_sym.
>
> gdb/testsuite/ChangeLog
> 2020-07-23  Andrew Burgess  <[hidden email]>
>
> PR symtab/26270:
> * gdb.cp/disasm-func-name.cc: New file.
> * gdb.cp/disasm-func-name.exp: New file.

We discussed this patch on IRC and I made the observation that (for
C++) even after this patch the behaviour of the disassemble command
has changed from the 9.x releases of GDB.

In 9.x a disassemble would show the demangled function name in the
header line, while, even after this patch, the default is to display
the mangled name.

I wondered about changing the default for 'set print asm-demangle' to
be on, then we'd maintain the same behaviour from 9.x through to 10.x.

It turns out that the bug that originally caused GDB to display
demangled names only applied for some languages, Ada, didn't have
this issue, and so has always displayed the mangled names.

As a result if I tried to "fix" the issue I reported in PR 26270 by
changing the default for 'set print asm-demangle' then I'll "fix" C++,
but change the default for Ada, which hardly seems fair.

So, I'm posting this patch here as it might be of interest ... at
least it feels like my notes should be available in case other people
have the same questions I had.

But I don't see how it can be merged in its current state.

Thanks,
Andrew

---

commit c5935e59dc4e775615df21f92c97844886b0a427
Author: Andrew Burgess <[hidden email]>
Date:   Fri Jul 24 09:41:43 2020 +0100

    gdb: Change 'set print asm-demangle' to an auto boolean, default on
   
    This commit relates to bug PR gdb/26270.
   
    When I raised this bug it was because I noticed, that when debugging a
    C++ application that the 'disassemble' command started displaying
    mangled names in its header line.  This was a change from what I was
    used to.
   
    I tested GDB 7.6, and confirmed that for at least the last 6 years
    disassemble would display demangled names in the header line when
    disassembling C++ code.
   
    On further investigation it also turns out that 'info line' output
    also displays demangled names, and the symbols referenced in
    disassembly output are also demangled for the same time period.
   
    The reason this changed recently is that a long standing bug was fixed
    in GDB where (if I understand correctly) GDB was storing the demangled
    names where it should have been storing the mangled names, this made
    commands line 'set print demangle' and 'set print asm-demangle' not
    work, and gave us the behaviour I was seeing on the older GDBs.  With
    this bug fix GDB now does what it "should" have all along.
   
    My problem here is that it feels like, if we've lived with things a
    particular way for 6 years, then when we change that behaviour it can
    seem confusing - it certainly confused me.
   
    So, one reason that we see things being printed as mangled is that
    though 'set print demangle' is on by default, 'set print asm-demangle'
    is off by default.
   
    So my initial thought was, lets make 'set print asm-demangle' be on by
    default.  Then we've still fixed the bug in GDB (about storing the
    mangled vs demangled symbols), but the behaviour of GDB doesn't
    actually change (vs 9.x release).  After a discussion on IRC the
    suggestion was made to have 'set print asm-demangle' be an auto-bool,
    then the default can be auto, which would defer to 'set print
    demangle', but the user can still take separate control if they want.
   
    So, that's what this patch does.
   
    Unfortunately there's some problems with this approach.
   
    First, and maybe the smallest problem, boolean variables have the
    "feature" that a user can just type 'set print asm-demangle', which is
    equivalent to 'set print asm-demangle on'.  However, this doesn't work
    for auto-booleans, in that case you must type 'set print asm-demangle
    on'.
   
    This could possibly be worked around by overriding handling of the
    'set print asm-demangle' command, but I haven't done that here.  So
    this currently is a breaking change, though a pretty minor one I
    think.
   
    The biggest problem, and the blocker I think, is that under testing I
    found that the issues I listed above are, apparently, language
    specific.  So for 6+ years GDB _for C++_ has been showing demangled
    names when it should have been displaying mangled ones, but for Ada,
    GDB has been doing the "right" thing.
   
    If I flip the default for 'asm-demangle' on the grounds that I don't
    think we should suddenly change GDB's behaviour then this would only
    be for C++, for Ada I would be suddenly changing the default.
   
    As a result I don't see any obvious way we could include this patch
    without changing GDB's behaviour for one language or another.
   
    What I did discover in my journey into GDB's demangling, is that these
    two options are a mess.  The manual entry for 'set print demangle' and
    'set print asm-demangle' are super unclear about which parts of GDB
    each are supposed to control.  The comment on the controlling
    variable (asm_demangle) in the source code (gdb-demangle.h) suggest
    this variable should be overridden by 'demangle' if 'demangle' is set
    false, however, in some places 'asm_demangle' is completely
    independent, and in other places 'asm_demangle' actually overrides
    'demangle' rather than deferring to it.
   
    gdb/ChangeLog:
   
            PR gdb/26270
            * NEWS: Mention change in default for asm-demangle.
            * cli/cli-cmds.c (disassemble_command): Switch to use
            asm_demangle_p.
            * gdb-demangle.c (asm_demangle): Change type to auto bool, default
            is auto.
            (show_asm_demangle): Update to deal with new asm_demangle type.
            (asm_demangle_p): New function.
            (_initialize_gdb_demangle): Register 'set print asm-demangle' as
            an auto-boolean.
            * gdb-demangle.h (asm_demangle): Delete declaration.
            (asm_demangle_p): Declare.
            * printcmd.c (build_address_symbolic): Call asm
   
    gdb/doc/ChangeLog:
   
            PR gdb/26270
            * gdb.texinfo (Print Settings): Note change in default for
            asm-demangle.  Rewrite the @item line for this option.
   
    gdb/testsuite/ChangeLog:
   
            PR gdb/26270
            * gdb.ada/win_fu_syms.exp: Update expected output.
            * gdb.base/default.exp: Update expected output.
            * gdb.cp/disasm-func-name.exp: Update for change in asm-demangle,
            add new tests.

diff --git a/gdb/NEWS b/gdb/NEWS
index ff0624ce6eb..f9e8b9c323a 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -25,6 +25,16 @@
 
   You can get the latest version from https://sourceware.org/elfutils.
 
+* 'set print asm-demangle' is now an auto boolean, meaning it can be
+  set to on, off, or auto.  It's default is now auto, which means it
+  defers to the value of 'set print demangle'.  For the last 6 years
+  GDB has contained a bug that, for some languages, caused 'set print
+  asm-demangle' to not function correctly, and effectively always be
+  on.  This bug in GDB has now been fixed, but this means that for the
+  effected languages GDB's behaviour has now changed.  In order to
+  avoid confusing users, by changing the default for this flag we
+  maintain the behaviour GDB has had for the last 6 years.
+
 * New features in the GDB remote stub, GDBserver
 
   ** GDBserver is now supported on RISC-V GNU/Linux.
diff --git a/gdb/cli/cli-cmds.c b/gdb/cli/cli-cmds.c
index 07b119038d2..05feaa79bf7 100644
--- a/gdb/cli/cli-cmds.c
+++ b/gdb/cli/cli-cmds.c
@@ -1541,7 +1541,7 @@ disassemble_command (const char *arg, int from_tty)
       if (!find_pc_partial_function_sym (pc, &symbol, &low, &high, &block))
  error (_("No function contains specified address."));
 
-      if (asm_demangle)
+      if (asm_demangle_p ())
  name = symbol->print_name ();
       else
  name = symbol->linkage_name ();
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 1b9f76573b9..b9c810d2299 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -11556,11 +11556,16 @@
 @item show print demangle
 Show whether C@t{++} names are printed in mangled or demangled form.
 
-@item set print asm-demangle
-@itemx set print asm-demangle on
-Print C@t{++} names in their source form rather than their mangled form, even
-in assembler code printouts such as instruction disassemblies.
-The default is off.
+@item set print asm-demangle [auto|on|off]
+Print C@t{++} names in their source form rather than their mangled
+form, even in assembler code printouts such as instruction
+disassemblies.  This flag interacts differently to @code{set print
+demangle} in different parts of @value{GDBN}, in some places this flag
+is independent to @code{set print demangle}, and in other places this
+flag will override the setting in @code{set print demangle}.
+
+The default is auto, which means this flag defers to @code{set print
+demangle}.
 
 @item show print asm-demangle
 Show whether C@t{++} names in assembly listings are printed in mangled
diff --git a/gdb/gdb-demangle.c b/gdb/gdb-demangle.c
index 6a664e99bfc..57fbe4af8f4 100644
--- a/gdb/gdb-demangle.c
+++ b/gdb/gdb-demangle.c
@@ -44,6 +44,7 @@
 #endif
 
 /* See documentation in gdb-demangle.h.  */
+
 bool demangle = true;
 
 static void
@@ -57,16 +58,35 @@ show_demangle (struct ui_file *file, int from_tty,
 }
 
 /* See documentation in gdb-demangle.h.  */
-bool asm_demangle = false;
 
+static enum auto_boolean asm_demangle = AUTO_BOOLEAN_AUTO;
 static void
 show_asm_demangle (struct ui_file *file, int from_tty,
    struct cmd_list_element *c, const char *value)
 {
-  fprintf_filtered (file,
-    _("Demangling of C++/ObjC names in "
-      "disassembly listings is %s.\n"),
-    value);
+  if (asm_demangle == AUTO_BOOLEAN_AUTO)
+    fprintf_filtered (file,
+      _("Demangling of C++/ObjC names in disassembly "
+ "listings is \"%s\" (currently \"%s\").\n"),
+      value, (demangle ? "on" : "off"));
+  else if (asm_demangle == AUTO_BOOLEAN_TRUE && !demangle)
+    fprintf_filtered (file,
+      _("Demangling of C++/ObjC names in disassembly "
+ "listings is \"%s\", but is overridden by "
+ "'set print demangle' which is \"off\".\n"),
+      value);
+  else
+    fprintf_filtered (file,
+      _("Demangling of C++/ObjC names in disassembly "
+ "listings is \"%s\"."), value);
+}
+
+/* See gdb-demangle.h.  */
+
+bool
+asm_demangle_p ()
+{
+  return demangle && (asm_demangle != AUTO_BOOLEAN_FALSE);
 }
 
 /* String name for the current demangling style.  Set by the
@@ -244,7 +264,7 @@ Show demangling of encoded C++/ObjC names when displaying symbols."), NULL,
    show_demangle,
    &setprintlist, &showprintlist);
 
-  add_setshow_boolean_cmd ("asm-demangle", class_support, &asm_demangle, _("\
+  add_setshow_auto_boolean_cmd ("asm-demangle", class_support, &asm_demangle, _("\
 Set demangling of C++/ObjC names in disassembly listings."), _("\
 Show demangling of C++/ObjC names in disassembly listings."), NULL,
    NULL,
diff --git a/gdb/gdb-demangle.h b/gdb/gdb-demangle.h
index f159cb130a2..a454a176445 100644
--- a/gdb/gdb-demangle.h
+++ b/gdb/gdb-demangle.h
@@ -23,10 +23,13 @@
    C++/ObjC form rather than raw.  */
 extern bool demangle;
 
-/* True means that encoded C++/ObjC names should be printed out in their
-   C++/ObjC form even in assembler language displays.  If this is set, but
-   DEMANGLE is false, names are printed raw, i.e. DEMANGLE controls.  */
-extern bool asm_demangle;
+/* Return true if symbols in assembler listing should be demangled.  The
+   return value depends on the value of both 'set print asm-demangle' and
+   'set print demangle'.  If 'set print demangle' is off then this will
+   always return true.  If 'set print asm-demangle' is set to auto then
+   this will mirror the value in 'set print demangle'.  */
+
+extern bool asm_demangle_p ();
 
 /* Check if a character is one of the commonly used C++ marker characters.  */
 extern bool is_cplus_marker (int);
diff --git a/gdb/printcmd.c b/gdb/printcmd.c
index 309d2cabfff..87020e465ba 100644
--- a/gdb/printcmd.c
+++ b/gdb/printcmd.c
@@ -623,7 +623,7 @@ build_address_symbolic (struct gdbarch *gdbarch,
       addr = gdbarch_addr_bits_remove (gdbarch, addr);
 
       name_location = BLOCK_ENTRY_PC (SYMBOL_BLOCK_VALUE (symbol));
-      if (do_demangle || asm_demangle)
+      if (do_demangle || asm_demangle_p ())
  name_temp = symbol->print_name ();
       else
  name_temp = symbol->linkage_name ();
@@ -666,7 +666,7 @@ build_address_symbolic (struct gdbarch *gdbarch,
 
   symbol = 0;
   name_location = BMSYMBOL_VALUE_ADDRESS (msymbol);
-  if (do_demangle || asm_demangle)
+  if (do_demangle || asm_demangle_p ())
     name_temp = msymbol.minsym->print_name ();
   else
     name_temp = msymbol.minsym->linkage_name ();
@@ -715,7 +715,7 @@ print_address (struct gdbarch *gdbarch,
        CORE_ADDR addr, struct ui_file *stream)
 {
   fputs_styled (paddress (gdbarch, addr), address_style.style (), stream);
-  print_address_symbolic (gdbarch, addr, stream, asm_demangle, " ");
+  print_address_symbolic (gdbarch, addr, stream, asm_demangle_p (), " ");
 }
 
 /* Return a prefix for instruction address:
diff --git a/gdb/testsuite/gdb.ada/win_fu_syms.exp b/gdb/testsuite/gdb.ada/win_fu_syms.exp
index f27b0a82ab6..8447b28a932 100644
--- a/gdb/testsuite/gdb.ada/win_fu_syms.exp
+++ b/gdb/testsuite/gdb.ada/win_fu_syms.exp
@@ -25,11 +25,11 @@ clean_restart ${testfile}
 
 set loc [gdb_get_line_number "Integer" ${testdir}/foo.adb]
 gdb_test "info line foo.adb:$loc" \
-         "Line $decimal of \".*foo\\.adb\" starts at address $hex <_ada_foo\\+$decimal> and ends at $hex <_ada_foo\\+$decimal>\\." \
+         "Line $decimal of \".*foo\\.adb\" starts at address $hex <foo\\+$decimal> and ends at $hex <foo\\+$decimal>\\." \
          "info line on variable declaration"
 
 set loc [gdb_get_line_number "Do_Nothing" ${testdir}/foo.adb]
 gdb_test "info line foo.adb:$loc" \
-         "Line $decimal of \".*foo\\.adb\" starts at address $hex <_ada_foo\\+$decimal> and ends at $hex <_ada_foo\\+$decimal>\\." \
+         "Line $decimal of \".*foo\\.adb\" starts at address $hex <foo\\+$decimal> and ends at $hex <foo\\+$decimal>\\." \
          "info line on Do_Nothing call"
 
diff --git a/gdb/testsuite/gdb.base/default.exp b/gdb/testsuite/gdb.base/default.exp
index ac1a0f5b6e6..ab4b5702a54 100644
--- a/gdb/testsuite/gdb.base/default.exp
+++ b/gdb/testsuite/gdb.base/default.exp
@@ -521,7 +521,9 @@ gdb_test_no_output "set print address" "set print address"
 #test set print array
 gdb_test_no_output "set print array" "set print array"
 #test set print asm-demangle
-gdb_test_no_output "set print asm-demangle" "set print asm-demangle"
+gdb_test "set print asm-demangle" \
+    "\"on\", \"off\" or \"auto\" expected\\." \
+    "set print asm-demangle"
 #test set print demangle
 gdb_test_no_output "set print demangle" "set print demangle"
 #test set print elements
@@ -663,7 +665,7 @@ gdb_test "show print address" "Printing of addresses is on."
 #test show print array
 gdb_test "show print array" "Pretty formatting of arrays is on."
 #test show print asm-demangle
-gdb_test "show print asm-demangle" "Demangling of C\[+\]+/ObjC names in disassembly listings is on."
+gdb_test "show print asm-demangle" "Demangling of C\[+\]+/ObjC names in disassembly listings is \"auto\" \\(currently \"on\"\\)\\."
 #test show print demangle
 gdb_test "show print demangle" "Demangling of encoded C\[+\]+/ObjC names when displaying symbols is on."
 #test show print elements
diff --git a/gdb/testsuite/gdb.cp/disasm-func-name.exp b/gdb/testsuite/gdb.cp/disasm-func-name.exp
index 3fb63c89a28..2d4cdeb8768 100644
--- a/gdb/testsuite/gdb.cp/disasm-func-name.exp
+++ b/gdb/testsuite/gdb.cp/disasm-func-name.exp
@@ -41,10 +41,20 @@ proc check_disassembly_header { request expected } {
  "Dump of assembler code for function ${expected}:\r\n.*"
 }
 
-gdb_test_no_output "set print asm-demangle on"
+with_test_prefix "asm-demangle=on" {
+    check_disassembly_header "main" "main\\(\\)"
+    check_disassembly_header "process" "process\\(A\\*, int\\)"
+    check_disassembly_header "A::A" "A::A\\(int\\)"
+    check_disassembly_header "A::get_i" "A::get_i\\(\\) const"
+    check_disassembly_header "A::set_i" "A::set_i\\(int\\)"
+}
+
+gdb_test_no_output "set print asm-demangle off"
 
-check_disassembly_header "main" "main\\(\\)"
-check_disassembly_header "process" "process\\(A\\*, int\\)"
-check_disassembly_header "A::A" "A::A\\(int\\)"
-check_disassembly_header "A::get_i" "A::get_i\\(\\) const"
-check_disassembly_header "A::set_i" "A::set_i\\(int\\)"
+with_test_prefix "asm-demangle=off" {
+    check_disassembly_header "main" "main\\(\\)"
+    check_disassembly_header "process" "\[^()\]+"
+    check_disassembly_header "A::A" "\[^()\]+"
+    check_disassembly_header "A::get_i" "\[^()\]+"
+    check_disassembly_header "A::set_i" "\[^()\]+"
+}
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Demangle function names when disassembling

Tom Tromey-2
Andrew> It turns out that the bug that originally caused GDB to display
Andrew> demangled names only applied for some languages, Ada, didn't have
Andrew> this issue, and so has always displayed the mangled names.

Ada is special here ... but I am not sure there's a very good reason for
it.  I've been meaning for a while to see if this difference can be
removed.  Mostly, I think this amounts to demangling Ada names during
symbol reading.

Andrew>     So my initial thought was, lets make 'set print asm-demangle' be on by
Andrew>     default.  Then we've still fixed the bug in GDB (about storing the
Andrew>     mangled vs demangled symbols), but the behaviour of GDB doesn't
Andrew>     actually change (vs 9.x release).

It seems like we could still do this.

Andrew>     First, and maybe the smallest problem, boolean variables have the
Andrew>     "feature" that a user can just type 'set print asm-demangle', which is
Andrew>     equivalent to 'set print asm-demangle on'.  However, this doesn't work
Andrew>     for auto-booleans, in that case you must type 'set print asm-demangle
Andrew>     on'.
   
I think we could just safely ignore this.  I wasn't even aware of this
feature; and anyway it's easy to work around; and finally nobody has
added a asm-demangle setting in 10 years or so due to the bug.

Andrew>     If I flip the default for 'asm-demangle' on the grounds that I don't
Andrew>     think we should suddenly change GDB's behaviour then this would only
Andrew>     be for C++, for Ada I would be suddenly changing the default.
   
I think this is probably fine as well.  I think it makes sense for the
option to do what the documentation says it will do.  Users, if they
notice this, can change the setting.

Yet another option would be to change my patch up-thread to use
"demangle" for the header rather than "asm-demangle".  I tend to think
that would be more confusing, but I figured I'd mention it.

I looked into the history of "asm-demangle", but it seems to predate the
sourceware import.

Andrew>     [...] in some places 'asm_demangle' is completely
Andrew>     independent, and in other places 'asm_demangle' actually overrides
Andrew>     'demangle' rather than deferring to it.
   
Bleah.  Can / should we fix this.

There's also "set demangle-style", which is both obsolete and inherently
broken.  It should be removed.

Andrew> +  if (asm_demangle == AUTO_BOOLEAN_AUTO)
Andrew> +    fprintf_filtered (file,
Andrew> +      _("Demangling of C++/ObjC names in disassembly "

We should probably use this opportunity to drop the C++/ObjC text, since
this applies to any language with mangling.

Tom
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Demangle function names when disassembling

Tom Tromey-4
In reply to this post by Tom Tromey-4
Tom> gdb/ChangeLog
Tom> 2020-07-23  Tom Tromey  <[hidden email]>

Tom> PR symtab/26270:
Tom> * symtab.h (find_pc_partial_function_sym): Declare.
Tom> * cli/cli-cmds.c (disassemble_command): Use
Tom> find_pc_partial_function_sym.  Check asm_demangle.
Tom> * blockframe.c (cache_pc_function_sym): New global.
Tom> (cache_pc_function_name): Remove.
Tom> (clear_pc_function_cache): Update.
Tom> (find_pc_partial_function_sym): New function, from
Tom> find_pc_partial_function.
Tom> (find_pc_partial_function): Rewrite using
Tom> find_pc_partial_function_sym.

I'm checking this in now.

Tom