[lld-macho] Implement and test resolution of common symbols

[int3]

Summary

Handle the case where there are both common and non-common definitions
of the same symbol. Add a bunch of tests to ensure compatibility with ld64.

[int3]

test common and regular symbols together in an archive

[int3]

fix test

[gkm]

lld/MachO/SymbolTable.cpp

84–85

s/</<=/. I assume that replaceSymbol implies extra work, so why do that for equal-sized data?

92–93

Tangent: I noticed that DSOHandle is not part of SymbolUnion. Should it be?

lld/test/MachO/common-symbol-resolution.s

69–73

I don't see a use for this order file.

[int3]

lld/MachO/SymbolTable.cpp

84–85

it's to change the alignment. (See also discussion in D86909)

92–93

oops yeah. I'll add it as part of D86909 since I need to add CommonSymbol there too

lld/test/MachO/common-symbol-resolution.s

69–73

oops...

[int3]

use order file

[gkm]

lld/MachO/SymbolTable.cpp

84–85

Let me be sure I understand:

• You coded it this way for sake of consistency with ld64.
• A later equal-sized common with smaller alignment will supersede the earlier with greater alignment.
• This seems wrong, but whatever. Call it bug-for-bug compatible.
• It must be a bug of little consequence, otherwise it would be fixed. Maybe we should fix it anyway and see if someone complains!

[int3]

lld/MachO/SymbolTable.cpp

84–85

yup that's right, I was going for bug4bug compatibility. From what I understand, common symbols are being phased out in modern projects, so it's probably not too important anyway

[omjavaid]

Temporarily reverted this change as it predecessor broke LLDB build on Arm. https://reviews.llvm.org/D86909
http://lab.llvm.org:8011/builders/lldb-arm-ubuntu/builds/4409

[int3]

Re-landed in 5213576fa25e6deb7c649032b2893726de2f8b6c.

