[PATCH] D132947: [lld-macho] Support synthesizing __TEXT,__init_offsets

[Nico Weber via Phabricator via llvm-commits]

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

Looks great!



================
Comment at: lld/MachO/Driver.cpp:1110
+            continue;
+          }
           isec->outSecOff = inputOrder++;
----------------
Looks like ld64 also adds the arg to `-init` (we don't implement this flag yet, but should probably have a FIXME somewhere to add it to initOffsets once we do?)


================
Comment at: lld/MachO/Driver.cpp:1440
       args.hasFlag(OPT_data_in_code_info, OPT_no_data_in_code_info, true);
+  config->emitInitOffsets = args.hasArg(OPT_init_offsets);
   config->icfLevel = getICFLevel(args);
----------------
I wanted to ask if this is enabled by default at some output os version, but it looks like it's on by default if chained fixups are on, which are on by default at some output os version. So D132560 will also enable this, I imagine.


================
Comment at: lld/MachO/MarkLive.cpp:280
     // mod_init_funcs, mod_term_funcs sections
     if (sectionType(isec->getFlags()) == S_MOD_INIT_FUNC_POINTERS ||
         sectionType(isec->getFlags()) == S_MOD_TERM_FUNC_POINTERS) {
----------------
Should we _not_ enqueue `S_MOD_INIT_FUNC_POINTERS` here if we're doing this transform, since `initOffsets` is what produces lifeness in that case?

…oh, I see, we don't even put `S_MOD_INIT_FUNC_POINTERS` sections into `inputSections` in that case, so this will never happen.

Should this loop `assert(!config->emitInitOffsets || sectionType(isec->getFlags()) != S_MOD_INIT_FUNC_POINTERS)` to make that more clear, maybe?


================
Comment at: lld/MachO/Writer.cpp:1136
+  if (in.initOffsets->isNeeded())
+    in.initOffsets->setup();
 
----------------
Very nit: I'd appreciate it if you could rename the existing two `setup()` methods to `setUp()` in a preparatory commit (just land, no need for review), and then call this new one `setUp` too. ("setup" is a noun, "to set up" is a verb, and methods are named after verbs.)


================
Comment at: lld/test/MachO/init-offsets.s:15
+# RUN: od -An -txI %t/section.bin >> %t/dump.txt
+# RUN: FileCheck --check-prefix=CONTENT %s < %t/dump.txt
+
----------------
Good test!


================
Comment at: llvm/lib/MC/MCSectionMachO.cpp:65
      StringLiteral("S_THREAD_LOCAL_INIT_FUNCTION_POINTERS")}, // 0x15
+    {StringLiteral("") /*FIXME??*/,
+     StringLiteral("S_INIT_FUNC_OFFSETS")}, // 0x16
----------------
I wanted to say something about this, but apparently it's local style in this file O_o


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

https://reviews.llvm.org/D132947