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

[oontvoo]

Summary

Details:

I've been getting a few weird errors similar to the following from our internal tests:

ld64.lld.darwinnew: error: Cannot merge section __eh_frame (type=0x0) into __eh_frame (type=0xB): inconsistent types
ld64.lld.darwinnew: error: Cannot merge section __eh_frame (flags=0x0) into __eh_frame (flags=0x6800000B): strict flags differ
ld64.lld.darwinnew: error: Cannot merge section __eh_frame (type=0x0) into __eh_frame (type=0xB): inconsistent types
ld64.lld.darwinnew: error: Cannot merge section __eh_frame (flags=0x0) into __eh_frame (flags=0x6800000B): strict flags differ

[int3]

Could you add a test?

[oontvoo]

In D103971#2808327, @int3 wrote:

Could you add a test?

Yup, I'll try to simplify the test case we had that tripped over this.

But can we first agree on the logic? If the flag is not set, then it should not be a flag or type conflict. Does that make sense?

(also the comment on top of this function is ... interesting :-) " this is most likely wrong; ...The logic presented here was written without any form of informed research.". )

[int3]

It makes sense, yes. Always good to cross-check it with what ld64 is doing though.

(also the comment on top of this function is ... interesting :-)

I didn't write that :P adding some informed research here would definitely be appreciated!

[int3]

Bump. Are you going to finish this? I've stumbled onto some programs that need it :)

[oontvoo]

In D103971#2823570, @int3 wrote:

Bump. Are you going to finish this? I've stumbled onto some programs that need it :)

Sorry for the delay, was OOO earlier this week for a surgery.
Anyway, ld64 had a slightly different approach - instead of trying to "merge" input and output flags, it had an elaborate switch-case to decide what the "final" set of flags should be.
We could do just that?

[oontvoo]

rework mergeFlag to closer minic what ld64 does.

[oontvoo]

fixed typos

[oontvoo]

fixed type-conversions warnings

[int3]

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?

lld/MachO/ConcatOutputSection.cpp

343–360

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

376

we're not likely to support kexts in the foreseeable future, so I don't think it's worth checking for here

377

I don't think this is right, ld64's equivalent check is for static executables, and we only ever emit dynamic ones

380–382

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)

383–384

hm, I think this is also only relevant if we are emitting object files

[oontvoo]

In D103971#2824744, @int3 wrote:

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?

I've updated the switch to simplify the logic - (still looks like we can't simply merge the type.... )

lld/MachO/ConcatOutputSection.cpp

376

Right, this is checking for that cases that are *not* kext :)

377

fixed logic

[oontvoo]

updated diff

[oontvoo]

updated tests

[int3]

lld/MachO/ConcatOutputSection.cpp

376

Right, this is checking for that cases that are *not* kext :)

yes I understood that... my point is that it's never going to be kext, so the check is redundant

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

I don't think you addressed this :)

[oontvoo]

lld/MachO/ConcatOutputSection.cpp

376

done (for real this time)

[oontvoo]

updated diff

[int3]

Thanks!

lld/MachO/ConcatOutputSection.cpp

360

since S_REGULAR is zero, there's no need to do the bitwise or for the default case

[oontvoo]

updated diff per request

[oontvoo]

rebase

lld/MachO/ConcatOutputSection.h

public:
   std::vector<ConcatInputSection *> inputs;
   std::vector<ConcatInputSection *> thunks;
 
   static bool classof(const OutputSection *sec) {
     return sec->kind() == ConcatKind;
   }
 
 private:
-  void mergeFlags(InputSection *input);
+  void finalizeFlags(InputSection *input);
 
   size_t size = 0;
   uint64_t fileSize = 0;
 };
 
 // We maintain one ThunkInfo per real function.
 //
 // The "active thunk" is represented by the sym/isec pair that

lld/MachO/ConcatOutputSection.cpp

 using namespace lld::macho;
 
 void ConcatOutputSection::addInput(ConcatInputSection *input) {
   if (inputs.empty()) {
     align = input->align;
     flags = input->flags;
   } else {
     align = std::max(align, input->align);
-    mergeFlags(input);
+    finalizeFlags(input);
   }
   inputs.push_back(input);
   input->parent = this;
 }
 
 // Branch-range extension can be implemented in two ways, either through ...
 //
 // (1) Branch islands: Single branch instructions (also of limited range),
while (i < ie || t < te) {
     }
     while (t < te && (i == ie || thunks[t]->outSecOff < inputs[i]->outSecOff)) {
       thunks[t]->writeTo(buf + thunks[t]->outSecFileOff);
       ++t;
     }
   }
 }
 
