[prev in list] [next in list] [prev in thread] [next in thread] 

List:       gdb-patches
Subject:    Re: [PATCH][gdb/symtab] Ignore cold clones
From:       Bernd Edlinger <bernd.edlinger () hotmail ! de>
Date:       2021-05-31 16:54:33
Message-ID: AM8PR10MB470872B58F078FD0332C25B2E43F9 () AM8PR10MB4708 ! EURPRD10 ! PROD ! OUTLOOK ! COM
[Download RAW message or body]

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
> 
[prev in list] [next in list] [prev in thread] [next in thread]