Re: [PATCH][gdb/symtab] Ignore cold clones

[Tom de Vries <tdevries@suse.de>]

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