lld/MachO/SymbolTable.cpp

 
 Symbol *SymbolTable::addCommon(StringRef name, InputFile *file, uint64_t size,
                                uint32_t align) {
   Symbol *s;
   bool wasInserted;
   std::tie(s, wasInserted) = insert(name);
 
   if (!wasInserted) {
     if (auto *common = dyn_cast<CommonSymbol>(s)) {
       if (size >= common->size) {
         common->file = file;
         common->size = size;
         common->align = align;
       }
       return s;
-    } else if (!isa<Undefined>(s)) {
-      error("TODO: implement common symbol resolution with other symbol kinds");
-      return s;
     }
+
+    if (isa<Defined>(s))
+      return s;
+    // Common symbols take priority over all non-Defined symbols, so in case of
+    // a name conflict, we fall through to the replaceSymbol() call below.
   }
 
   replaceSymbol<CommonSymbol>(s, name, file, size, align);
   return s;
 }
 
 Symbol *SymbolTable::addDylib(StringRef name, DylibFile *file, bool isWeakDef,
                               bool isTlv) {

lld/test/MachO/common-symbol-resolution.s

+# REQUIRES: x86
+# RUN: split-file %s %t
+
+# RUN: llvm-mc -filetype=obj -triple=x86_64-apple-darwin %t/common.s -o %t/common.o
+# RUN: llvm-mc -filetype=obj -triple=x86_64-apple-darwin %t/weak-common.s -o %t/weak-common.o
+# RUN: llvm-mc -filetype=obj -triple=x86_64-apple-darwin %t/defined.s -o %t/defined.o
+# RUN: llvm-mc -filetype=obj -triple=x86_64-apple-darwin %t/weak-defined.s -o %t/weak-defined.o
+# RUN: llvm-mc -filetype=obj -triple=x86_64-apple-darwin %t/libfoo.s -o %t/libfoo.o
+# RUN: llvm-mc -filetype=obj -triple=x86_64-apple-darwin %t/test.s -o %t/test.o
+# RUN: llvm-mc -filetype=obj -triple=x86_64-apple-darwin %t/calls-foo.s -o %t/calls-foo.o
+
+# RUN: lld -flavor darwinnew -syslibroot %S/Inputs/MacOSX.sdk -lSystem -dylib %t/libfoo.o -o %t/libfoo.dylib
+# RUN: lld -flavor darwinnew -syslibroot %S/Inputs/MacOSX.sdk -lSystem -dylib %t/libweakfoo.o -o %t/libweakfoo.dylib
+
+# RUN: rm -f %t/defined.a %t/weak-defined-and-common.a
+# RUN: llvm-ar rcs %t/defined.a %t/defined.o
+# RUN: llvm-ar rcs %t/weak-defined-and-common.a %t/weak-defined.o %t/common.o
+
+## The weak attribute appears to have no effect on common symbols. Given two
+## common symbols of the same name, we always pick the one with the larger size,
+## regardless of whether it is weak. Moreover, the resolved symbol in the output
+## file will always be non-weak, even if the winning input symbol definition was
+## weak.
+# RUN: lld -flavor darwinnew -syslibroot %S/Inputs/MacOSX.sdk -lSystem %t/common.o %t/weak-common.o %t/test.o -o %t/test
+# RUN: llvm-objdump --syms %t/test | FileCheck %s --check-prefix=LARGER-COMMON
+# RUN: lld -flavor darwinnew -syslibroot %S/Inputs/MacOSX.sdk -lSystem %t/weak-common.o %t/common.o %t/test.o -o %t/test
+# RUN: llvm-objdump --syms %t/test | FileCheck %s --check-prefix=LARGER-COMMON
+
+## Defined symbols are the only ones that take precedence over common symbols.
+# RUN: lld -flavor darwinnew -syslibroot %S/Inputs/MacOSX.sdk -lSystem %t/defined.o %t/common.o %t/test.o -o %t/test
+# RUN: llvm-objdump --syms %t/test | FileCheck %s --check-prefix=DEFINED
+# RUN: lld -flavor darwinnew -syslibroot %S/Inputs/MacOSX.sdk -lSystem %t/common.o %t/defined.o %t/test.o -o %t/test
+# RUN: llvm-objdump --syms %t/test | FileCheck %s --check-prefix=DEFINED
+
+# RUN: lld -flavor darwinnew -syslibroot %S/Inputs/MacOSX.sdk -lSystem %t/weak-defined.o %t/common.o %t/test.o -o %t/test
+# RUN: llvm-objdump --syms %t/test | FileCheck %s --check-prefix=WEAK-DEFINED
+# RUN: lld -flavor darwinnew -syslibroot %S/Inputs/MacOSX.sdk -lSystem %t/common.o %t/weak-defined.o %t/test.o -o %t/test
+# RUN: llvm-objdump --syms %t/test | FileCheck %s --check-prefix=WEAK-DEFINED
+
+## Common symbols take precedence over archive symbols.
+# RUN: lld -flavor darwinnew -syslibroot %S/Inputs/MacOSX.sdk -lSystem %t/defined.a %t/weak-common.o %t/test.o -o %t/test
+# RUN: llvm-objdump --syms %t/test | FileCheck %s --check-prefix=LARGER-COMMON
+# RUN: lld -flavor darwinnew -syslibroot %S/Inputs/MacOSX.sdk -lSystem %t/weak-common.o %t/defined.a %t/test.o -o %t/test
+# RUN: llvm-objdump --syms %t/test | FileCheck %s --check-prefix=LARGER-COMMON
+
+## If an archive has both a common and a defined symbol, the defined one should
+## win.
+# RUN: lld -flavor darwinnew -syslibroot %S/Inputs/MacOSX.sdk -lSystem %t/weak-defined-and-common.a %t/calls-foo.o -o %t/calls-foo
+# RUN: llvm-objdump --syms %t/calls-foo | FileCheck %s --check-prefix=WEAK-DEFINED
+# RUN: lld -flavor darwinnew -syslibroot %S/Inputs/MacOSX.sdk -lSystem %t/calls-foo.o %t/weak-defined-and-common.a -o %t/calls-foo
+# RUN: llvm-objdump --syms %t/calls-foo | FileCheck %s --check-prefix=WEAK-DEFINED
+
+## Common symbols take precedence over dylib symbols.
+# RUN: lld -flavor darwinnew -syslibroot %S/Inputs/MacOSX.sdk -lSystem %t/libfoo.dylib %t/weak-common.o %t/test.o -o %t/test
+# RUN: llvm-objdump --syms %t/test | FileCheck %s --check-prefix=LARGER-COMMON
+# RUN: lld -flavor darwinnew -syslibroot %S/Inputs/MacOSX.sdk -lSystem %t/weak-common.o %t/libfoo.dylib %t/test.o -o %t/test
+# RUN: llvm-objdump --syms %t/test | FileCheck %s --check-prefix=LARGER-COMMON
+
+# LARGER-COMMON-LABEL: SYMBOL TABLE:
+# LARGER-COMMON-DAG:   [[#%x, FOO_ADDR:]] g     O __DATA,__common _foo
+# LARGER-COMMON-DAG:   [[#FOO_ADDR + 2]]  g     O __DATA,__common _foo_end
+
+# DEFINED-LABEL:       SYMBOL TABLE:
+# DEFINED:             g     F __TEXT,__text _foo
+
+# WEAK-DEFINED-LABEL:  SYMBOL TABLE:
+# WEAK-DEFINED:        w     F __TEXT,__text _foo
+
+#--- order
+## Order is important as we determine the size of a given symbol via the
+## address of the next symbol.
+_foo
+_foo_end
+
+#--- common.s
+.comm _foo, 1
+
+.globl _bar
+_bar:
+
+#--- weak-common.s
+.weak_definition _foo
+.comm _foo, 2
+
+#--- defined.s
+.globl _foo
+_foo:
+  .quad 0x1234
+
+#--- weak-defined.s
+.globl _foo
+.weak_definition _foo
+_foo:
+  .quad 0x1234
+
+#--- libfoo.s
+.globl _foo
+_foo:
+  .quad 0x1234
+
+#--- test.s
+.comm _foo_end, 1
+
+.globl _main
+_main:
+  ret
+
+#--- calls-foo.s
+.comm _foo_end, 1
+
+.globl _main
+_main:
+  callq _foo
+  ret

lld/test/MachO/nonweak-definition-override.s

 # REQUIRES: x86
 # RUN: split-file %s %t
 # RUN: llvm-mc -filetype=obj -triple=x86_64-apple-darwin %t/libfoo.s -o %t/libfoo.o
 # RUN: llvm-mc -filetype=obj -triple=x86_64-apple-darwin %t/nonweakdef.s -o %t/nonweakdef.o
 # RUN: llvm-mc -filetype=obj -triple=x86_64-apple-darwin %t/weakdef.s -o %t/weakdef.o
+# RUN: llvm-mc -filetype=obj -triple=x86_64-apple-darwin %t/common.s -o %t/common.o
 # RUN: lld -flavor darwinnew -syslibroot %S/Inputs/MacOSX.sdk -dylib %t/libfoo.o -o %t/libfoo.dylib
 
 ## Check that non-weak defined symbols override weak dylib symbols.
 # RUN: lld -flavor darwinnew -syslibroot %S/Inputs/MacOSX.sdk %t/nonweakdef.o -L%t -lfoo -o %t/nonweakdef -lSystem
 # RUN: llvm-objdump --macho --weak-bind %t/nonweakdef | FileCheck %s
 
 ## Test loading the dylib before the obj file.
 # RUN: lld -flavor darwinnew -syslibroot %S/Inputs/MacOSX.sdk -L%t -lfoo %t/nonweakdef.o -o %t/nonweakdef -lSystem

 ## Test loading the dylib before the obj file.
 # RUN: lld -flavor darwinnew -syslibroot %S/Inputs/MacOSX.sdk -L%t -lfoo %t/weakdef.o -o %t/weakdef -lSystem
 # RUN: llvm-objdump --macho --weak-bind %t/weakdef | FileCheck %s --check-prefix=NO-WEAK-OVERRIDE
 
 # NO-WEAK-OVERRIDE:       Weak bind table:
 # NO-WEAK-OVERRIDE-NEXT:  segment section address type addend symbol
 # NO-WEAK-OVERRIDE-EMPTY:
 
+## Check that common symbols take precedence over weak dylib symbols, but do not
+## generate an overridding weak binding.
+# RUN: lld -flavor darwinnew -syslibroot %S/Inputs/MacOSX.sdk -L%t -lfoo %t/common.o -o %t/common -lSystem
+# RUN: llvm-objdump --macho --weak-bind %t/common | FileCheck %s --check-prefix=NO-WEAK-OVERRIDE
+# RUN: llvm-objdump --syms %t/common | FileCheck %s --check-prefix=COMMON
+# COMMON-DAG: g     O __DATA,__common _nonweak_in_dylib
+# COMMON-DAG: g     O __DATA,__common _weak_in_dylib
+
 #--- libfoo.s
 
 .globl _weak_in_dylib, _nonweak_in_dylib
 .weak_definition _weak_in_dylib
 
 _weak_in_dylib:
 _nonweak_in_dylib:
 

 .globl _main, _weak_in_dylib, _nonweak_in_dylib
 .weak_definition _weak_in_dylib, _nonweak_in_dylib
 
 _weak_in_dylib:
 _nonweak_in_dylib:
 
 _main:
   ret
+
+#--- common.s
+
+.globl _main
+.comm _weak_in_dylib, 1
+.comm _nonweak_in_dylib, 1
+
+_main:
+  ret