Re: [PATCH] Generate a label for the split cold function while using -freorder-blocks-and-partition

[Teresa Johnson <tejohnson at google dot com>]

On Fri, Nov 8, 2013 at 8:14 AM, Steven Bosscher <stevenb.gcc@gmail.com> wrote:
> On Fri, Nov 8, 2013 at 4:45 PM, Teresa Johnson wrote:
>> On Tue, May 14, 2013 at 3:47 PM, Steven Bosscher wrote:
>> I'd like to revisit this old patch from Sri to generate a label for
>> the split out cold section, now that all the patches to fix the
>> failures and insanities from -freorder-blocks-and-partition are in
>> (and I want to send a patch to enable it by default with fdo). The
>> problem is that the split code doesn't currently have any label to
>> identify where it came from, so in the final binary it appears to be
>> part of whatever non-cold function the linker happens to place it
>> after. This confuses tools like objdump, profilers and the debugger.
>
> Surely there are .loc directives in the code to clarify where the code
> came from? So at least the debugger should know what function the code
> belongs to.
>
>> For example, from mcf with -freorder-blocks-and-partition:
>>
>> main:
>> .LFB40:
>>         .cfi_startproc
>>         ...
>>         jne     .L27           <---- jump to main's text.unlikely
>>         ...
>>         .cfi_endproc
>>         .section        .text.unlikely
>>         .cfi_startproc
>> .L27:                           <--- start of main's text.unlikely
>>         ...
>>
>> $ objdump -d mcf
>>
>> 0000000000400aa0 <main>:
>>   ...
>>   400b1a:       0f 85 1b fb ff ff       jne    40063b
>> <replace_weaker_arc+0x17b>   <---- jump to main's text.unlikely
>>
>> 00000000004004c0 <replace_weaker_arc>:
>>   ...
>>   40063b:       bf 06 8a 49 00          mov    $0x498a06,%edi    <---
>> start of main's text.unlikely
>>   ...
>>   40065e:       e9 0d 05 00 00          jmpq   400b70 <main+0xd0>
>> <--- jump back to main's hot text
>>
>>
>> Note that objdump thinks we are jumping to/from another function. If
>> we end up executing the cold section (e.g. due to non-representative
>> profiles), profiling tools and gdb are going to think we are executing
>> replace_weaker_arc. With Sri's patch, this becomes much clearer:
>>
>> 0000000000400aa0 <main>:
>>   ...
>>   400b1a:       0f 85 1b fb ff ff       jne    40063b <main.cold>
>>
>> 000000000040063b <main.cold>:
>>   40063b:       bf 06 8a 49 00          mov    $0x498a06,%edi
>>   ...
>>   40065e:       e9 0d 05 00 00          jmpq   400b70 <main+0xd0>
>>
>
> Does -ffunction-sections help here?

No. It just moves the cold section to a different spot (so the
function it appears part of is different, but same issue).

>
>
>> Steven, I think the ideas you describe above about passing profile
>> information in DWARF and doing some special handling in gdb to use
>> that seem orthogonal to this.
>
> It's not just about profile information but about what function some
> code belongs to. I just cannot believe that there isn't already
> something in DWARF to mark address ranges as being part of a given
> function.

Cary, any ideas here?

>
>
>> Given that, is this patch ok for trunk (pending successful re-runs of
>> regression testing)
>
>
> IMHO, no. This is conceptually wrong. You create artificial named
> labels in the compiler that might clash with user labels. Is that
> likely to happen? No. But it could and therefore the approach of
> adding functionname.cold labels is wrong.

Is there a format for compiler-defined labels that would not be able
to clash with other user-generated labels?

Teresa

>
> Ciao!
> Steven



-- 
Teresa Johnson | Software Engineer | tejohnson@google.com | 408-460-2413