[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 ↗

(On Diff #383943)

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 ↗

(On Diff #383943)

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

81–86

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

96

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

81–86

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

81–82

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

81–86

oh right. okay yeah I was confusing myself

[oontvoo]

lld/MachO/SymbolTable.cpp

81–82

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

81–82

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

81–82

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

[thevinster]

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

16

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

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

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

91

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

91

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

91

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

91

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/SymbolTable.cpp

Defined *SymbolTable::addDefined(StringRef name, InputFile *file,
   std::tie(s, wasInserted) = insert(name, file);
 
   assert(!isWeakDef || (isa<BitcodeFile>(file) && !isec) ||
          (isa<ObjFile>(file) && file == isec->getFile()));
 
   if (!wasInserted) {
     if (auto *defined = dyn_cast<Defined>(s)) {
       if (isWeakDef) {
+
+        // Note: Unlike what's implemented here, LD64 chooses one of the
+        // two weak-symbols based on different criteria (alignment, scopes,
+        // ...). We could imitate it in the future but for now, this seems to
+        // wor,.
         if (defined->isWeakDef()) {
-          // Both old and new symbol weak (e.g. inline function in two TUs):
-          // If one of them isn't private extern, the merged symbol isn't.
+          // privateExtern is special.
+          // When both symbols are weak and  one of them isn't private extern,
+          // the merged symbol isn't.
           defined->privateExtern &= isPrivateExtern;
+
+          // Merge the rest of the attributes.
           defined->referencedDynamically |= isReferencedDynamically;
           defined->noDeadStrip |= noDeadStrip;
-
-          // FIXME: Handle this for bitcode files.
-          // FIXME: We currently only do this if both symbols are weak.
-          //        We could do this if either is weak (but getting the
-          //        case where !isWeakDef && defined->isWeakDef() right
-          //        requires some care and testing).
-          if (auto concatIsec = dyn_cast_or_null<ConcatInputSection>(isec))
-            concatIsec->wasCoalesced = true;
-        }
-
+        }
         return defined;
       }
-      if (!defined->isWeakDef())
+
+      if (defined->isWeakDef()) {
+        // FIXME: Handle this for bitcode files.
+        if (auto concatIsec =
+                dyn_cast_or_null<ConcatInputSection>(defined->isec))
+          concatIsec->wasCoalesced = true;
+      } else {
         error("duplicate symbol: " + name + "\n>>> defined in " +
               toString(defined->getFile()) + "\n>>> defined in " +
               toString(file));
+      }
+
     } else if (auto *dysym = dyn_cast<DylibSymbol>(s)) {
       overridesWeakDef = !isWeakDef && dysym->isWeakDef();
       dysym->unreference();
     }
     // Defined symbols take priority over other types of symbols, so in case
     // of a name conflict, we fall through to the replaceSymbol() call below.
   }
 

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

+# REQUIRES: x86
+# RUN: rm -rf %t; split-file %s %t
+
+# RUN: llvm-mc -filetype=obj -triple=x86_64-apple-macos  %t/lib_def.s -o %t/lib_def.o
+# RUN: llvm-mc -filetype=obj -triple=x86_64-apple-macos %t/lib_weak.s -o %t/lib_weak.o
+# RUN: %lld -dylib -lSystem -dynamic  %t/lib_def.o %t/lib_weak.o -o %t/out.dylib
+# RUN: llvm-objdump --macho --syms %t/out.dylib | FileCheck %s
+
+# CHECK: SYMBOL TABLE:
+# CHECK-NEXT: 0{{.*}} g     F __TEXT,__text _uprv_getICUData_core
+
+        
+#--- 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