[PATCH][gdb/symtab] Fix language of duplicate static minimal symbol

[Tom de Vries]

Hi,

Consider a test-case with source files msym.c:
...
static int foo (void) { return 1; }
...
and msym_main.c:
...
static int foo (void) { return 2; }
int main (void) { return 0; }
..
compiled as c++ with minimal symbols:
...
$ g++ msym_main.c msym.c
...

With objdump -x we find the two foo symbols prefixed with their corresponding
files in the symbol table:
...
0000000000000000 l df *ABS* 0000000000000000 msym_main.c
00000000004004c7 l F .text 000000000000000b _ZL3foov
0000000000000000 l df *ABS* 0000000000000000 msym.c
00000000004004dd l F .text 000000000000000b _ZL3foov
...

However, when we use gdb to print info on foo, both foos are listed, but we
get one symbol mangled and one symbol demangled:
...
$ gdb ./a.out -batch -ex "info func foo"
All functions matching regular expression "foo":

Non-debugging symbols:
0x00000000004004c7 foo()
0x00000000004004dd _ZL3foov
...

During minimal symbol reading symbol_set_names is called for each symbol.

First, it's called with foo from msym.c, an entry is created in
per_bfd->demangled_names_hash and symbol_find_demangled_name is called, which
has the side effect of setting the language of the symbol to language_cplus.

Then, it's called with foo from msym_main.c. Since
per_bfd->demangled_names_hash already has an entry for that name,
symbol_find_demangled_name is not called, and the language of the symbol
remains language_auto.

Fix this by doing the symbol_find_demangled_name call unconditionally.

Build and reg-tested on x86_64-linux.

OK for trunk?

Thanks,
- Tom

[gdb/symtab] Fix language of duplicate static minimal symbol

2018-10-30 Tom de Vries <***@suse.de>

* symtab.c (symbol_set_names): Call symbol_find_demangled_name
unconditionally, to get the language of the symbol.

* gdb.base/msym.c: New test.
* gdb.base/msym.exp: New file.
* gdb.base/msym_main.c: New test.

---
gdb/symtab.c | 6 ++++--
gdb/testsuite/gdb.base/msym.c | 22 ++++++++++++++++++++++
gdb/testsuite/gdb.base/msym.exp | 25 +++++++++++++++++++++++++
gdb/testsuite/gdb.base/msym_main.c | 28 ++++++++++++++++++++++++++++
4 files changed, 79 insertions(+), 2 deletions(-)

