[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

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

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

see ld64's LLVMBitcode class :)

[oontvoo]

lld/test/MachO/ignore-symbols.s

11–17

Ah, I see. Thanks!

[thevinster]

Address feedback

[int3]

lgtm

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

6 ↗

(On Diff #366663)

nit

18 ↗

(On Diff #366663)

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–285

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–285

Created follow up - D108167

[MaskRay]

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

26 ↗

(On Diff #366716)

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) {
+        // Sections within the llvm segment aren't known to induce symbols errors.
+        // Push an empty map to ensure indices line up for the remaining sections.
+        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/ignore-symbols.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.
+# RUN: %lld -dylib %t/foo.o %t/bar.o -o %t/libDuplicate.dylib
+
+#--- foo.s
+.globl _llvm.foo
+.section __LLVM,__bitcode
+_llvm.foo:
+.asciz "test"
+
+#--- bar.s
+.globl _llvm.foo
+.section __LLVM,__bitcode
+_llvm.foo:
+.asciz "test"