[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

336–353

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

369

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

370

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

373–375

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)

376–377

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

369

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

370

fixed logic

[oontvoo]

updated diff

[oontvoo]

updated tests

[int3]

lld/MachO/ConcatOutputSection.cpp

369

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

369

done (for real this time)

[oontvoo]

updated diff

[int3]

Thanks!

lld/MachO/ConcatOutputSection.cpp

353

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]->outSecOff);
       ++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 = sectionType(flags);
-  uint8_t inputType = sectionType(input->flags);
-  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
+void ConcatOutputSection::finalizeFlags(InputSection *input) {
+  uint8_t inputType = input->flags & SECTION_TYPE;
+  switch (inputType) {
+  default /*type-unspec'ed*/:
+    // FIXME: Add additional logics here when supporting emitting obj files.
+    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:
+  case S_NON_LAZY_SYMBOL_POINTERS:
+  case S_SYMBOL_STUBS:
     flags |= input->flags;
-  flags &= pureMask;
+    break;
+  }
 }
 
 void ConcatOutputSection::eraseOmittedInputSections() {
   // Remove the duplicates from inputs
   inputs.erase(std::remove_if(inputs.begin(), inputs.end(),
                               [](const ConcatInputSection *isec) -> bool {
                                 return isec->shouldOmitFromOutput();
                               }),
                inputs.end());
 }

lld/test/MachO/builtin-rename.s

 # REQUIRES: x86
 # RUN: rm -rf %t; split-file %s %t
 
 # RUN: llvm-mc -filetype=obj -triple=x86_64-apple-darwin \
 # RUN:     %t/main.s -o %t/main.o
 # RUN: llvm-mc -filetype=obj -triple=x86_64-apple-darwin \
 # RUN:     %t/renames.s -o %t/renames.o
-# RUN: llvm-mc -filetype=obj -triple=x86_64-apple-darwin \
-# RUN:     %t/error.s -o %t/error.o
-
-# RUN: not %lld            -o %t/error %t/main.o %t/error.o -lSystem 2>&1 | \
-# RUN:     FileCheck %s --check-prefix=ERROR
-
-## Check the error diagnostic for merging mismatched section types
-# ERROR: Cannot merge section __pointers (type=0x0) into __nl_symbol_ptr (type=0x6): inconsistent types
 
 ## Check that section and segment renames happen as expected
 # RUN: %lld                -o %t/ydata %t/main.o %t/renames.o -lSystem
 # RUN: %lld -no_data_const -o %t/ndata %t/main.o %t/renames.o -lSystem
 # RUN: %lld -no_pie        -o %t/nopie %t/main.o %t/renames.o -lSystem
 
 # RUN: llvm-objdump --syms %t/ydata | \
 # RUN:     FileCheck %s --check-prefixes=CHECK,YDATA
__IMPORT__pointers:
   .space 8
 
 .section __TEXT,__StaticInit
 .section __TEXT,__StaticInit
 .global __TEXT__StaticInit
 __TEXT__StaticInit:
   .space 8
 
-#--- error.s
-
-.section __DATA,__nl_symbol_ptr
-.global __DATA__nl_symbol_ptr
-__DATA__nl_symbol_ptr:
-  .space 8
-
-.section __IMPORT,__pointers
-.global __IMPORT__pointers
-__IMPORT__pointers:
-  .space 8
-
 #--- main.s
 .text
 .global _main
 _main:
   ret