This is an archive of the discontinued LLVM Phabricator instance.

[MC] Teach the MachO object writer about N_FUNC_COLD
ClosedPublic

Authored by vsk on Jan 24 2019, 1:53 PM.

Details

Summary

N_FUNC_COLD is a new MachO symbol attribute. It's a hint to the linker
to order a symbol towards the end of its section, to improve locality.

Example:

void a1() {}
__attribute__((cold)) void a2() {}
void a3() {}
int main() {
  a1();
  a2();
  a3();
  return 0;
}

A linker that supports N_FUNC_COLD will order _a2 to the end of the text
section. From nm -njU output, we see:

_a1
_a3
_main
_a2

Diff Detail

Event Timeline

vsk created this revision.Jan 24 2019, 1:53 PM
vsk added a reviewer: mtrent.Jan 24 2019, 4:23 PM

+ Mike, who's touched nm recently.

llvm-nm and BinaryFormat changes look good to me.

mtrent accepted this revision.Jan 24 2019, 4:58 PM
This revision is now accepted and ready to land.Jan 24 2019, 4:58 PM
pcc added a subscriber: pcc.Jan 24 2019, 5:11 PM

Interesting, what's the advantage of this over doing something like creating a __cold section in the __TEXT segment?

llvm/lib/MC/MCAsmStreamer.cpp
660

It should be possible to teach MC to parse this at least, right?

vsk updated this revision to Diff 183472.Jan 24 2019, 6:47 PM
vsk marked an inline comment as done.

Per @pcc's suggestion, add basic AsmParser support for the .cold directive.

vsk added a comment.Jan 24 2019, 6:48 PM
In D57190#1370535, @pcc wrote:

Interesting, what's the advantage of this over doing something like creating a __cold section in the __TEXT segment?

A .cold directive allows us to perform reordering in projects/build systems we don't control, which may have custom linker scripts, or other tooling which depends on text remaining in the __text section.

This revision was automatically updated to reflect the committed changes.
mtrent reopened this revision.Jan 25 2019, 4:25 PM
mtrent added inline comments.
llvm/trunk/tools/llvm-nm/llvm-nm.cpp
560 ↗(On Diff #183573)

Actually, upon further review, the text should be "[cold func]" to match the name of the symbol.

This revision is now accepted and ready to land.Jan 25 2019, 4:25 PM
vsk marked 2 inline comments as done.Jan 25 2019, 4:33 PM
vsk added inline comments.
llvm/trunk/tools/llvm-nm/llvm-nm.cpp
560 ↗(On Diff #183573)

Done in r352258.

vsk closed this revision.Jan 25 2019, 4:57 PM
vsk marked an inline comment as done.
kledzik added inline comments.Jan 25 2019, 5:01 PM
llvm/trunk/lib/MC/MCAsmStreamer.cpp
660 ↗(On Diff #183573)

Will there be a follow on patch to add a directive to allow cold attribute on symbols? Otherwise work flows that go through .s files will lose cold information.

vsk marked an inline comment as done.Jan 28 2019, 9:14 AM
vsk added inline comments.
llvm/trunk/lib/MC/MCAsmStreamer.cpp
660 ↗(On Diff #183573)

Yes, I'm tracking this in rdar://47598547 and rdar://47598666.

thakis added a subscriber: thakis.Dec 15 2020, 11:38 AM
thakis added inline comments.
llvm/trunk/include/llvm/BinaryFormat/MachO.h
337 ↗(On Diff #183573)

This (and the previous bits too) overlap with the ordinal written by http://llvm-cs.pcc.me.uk/include/llvm/BinaryFormat/MachO.h/rSET_LIBRARY_ORDINAL

I suppose the thinking is that that's safe because the ordinal is only set on undefs while these 3 bits here can't be set on undefs? If so, maybe this warrants a comment?

Herald added a project: Restricted Project. · View Herald Transcript
Herald added a subscriber: MaskRay. · View Herald Transcript
vsk added subscribers: davide, pete.Jan 3 2021, 4:45 PM
vsk added inline comments.
llvm/trunk/include/llvm/BinaryFormat/MachO.h
337 ↗(On Diff #183573)

I wasn't aware of the overlap and am not sure I understand whether it's safe. I know N_COLD_FUNC /shouldn't/ be set on undefs -- we may want to assert that -- but am not sure about the other two. @pete & @davide - your thoughts?