[gdb/symtab] Fix language of duplicate static minimal symbol
Commit Message
On 10/30/18 10:21 PM, Keith Seitz wrote:
> On 10/30/18 12:11 PM, Tom de Vries wrote:
>> 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
>> ...
>>
>
> Good catch!
> >> Build and reg-tested on x86_64-linux.
>>
>> OK for trunk?
>
> I have just a few comments.
>
>> 2018-10-30 Tom de Vries <tdevries@suse.de>
>>
>> * 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.
>>
>> * 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]
>
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.
> "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.
> Sorry, some of that is kinda nitpicky.
>
Np, I'm still kinda new to gdb, all input appreciated.
>> 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.
> Otherwise, LGTM. Thanks very much for the patch.
>
And thanks for the review.
Re-tested as attached.
Thanks,
- Tom
Comments
On 10/31/18 10:09 AM, Tom de Vries wrote:
> On 10/30/18 10:21 PM, Keith Seitz wrote:
>> On 10/30/18 12:11 PM, Tom de Vries wrote:
>>> 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
>>> ...
>>>
>> Good catch!
>>>> Build and reg-tested on x86_64-linux.
>>> OK for trunk?
>> I have just a few comments.
>>
>>> 2018-10-30 Tom de Vries <tdevries@suse.de>
>>>
>>> * 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.
>
>>> * 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]
>>
> 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.
>
>> "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.
>
>> Sorry, some of that is kinda nitpicky.
>>
> Np, I'm still kinda new to gdb, all input appreciated.
>
>>> 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.
>
>> Otherwise, LGTM. Thanks very much for the patch.
>>
> And thanks for the review.
>
> Re-tested as attached.
>
Ping. Ok for trunk?
Thanks,
- Tom
> 0001-gdb-symtab-Fix-language-of-duplicate-static-minimal-symbol.patch
>
> [gdb/symtab] Fix language of duplicate static minimal symbol
>
> 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.
>
> 2018-10-30 Tom de Vries <tdevries@suse.de>
>
> * 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\\(\\).*"
>
On 2018-10-31 05:09, Tom de Vries wrote:
> 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
On 2018-11-07 10:23, Simon Marchi wrote:
> On 2018-10-31 05:09, Tom de Vries wrote:
>> 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
[gdb/symtab] Fix language of duplicate static minimal symbol
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.
2018-10-30 Tom de Vries <tdevries@suse.de>
* 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(-)
@@ -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';
}
new file mode 100644
@@ -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;
+}
new file mode 100644
@@ -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;
+}
new file mode 100644
@@ -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\\(\\).*"