[lld-macho] Handle weak vs strong case in symbol resolution

[oontvoo]

Summary

Previously, LLD only allows symbols with the same name if all of them are weak.

With this patch, we can allow symbols with the same name if one of them is weak. (This takes care of a TODO)

PR/52372

[oontvoo]

Oh, wait - actually we should just remove the assert
ICF also needs fold CompactUnwind ...

[thakis]

This looks wrong to me.

lld/MachO/Driver.cpp

1475

We definitely want to run dead stripping before ICF (no need to spend time ICF'ing code that's dead -- dead-stripping is faster than ICFing). This doesn't lg.

[oontvoo]

lld/MachO/Driver.cpp

1475

Right - on the other hand, compactUnwindEntries need to be setup before deadstripping (but it shouldn't be done before ICF ...)

Let me re-think this ... there's a cyclic depependency here.

[int3]

I don't think this has anything to do with ICF. The assert was trying to verify that any subsections associated with a coalesced weak symbol has wasCoalesced = true. But now I see that it's not always true, because of this FIXME: https://github.com/llvm/llvm-project/blob/main/lld/MachO/SymbolTable.cpp#L73

We could *probably* remove the assert with no ill effects; I think the only effect is that we will emit some subsections containing dead code that have their associated compact unwind info dropped. But we should ideally handle the FIXME. @oontvoo do you want to take a stab at it, or should I?

[oontvoo]

handle FIXME, as pointed out by int3

The test attached (and all the existing tests) here should cover it.
Please let me know if additional tests are needed

[int3]

We can probably just extend weak-definition-gc.s instead of creating a new test. And we should verify that the associated contents are indeed deleted.

IMO once we cover that case, I don't think it's particularly important to test that the assert in registerCompactUnwind isn't fired. The "coalesced symbol => coalesced section contents" invariant is the more fundamental thing.

cc @thakis since he wrote the original weak symbol GC code

lld/MachO/SymbolTable.cpp

69–74 ↗

(On Diff #384206)

why are we moving these outside the if block? we should be coalescing away the subsection associated with the weak symbol, but not the subsection associated with the non-weak symbol

82 ↗

(On Diff #384206)

given that we only assign to curIsec in one place, we can just inline the block below here

[int3]

Also we should test that coalescing works correctly regardless of which order we encounter the symbols (i.e. weak first then non-weak, and vice versa)

[oontvoo]

updated diff

[oontvoo]

lld/MachO/SymbolTable.cpp

69–74 ↗

(On Diff #384206)

Yes, for this if-branch (ie., both are weak, then we're coalescing the new one)
For the other branch, we're coalescing the weak symbol's (ie., the old one)

[int3]

lld/MachO/SymbolTable.cpp

71–72 ↗

(On Diff #384238)

is this what ld64 does for these two attributes too? (can we test it?)

69–74 ↗

(On Diff #384206)

oh right. okay yeah I was confusing myself

[oontvoo]

lld/MachO/SymbolTable.cpp

71–72 ↗

(On Diff #384238)

No, this is NOT what LD64 does. (In fact this was one of the differences I was trying to note in the draft doc a couple weeks ago).
LD64 picks one of the two (based on things like scope, alignment, whether symbols are auto-hide ) and doesn't merge anything.
I guess we could imitate that here ... didn't want to change that since this current approach seems to be working fine...

[int3]

lld/MachO/SymbolTable.cpp

71–72 ↗

(On Diff #384238)

gotcha. Nonetheless, this is changing our behavior slightly, since we are now setting referencedDynamically and noDeadStrip when coalescing a weak with a non-weak, whereas before we would only do it when both symbols were weak. Can we keep the previous behavior for now?

[oontvoo]

move the attributes merging back to the weak-weak case to presrve old behaviour - also added some note in case we want to revisit in the future

lld/MachO/SymbolTable.cpp

71–72 ↗

(On Diff #384238)

^ is done but. leaving open in case thakis@ wants to chime in

[thevinster]

lld/test/MachO/dup-symbols-weak-def.s

16 ↗

(On Diff #384459)

nit: I imagine this might be a name mangled from your internal systems. I think it would cleaner if we just renamed these to our usual names in all of our tests - foo, bar, baz, etc.

20 ↗

(On Diff #384459)

nit: Here to .cfi_endproc, could we align this with .cfi_startproc? Same for the below function as well.

[oontvoo]

fixed name in tests + fixed inconsistent indentations

[thakis]

lld/MachO/SymbolTable.cpp

69 ↗

(On Diff #384742)

There's a long comment about this in createDefined() in InputFiles.cpp. Maybe refer to that instead.

(Also, typo ,.)

[int3]

Hmm I think you missed my earlier comment about the test:

We can probably just extend weak-definition-gc.s instead of creating a new test. And we should verify that the associated contents are indeed deleted.

IMO once we cover that case, I don't think it's particularly important to test that the assert in registerCompactUnwind isn't fired. The "coalesced symbol => coalesced section contents" invariant is the more fundamental thing.

[oontvoo]

added test

[oontvoo]

rebase

[int3]

Thanks! Last nits: can we remove the dup-symbols-weak-def.s test now since the new logic is already covered by weak-definition-gc.s, and can we update the commit message to reflect the new approach?

lld/MachO/SymbolTable.cpp

83 ↗

(On Diff #385517)

IMO this is redundant since we should never be using coalesced sections in any meaningful capacity. Maybe comment it out and leave a note as to why it's not needed?

lld/test/MachO/weak-definition-gc.s

83–85 ↗

(On Diff #385517)

[oontvoo]

deleted redundant tests + removed extra spaces in tests

[oontvoo]

lld/MachO/SymbolTable.cpp

83 ↗

(On Diff #385517)

This was needed because only **coalesced-weak** sections are omitted from output.

Eg., without this, you could see my new tests emitting the following (commentary is mine, of course):

$ otool -jtV /Users/vyng/repo/llvm-project/build_lld/tools/lld/test/MachO/Output/weak-definition-gc.s.tmp/weak-strong-mixed.dylib
/Users/vyng/repo/llvm-project/build_lld/tools/lld/test/MachO/Output/weak-definition-gc.s.tmp/weak-strong-mixed.dylib:
(__TEXT,__text) section
>>>>>> all this stuff is from the weak symbol and should not have made it here.
0000000000000440	55              	pushq	%rbp
0000000000000441	4889e5          	movq	%rsp, %rbp
0000000000000444	5d              	popq	%rbp
0000000000000445	c3              	retq
<<<<<<<   END
_foo:
0000000000000446	3333            	xorl	(%rbx), %esi
0000000000000448	3333            	xorl	(%rbx), %esi
000000000000044a	c3              	retq

We could change the logic everywhere but I thought the easiest way was to remove the weak symbol here (as if it'd never been added when we already had a strong symbol)

WDYT?

[int3]

lld/MachO/SymbolTable.cpp

83 ↗

(On Diff #385517)

huh interesting. I thought we would have omitted the section content regardless of whether that symbol were removed from the coalesced section... but I'm too lazy to dig into it right now. I also realized that ConcatInputSection::foldIdentical also takes care to remove the symbols, and this is basically a truncated version of that. Yeah ship it, and maybe I'll look into it another day

[oontvoo]

Thanks!

lld/MachO/SymbolTable.cpp

83 ↗

(On Diff #385517)

foldIdenticalSections() isn't run for this test. (It's not run by default ....)
But yeah, i'll have a look in a separate patch.

[oontvoo]

@thakis friendly ping?

[int3]

my 2c is that you can land this without waiting for thakis :)

[oontvoo]

hmm, seems to break windows build - reverted to investigate ...

[oontvoo]

s/$otool/llvm-otool

otool doesn't exist on non-OSX platforms!

[oontvoo]

• add llvm-otool to the set of tools used by test so that the bot will use the <build_dir>/bin/llvm-otool instead of the unqualified llvm-otool (which may not exist)
• update tests since the latest (TOT) llvm-otool prints a space between two bytes and the old one doesn't.

[oontvoo]

the bots look green now - submitting now. Thanks!

[oontvoo]

https://reviews.llvm.org/rG2e1be96df696edc352255779e0498034c35daf57

lld/MachO/Driver.cpp

for (const Arg *arg : args.filtered(OPT_sectcreate)) {
       StringRef fileName = arg->getValue(2);
       Optional<MemoryBufferRef> buffer = readFile(fileName);
       if (buffer)
         inputFiles.insert(make<OpaqueFile>(*buffer, segName, sectName));
     }
 
     gatherInputSections();
 
-    if (config->deadStrip)
-      markLive();
-
     // ICF assumes that all literals have been folded already, so we must run
     // foldIdenticalLiterals before foldIdenticalSections.
     foldIdenticalLiterals();
     if (config->icfLevel != ICFLevel::none)
       foldIdenticalSections();
+    registerCompactUnwindSections();
+
+    if (config->deadStrip)
+      markLive();
 
     // Write to an output file.
     if (target->wordSize == 8)
       writeResult<LP64>();
     else
       writeResult<ILP32>();
 
     depTracker->write(getLLDVersion(), inputFiles, config->outputFile);

lld/MachO/InputFiles.h

 public:
   ObjFile(MemoryBufferRef mb, uint32_t modTime, StringRef archiveName);
   static bool classof(const InputFile *f) { return f->kind() == ObjKind; }
 
   llvm::DWARFUnit *compileUnit = nullptr;
   const uint32_t modTime;
   std::vector<ConcatInputSection *> debugSections;
   ArrayRef<llvm::MachO::data_in_code_entry> dataInCodeEntries;
+  void registerCompactUnwind();
 
 private:
   template <class LP> void parse();
   template <class Section> void parseSections(ArrayRef<Section>);
   template <class LP>
   void parseSymbols(ArrayRef<typename LP::section> sectionHeaders,
                     ArrayRef<typename LP::nlist> nList, const char *strtab,
                     bool subsectionsViaSymbols);
   template <class NList>
   Symbol *parseNonSectionSymbol(const NList &sym, StringRef name);
   template <class Section>
   void parseRelocations(ArrayRef<Section> sectionHeaders, const Section &,
                         SubsectionMap &);
   void parseDebugInfo();
   void parseDataInCode();
-  void registerCompactUnwind();
+
+  // FIXME: not thread-safe?
+  bool doneCUReg = false;
 };
 
 // command-line -sectcreate file
 class OpaqueFile final : public InputFile {
 public:
   OpaqueFile(MemoryBufferRef mb, StringRef segName, StringRef sectName);
   static bool classof(const InputFile *f) { return f->kind() == OpaqueKind; }
 };
explicit BitcodeFile(MemoryBufferRef mb, StringRef archiveName,
                        uint64_t offsetInArchive);
   static bool classof(const InputFile *f) { return f->kind() == BitcodeKind; }
 
   std::unique_ptr<llvm::lto::InputFile> obj;
 };
 
 extern llvm::SetVector<InputFile *> inputFiles;
 
+void registerCompactUnwindSections();
+
 llvm::Optional<MemoryBufferRef> readFile(StringRef path);
 
 namespace detail {
 
 template <class CommandType, class... Types>
 std::vector<const CommandType *>
 findCommands(const void *anyHdr, size_t maxCommands, Types... types) {
   std::vector<const CommandType *> cmds;

lld/MachO/InputFiles.cpp

 #include "lld/Common/ErrorHandler.h"
 #include "lld/Common/Memory.h"
 #include "lld/Common/Reproduce.h"
 #include "llvm/ADT/iterator.h"
 #include "llvm/BinaryFormat/MachO.h"
 #include "llvm/LTO/LTO.h"
 #include "llvm/Support/Endian.h"
 #include "llvm/Support/MemoryBuffer.h"
+#include "llvm/Support/Parallel.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/TarWriter.h"
 #include "llvm/TextAPI/Architecture.h"
 #include "llvm/TextAPI/InterfaceFile.h"
 
 using namespace llvm;
 using namespace llvm::MachO;
 using namespace llvm::support::endian;
if (f->getName().endswith(".tbd"))
       return (f->getName() + "(" + dylibFile->installName + ")").str();
 
   if (f->archiveName.empty())
     return std::string(f->getName());
   return (f->archiveName + "(" + path::filename(f->getName()) + ")").str();
 }
 
 SetVector<InputFile *> macho::inputFiles;
+
 std::unique_ptr<TarWriter> macho::tar;
 int InputFile::idCount = 0;
 
 static VersionTuple decodeVersion(uint32_t version) {
   unsigned major = version >> 16;
   unsigned minor = (version >> 8) & 0xffu;
   unsigned subMinor = version & 0xffu;
   return VersionTuple(major, minor, subMinor);
static bool checkCompatibility(const InputFile *input) {
   if (it->minimum > config->platformInfo.minimum)
     warn(toString(input) + " has version " + it->minimum.getAsString() +
          ", which is newer than target minimum of " +
          config->platformInfo.minimum.getAsString());
 
   return true;
 }
 
+void macho::registerCompactUnwindSections() {
+  parallelForEach(inputFiles, [](InputFile *file) {
+    if (ObjFile *objFile = dyn_cast_or_null<ObjFile>(file)) {
+      objFile->registerCompactUnwind();
+    }
+  });
+}
+
 // Open a given file path and return it as a memory-mapped file.
 Optional<MemoryBufferRef> macho::readFile(StringRef path) {
   ErrorOr<std::unique_ptr<MemoryBuffer>> mbOrErr = MemoryBuffer::getFile(path);
   if (std::error_code ec = mbOrErr.getError()) {
     error("cannot open " + path + ": " + ec.message());
     return None;
   }
 
template <class LP> void ObjFile::parse() {
   // parsed all the symbols.
   for (size_t i = 0, n = subsections.size(); i < n; ++i)
     if (!subsections[i].empty())
       parseRelocations(sectionHeaders, sectionHeaders[i], subsections[i]);
 
   parseDebugInfo();
   if (config->emitDataInCodeInfo)
     parseDataInCode();
-  registerCompactUnwind();
 }
 
 void ObjFile::parseDebugInfo() {
   std::unique_ptr<DwarfObject> dObj = DwarfObject::create(this);
   if (!dObj)
     return;
 
   auto *ctx = make<DWARFContext>(
void ObjFile::parseDataInCode() {
   assert(is_sorted(dataInCodeEntries, [](const data_in_code_entry &lhs,
                                          const data_in_code_entry &rhs) {
     return lhs.offset < rhs.offset;
   }));
 }
 
 // Create pointers from symbols to their associated compact unwind entries.
 void ObjFile::registerCompactUnwind() {
+  assert(!doneCUReg);
   // First, locate the __compact_unwind section.
   SubsectionMap *cuSubsecMap = nullptr;
   for (SubsectionMap &map : subsections) {
     if (map.empty())
       continue;
     if (map[0].isec->getSegName() != segment_names::ld)
       continue;
     cuSubsecMap = &map;
for (const Reloc &r : isec->relocs) {
       // However, unwind info operates on a per-symbol basis, so we search for
       // the function symbol here.
       auto it = llvm::lower_bound(
           referentIsec->symbols, add,
           [](Defined *d, uint64_t add) { return d->value < add; });
       // The relocation should point at the exact address of a symbol (with no
       // addend).
       if (it == referentIsec->symbols.end() || (*it)->value != add) {
-        assert(referentIsec->wasCoalesced);
+        assert(config->icfLevel == ICFLevel::none ||
+               referentIsec->wasCoalesced);
         continue;
       }
       (*it)->compactUnwind = isec;
     }
   }
+  doneCUReg = true;
 }
 
 // The path can point to either a dylib or a .tbd file.
 static DylibFile *loadDylib(StringRef path, DylibFile *umbrella) {
   Optional<MemoryBufferRef> mbref = readFile(path);
   if (!mbref) {
     error("could not read dylib file at " + path);
     return nullptr;

lld/test/MachO/weak-sym.s

+# REQUIRES: x86
+# RUN: rm -rf %t; split-file %s %t
+
+# RUN: llvm-mc -filetype=obj -triple=x86_64-apple-iossimulator %t/lib_def.s -o %t/lib_def.o
+# RUN: llvm-mc -filetype=obj -triple=x86_64-apple-iossimulator %t/lib_weak.s -o %t/lib_weak.o
+# RUN: %lld -dylib -lSystem -dynamic  -platform_version ios-simulator 13.0.0 14.5 %t/lib_def.o %t/lib_weak.o -o %t/out.dylib
+
+#--- lib_def.s
+.section __TEXT,__text,regular,pure_instructions
+
+.globl  _uprv_getICUData_core
+
+_uprv_getICUData_core:
+  .cfi_startproc
+        pushq   %rbp
+        .cfi_def_cfa_offset 16
+        .cfi_offset %rbp, -16
+        movq    %rsp, %rbp
+        .cfi_def_cfa_register %rbp
+        popq    %rbp
+        retq
+        .cfi_endproc
+
+.subsections_via_symbols
+
+#--- lib_weak.s
+.section        __TEXT,__text,regular,pure_instructions
+
+.globl  _uprv_getICUData_core
+.weak_definition _uprv_getICUData_core
+
+_uprv_getICUData_core:
+  .cfi_startproc
+        pushq   %rbp
+        .cfi_def_cfa_offset 16
+        .cfi_offset %rbp, -16
+        movq    %rsp, %rbp
+        .cfi_def_cfa_register %rbp
+        popq    %rbp
+        retq
+        .cfi_endproc
+
+.subsections_via_symbols