[PATCH] D112160: [MachO] Port call graph profile section and directive

[Nico Weber via Phabricator via llvm-commits]

thakis accepted this revision.
thakis added a comment.
This revision is now accepted and ready to land.

Nice! lgtm. One bikeshed comment about naming below (in two places, but about the same name).

I'll note: It looks like there's no attribute to tell the linker "omit this section from the final executable" – but segment name __LLVM _almost_ has this effect. The exception is that if someone were to:

- compile with -fprofile-use= (ie do a PGO build)
- pass `-Wl,-bitcode_process_mode,data` to ld64
- NOT pass `-Wl,-dead_strip` to ld64

…then they'd end up with an `__LLVM,__cgprofile` in their binary:

  % ~/src/llvm-project/out/gn/bin/clang baz.o bar.o -O2 -isysroot $(xcrun -show-sdk-path) -Wl,-bitcode_process_mode,data -Wl,-dead_strip 
  thakis at Nicos-MacBook-Pro pgotest % otool -vl a.out | grep __LLVM
  thakis at Nicos-MacBook-Pro pgotest % ~/src/llvm-project/out/gn/bin/clang baz.o bar.o -O2 -isysroot -isysroot $(xcrun -show-sdk-path) -Wl,-bitcode_process_mode,data     
  thakis at Nicos-MacBook-Pro pgotest % otool -vl a.out | grep -C2 __LLVM
        cmd LC_SEGMENT_64
    cmdsize 152
    segname __LLVM
     vmaddr 0x000000010000c000
     vmsize 0x0000000000004000
  --
  Section
    sectname __cg_profile
     segname __LLVM
        addr 0x000000010000c000
        size 0x0000000000000020

(.o files generated by adding `-c` to this line https://github.com/nico/hack/blob/main/pgotest/build.sh#L21)

This seems like a very unlikely sequence of events:

- `-bitcode_process_mode` is undocumented
- people who use flags as fancy as this likely also use `-dead_strip`

And there doesn't seem to be a better way to go about this – as far as I can tell, Mach-O doesn't have anything like `IMAGE_SCN_LNK_REMOVE`.



================
Comment at: llvm/lib/MC/MCMachOStreamer.cpp:539
+  // and set its size now so that it's accounted for in layout.
+  MCSection *CGProfile = Asm.getContext().getMachOSection(
+      "__LLVM", "__cg_profile", 0, SectionKind::getMetadata());
----------------
(see below for name)


================
Comment at: llvm/lib/MC/MCMachOStreamer.cpp:542
+  Asm.registerSection(*CGProfile);
+  auto *Frag = new MCDataFragment(CGProfile);
+  // For each entry, reserve space for 2 32-bit indices and a 64-bit count.
----------------
who owns this?

…oh, I see:
http://llvm-cs.pcc.me.uk/lib/MC/MCFragment.cpp#242

But is that owning?

http://llvm-cs.pcc.me.uk/include/llvm/MC/MCSection.h#50
http://llvm-cs.pcc.me.uk/include/llvm/ADT/ilist.h#307
http://llvm-cs.pcc.me.uk/include/llvm/ADT/ilist.h#41

Huh, subtle. Someone should probably change the API to use unique_ptrs one day. Bug I guess people familiar with these classes just know this.


================
Comment at: llvm/lib/MC/MachObjectWriter.cpp:763
+  if (!Asm.CGProfile.empty()) {
+    MCSection *CGProfile = Asm.getContext().getMachOSection(
+        "__LLVM", "__cg_profile", 0, SectionKind::getMetadata());
----------------
nit: Maybe call this CGProfileSection, else it looks very similar to Asm.CGProfile

(would also match COFF)


================
Comment at: llvm/test/MC/MachO/cgprofile.s:45
+# CHECK-NEXT:   }
+# CHECK-NEXT: ]
----------------
nit: add newline


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D112160/new/

https://reviews.llvm.org/D112160