* [PATCH][gdb/symtab] Ignore cold clones
@ 2021-05-31 14:15 Tom de Vries
2021-05-31 14:26 ` Simon Marchi via Gdb-patches
0 siblings, 1 reply; 7+ messages in thread
From: Tom de Vries @ 2021-05-31 14:15 UTC (permalink / raw)
To: gdb-patches
Hi,
Consider the test-case contained in this patch, compiled for c using gcc-10:
...
$ gcc-10 -x c src/gdb/testsuite/gdb.cp/cold-clone.cc -O2 -g -Wall -Wextra
...
When setting a breakpoint on foo, we get one breakpoint location:
...
$ gdb -q -batch a.out -ex "b foo"
Breakpoint 1 at 0x400560: file cold-clone.cc, line 28.
...
However, when we compile for c++ instead, we get two breakpoint locations:
...
$ gdb -q -batch a.out -ex "b foo" -ex "info break"
Breakpoint 1 at 0x400430: foo. (2 locations)
Num Type Disp Enb Address What
1 breakpoint keep y <MULTIPLE>
1.1 y 0x0000000000400430 in foo() at cold-clone.cc:30
1.2 y 0x0000000000400560 in foo() at cold-clone.cc:28
...
The additional breakpoint location at 0x400430 corresponds to the cold clone:
...
$ nm a.out | grep foo
0000000000400560 t _ZL3foov
0000000000400430 t _ZL3foov.cold
...
which demangled looks like this:
...
$ nm -C a.out | grep foo
0000000000400560 t foo()
0000000000400430 t foo() [clone .cold]
...
[ Or, in the case of the cc1 mentioned in PR23710:
...
$ nm cc1 | grep do_rpo_vn.*cold
000000000058659d t \
_ZL9do_rpo_vnP8functionP8edge_defP11bitmap_headbb.cold.138
$ nm -C cc1 | grep do_rpo_vn.*cold
000000000058659d t \
do_rpo_vn(function*, edge_def*, bitmap_head*, bool, bool) [clone .cold.138]
... ]
The cold clone is a part of the function that is split off from the rest of
the function because it's considered cold (not frequently executed). So while
the symbol points to code that is part of a function, it doesn't point to a
function entry, so the desirable behaviour for "break foo" is to ignore this
symbol.
When compiling for c, the symbol "foo.cold" is entered as minimal symbol
with the search name "foo.cold", and the lookup using "foo" fails to find that
symbol.
But when compiling for c++, the symbol "foo.cold" is entered as minimal symbol
with both the mangled and demangled name, and for the demangled name
"foo() [clone .cold]" we get the search name "foo" (because
cp_search_name_hash stops hashing at '('), and the lookup using "foo" succeeds.
Fix this by recognizing the cold clone suffix and returning false for such a
minimal symbol in msymbol_is_function.
Tested on x86_64-linux.
Any comments?
Thanks,
- Tom
[gdb/symtab] Ignore cold clones
gdb/ChangeLog:
2021-05-31 Tom de Vries <tdevries@suse.de>
PR symtab/26096
* minsyms.c (msymbol_is_cold_clone): New function.
(msymbol_is_function): Use msymbol_is_cold_clone.
gdb/testsuite/ChangeLog:
2021-05-31 Tom de Vries <tdevries@suse.de>
PR symtab/26096
* gdb.cp/cold-clone.cc: New test.
* gdb.cp/cold-clone.exp: New file.
---
gdb/minsyms.c | 52 +++++++++++++++++++++++++++++++++++
gdb/testsuite/gdb.cp/cold-clone.cc | 54 +++++++++++++++++++++++++++++++++++++
gdb/testsuite/gdb.cp/cold-clone.exp | 30 +++++++++++++++++++++
3 files changed, 136 insertions(+)
diff --git a/gdb/minsyms.c b/gdb/minsyms.c
index 8ffd9076124..80c3d431209 100644
--- a/gdb/minsyms.c
+++ b/gdb/minsyms.c
@@ -60,6 +60,53 @@
#include <mutex>
#endif
+/* Return true if MINSYM is a cold clone symbol.
+ Recognize f.i. these symbols (mangled/demangled):
+ - _ZL3foov.cold
+ foo() [clone .cold]
+ - _ZL9do_rpo_vnP8functionP8edge_defP11bitmap_headbb.cold.138
+ do_rpo_vn(function*, edge_def*, bitmap_head*, bool, bool) \
+ [clone .cold.138]. */
+
+static bool
+msymbol_is_cold_clone (minimal_symbol *minsym)
+{
+ const char *name = minsym->natural_name ();
+ size_t name_len = strlen (name);
+ if (name_len < 1)
+ return false;
+
+ const char *last = &name[name_len - 1];
+ if (*last != ']')
+ return false;
+
+ const char *suffix = " [clone .cold";
+ size_t suffix_len = strlen (suffix);
+ const char *found = strstr (name, suffix);
+ if (found == nullptr)
+ return false;
+
+ const char *start = &found[suffix_len];
+ if (*start == ']')
+ return true;
+
+ if (*start != '.')
+ return false;
+
+ const char *p;
+ for (p = start + 1; p <= last; ++p)
+ {
+ if (*p >= '0' && *p <= '9')
+ continue;
+ break;
+ }
+
+ if (p == last)
+ return true;
+
+ return false;
+}
+
/* See minsyms.h. */
bool
@@ -89,6 +136,11 @@ msymbol_is_function (struct objfile *objfile, minimal_symbol *minsym,
}
return false;
}
+ case mst_file_text:
+ /* Ignore function symbol that is not a function entry. */
+ if (msymbol_is_cold_clone (minsym))
+ return false;
+ /* fallthru */
default:
if (func_address_p != NULL)
*func_address_p = msym_addr;
diff --git a/gdb/testsuite/gdb.cp/cold-clone.cc b/gdb/testsuite/gdb.cp/cold-clone.cc
new file mode 100644
index 00000000000..233a84a7ee7
--- /dev/null
+++ b/gdb/testsuite/gdb.cp/cold-clone.cc
@@ -0,0 +1,54 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+ Copyright 2021 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/>. */
+
+#include <stdlib.h>
+#include "../lib/attributes.h"
+
+int a;
+int b;
+int c;
+
+static int __attribute__((used, noinline)) ATTRIBUTE_NOCLONE
+foo (void)
+{
+ a = 2;
+ if (b)
+ abort ();
+
+ return c;
+}
+
+static int __attribute__((used, noinline)) ATTRIBUTE_NOCLONE
+bar (void)
+{
+ a = 1;
+ if (c)
+ abort ();
+ return b;
+}
+
+int
+main (int argc, char **argv __attribute__((unused)))
+{
+ b = argc * 2;
+ c = argc / 2;
+
+ if (b + c == 5)
+ abort ();
+
+ return foo () + bar ();
+}
diff --git a/gdb/testsuite/gdb.cp/cold-clone.exp b/gdb/testsuite/gdb.cp/cold-clone.exp
new file mode 100644
index 00000000000..07b9bdc0a93
--- /dev/null
+++ b/gdb/testsuite/gdb.cp/cold-clone.exp
@@ -0,0 +1,30 @@
+# Copyright 2021 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 .cc
+
+if {[prepare_for_testing "failed to prepare" $testfile $srcfile \
+ {debug c++ optimize=-O2}]} {
+ return -1
+}
+
+gdb_test_multiple "break foo" "" {
+ -re -wrap "\\($decimal locations\\)" {
+ fail $gdb_test_name
+ }
+ -re -wrap "" {
+ pass $gdb_test_name
+ }
+}
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH][gdb/symtab] Ignore cold clones
2021-05-31 14:15 [PATCH][gdb/symtab] Ignore cold clones Tom de Vries
@ 2021-05-31 14:26 ` Simon Marchi via Gdb-patches
2021-05-31 14:59 ` Tom de Vries
0 siblings, 1 reply; 7+ messages in thread
From: Simon Marchi via Gdb-patches @ 2021-05-31 14:26 UTC (permalink / raw)
To: Tom de Vries, gdb-patches
On 2021-05-31 10:15 a.m., Tom de Vries wrote:
> Hi,
>
> Consider the test-case contained in this patch, compiled for c using gcc-10:
> ...
> $ gcc-10 -x c src/gdb/testsuite/gdb.cp/cold-clone.cc -O2 -g -Wall -Wextra
> ...
>
> When setting a breakpoint on foo, we get one breakpoint location:
> ...
> $ gdb -q -batch a.out -ex "b foo"
> Breakpoint 1 at 0x400560: file cold-clone.cc, line 28.
> ...
>
> However, when we compile for c++ instead, we get two breakpoint locations:
> ...
> $ gdb -q -batch a.out -ex "b foo" -ex "info break"
> Breakpoint 1 at 0x400430: foo. (2 locations)
> Num Type Disp Enb Address What
> 1 breakpoint keep y <MULTIPLE>
> 1.1 y 0x0000000000400430 in foo() at cold-clone.cc:30
> 1.2 y 0x0000000000400560 in foo() at cold-clone.cc:28
> ...
>
> The additional breakpoint location at 0x400430 corresponds to the cold clone:
> ...
> $ nm a.out | grep foo
> 0000000000400560 t _ZL3foov
> 0000000000400430 t _ZL3foov.cold
> ...
> which demangled looks like this:
> ...
> $ nm -C a.out | grep foo
> 0000000000400560 t foo()
> 0000000000400430 t foo() [clone .cold]
> ...
>
> [ Or, in the case of the cc1 mentioned in PR23710:
> ...
> $ nm cc1 | grep do_rpo_vn.*cold
> 000000000058659d t \
> _ZL9do_rpo_vnP8functionP8edge_defP11bitmap_headbb.cold.138
> $ nm -C cc1 | grep do_rpo_vn.*cold
> 000000000058659d t \
> do_rpo_vn(function*, edge_def*, bitmap_head*, bool, bool) [clone .cold.138]
> ... ]
>
> The cold clone is a part of the function that is split off from the rest of
> the function because it's considered cold (not frequently executed). So while
> the symbol points to code that is part of a function, it doesn't point to a
> function entry, so the desirable behaviour for "break foo" is to ignore this
> symbol.
>
> When compiling for c, the symbol "foo.cold" is entered as minimal symbol
> with the search name "foo.cold", and the lookup using "foo" fails to find that
> symbol.
>
> But when compiling for c++, the symbol "foo.cold" is entered as minimal symbol
> with both the mangled and demangled name, and for the demangled name
> "foo() [clone .cold]" we get the search name "foo" (because
> cp_search_name_hash stops hashing at '('), and the lookup using "foo" succeeds.
>
> Fix this by recognizing the cold clone suffix and returning false for such a
> minimal symbol in msymbol_is_function.
>
> Tested on x86_64-linux.
>
> Any comments?
Is this related to
https://sourceware.org/pipermail/gdb-patches/2021-May/179371.html
?
Simon
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH][gdb/symtab] Ignore cold clones
2021-05-31 14:26 ` Simon Marchi via Gdb-patches
@ 2021-05-31 14:59 ` Tom de Vries
2021-05-31 16:54 ` Bernd Edlinger
0 siblings, 1 reply; 7+ messages in thread
From: Tom de Vries @ 2021-05-31 14:59 UTC (permalink / raw)
To: Simon Marchi, gdb-patches; +Cc: Bernd Edlinger
On 5/31/21 4:26 PM, Simon Marchi wrote:
> On 2021-05-31 10:15 a.m., Tom de Vries wrote:
>> Hi,
>>
>> Consider the test-case contained in this patch, compiled for c using gcc-10:
>> ...
>> $ gcc-10 -x c src/gdb/testsuite/gdb.cp/cold-clone.cc -O2 -g -Wall -Wextra
>> ...
>>
>> When setting a breakpoint on foo, we get one breakpoint location:
>> ...
>> $ gdb -q -batch a.out -ex "b foo"
>> Breakpoint 1 at 0x400560: file cold-clone.cc, line 28.
>> ...
>>
>> However, when we compile for c++ instead, we get two breakpoint locations:
>> ...
>> $ gdb -q -batch a.out -ex "b foo" -ex "info break"
>> Breakpoint 1 at 0x400430: foo. (2 locations)
>> Num Type Disp Enb Address What
>> 1 breakpoint keep y <MULTIPLE>
>> 1.1 y 0x0000000000400430 in foo() at cold-clone.cc:30
>> 1.2 y 0x0000000000400560 in foo() at cold-clone.cc:28
>> ...
>>
>> The additional breakpoint location at 0x400430 corresponds to the cold clone:
>> ...
>> $ nm a.out | grep foo
>> 0000000000400560 t _ZL3foov
>> 0000000000400430 t _ZL3foov.cold
>> ...
>> which demangled looks like this:
>> ...
>> $ nm -C a.out | grep foo
>> 0000000000400560 t foo()
>> 0000000000400430 t foo() [clone .cold]
>> ...
>>
>> [ Or, in the case of the cc1 mentioned in PR23710:
>> ...
>> $ nm cc1 | grep do_rpo_vn.*cold
>> 000000000058659d t \
>> _ZL9do_rpo_vnP8functionP8edge_defP11bitmap_headbb.cold.138
>> $ nm -C cc1 | grep do_rpo_vn.*cold
>> 000000000058659d t \
>> do_rpo_vn(function*, edge_def*, bitmap_head*, bool, bool) [clone .cold.138]
>> ... ]
>>
>> The cold clone is a part of the function that is split off from the rest of
>> the function because it's considered cold (not frequently executed). So while
>> the symbol points to code that is part of a function, it doesn't point to a
>> function entry, so the desirable behaviour for "break foo" is to ignore this
>> symbol.
>>
>> When compiling for c, the symbol "foo.cold" is entered as minimal symbol
>> with the search name "foo.cold", and the lookup using "foo" fails to find that
>> symbol.
>>
>> But when compiling for c++, the symbol "foo.cold" is entered as minimal symbol
>> with both the mangled and demangled name, and for the demangled name
>> "foo() [clone .cold]" we get the search name "foo" (because
>> cp_search_name_hash stops hashing at '('), and the lookup using "foo" succeeds.
>>
>> Fix this by recognizing the cold clone suffix and returning false for such a
>> minimal symbol in msymbol_is_function.
>>
>> Tested on x86_64-linux.
>>
>> Any comments?
>
> Is this related to
>
> https://sourceware.org/pipermail/gdb-patches/2021-May/179371.html
>
Yes, it looks like both attempt to fix the same PR, thanks for the pointer.
At first glance, that one doesn't handle symbol
"_ZL9do_rpo_vnP8functionP8edge_defP11bitmap_headbb.cold.138".
Another difference seems to be how the cold clone status is determined:
- this patch uses the demangled name, and treats finding the .cold
string (used in a specific way) as proof
- Bernd's patch uses the mangled name, and treats finding the .cold
string as a hint, and proceeds to find the corresponding function
entry symbol to proof it
[ FWIW, that patch mentions it fixes 77f2120b200 ("Don't drop static
function bp locations w/o debug info"), but the PR did exist before that
commit, that commit just made it more frequent. ]
Thanks,
- Tom
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH][gdb/symtab] Ignore cold clones
2021-05-31 14:59 ` Tom de Vries
@ 2021-05-31 16:54 ` Bernd Edlinger
2021-06-01 5:18 ` Bernd Edlinger
2021-06-01 13:23 ` Tom de Vries
0 siblings, 2 replies; 7+ messages in thread
From: Bernd Edlinger @ 2021-05-31 16:54 UTC (permalink / raw)
To: Tom de Vries, Simon Marchi, gdb-patches
On 5/31/21 4:59 PM, Tom de Vries wrote:
> On 5/31/21 4:26 PM, Simon Marchi wrote:
>> On 2021-05-31 10:15 a.m., Tom de Vries wrote:
>>> Hi,
>>>
>>> Consider the test-case contained in this patch, compiled for c using gcc-10:
>>> ...
>>> $ gcc-10 -x c src/gdb/testsuite/gdb.cp/cold-clone.cc -O2 -g -Wall -Wextra
>>> ...
>>>
>>> When setting a breakpoint on foo, we get one breakpoint location:
>>> ...
>>> $ gdb -q -batch a.out -ex "b foo"
>>> Breakpoint 1 at 0x400560: file cold-clone.cc, line 28.
>>> ...
>>>
>>> However, when we compile for c++ instead, we get two breakpoint locations:
>>> ...
>>> $ gdb -q -batch a.out -ex "b foo" -ex "info break"
>>> Breakpoint 1 at 0x400430: foo. (2 locations)
>>> Num Type Disp Enb Address What
>>> 1 breakpoint keep y <MULTIPLE>
>>> 1.1 y 0x0000000000400430 in foo() at cold-clone.cc:30
>>> 1.2 y 0x0000000000400560 in foo() at cold-clone.cc:28
>>> ...
>>>
>>> The additional breakpoint location at 0x400430 corresponds to the cold clone:
>>> ...
>>> $ nm a.out | grep foo
>>> 0000000000400560 t _ZL3foov
>>> 0000000000400430 t _ZL3foov.cold
>>> ...
>>> which demangled looks like this:
>>> ...
>>> $ nm -C a.out | grep foo
>>> 0000000000400560 t foo()
>>> 0000000000400430 t foo() [clone .cold]
>>> ...
>>>
>>> [ Or, in the case of the cc1 mentioned in PR23710:
>>> ...
>>> $ nm cc1 | grep do_rpo_vn.*cold
>>> 000000000058659d t \
>>> _ZL9do_rpo_vnP8functionP8edge_defP11bitmap_headbb.cold.138
>>> $ nm -C cc1 | grep do_rpo_vn.*cold
>>> 000000000058659d t \
>>> do_rpo_vn(function*, edge_def*, bitmap_head*, bool, bool) [clone .cold.138]
>>> ... ]
>>>
>>> The cold clone is a part of the function that is split off from the rest of
>>> the function because it's considered cold (not frequently executed). So while
>>> the symbol points to code that is part of a function, it doesn't point to a
>>> function entry, so the desirable behaviour for "break foo" is to ignore this
>>> symbol.
>>>
>>> When compiling for c, the symbol "foo.cold" is entered as minimal symbol
>>> with the search name "foo.cold", and the lookup using "foo" fails to find that
>>> symbol.
>>>
>>> But when compiling for c++, the symbol "foo.cold" is entered as minimal symbol
>>> with both the mangled and demangled name, and for the demangled name
>>> "foo() [clone .cold]" we get the search name "foo" (because
>>> cp_search_name_hash stops hashing at '('), and the lookup using "foo" succeeds.
>>>
>>> Fix this by recognizing the cold clone suffix and returning false for such a
>>> minimal symbol in msymbol_is_function.
>>>
>>> Tested on x86_64-linux.
>>>
>>> Any comments?
>>
>> Is this related to
>>
>> https://sourceware.org/pipermail/gdb-patches/2021-May/179371.html
>>
>
> Yes, it looks like both attempt to fix the same PR, thanks for the pointer.
>
> At first glance, that one doesn't handle symbol
> "_ZL9do_rpo_vnP8functionP8edge_defP11bitmap_headbb.cold.138".
>
Is this symbol from gcc?
I never saw this kind of mangled symbol.
TBH, I was not sure if I should handle foo$cold or __foo_cold.
In theory there may be targets where the assembler does not like ".",
then gcc would emit foo$cold
If neither "." nor "$" is acceptable then gcc would emit __foo_cold
but I was not sure if such targets do really exist.
Bernd.
> Another difference seems to be how the cold clone status is determined:
> - this patch uses the demangled name, and treats finding the .cold
> string (used in a specific way) as proof
> - Bernd's patch uses the mangled name, and treats finding the .cold
> string as a hint, and proceeds to find the corresponding function
> entry symbol to proof it
>
> [ FWIW, that patch mentions it fixes 77f2120b200 ("Don't drop static
> function bp locations w/o debug info"), but the PR did exist before that
> commit, that commit just made it more frequent. ]
>
> Thanks,
> - Tom
>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH][gdb/symtab] Ignore cold clones
2021-05-31 16:54 ` Bernd Edlinger
@ 2021-06-01 5:18 ` Bernd Edlinger
2021-06-01 11:33 ` Tom de Vries
2021-06-01 13:23 ` Tom de Vries
1 sibling, 1 reply; 7+ messages in thread
From: Bernd Edlinger @ 2021-06-01 5:18 UTC (permalink / raw)
To: Tom de Vries, Simon Marchi, gdb-patches
On 5/31/21 6:54 PM, Bernd Edlinger wrote:
> On 5/31/21 4:59 PM, Tom de Vries wrote:
>> On 5/31/21 4:26 PM, Simon Marchi wrote:
>>> On 2021-05-31 10:15 a.m., Tom de Vries wrote:
>>>> Hi,
>>>>
>>>> Consider the test-case contained in this patch, compiled for c using gcc-10:
>>>> ...
>>>> $ gcc-10 -x c src/gdb/testsuite/gdb.cp/cold-clone.cc -O2 -g -Wall -Wextra
>>>> ...
>>>>
>>>> When setting a breakpoint on foo, we get one breakpoint location:
>>>> ...
>>>> $ gdb -q -batch a.out -ex "b foo"
>>>> Breakpoint 1 at 0x400560: file cold-clone.cc, line 28.
>>>> ...
>>>>
>>>> However, when we compile for c++ instead, we get two breakpoint locations:
>>>> ...
>>>> $ gdb -q -batch a.out -ex "b foo" -ex "info break"
>>>> Breakpoint 1 at 0x400430: foo. (2 locations)
>>>> Num Type Disp Enb Address What
>>>> 1 breakpoint keep y <MULTIPLE>
>>>> 1.1 y 0x0000000000400430 in foo() at cold-clone.cc:30
>>>> 1.2 y 0x0000000000400560 in foo() at cold-clone.cc:28
>>>> ...
>>>>
>>>> The additional breakpoint location at 0x400430 corresponds to the cold clone:
>>>> ...
>>>> $ nm a.out | grep foo
>>>> 0000000000400560 t _ZL3foov
>>>> 0000000000400430 t _ZL3foov.cold
>>>> ...
>>>> which demangled looks like this:
>>>> ...
>>>> $ nm -C a.out | grep foo
>>>> 0000000000400560 t foo()
>>>> 0000000000400430 t foo() [clone .cold]
>>>> ...
>>>>
>>>> [ Or, in the case of the cc1 mentioned in PR23710:
>>>> ...
>>>> $ nm cc1 | grep do_rpo_vn.*cold
>>>> 000000000058659d t \
>>>> _ZL9do_rpo_vnP8functionP8edge_defP11bitmap_headbb.cold.138
>>>> $ nm -C cc1 | grep do_rpo_vn.*cold
>>>> 000000000058659d t \
>>>> do_rpo_vn(function*, edge_def*, bitmap_head*, bool, bool) [clone .cold.138]
>>>> ... ]
>>>>
>>>> The cold clone is a part of the function that is split off from the rest of
>>>> the function because it's considered cold (not frequently executed). So while
>>>> the symbol points to code that is part of a function, it doesn't point to a
>>>> function entry, so the desirable behaviour for "break foo" is to ignore this
>>>> symbol.
>>>>
>>>> When compiling for c, the symbol "foo.cold" is entered as minimal symbol
>>>> with the search name "foo.cold", and the lookup using "foo" fails to find that
>>>> symbol.
>>>>
>>>> But when compiling for c++, the symbol "foo.cold" is entered as minimal symbol
>>>> with both the mangled and demangled name, and for the demangled name
>>>> "foo() [clone .cold]" we get the search name "foo" (because
>>>> cp_search_name_hash stops hashing at '('), and the lookup using "foo" succeeds.
>>>>
>>>> Fix this by recognizing the cold clone suffix and returning false for such a
>>>> minimal symbol in msymbol_is_function.
>>>>
>>>> Tested on x86_64-linux.
>>>>
>>>> Any comments?
>>>
>>> Is this related to
>>>
>>> https://sourceware.org/pipermail/gdb-patches/2021-May/179371.html
>>>
>>
>> Yes, it looks like both attempt to fix the same PR, thanks for the pointer.
>>
>> At first glance, that one doesn't handle symbol
>> "_ZL9do_rpo_vnP8functionP8edge_defP11bitmap_headbb.cold.138".
>>
>
> Is this symbol from gcc?
> I never saw this kind of mangled symbol.
>
So, I did a gcc lto-bootstrap over night with the current trunk version,
and got these symbols in cc1:
$ nm cc1|grep do_rpo_vn
0000000000fb9a70 T _Z9do_rpo_vnP8functionP8edge_defP11bitmap_head
0000000000fb7a40 t _ZL9do_rpo_vnP8functionP8edge_defP11bitmap_headbb.lto_priv.0
0000000000719052 t _ZL9do_rpo_vnP8functionP8edge_defP11bitmap_headbb.lto_priv.0.cold
does your patch handle these?
Bernd.
> TBH, I was not sure if I should handle foo$cold or __foo_cold.
> In theory there may be targets where the assembler does not like ".",
> then gcc would emit foo$cold
> If neither "." nor "$" is acceptable then gcc would emit __foo_cold
> but I was not sure if such targets do really exist.
>
>
> Bernd.
>
>> Another difference seems to be how the cold clone status is determined:
>> - this patch uses the demangled name, and treats finding the .cold
>> string (used in a specific way) as proof
>> - Bernd's patch uses the mangled name, and treats finding the .cold
>> string as a hint, and proceeds to find the corresponding function
>> entry symbol to proof it
>>
>> [ FWIW, that patch mentions it fixes 77f2120b200 ("Don't drop static
>> function bp locations w/o debug info"), but the PR did exist before that
>> commit, that commit just made it more frequent. ]
>>
>> Thanks,
>> - Tom
>>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH][gdb/symtab] Ignore cold clones
2021-06-01 5:18 ` Bernd Edlinger
@ 2021-06-01 11:33 ` Tom de Vries
0 siblings, 0 replies; 7+ messages in thread
From: Tom de Vries @ 2021-06-01 11:33 UTC (permalink / raw)
To: Bernd Edlinger, Simon Marchi, gdb-patches
On 6/1/21 7:18 AM, Bernd Edlinger wrote:
> So, I did a gcc lto-bootstrap over night with the current trunk version,
> and got these symbols in cc1:
>
> $ nm cc1|grep do_rpo_vn
> 0000000000fb9a70 T _Z9do_rpo_vnP8functionP8edge_defP11bitmap_head
> 0000000000fb7a40 t _ZL9do_rpo_vnP8functionP8edge_defP11bitmap_headbb.lto_priv.0
> 0000000000719052 t _ZL9do_rpo_vnP8functionP8edge_defP11bitmap_headbb.lto_priv.0.cold
>
> does your patch handle these?
It does.
Demangled we have:
...
$ c++filt _ZL9do_rpo_vnP8functionP8edge_defP11bitmap_headbb.lto_priv.0.cold
do_rpo_vn(function*, edge_def*, bitmap_head*, bool, bool) [clone
.lto_priv.0] [clone .cold]
...
I build gdb using gcc-11 and -O2 -flto=auto, and found a similar simbol:
...
$ nm -C gdb.lto | grep " init_syscalls_info"
0000000000878a10 T init_syscalls_info(gdbarch*) [clone .lto_priv.0]
0000000000436d19 t init_syscalls_info(gdbarch*) [clone .lto_priv.0] \
[clone .cold]
...
With the patch, I have:
...
$ gdb -q -batch ./gdb.lto -ex "b init_syscalls_info" -ex "info break"
Breakpoint 1 at 0x878a10: file xml-syscall.c, line 338.
Num Type Disp Enb Address What
1 breakpoint keep y 0x0000000000878a10 in
init_syscalls_info(gdbarch*) at xml-syscall.c:338
...
and without:
...
$ gdb -q -batch ./gdb.lto -ex "b init_syscalls_info" -ex "info break"
Breakpoint 1 at 0x436d19: init_syscalls_info. (2 locations)
Num Type Disp Enb Address
1 breakpoint keep y <MULTIPLE>
1.1 y 0x0000000000436d19
1.2 y 0x0000000000878a10
...
Thanks,
- Tom
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH][gdb/symtab] Ignore cold clones
2021-05-31 16:54 ` Bernd Edlinger
2021-06-01 5:18 ` Bernd Edlinger
@ 2021-06-01 13:23 ` Tom de Vries
1 sibling, 0 replies; 7+ messages in thread
From: Tom de Vries @ 2021-06-01 13:23 UTC (permalink / raw)
To: Bernd Edlinger, Simon Marchi, gdb-patches
On 5/31/21 6:54 PM, Bernd Edlinger wrote:
> On 5/31/21 4:59 PM, Tom de Vries wrote:
>> On 5/31/21 4:26 PM, Simon Marchi wrote:
>>> On 2021-05-31 10:15 a.m., Tom de Vries wrote:
>>>> Hi,
>>>>
>>>> Consider the test-case contained in this patch, compiled for c using gcc-10:
>>>> ...
>>>> $ gcc-10 -x c src/gdb/testsuite/gdb.cp/cold-clone.cc -O2 -g -Wall -Wextra
>>>> ...
>>>>
>>>> When setting a breakpoint on foo, we get one breakpoint location:
>>>> ...
>>>> $ gdb -q -batch a.out -ex "b foo"
>>>> Breakpoint 1 at 0x400560: file cold-clone.cc, line 28.
>>>> ...
>>>>
>>>> However, when we compile for c++ instead, we get two breakpoint locations:
>>>> ...
>>>> $ gdb -q -batch a.out -ex "b foo" -ex "info break"
>>>> Breakpoint 1 at 0x400430: foo. (2 locations)
>>>> Num Type Disp Enb Address What
>>>> 1 breakpoint keep y <MULTIPLE>
>>>> 1.1 y 0x0000000000400430 in foo() at cold-clone.cc:30
>>>> 1.2 y 0x0000000000400560 in foo() at cold-clone.cc:28
>>>> ...
>>>>
>>>> The additional breakpoint location at 0x400430 corresponds to the cold clone:
>>>> ...
>>>> $ nm a.out | grep foo
>>>> 0000000000400560 t _ZL3foov
>>>> 0000000000400430 t _ZL3foov.cold
>>>> ...
>>>> which demangled looks like this:
>>>> ...
>>>> $ nm -C a.out | grep foo
>>>> 0000000000400560 t foo()
>>>> 0000000000400430 t foo() [clone .cold]
>>>> ...
>>>>
>>>> [ Or, in the case of the cc1 mentioned in PR23710:
>>>> ...
>>>> $ nm cc1 | grep do_rpo_vn.*cold
>>>> 000000000058659d t \
>>>> _ZL9do_rpo_vnP8functionP8edge_defP11bitmap_headbb.cold.138
>>>> $ nm -C cc1 | grep do_rpo_vn.*cold
>>>> 000000000058659d t \
>>>> do_rpo_vn(function*, edge_def*, bitmap_head*, bool, bool) [clone .cold.138]
>>>> ... ]
>>>>
>>>> The cold clone is a part of the function that is split off from the rest of
>>>> the function because it's considered cold (not frequently executed). So while
>>>> the symbol points to code that is part of a function, it doesn't point to a
>>>> function entry, so the desirable behaviour for "break foo" is to ignore this
>>>> symbol.
>>>>
>>>> When compiling for c, the symbol "foo.cold" is entered as minimal symbol
>>>> with the search name "foo.cold", and the lookup using "foo" fails to find that
>>>> symbol.
>>>>
>>>> But when compiling for c++, the symbol "foo.cold" is entered as minimal symbol
>>>> with both the mangled and demangled name, and for the demangled name
>>>> "foo() [clone .cold]" we get the search name "foo" (because
>>>> cp_search_name_hash stops hashing at '('), and the lookup using "foo" succeeds.
>>>>
>>>> Fix this by recognizing the cold clone suffix and returning false for such a
>>>> minimal symbol in msymbol_is_function.
>>>>
>>>> Tested on x86_64-linux.
>>>>
>>>> Any comments?
>>>
>>> Is this related to
>>>
>>> https://sourceware.org/pipermail/gdb-patches/2021-May/179371.html
>>>
>>
>> Yes, it looks like both attempt to fix the same PR, thanks for the pointer.
>>
>> At first glance, that one doesn't handle symbol
>> "_ZL9do_rpo_vnP8functionP8edge_defP11bitmap_headbb.cold.138".
>>
>
> Is this symbol from gcc?
Yes (as you found out by now).
And looking at the DW_AT_producer it's generated by gcc-9 flto:
...
DW_AT_producer : GNU C++14 9.0.0 20180925 (experimental) [trunk
revision 259470] -mtune=generic -march=x86-64 -g -O2 -fno-PIE
-flto=jobserver -frandom-seed=1 -fno-exceptions -fno-rtti
-fasynchronous-unwind-tables
...
> I never saw this kind of mangled symbol.
>
> TBH, I was not sure if I should handle foo$cold or __foo_cold.
> In theory there may be targets where the assembler does not like ".",
> then gcc would emit foo$cold
> If neither "." nor "$" is acceptable then gcc would emit __foo_cold
> but I was not sure if such targets do really exist.
>
Yeah, good point.
Well, there's nvptx for example, but that one doesn't have upstream gdb
support. Anyway, it might be that this is dealt with somehow in the
demangler, I'm not sure. So I don't think it makes sense to try to fix
this until we see proof that it's actually broken.
Thanks,
- Tom
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-06-01 13:23 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-31 14:15 [PATCH][gdb/symtab] Ignore cold clones Tom de Vries
2021-05-31 14:26 ` Simon Marchi via Gdb-patches
2021-05-31 14:59 ` Tom de Vries
2021-05-31 16:54 ` Bernd Edlinger
2021-06-01 5:18 ` Bernd Edlinger
2021-06-01 11:33 ` Tom de Vries
2021-06-01 13:23 ` Tom de Vries
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox