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

[Bernd Edlinger <bernd.edlinger@hotmail.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?
> 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
>>