[RFA] fix Ada SYMBOL_PRINT_NAME problem

[RFA] fix Ada SYMBOL_PRINT_NAME problem

[Joel Brobecker]

[-- Attachment #1: Type: text/plain, Size: 2472 bytes --]

Hello,

The debugger sometimes prints weird names for certain Ada entities.
This shows up inside multiple-choice menus printed when an expression
is ambiguous. For instance, given the following two variables declared
with the same name but in a different (global) scope:

   package First is
      I : Integer := 1;
   end First;

   package Second is
      I : Integer := 2;
   end Second;

Trying to print "i" will result in the following menu:

    (gdb) print i
    Multiple matches for i
    [0] cancel
    [1] pck__first(int) at pck.adb:3
    [2] pck__second(int) at pck.adb:7
    >

The expected output (particularly for choices 1 and 2) is:

    (gdb) p i
    Multiple matches for i
    [0] cancel
    [1] pck.first.i at pck.adb:3
    [2] pck.second.i at pck.adb:7
    > 

What happened in our case is that symtab.c:symbol_natural_name
checks for gsymbol->language_specific.cplus_specific.demangled_name,
and if not null, returns it. For Ada symbols, we expect this field
to be not null (as a side question, I wonder why we even check this
field when dealing with Ada symbols at all, but that's a separate
question - I will ask Paul).  The following comment that I added
with the patch explains why:

     Unfortunately, there is a case that we have to watch out for:
     During the symbol table processing, when creating the minimal
     symbols, we don't know the language associated to each entry.
     As a result, symbol_find_demangled_name tries various non-Ada
     demanglers, and sometimes, a demangler accepts the symbol and
     returns an unexpected demangled form of our symbol name.  This
     has consequences during the debugging info processing: This time,
     we do know the language, but since we have already found an entry
     in the demangled_name_hash using the linkage name, we use the
     demangled name stored in the hash instead of trying to recompute it.

So we had to add an explicit guard for Ada languages inside
symtab.c:symbol_set_names to make sure that Ada symbol do not
unexpectedly get the demangled_name field set.

2008-01-08  Joel Brobecker  <brobecker@adacore.com>

        * symtab.c (symbol_set_names): Do not set the demangled_name
        for Ada symbols.

I also wrote a small testcase.

2008-01-08  Joel Brobecker  <brobecker@adacore.com>

        * gdb.ada/sym_print_name: New test program.
        * gdb.ada/sym_print_name.exp: New testcase.

Tested on x86-linux, no regression.
OK to commit?

Thanks,
-- 
Joel

[-- Attachment #2: demangle.diff --]
[-- Type: text/plain, Size: 1548 bytes --]

Index: symtab.c
===================================================================
--- symtab.c	(revision 94)
+++ symtab.c	(revision 97)
@@ -577,8 +577,27 @@ symbol_set_names (struct general_symbol_
 	(*slot)[lookup_len + 1] = '\0';
     }
 
+  /* Set the name and demangled_name.
+     
+     For Ada symbols, the demangled_name does not apply and should
+     be NULL (see how symbol_find_demangled_name always return NULL
+     for Ada symbols).
+
+     Unfortunately, there is a case that we have to watch out for:
+     During the symbol table processing, when creating the minimal
+     symbols, we don't know the language associated to each entry.
+     As a result, symbol_find_demangled_name tries various non-Ada
+     demanglers, and sometimes, a demangler accepts the symbol and
+     returns an unexpected demangled form of our symbol name.  This
+     has consequences during the debugging info processing: This time,
+     we do know the language, but since we have already found an entry
+     in the demangled_name_hash using the linkage name, we use the
+     demangled name stored in the hash instead of trying to recompute it.
+
+     To guard against this case, we add an explicit check against
+     Ada symbols before setting the symbol demangled_name.  */
   gsymbol->name = *slot + lookup_len - len;
-  if ((*slot)[lookup_len + 1] != '\0')
+  if (gsymbol->language != language_ada && (*slot)[lookup_len + 1] != '\0')
     gsymbol->language_specific.cplus_specific.demangled_name
       = &(*slot)[lookup_len + 1];
   else

[-- Attachment #3: demangle-tc.diff --]
[-- Type: text/plain, Size: 5797 bytes --]

Index: gdb.ada/sym_print_name.exp
===================================================================
--- gdb.ada/sym_print_name.exp	(revision 0)
+++ gdb.ada/sym_print_name.exp	(revision 96)
@@ -0,0 +1,73 @@
+# Copyright 2008 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/>.
+
+if $tracelevel then {
+    strace $tracelevel
+}
+
+load_lib "ada.exp"
+
+set testdir "sym_print_name"
+set testfile "${testdir}/foo"
+set srcfile ${srcdir}/${subdir}/${testfile}.adb
+set binfile ${objdir}/${subdir}/${testfile}
+
+file mkdir ${objdir}/${subdir}/${testdir}
+if {[gdb_compile_ada "${srcfile}" "${binfile}" executable [list debug]] != "" } {
+  return -1
+}
+
+# A convenience function that joins all the arguments together,
+# with a regexp that matches zero-or-more end of lines in between
+# each argument.  This function is ideal to write the expected output
+# of a GDB command that generates more than a couple of lines, as
+# this allows us to write each line as a separate string, which is
+# easier to read by a human being.
+
+proc multi_line { args } {
+    return [join $args "\[\r\n\]*"]
+}
+
+gdb_exit
+gdb_start
+gdb_reinitialize_dir $srcdir/$subdir
+gdb_load ${binfile}
+
+set bp_location [gdb_get_line_number "STOP" ${testdir}/foo.adb]
+runto "foo.adb:$bp_location"
+
+set menu [multi_line "Multiple matches for i" \
+                     "\\\[0\\\] cancel" \
+                     "\\\[1\\\] pck\\.first\\.i.*" \
+                     "\\\[2\\\] pck\\.second\\.i.*" \
+                     "> $" ]
+
+set test_name "multiple matches for symbol i"
+gdb_test_multiple "print i" "$test_name" \
+{
+      -re "$menu" {
+          pass "$test_name"
+      }
+
+      default {
+          fail "$test_name"
+      }
+}
+
+# Select the first choice from the multiple-choice menu above.
+gdb_test "1" \
+         "48" \
+         "select first choice from multiple-choice menu"
+
Index: gdb.ada/sym_print_name/pck.adb
===================================================================
--- gdb.ada/sym_print_name/pck.adb	(revision 0)
+++ gdb.ada/sym_print_name/pck.adb	(revision 96)
@@ -0,0 +1,21 @@
+--  Copyright 2008 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 (Val : in out Integer) is
+   begin
+      null;
+   end Do_Nothing;
+end Pck;
Index: gdb.ada/sym_print_name/pck.ads
===================================================================
--- gdb.ada/sym_print_name/pck.ads	(revision 0)
+++ gdb.ada/sym_print_name/pck.ads	(revision 96)
@@ -0,0 +1,26 @@
+--  Copyright 2008 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 Pck is
+   package First is
+      I : Integer := 48;
+   end First;
+
+   package Second is
+      I : Integer := 74;
+   end Second;
+
+   procedure Do_Nothing (Val : in out Integer);
+end Pck;
Index: gdb.ada/sym_print_name/foo.adb
===================================================================
--- gdb.ada/sym_print_name/foo.adb	(revision 0)
+++ gdb.ada/sym_print_name/foo.adb	(revision 96)
@@ -0,0 +1,22 @@
+--  Copyright 2008 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;
+
+procedure Foo is
+begin
+   Do_Nothing (First.I);  -- STOP
+   Do_Nothing (Second.I);
+end Foo;

Re: [RFA] fix Ada SYMBOL_PRINT_NAME problem

[Daniel Jacobowitz]

On Tue, Jan 08, 2008 at 03:43:58AM -0800, Joel Brobecker wrote:
> Trying to print "i" will result in the following menu:
> 
>     (gdb) print i
>     Multiple matches for i
>     [0] cancel
>     [1] pck__first(int) at pck.adb:3
>     [2] pck__second(int) at pck.adb:7
>     >
> 
> The expected output (particularly for choices 1 and 2) is:
> 
>     (gdb) p i
>     Multiple matches for i
>     [0] cancel
>     [1] pck.first.i at pck.adb:3
>     [2] pck.second.i at pck.adb:7
>     > 

Of course, as far as I'm concerned, pck.first.i is the demangled
version of pck__first__i.  You just don't have a convenient demangler
for it.  This is related to a lot of the code duplication in
ada-lang.c that I was complaining about earlier.

The clever demangling lookup table was clever for C++, but less clever
for other languages; you'll see that Java already has some
special-case code, and the comments there (e.g. about the msymbol
having the wrong demangling) also apply to Ada.

I've no objection to this patch in principle, but you're doing it in
the wrong place.  If you already know the symbol is an Ada symbol, why
go through all the rigamarole of putting it in the hash table?


-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFA] fix Ada SYMBOL_PRINT_NAME problem

[Joel Brobecker]

[-- Attachment #1: Type: text/plain, Size: 1625 bytes --]

> > Trying to print "i" will result in the following menu:
> > 
> >     (gdb) print i
> >     Multiple matches for i
> >     [0] cancel
> >     [1] pck__first(int) at pck.adb:3
> >     [2] pck__second(int) at pck.adb:7
> >     >
> > 
> > The expected output (particularly for choices 1 and 2) is:
> > 
> >     (gdb) p i
> >     Multiple matches for i
> >     [0] cancel
> >     [1] pck.first.i at pck.adb:3
> >     [2] pck.second.i at pck.adb:7
[...]
> The clever demangling lookup table was clever for C++, but less clever
> for other languages; you'll see that Java already has some
> special-case code, and the comments there (e.g. about the msymbol
> having the wrong demangling) also apply to Ada.
> 
> I've no objection to this patch in principle, but you're doing it in
> the wrong place.  If you already know the symbol is an Ada symbol, why
> go through all the rigamarole of putting it in the hash table?

Duh! I feel ashamed I didn't think of that, and even more so that
it took me a long while to understand what you meant. Thanks for
the suggestion. I propose the following patch:

2008-01-30  Joel Brobecker  <brobecker@adacore.com>
                        
        * symtab.c (symbol_set_names): Do not add an entry in the demangling
        hash table for Ada symbols. Just store the linkage name as is,
        and leave the demangled_name as NULL.

Testcase unchanged:

2008-01-08  Joel Brobecker  <brobecker@adacore.com>

        * gdb.ada/sym_print_name: New test program.
        * gdb.ada/sym_print_name.exp: New testcase.

Does it look better? All tested on x86-linux, no regression.

Thanks,
-- 
Joel

[-- Attachment #2: demangle.diff --]
[-- Type: text/plain, Size: 1240 bytes --]

Index: symtab.c
===================================================================
--- symtab.c	(revision 135)
+++ symtab.c	(revision 136)
@@ -541,6 +541,24 @@ symbol_set_names (struct general_symbol_
   if (objfile->demangled_names_hash == NULL)
     create_demangled_names_hash (objfile);
 
+  if (gsymbol->language == language_ada)
+    {
+      /* In Ada, we do the symbol lookups using the mangled name, so
+         we can save some space by not storing the demangled name.
+
+         As a side note, we have also observed some overlap between
+         the C++ mangling and Ada mangling, similarly to what has
+         been observed with Java.  Because we don't store the demangled
+         name with the symbol, we don't need to use the same trick
+         as Java.  */
+      gsymbol->name = obstack_alloc (&objfile->objfile_obstack, len + 1);
+      memcpy (gsymbol->name, linkage_name, len);
+      gsymbol->name[len] = '\0';
+      gsymbol->language_specific.cplus_specific.demangled_name = NULL;
+
+      return;
+    }
+
   /* The stabs reader generally provides names that are not
      NUL-terminated; most of the other readers don't do this, so we
      can just use the given copy, unless we're in the Java case.  */

[-- Attachment #3: demangle-tc.diff --]
[-- Type: text/plain, Size: 5797 bytes --]

Index: gdb.ada/sym_print_name.exp
===================================================================
--- gdb.ada/sym_print_name.exp	(revision 0)
+++ gdb.ada/sym_print_name.exp	(revision 96)
@@ -0,0 +1,73 @@
+# Copyright 2008 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/>.
+
+if $tracelevel then {
+    strace $tracelevel
+}
+
+load_lib "ada.exp"
+
+set testdir "sym_print_name"
+set testfile "${testdir}/foo"
+set srcfile ${srcdir}/${subdir}/${testfile}.adb
+set binfile ${objdir}/${subdir}/${testfile}
+
+file mkdir ${objdir}/${subdir}/${testdir}
+if {[gdb_compile_ada "${srcfile}" "${binfile}" executable [list debug]] != "" } {
+  return -1
+}
+
+# A convenience function that joins all the arguments together,
+# with a regexp that matches zero-or-more end of lines in between
+# each argument.  This function is ideal to write the expected output
+# of a GDB command that generates more than a couple of lines, as
+# this allows us to write each line as a separate string, which is
+# easier to read by a human being.
+
+proc multi_line { args } {
+    return [join $args "\[\r\n\]*"]
+}
+
+gdb_exit
+gdb_start
+gdb_reinitialize_dir $srcdir/$subdir
+gdb_load ${binfile}
+
+set bp_location [gdb_get_line_number "STOP" ${testdir}/foo.adb]
+runto "foo.adb:$bp_location"
+
+set menu [multi_line "Multiple matches for i" \
+                     "\\\[0\\\] cancel" \
+                     "\\\[1\\\] pck\\.first\\.i.*" \
+                     "\\\[2\\\] pck\\.second\\.i.*" \
+                     "> $" ]
+
+set test_name "multiple matches for symbol i"
+gdb_test_multiple "print i" "$test_name" \
+{
+      -re "$menu" {
+          pass "$test_name"
+      }
+
+      default {
+          fail "$test_name"
+      }
+}
+
+# Select the first choice from the multiple-choice menu above.
+gdb_test "1" \
+         "48" \
+         "select first choice from multiple-choice menu"
+
Index: gdb.ada/sym_print_name/pck.adb
===================================================================
--- gdb.ada/sym_print_name/pck.adb	(revision 0)
+++ gdb.ada/sym_print_name/pck.adb	(revision 96)
@@ -0,0 +1,21 @@
+--  Copyright 2008 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 (Val : in out Integer) is
+   begin
+      null;
+   end Do_Nothing;
+end Pck;
Index: gdb.ada/sym_print_name/pck.ads
===================================================================
--- gdb.ada/sym_print_name/pck.ads	(revision 0)
+++ gdb.ada/sym_print_name/pck.ads	(revision 96)
@@ -0,0 +1,26 @@
+--  Copyright 2008 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 Pck is
+   package First is
+      I : Integer := 48;
+   end First;
+
+   package Second is
+      I : Integer := 74;
+   end Second;
+
+   procedure Do_Nothing (Val : in out Integer);
+end Pck;
Index: gdb.ada/sym_print_name/foo.adb
===================================================================
--- gdb.ada/sym_print_name/foo.adb	(revision 0)
+++ gdb.ada/sym_print_name/foo.adb	(revision 96)
@@ -0,0 +1,22 @@
+--  Copyright 2008 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;
+
+procedure Foo is
+begin
+   Do_Nothing (First.I);  -- STOP
+   Do_Nothing (Second.I);
+end Foo;

Re: [RFA] fix Ada SYMBOL_PRINT_NAME problem

[Daniel Jacobowitz]

On Wed, Jan 30, 2008 at 01:38:33PM -0800, Joel Brobecker wrote:
> 2008-01-30  Joel Brobecker  <brobecker@adacore.com>
>                         
>         * symtab.c (symbol_set_names): Do not add an entry in the demangling
>         hash table for Ada symbols. Just store the linkage name as is,
>         and leave the demangled_name as NULL.

I like this one much better :-)

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFA] fix Ada SYMBOL_PRINT_NAME problem

[Joel Brobecker]

> > 2008-01-30  Joel Brobecker  <brobecker@adacore.com>
> >                         
> >         * symtab.c (symbol_set_names): Do not add an entry in the demangling
> >         hash table for Ada symbols. Just store the linkage name as is,
> >         and leave the demangled_name as NULL.
> 
> I like this one much better :-)

Great! Patch checked in, together with the testcase.

Thanks,
-- 
Joel