[lld-macho] Deduplicate CFStrings during ICF

`__cfstring` has embedded addends that foil ICF's hashing / equality
checks. (We can ignore embedded addends when doing ICF because the same
information gets recorded in our Reloc structs.) Therefore, in order to
properly dedup CFStrings, we create a mutable copy of the CFString and
zero out the embedded addends before performing any hashing / equality
checks.

(We did in fact have a partial implementation of CFString deduplication
already. However, it only worked when the cstrings they point to are at
identical offsets in their object files.)

I anticipate this approach can be extended to other similar
statically-allocated struct sections in the future.

In addition, we previously treated all references with differing addends
as unequal. This is not true when the references are to literals:
different addends may point to the same literal in the output binary. In
particular, `__cfstring` has such references to `__cstring`. I've
adjusted ICF's `equalsConstant` logic accordingly, and I've added a few
more tests to make sure the addend-comparison code path is adequately
covered.

Fixes #51281.

Reviewed By: #lld-macho, Roger

Differential Revision: https://reviews.llvm.org/D120137

lld/MachO/ICF.cpp

@@ -111,8 +111,6 @@ bool ICF::equalsConstant(const ConcatInputSection *ia,
       return false;
     if (ra.offset != rb.offset)
       return false;
-    if (ra.addend != rb.addend)
-      return false;
     if (ra.referent.is<Symbol *>() != rb.referent.is<Symbol *>())
       return false;
 
@@ -125,16 +123,16 @@ bool ICF::equalsConstant(const ConcatInputSection *ia,
       const auto *sb = rb.referent.get<Symbol *>();
       if (sa->kind() != sb->kind())
         return false;
-      if (!isa<Defined>(sa)) {
-        // ICF runs before Undefineds are reported.
-        assert(isa<DylibSymbol>(sa) || isa<Undefined>(sa));
-        return sa == sb;
-      }
+      // ICF runs before Undefineds are treated (and potentially converted into
+      // DylibSymbols).
+      if (isa<DylibSymbol>(sa) || isa<Undefined>(sa))
+        return sa == sb && ra.addend == rb.addend;
+      assert(isa<Defined>(sa));
       const auto *da = cast<Defined>(sa);
       const auto *db = cast<Defined>(sb);
       if (!da->isec || !db->isec) {
         assert(da->isAbsolute() && db->isAbsolute());
-        return da->value == db->value;
+        return da->value + ra.addend == db->value + rb.addend;
       }
       isecA = da->isec;
       valueA = da->value;
@@ -151,7 +149,7 @@ bool ICF::equalsConstant(const ConcatInputSection *ia,
     assert(isecA->kind() == isecB->kind());
     // We will compare ConcatInputSection contents in equalsVariable.
     if (isa<ConcatInputSection>(isecA))
-      return true;
+      return ra.addend == rb.addend;
     // Else we have two literal sections. References to them are equal iff their
     // offsets in the output section are equal.
     return isecA->getOffset(valueA + ra.addend) ==
@@ -389,6 +387,17 @@ void macho::foldIdenticalSections() {
     }
   }
   parallelForEach(hashable, [](ConcatInputSection *isec) {
+    // __cfstring has embedded addends that foil ICF's hashing / equality
+    // checks. (We can ignore embedded addends when doing ICF because the same
+    // information gets recorded in our Reloc structs.) We therefore create a
+    // mutable copy of the CFString and zero out the embedded addends before
+    // performing any hashing / equality checks.
+    if (isCfStringSection(isec)) {
+      MutableArrayRef<uint8_t> copy = isec->data.copy(bAlloc());
+      for (const Reloc &r : isec->relocs)
+        target->relocateOne(copy.data() + r.offset, r, /*va=*/0, /*relocVA=*/0);
+      isec->data = copy;
+    }
     assert(isec->icfEqClass[0] == 0); // don't overwrite a unique ID!
     // Turn-on the top bit to guarantee that valid hashes have no collisions
     // with the small-integer unique IDs for ICF-ineligible sections

lld/test/MachO/cfstring-dedup.s

@@ -37,6 +37,10 @@
 
 #--- foo1.s
 .cstring
+L_.str.0:
+  .asciz  "bar"
+## This string is at a different offset than the corresponding "foo" string in
+## foo2.s. Make sure that we treat references to either string as equivalent.
 L_.str:
   .asciz  "foo"
 
@@ -57,7 +61,7 @@ _named_cfstring:
   .quad  3 ## strlen
 
 .section  __TEXT,__ustring
-l_.str.2:
+l_.ustr:
   .short  102 ## f
   .short  111 ## o
   .short  0   ## \0
@@ -77,7 +81,7 @@ L__unnamed_cfstring_.2:
   .quad  ___CFConstantStringClassReference
   .long  2000 ## utf-16
   .space  4
-  .quad  l_.str.2
+  .quad  l_.ustr
   .quad  4 ## strlen
 
 .text
@@ -116,7 +120,7 @@ _named_cfstring:
 
 .section  __TEXT,__ustring
   .p2align  1
-l_.str.2:
+l_.ustr:
   .short  102 ## f
   .short  111 ## o
   .short  0   ## \0
@@ -129,7 +133,7 @@ L__unnamed_cfstring_.2:
   .quad  ___CFConstantStringClassReference
   .long  2000 ## utf-16
   .space  4
-  .quad  l_.str.2
+  .quad  l_.ustr
   .quad  4 ## strlen
 
 .text

lld/test/MachO/icf-undef.s

@@ -0,0 +1,28 @@
+# REQUIRES: x86
+# RUN: rm -rf %t; split-file %s %t
+
+# RUN: llvm-mc -filetype=obj -triple=x86_64-apple-darwin19.0.0 %t/test.s -o %t/test.o
+
+## Check that we correctly dedup sections that reference dynamic-lookup symbols.
+# RUN: %lld -lSystem -dylib --icf=all -undefined dynamic_lookup -o %t/test %t/test.o
+# RUN: llvm-objdump --macho --syms %t/test | FileCheck %s
+
+## Check that we still raise an error when using regular undefined symbol
+## treatment.
+# RUN: not %lld -lSystem -dylib --icf=all -o /dev/null %t/test.o 2>&1 | \
+# RUN:   FileCheck %s --check-prefix=ERR
+
+# CHECK: [[#%x,ADDR:]] l    F __TEXT,__text _foo
+# CHECK: [[#ADDR]]     l    F __TEXT,__text _bar
+
+# ERR: error: undefined symbol: _undef
+
+#--- test.s
+
+.subsections_via_symbols
+
+_foo:
+  callq _undef + 1
+
+_bar:
+  callq _undef + 1

lld/test/MachO/icf.s

@@ -22,11 +22,13 @@
 # CHECK: [[#%x,DYLIB_REF_2:]]               l     F __TEXT,__text _dylib_ref_1
 # CHECK: [[#%x,DYLIB_REF_2:]]               l     F __TEXT,__text _dylib_ref_2
 # CHECK: [[#%x,DYLIB_REF_3:]]               l     F __TEXT,__text _dylib_ref_3
+# CHECK: [[#%x,DYLIB_REF_4:]]               l     F __TEXT,__text _dylib_ref_4
 # CHECK: [[#%x,ALT:]]                       l     F __TEXT,__text _alt
 # CHECK: [[#%x,WITH_ALT_ENTRY:]]            l     F __TEXT,__text _with_alt_entry
 # CHECK: [[#%x,WITH_ALT_ENTRY:]]            l     F __TEXT,__text _no_alt_entry
 # CHECK: [[#%x,DEFINED_REF_WITH_ADDEND_2:]] l     F __TEXT,__text _defined_ref_with_addend_1
 # CHECK: [[#%x,DEFINED_REF_WITH_ADDEND_2:]] l     F __TEXT,__text _defined_ref_with_addend_2
+# CHECK: [[#%x,DEFINED_REF_WITH_ADDEND_3:]] l     F __TEXT,__text _defined_ref_with_addend_3
 # CHECK: [[#%x,RECURSIVE:]]                 l     F __TEXT,__text _recursive
 # CHECK: [[#%x,CALL_RECURSIVE_2:]]          l     F __TEXT,__text _call_recursive_1
 # CHECK: [[#%x,CALL_RECURSIVE_2:]]          l     F __TEXT,__text _call_recursive_2
@@ -55,11 +57,13 @@
 # CHECK: callq 0x[[#%x,DYLIB_REF_2:]]               <_dylib_ref_2>
 # CHECK: callq 0x[[#%x,DYLIB_REF_2:]]               <_dylib_ref_2>
 # CHECK: callq 0x[[#%x,DYLIB_REF_3:]]               <_dylib_ref_3>
+# CHECK: callq 0x[[#%x,DYLIB_REF_4:]]               <_dylib_ref_4>
 # CHECK: callq 0x[[#%x,ALT:]]                       <_alt>
 # CHECK: callq 0x[[#%x,WITH_ALT_ENTRY:]]            <_with_alt_entry>
 # CHECK: callq 0x[[#%x,WITH_ALT_ENTRY:]]            <_with_alt_entry>
 # CHECK: callq 0x[[#%x,DEFINED_REF_WITH_ADDEND_2:]] <_defined_ref_with_addend_2>
 # CHECK: callq 0x[[#%x,DEFINED_REF_WITH_ADDEND_2:]] <_defined_ref_with_addend_2>
+# CHECK: callq 0x[[#%x,DEFINED_REF_WITH_ADDEND_3:]] <_defined_ref_with_addend_3>
 # CHECK: callq 0x[[#%x,RECURSIVE:]]                 <_recursive>
 # CHECK: callq 0x[[#%x,CALL_RECURSIVE_2:]]          <_call_recursive_2>
 # CHECK: callq 0x[[#%x,CALL_RECURSIVE_2:]]          <_call_recursive_2>
@@ -132,6 +136,11 @@ _dylib_ref_3:
   mov ___inf@GOTPCREL(%rip), %rax
   callq ___inf
 
+## No fold: referent dylib addend differs
+_dylib_ref_4:
+  mov ___nan + 1@GOTPCREL(%rip), %rax
+  callq ___inf + 1
+
 ## We can merge two sections even if one of them has an alt entry. Just make
 ## sure we don't merge the alt entry symbol with a regular symbol.
 .alt_entry _alt
@@ -150,6 +159,10 @@ _defined_ref_with_addend_1:
 _defined_ref_with_addend_2:
   callq _with_alt_entry + 4
 
+# No fold: addend differs
+_defined_ref_with_addend_3:
+  callq _with_alt_entry + 8
+
 ## _recursive has the same body as its next two callers, but cannot be folded
 ## with them.
 _recursive:
@@ -251,11 +264,13 @@ _main:
   callq _dylib_ref_1
   callq _dylib_ref_2
   callq _dylib_ref_3
+  callq _dylib_ref_4
   callq _alt
   callq _with_alt_entry
   callq _no_alt_entry
   callq _defined_ref_with_addend_1
   callq _defined_ref_with_addend_2
+  callq _defined_ref_with_addend_3
   callq _recursive
   callq _call_recursive_1
   callq _call_recursive_2