-// TODO: this is most likely wrong; reconsider how section flags
-// are actually merged. The logic presented here was written without
-// any form of informed research.
-void ConcatOutputSection::mergeFlags(InputSection *input) {
-  uint8_t baseType = flags & SECTION_TYPE;
+void ConcatOutputSection::finalizeFlags(InputSection *input) {
+
   uint8_t inputType = input->flags & SECTION_TYPE;
-  if (baseType != inputType)
-    error("Cannot merge section " + input->name + " (type=0x" +
-          to_hexString(inputType) + ") into " + name + " (type=0x" +
-          to_hexString(baseType) + "): inconsistent types");
-
-  constexpr uint32_t strictFlags = S_ATTR_DEBUG | S_ATTR_STRIP_STATIC_SYMS |
-                                   S_ATTR_NO_DEAD_STRIP | S_ATTR_LIVE_SUPPORT;
-  if ((input->flags ^ flags) & strictFlags)
-    error("Cannot merge section " + input->name + " (flags=0x" +
-          to_hexString(input->flags) + ") into " + name + " (flags=0x" +
-          to_hexString(flags) + "): strict flags differ");
-
-  // Negate pure instruction presence if any section isn't pure.
-  uint32_t pureMask = ~S_ATTR_PURE_INSTRUCTIONS | (input->flags & flags);
-
-  // Merge the rest
-  flags |= input->flags;
-  flags &= pureMask;
+  uint8_t adjusted = S_REGULAR;
+  switch (inputType) {
+  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))
+      adjusted = S_REGULAR | S_ATTR_NO_DEAD_STRIP;
+    else if (input->segName = segment_names::data &&
+                              input->sectName == section_names::objcSelfrefs)
+      adjusted = S_LITERAL_POINTERS | S_ATTR_NO_DEAD_STRIP;
+    else if (input->segName == segment_names::objc &&
+             input->sectName == section_names::classSect)
+      adjusted = S_REGULAR | S_ATTR_NO_DEAD_STRIP;
+    else if (input->segName == segmen_names::ld &&
+             input->sectName == compactUnwind)
+      adjusted = S_REGULAR | S_ATTR_DEBUG;
+    break;
+  case S_4BYTE_LITERALS:
+  case S_8BYTE_LITERALS:
+  case S_16BYTE_LITERALS:
+  case S_CSTRING_LITERALS:
+  case S_ZEROFILL:
+  case S_LAZY_SYMBOL_POINTERS:
+  case S_MOD_TERM_FUNC_POINTERS:
+  case S_THREAD_LOCAL_REGULAR:
+  case S_THREAD_LOCAL_ZEROFILL:
+  case S_THREAD_LOCAL_VARIABLES:
+  case S_THREAD_LOCAL_INIT_FUNCTION_POINTERS:
+  case S_THREAD_LOCAL_VARIABLE_POINTERS:
+    adjusted = inputType;
+    break;
+  case S_NON_LAZY_SYMBOL_POINTERS:
+    if (config->outputType != MH_KEXT_BUNDLE &&
+        !(config->outputType == MH_EXECUTE && config->isPic))
+      adjusted = S_NON_LAZY_SYMBOL_POINTERS;
+    break;
+  case S_SYMBOL_STUBS:
+    adjusted =
+        S_SYMBOL_STUBS | S_ATTR_SOME_INSTRUCTIONS | S_ATTR_PURE_INSTRUCTIONS;
+    if (!input->relocs.empty())
+      adjusted |= S_ATTR_LOC_RELOC;
+    break;
+  }
+
+  flags |= adjusted;
 }

lld/MachO/InputSection.h

 namespace section_names {
 
 constexpr const char authGot[] = "__auth_got";
 constexpr const char authPtr[] = "__auth_ptr";
 constexpr const char binding[] = "__binding";
 constexpr const char bitcodeBundle[] = "__bundle";
 constexpr const char cString[] = "__cstring";
 constexpr const char cfString[] = "__cfstring";
+constexpr const char classSect[] = "__class";
 constexpr const char codeSignature[] = "__code_signature";
 constexpr const char common[] = "__common";
 constexpr const char compactUnwind[] = "__compact_unwind";
 constexpr const char data[] = "__data";
 constexpr const char debugAbbrev[] = "__debug_abbrev";
 constexpr const char debugInfo[] = "__debug_info";
 constexpr const char debugStr[] = "__debug_str";
 constexpr const char ehFrame[] = "__eh_frame";

 constexpr const char nonLazySymbolPtr[] = "__nl_symbol_ptr";
 constexpr const char objcCatList[] = "__objc_catlist";
 constexpr const char objcClassList[] = "__objc_classlist";
 constexpr const char objcConst[] = "__objc_const";
 constexpr const char objcImageInfo[] = "__objc_imageinfo";
 constexpr const char objcNonLazyCatList[] = "__objc_nlcatlist";
 constexpr const char objcNonLazyClassList[] = "__objc_nlclslist";
 constexpr const char objcProtoList[] = "__objc_protolist";
+constexpr const char objcSelfrefs[] = "__objc_selrefs";
+constexpr const char objSuperrefs[] = "__objc_superrefs";
 constexpr const char pageZero[] = "__pagezero";
 constexpr const char pointers[] = "__pointers";
 constexpr const char rebase[] = "__rebase";
 constexpr const char staticInit[] = "__StaticInit";
 constexpr const char stringTable[] = "__string_table";
 constexpr const char stubHelper[] = "__stub_helper";
 constexpr const char stubs[] = "__stubs";
 constexpr const char swift[] = "__swift";