On Tue, May 14, 2013 at 3:47 PM, Steven Bosscher <stevenb....@gmail.com> wrote:
> On Wed, Apr 24, 2013 at 12:58 AM, Sriraman Tallam wrote:
>> Hi,
>>
>> This patch generates labels for cold function parts that are split when
>> using the option -freorder-blocks-and-partition. The cold label name
>> is generated by suffixing ".cold" to the assembler name of the hot
>> function.
>>
>> This is useful when getting back traces from gdb when the cold function
>> part does get executed.
>
> Can you show how this changes your gdb back traces? Can you not
> already let GDB inform you that it's in another .text section?
>
> I don't like this label idea much. It seems so arbitrary, unless
> you're only interested in knowing when a piece of function from
> another .text section is executed. But I'm personally more interested
> in the whole function, i.e. also in not-so-hot (or even cold) blocks
> that are not in the .text.unlikely section. That means somehow passing
> the profile-use information from the compiler to the debugger.
>
> So what I would really like to have instead, is some kind of DWARF
> annotations for profile information in the function, with some way in
> GDB to inspect and modify the values, and to add break points based on
> execution count threshold.
>
> I haven't really thought this through yet, but I have this dream of
> having the data of a gcno file encoded in DWARF instead, and to have
> profile information included in the FDO-compiled binary to add the
> ability to verify that those parts of the program that you expect to
> be hot are really also considered hot by the compiler. The only way to
> check this now is by inspecting some dumps and that's IMHO not very
> practical.
>
> Ciao!
> Steven
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.
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>
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.
Given that, is this patch ok for trunk (pending successful re-runs of
regression testing)
Thanks,
Teresa
--
Teresa Johnson | Software Engineer | tejohn...@google.com | 408-460-2413