diff --git a/gdb/symtab.c b/gdb/symtab.c
index cd27a75e8c..481428f733 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -818,6 +818,10 @@ symbol_set_names (struct general_symbol_info *gsymbol,
else
linkage_name_copy = linkage_name;

+ /* Set the symbol language. */
+ char *demangled_name = symbol_find_demangled_name (gsymbol,
+ linkage_name_copy);
+
entry.mangled = linkage_name_copy;
slot = ((struct demangled_name_entry **)
htab_find_slot (per_bfd->demangled_names_hash,
@@ -830,8 +834,6 @@ symbol_set_names (struct general_symbol_info *gsymbol,
|| (gsymbol->language == language_go
&& (*slot)->demangled[0] == '\0'))
{
- char *demangled_name = symbol_find_demangled_name (gsymbol,
- linkage_name_copy);
int demangled_len = demangled_name ? strlen (demangled_name) : 0;

/* Suppose we have demangled_name==NULL, copy_name==0, and
diff --git a/gdb/testsuite/gdb.base/msym.c b/gdb/testsuite/gdb.base/msym.c
new file mode 100644
index 0000000000..c9610154dd
--- /dev/null
+++ b/gdb/testsuite/gdb.base/msym.c
@@ -0,0 +1,22 @@
+/* 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/>. */
+
+static int
+foo (void)
+{
+ return 1;
+}
diff --git a/gdb/testsuite/gdb.base/msym.exp b/gdb/testsuite/gdb.base/msym.exp
new file mode 100644
index 0000000000..bb2143447e
--- /dev/null
+++ b/gdb/testsuite/gdb.base/msym.exp
@@ -0,0 +1,25 @@
+# Copyright (C) 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/>.
+
+standard_testfile msym_main.c msym.c
+
+if {[prepare_for_testing "failed to prepare" $testfile [list $srcfile $srcfile2] \
+ {c++}]} {
+ return -1
+}
+
+clean_restart ${testfile}
+
+gdb_test "info func foo" ".* foo\\(\\).* foo\\(\\).*"
diff --git a/gdb/testsuite/gdb.base/msym_main.c b/gdb/testsuite/gdb.base/msym_main.c
new file mode 100644
index 0000000000..f093f71ca4
--- /dev/null
+++ b/gdb/testsuite/gdb.base/msym_main.c
@@ -0,0 +1,28 @@
+/* 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/>. */
+
+static int
+foo (void)
+{
+ return 2;
+}
+
+int
+main (void)
+{
+ return 0;
+}

[Keith Seitz]

Post by Tom de Vries
However, when we use gdb to print info on foo, both foos are listed, but we
...
$ gdb ./a.out -batch -ex "info func foo"
0x00000000004004c7 foo()
0x00000000004004dd _ZL3foov
...

Good catch!

Post by Tom de Vries
Build and reg-tested on x86_64-linux.
OK for trunk?

I have just a few comments.

Post by Tom de Vries
* symtab.c (symbol_set_names): Call symbol_find_demangled_name
unconditionally, to get the language of the symbol.

s/get/set/ ? When reading the patch, I originally wondered how that would help,
but symbol_find_demangled_name actually *sets* the gsymbol's language if
it is language_auto.

Post by Tom de Vries
* gdb.base/msym.c: New test.
* gdb.base/msym.exp: New file.
* gdb.base/msym_main.c: New test.

These entries should be listed under the testsuite ChangeLog, like:

testsuite/ChangeLog:
YYYY-MM-DD ....

* gdb.xyz/file1.c: ...
* gdb.xyz/file1.exp: ....

[https://sourceware.org/gdb/wiki/ContributionChecklist#Properly_Formatted_GNU_ChangeLog]

"msym.c" et al aren't particularly descriptive, though. Can we devise a better,
more explicit name? Something along the lines of "multiple-language-msyms.exp".
It's long, but it describes things better. [I'm not saying you should use
this name, but something other than just "msym.{c,exp}".]

Sorry, some of that is kinda nitpicky.

Post by Tom de Vries
diff --git a/gdb/symtab.c b/gdb/symtab.c
index cd27a75e8c..481428f733 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -818,6 +818,10 @@ symbol_set_names (struct general_symbol_info *gsymbol,
else
linkage_name_copy = linkage_name;
+ /* Set the symbol language. */
+ char *demangled_name = symbol_find_demangled_name (gsymbol,
+ linkage_name_copy);
+
entry.mangled = linkage_name_copy;
slot = ((struct demangled_name_entry **)
htab_find_slot (per_bfd->demangled_names_hash,

symbol_find_demangled_name returns a malloc'd string which will leak here unless
the code goes through the branch. You'll need to save the result into a
unique_xmalloc_ptr and adjust the code accordingly.

Otherwise, LGTM. Thanks very much for the patch.

Keith (IANAM*)

* I am not a maintainer, you'll need to await final review by a global maintainer.

[Tom de Vries]

Post by Keith Seitz

Post by Tom de Vries
However, when we use gdb to print info on foo, both foos are listed, but we
...
$ gdb ./a.out -batch -ex "info func foo"
0x00000000004004c7 foo()
0x00000000004004dd _ZL3foov
...

Good catch!

Post by Tom de Vries

Post by Tom de Vries
Build and reg-tested on x86_64-linux.

OK for trunk?

I have just a few comments.

Post by Tom de Vries
* symtab.c (symbol_set_names): Call symbol_find_demangled_name
unconditionally, to get the language of the symbol.

s/get/set/ ? When reading the patch, I originally wondered how that would help,
but symbol_find_demangled_name actually *sets* the gsymbol's language if
it is language_auto.

Typo fixed.

Post by Keith Seitz

Post by Tom de Vries
* gdb.base/msym.c: New test.
* gdb.base/msym.exp: New file.
* gdb.base/msym_main.c: New test.

YYYY-MM-DD ....
* gdb.xyz/file1.c: ...
* gdb.xyz/file1.exp: ....
[https://sourceware.org/gdb/wiki/ContributionChecklist#Properly_Formatted_GNU_ChangeLog]

I have a pre-commit script (for both gcc and gdb) that uses this format,
and puts ChangeLog hunks in the correct ChangeLog file ( more detail
here: https://sourceware.org/ml/gdb-patches/2018-06/msg00351.html ).
Sofar this format has been acceptable for maintainers.

Post by Keith Seitz
"msym.c" et al aren't particularly descriptive, though. Can we devise a better,
more explicit name? Something along the lines of "multiple-language-msyms.exp".
It's long, but it describes things better. [I'm not saying you should use
this name, but something other than just "msym.{c,exp}".]

Renamed to msym-lang.exp et al.

Post by Keith Seitz
Sorry, some of that is kinda nitpicky.

Np, I'm still kinda new to gdb, all input appreciated.

Post by Keith Seitz

Post by Tom de Vries
diff --git a/gdb/symtab.c b/gdb/symtab.c
index cd27a75e8c..481428f733 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -818,6 +818,10 @@ symbol_set_names (struct general_symbol_info *gsymbol,
else
linkage_name_copy = linkage_name;
+ /* Set the symbol language. */
+ char *demangled_name = symbol_find_demangled_name (gsymbol,
+ linkage_name_copy);
+
entry.mangled = linkage_name_copy;
slot = ((struct demangled_name_entry **)
htab_find_slot (per_bfd->demangled_names_hash,

symbol_find_demangled_name returns a malloc'd string which will leak here unless
the code goes through the branch. You'll need to save the result into a
unique_xmalloc_ptr and adjust the code accordingly.

Done, thanks for pointing that out.

Post by Keith Seitz
Otherwise, LGTM. Thanks very much for the patch.

And thanks for the review.

Re-tested as attached.

Thanks,
- Tom

[Tom de Vries]

Post by Tom de Vries

Post by Keith Seitz

Post by Tom de Vries
However, when we use gdb to print info on foo, both foos are listed, but we
...
$ gdb ./a.out -batch -ex "info func foo"
0x00000000004004c7 foo()
0x00000000004004dd _ZL3foov
...

Good catch!

Post by Tom de Vries

Post by Tom de Vries
Build and reg-tested on x86_64-linux.

OK for trunk?

I have just a few comments.

Post by Tom de Vries
* symtab.c (symbol_set_names): Call symbol_find_demangled_name
unconditionally, to get the language of the symbol.

s/get/set/ ? When reading the patch, I originally wondered how that would help,
but symbol_find_demangled_name actually *sets* the gsymbol's language if
it is language_auto.

Typo fixed.

Post by Keith Seitz

Post by Tom de Vries
* gdb.base/msym.c: New test.
* gdb.base/msym.exp: New file.
* gdb.base/msym_main.c: New test.

YYYY-MM-DD ....
* gdb.xyz/file1.c: ...
* gdb.xyz/file1.exp: ....
[https://sourceware.org/gdb/wiki/ContributionChecklist#Properly_Formatted_GNU_ChangeLog]

I have a pre-commit script (for both gcc and gdb) that uses this format,
and puts ChangeLog hunks in the correct ChangeLog file ( more detail
here: https://sourceware.org/ml/gdb-patches/2018-06/msg00351.html ).
Sofar this format has been acceptable for maintainers.

Post by Keith Seitz
"msym.c" et al aren't particularly descriptive, though. Can we devise a better,
more explicit name? Something along the lines of "multiple-language-msyms.exp".
It's long, but it describes things better. [I'm not saying you should use
this name, but something other than just "msym.{c,exp}".]

Renamed to msym-lang.exp et al.

Post by Keith Seitz
Sorry, some of that is kinda nitpicky.

Np, I'm still kinda new to gdb, all input appreciated.

Post by Keith Seitz

Post by Tom de Vries
diff --git a/gdb/symtab.c b/gdb/symtab.c
index cd27a75e8c..481428f733 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -818,6 +818,10 @@ symbol_set_names (struct general_symbol_info *gsymbol,
else
linkage_name_copy = linkage_name;
+ /* Set the symbol language. */
+ char *demangled_name = symbol_find_demangled_name (gsymbol,
+ linkage_name_copy);
+
entry.mangled = linkage_name_copy;
slot = ((struct demangled_name_entry **)
htab_find_slot (per_bfd->demangled_names_hash,

symbol_find_demangled_name returns a malloc'd string which will leak here unless
the code goes through the branch. You'll need to save the result into a
unique_xmalloc_ptr and adjust the code accordingly.

Done, thanks for pointing that out.

Post by Keith Seitz
Otherwise, LGTM. Thanks very much for the patch.

And thanks for the review.
Re-tested as attached.

Ping. Ok for trunk?

Thanks,
- Tom

Post by Tom de Vries
0001-gdb-symtab-Fix-language-of-duplicate-static-minimal-symbol.patch
[gdb/symtab] Fix language of duplicate static minimal symbol
...
static int foo (void) { return 1; }
...
...
static int foo (void) { return 2; }
int main (void) { return 0; }
..
...
$ g++ msym_main.c msym.c
...
With objdump -x we find the two foo symbols prefixed with their corresponding
...
0000000000000000 l df *ABS* 0000000000000000 msym_main.c
00000000004004c7 l F .text 000000000000000b _ZL3foov
0000000000000000 l df *ABS* 0000000000000000 msym.c
00000000004004dd l F .text 000000000000000b _ZL3foov
...
However, when we use gdb to print info on foo, both foos are listed, but we
...
$ gdb ./a.out -batch -ex "info func foo"
0x00000000004004c7 foo()
0x00000000004004dd _ZL3foov
...
During minimal symbol reading symbol_set_names is called for each symbol.
First, it's called with foo from msym.c, an entry is created in
per_bfd->demangled_names_hash and symbol_find_demangled_name is called, which
has the side effect of setting the language of the symbol to language_cplus.
Then, it's called with foo from msym_main.c. Since
per_bfd->demangled_names_hash already has an entry for that name,
symbol_find_demangled_name is not called, and the language of the symbol
remains language_auto.
Fix this by doing the symbol_find_demangled_name call unconditionally.
Build and reg-tested on x86_64-linux.
* symtab.c (symbol_set_names): Call symbol_find_demangled_name
unconditionally, to set the language of the symbol. Manage freeing
returned pointer using gdb::unique_xmalloc_ptr.
* gdb.base/msym-lang.c: New test.
* gdb.base/msym-lang.exp: New file.
* gdb.base/msym-lang-main.c: New test.
---
gdb/symtab.c | 14 +++++++-------
gdb/testsuite/gdb.base/msym-lang-main.c | 28 ++++++++++++++++++++++++++++
gdb/testsuite/gdb.base/msym-lang.c | 22 ++++++++++++++++++++++
gdb/testsuite/gdb.base/msym-lang.exp | 25 +++++++++++++++++++++++++
4 files changed, 82 insertions(+), 7 deletions(-)
diff --git a/gdb/symtab.c b/gdb/symtab.c
index cd27a75e8c..aee4c0c710 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -818,6 +818,11 @@ symbol_set_names (struct general_symbol_info *gsymbol,
else
linkage_name_copy = linkage_name;
+ /* Set the symbol language. */
+ char *demangled_name_ptr
+ = symbol_find_demangled_name (gsymbol, linkage_name_copy);
+ gdb::unique_xmalloc_ptr<char> demangled_name (demangled_name_ptr);
+
entry.mangled = linkage_name_copy;
slot = ((struct demangled_name_entry **)
htab_find_slot (per_bfd->demangled_names_hash,
@@ -830,9 +835,7 @@ symbol_set_names (struct general_symbol_info *gsymbol,
|| (gsymbol->language == language_go
&& (*slot)->demangled[0] == '\0'))
{
- char *demangled_name = symbol_find_demangled_name (gsymbol,
- linkage_name_copy);
- int demangled_len = demangled_name ? strlen (demangled_name) : 0;
+ int demangled_len = demangled_name ? strlen (demangled_name.get ()) : 0;
/* Suppose we have demangled_name==NULL, copy_name==0, and
linkage_name_copy==linkage_name. In this case, we already have the
@@ -870,10 +873,7 @@ symbol_set_names (struct general_symbol_info *gsymbol,
}
if (demangled_name != NULL)
- {
- strcpy ((*slot)->demangled, demangled_name);
- xfree (demangled_name);
- }
+ strcpy ((*slot)->demangled, demangled_name.get());
else
(*slot)->demangled[0] = '\0';
}
diff --git a/gdb/testsuite/gdb.base/msym-lang-main.c b/gdb/testsuite/gdb.base/msym-lang-main.c
new file mode 100644
index 0000000000..f093f71ca4
--- /dev/null
+++ b/gdb/testsuite/gdb.base/msym-lang-main.c
@@ -0,0 +1,28 @@
+/* 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/>. */
+
+static int
+foo (void)
+{
+ return 2;
+}
+
+int
+main (void)
+{
+ return 0;
+}
diff --git a/gdb/testsuite/gdb.base/msym-lang.c b/gdb/testsuite/gdb.base/msym-lang.c
new file mode 100644
index 0000000000..c9610154dd
--- /dev/null
+++ b/gdb/testsuite/gdb.base/msym-lang.c
@@ -0,0 +1,22 @@
+/* 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/>. */
+
+static int
+foo (void)
+{
+ return 1;
+}
diff --git a/gdb/testsuite/gdb.base/msym-lang.exp b/gdb/testsuite/gdb.base/msym-lang.exp
new file mode 100644
index 0000000000..993225c9d6
--- /dev/null
+++ b/gdb/testsuite/gdb.base/msym-lang.exp
@@ -0,0 +1,25 @@
+# Copyright (C) 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/>.
+
+standard_testfile msym-lang-main.c msym-lang.c
+
+if {[prepare_for_testing "failed to prepare" $testfile [list $srcfile $srcfile2] \
+ {c++}]} {
+ return -1
+}
+
+clean_restart ${testfile}
+
+gdb_test "info func foo" ".* foo\\(\\).* foo\\(\\).*"

[Simon Marchi]

Post by Tom de Vries
I have a pre-commit script (for both gcc and gdb) that uses this format,
and puts ChangeLog hunks in the correct ChangeLog file ( more detail
here: https://sourceware.org/ml/gdb-patches/2018-06/msg00351.html ).
Sofar this format has been acceptable for maintainers.

Yes, as long as they end up in the right file it's fine.

The patch LGTM too. Before, pushing could you just add a one liner
description in your .exp file? Something like:

# Test that when two symbols with the same linkage name are present in
the
# executable, they both get assigned the right language.

Thanks,

Simon

[Simon Marchi]

Post by Simon Marchi

Post by Tom de Vries
I have a pre-commit script (for both gcc and gdb) that uses this format,
and puts ChangeLog hunks in the correct ChangeLog file ( more detail
here: https://sourceware.org/ml/gdb-patches/2018-06/msg00351.html ).
Sofar this format has been acceptable for maintainers.

Yes, as long as they end up in the right file it's fine.

Keith made me look at the contribution checklist again, and it says:

"In your patch email you should also specify which changelog is being
modified."

I agree that for clarity, it's better to state in which ChangeLog each
snippet goes. It would be preferable to follow that.

Personally, in the patch email, I don't include the date/name/email for
ChangeLog entries that will bear my name, since it would be redundant.
And putting today's date would be meaningless, as I would update it
anyway to the current date when I push the patch. And I think putting
"YYYY-MM-DD" doesn't serve much purpose.

Simon