[lld-macho] Ignore LLVM segments to prevent duplicate syms

[thevinster]

Summary

There was an instance of a third-party archive containing multiple
_llvm symbols from different files that clashed with each other
producing duplicate symbols. Symbols under the LLVM segment
don't seem to be producing any meaningful value, so just ignore them.

[thevinster]

Cleanup wording

[oontvoo]

lld/test/MachO/ignore-symbols.s

11–17 ↗

(On Diff #366197)

How does ld64 deal with this, though? (quick glance at the code, I didn't see any special handling for _llvm segments)

Just concerned that this change is hiding a bug somewhere else

[int3]

lld/MachO/InputFiles.cpp

294

I would say that ld64 does not appear to emit contents from sections within the __LLVM segment, and that the symbols in those sections point to bitcode metadata rather than being actual symbols. In particular, global symbols within those sections can have the same name without causing duplicate symbol errors.

maybe also add a TODO to figure out if we are supposed to do anything useful with said metadata...

lld/test/MachO/ignore-symbols.s

6–8 ↗

(On Diff #366197)

we need to test a few more things:

1. that we discard __LLVM sections entirely, just like ld64 (you can use llvm-objdump --section-headers)
2. when an _llvm.foo symbol is not under an __LLVM section, a duplicate symbol error is indeed raised

(test should be retitled too, something like discard-llvm-sections.s)

11–17 ↗

(On Diff #366197)

see ld64's LLVMBitcode class :)

[oontvoo]

lld/test/MachO/ignore-symbols.s

11–17 ↗

(On Diff #366197)

Ah, I see. Thanks!

[thevinster]

Address feedback

[int3]

lgtm

lld/test/MachO/discard-llvm-sections.s

7

nit

19

looks like the windows test is failing as the address is slightly different there. We should probably figure out why at some point, but since it's not really important to this test we can just match any hex value

[thevinster]

Fix minor nits on tests

[int3]

lld/MachO/InputFiles.cpp

283–284

I realized that we are now constructing isec here redundantly whenever we have an __LLVM section. Can you fix things so that we only construct the isec when it actually gets used?

[thevinster]

lld/MachO/InputFiles.cpp

283–284

Created follow up - D108167

[MaskRay]

lld/test/MachO/discard-llvm-sections.s

26

The convention is not to add a prefix before error: so I dropped it in f74b70ef57fd038e9e6a781ef5cc72bb9734abe4

lld/MachO/InputFiles.cpp

if (sectionType(sec.flags) == S_CSTRING_LITERALS ||
       subsections.push_back({});
       SubsectionMap &subsecMap = subsections.back();
       for (uint64_t off = 0; off < data.size(); off += literalSize)
         subsecMap.push_back(
             {off, make<ConcatInputSection>(segname, name, this,
                                            data.slice(off, literalSize), align,
                                            flags)});
     } else {
       auto *isec =
           make<ConcatInputSection>(segname, name, this, data, align, flags);
-      if (!(isDebugSection(isec->getFlags()) &&
-            isec->getSegName() == segment_names::dwarf)) {
-        subsections.push_back({{0, isec}});
-      } else {
+      if (isDebugSection(isec->getFlags()) &&
+          isec->getSegName() == segment_names::dwarf) {
         // Instead of emitting DWARF sections, we emit STABS symbols to the
         // object files that contain them. We filter them out early to avoid
         // parsing their relocations unnecessarily. But we must still push an
         // empty map to ensure the indices line up for the remaining sections.
         subsections.push_back({});
         debugSections.push_back(isec);
+      } else if (isec->getSegName() == segment_names::llvm) {
+        // ld64 does not appear to emit contents from sections within the __LLVM
+        // segment. Symbols within those sections point to bitcode metadata
+        // instead of actual symbols. Global symbols within those sections could
+        // have the same name without causing duplicate symbol errors. Push an
+        // empty map to ensure indices line up for the remaining sections.
+        // TODO: Evaluate whether the bitcode metadata is needed.
+        subsections.push_back({});
+      } else {
+        subsections.push_back({{0, isec}});
       }
     }
   }
 }
 
 // Find the subsection corresponding to the greatest section offset that is <=
 // that of the given offset.
 //

lld/test/MachO/discard-llvm-sections.s

+# REQUIRES: x86
+# RUN: rm -rf %t; split-file %s %t
+# RUN: llvm-mc -filetype=obj -triple=x86_64-apple-darwin %t/foo.s -o %t/foo.o
+# RUN: llvm-mc -filetype=obj -triple=x86_64-apple-darwin %t/bar.s -o %t/bar.o
+
+## "_llvm." symbols are not special. LLD would produce duplicate symbol errors
+## if they were not within the LLVM segment.
+
+## 1/ Test that LLD does not produce duplicate symbols errors when linking global symbols
+##    with the same name under the LLVM segment.
+# RUN: %lld -dylib %t/foo.o %t/bar.o -o %t/libDuplicate.dylib
+
+## 2/ Test that all sections within an LLVM segment are dropped.
+# RUN: llvm-objdump --section-headers %t/libDuplicate.dylib | FileCheck %s
+
+# CHECK-LABEL: Sections:
+# CHECK-NEXT:  Idx  Name    Size      VMA            Type
+# CHECK-NEXT:  0    __text  00000000  {{[0-9a-f]+}}  TEXT
+
+## 3/ Test that linking global symbol that is not under the LLVM segment produces duplicate
+##    symbols
+# RUN: llvm-mc -filetype=obj -triple=x86_64-apple-darwin --defsym TEXT=0 %t/foo.s -o %t/foo.o
+# RUN: llvm-mc -filetype=obj -triple=x86_64-apple-darwin --defsym TEXT=0 %t/bar.s -o %t/bar.o
+# RUN: not %lld -dylib %t/foo.o %t/bar.o -o %t/libDuplicate.dylib 2>&1 | FileCheck %s --check-prefix=DUP
+
+# DUP: ld64.lld: error: duplicate symbol: _llvm.foo
+
+#--- foo.s
+.globl _llvm.foo
+.ifdef TEXT
+  .section __TEXT,__cstring
+.else
+  .section __LLVM,__bitcode
+.endif
+  _llvm.foo:
+    .asciz "test"
+
+#--- bar.s
+.globl _llvm.foo
+.ifdef TEXT
+  .section __TEXT,__cstring
+.else
+  .section __LLVM,__bitcode
+.endif
+  _llvm.foo:
+    .asciz "test"