This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


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?


> 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.


> 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.

Ciao!
Steven


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]