[PATCH] D103971: [lld-macho] Rework mergeFlag to closer mimic what LD64 does.

[Jez Ng via Phabricator via llvm-commits]

int3 added a comment.

> Sorry for the delay, was OOO earlier this week for a surgery.

Sorry to hear that, hope you're recovering well!

So after examining all the cases... I think that perhaps all this checking logic is not needed after all, and we should just do a bitwise-or merge for now?



================
Comment at: lld/MachO/ConcatOutputSection.cpp:343-360
+  default /*type-unspec'ed*/:
+    if (input->segName == segment_names::data &&
+        (input->sectName == section_names::objcClassList ||
+         input->sectName == section_names::objcCatList ||
+         input->sectName == section_names::objcNonLazyClassList ||
+         input->sectName == section_names::objcNonLazyCatList ||
+         input->sectName == section_names::objSuperrefs))
----------------
all these seem relevant only if we are re-emitting object files, which we probably won't bother with for a while. In particular, I think `S_ATTR_NO_DEAD_STRIP` is only relevant as input to the linker, and `___compact_unwind` gets converted to `__unwind info`  as part of the usual link process. IMO we can just leave a FIXME comment for now for whomever wants to add object file support


================
Comment at: lld/MachO/ConcatOutputSection.cpp:376
+  case S_NON_LAZY_SYMBOL_POINTERS:
+    if (config->outputType != MH_KEXT_BUNDLE &&
+        !(config->outputType == MH_EXECUTE && config->isPic))
----------------
we're not likely to support kexts in the foreseeable future, so I don't think it's worth checking for here


================
Comment at: lld/MachO/ConcatOutputSection.cpp:377
+    if (config->outputType != MH_KEXT_BUNDLE &&
+        !(config->outputType == MH_EXECUTE && config->isPic))
+      adjusted = S_NON_LAZY_SYMBOL_POINTERS;
----------------
I don't think this is right, ld64's equivalent check is for static executables, and we only ever emit dynamic ones


================
Comment at: lld/MachO/ConcatOutputSection.cpp:380-382
+  case S_SYMBOL_STUBS:
+    adjusted =
+        S_SYMBOL_STUBS | S_ATTR_SOME_INSTRUCTIONS | S_ATTR_PURE_INSTRUCTIONS;
----------------
I don't believe any ConcatInputSection should have S_SYMBOL_STUBS set. But our StubsSection does have these three flags set, so I think we're good here

(I think the same applies for `S_NON_LAZY_SYMBOL_POINTERS` above -- we shouldn't have ConcatInputSections with that flag set, at least as far as arm64/x86_64 is concerned. I think arm32 explicitly encodes some of these stubs in the input, so that's a whole other problem for another day)


================
Comment at: lld/MachO/ConcatOutputSection.cpp:383-384
+        S_SYMBOL_STUBS | S_ATTR_SOME_INSTRUCTIONS | S_ATTR_PURE_INSTRUCTIONS;
+    if (!input->relocs.empty())
+      adjusted |= S_ATTR_LOC_RELOC;
+    break;
----------------
hm, I think this is also only relevant if we are emitting object files


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103971