[lld/mac] Fix mislink with ICF

[thakis]

Summary

When comparing relocations against two symbols, ICF's equalsConstant() did not
look at the value of the two symbols. With subsections_via_symbols, the value
is usually 0 but not always: In particular, it isn't 0 for constants in string
and literal sections. Since we ignored the value, comparing two constant string
symbols or two literal symbols always compared the 0th's element, so functions
in the same TU always compared as equal.

This can cause mislinks, and with -dead_strip crashes.

Fixes PR52349, see that bug for lots of details and examples of mislinks.

While here, make the existing assembly in icf-literals.s a bit more realistic
(use leaq instead of movq with strings, and use foo(%rip) instead of
foo@gotpcrel(%rip)). This has no interesting effect, it just maybe makes the
test look a bit less surprising.

[thakis]

tweak

[int3]

Oof, yeah, this was quite an oversight. Thanks for the fix!

[thakis]

Thanks!

[int3]

Heh, I belatedly realized that I'd put up a similar fix in D106214: [lld-macho] ICF: Fold some sections with differing addends, but never got it reviewed / landed...

lld/MachO/ICF.cpp

auto f = [](const Reloc &ra, const Reloc &rb) {
     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;
 
     InputSection *isecA, *isecB;
+
+    uint64_t valueA = 0;
+    uint64_t valueB = 0;
     if (ra.referent.is<Symbol *>()) {
       const auto *sa = ra.referent.get<Symbol *>();
       const auto *sb = rb.referent.get<Symbol *>();
       if (sa->kind() != sb->kind())
         return false;
       if (!isa<Defined>(sa)) {
         assert(isa<DylibSymbol>(sa));
         return sa == sb;
       }
       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;
       }
       isecA = da->isec;
+      valueA = da->value;
       isecB = db->isec;
+      valueB = db->value;
     } else {
       isecA = ra.referent.get<InputSection *>();
       isecB = rb.referent.get<InputSection *>();
     }
 
     if (isecA->parent != isecB->parent)
       return false;
     // Sections with identical parents should be of the same kind.
     assert(isecA->kind() == isecB->kind());
     // We will compare ConcatInputSection contents in equalsVariable.
     if (isa<ConcatInputSection>(isecA))
       return true;
     // Else we have two literal sections. References to them are equal iff their
     // offsets in the output section are equal.
-    return isecA->getOffset(ra.addend) == isecB->getOffset(rb.addend);
+    return isecA->getOffset(valueA + ra.addend) ==
+           isecB->getOffset(valueB + rb.addend);
   };
   return std::equal(ia->relocs.begin(), ia->relocs.end(), ib->relocs.begin(),
                     f);
 }
 
 // Compare the "moving" parts of two ConcatInputSections -- i.e. everything not
 // handled by equalsConstant().
 static bool equalsVariable(const ConcatInputSection *ia,

lld/test/MachO/icf-literals.s

 # REQUIRES: x86
 # RUN: rm -rf %t; mkdir %t
 # RUN: llvm-mc -filetype=obj -triple=x86_64-apple-darwin %s -o %t/test.o
 # RUN: %lld -lSystem --icf=all -o %t/test %t/test.o
 # RUN: llvm-objdump --macho --syms -d %t/test | FileCheck %s
 
 # CHECK:      _main:
 # CHECK-NEXT: callq   _foo2_ref
 # CHECK-NEXT: callq   _foo2_ref
 # CHECK-NEXT: callq   _bar2_ref
 # CHECK-NEXT: callq   _bar2_ref
 # CHECK-NEXT: callq   _baz2_ref
 # CHECK-NEXT: callq   _baz2_ref
 # CHECK-NEXT: callq   _qux2_ref
 # CHECK-NEXT: callq   _qux2_ref
+# CHECK-NEXT: callq   _sub_str_a_b
+# CHECK-NEXT: callq   _sub_str_b_a
+# CHECK-NEXT: callq   _sub_lit_a_b
+# CHECK-NEXT: callq   _sub_lit_b_a
 
 # CHECK:      [[#%.16x,FOO:]]     l     O __TEXT,__cstring _foo1
 # CHECK-NEXT: [[#%.16x,FOO:]]     l     O __TEXT,__cstring _foo2
 # CHECK-NEXT: [[#%.16x,BAR:]]     l     O __TEXT,__cstring _bar1
 # CHECK-NEXT: [[#%.16x,BAR:]]     l     O __TEXT,__cstring _bar2
 # CHECK-NEXT: [[#%.16x,BAZ:]]     l     O __TEXT,__literals _baz1
 # CHECK-NEXT: [[#%.16x,BAZ:]]     l     O __TEXT,__literals _baz2
 # CHECK-NEXT: [[#%.16x,QUX:]]     l     O __TEXT,__literals _qux1
_baz2:
   .quad 0xdead
 _qux1:
   .quad 0xbeef
 _qux2:
   .quad 0xbeef
 
 .text
 _foo1_ref:
-  mov _foo1@GOTPCREL(%rip), %rax
+  leaq _foo1(%rip), %rax
 _foo2_ref:
-  mov _foo2@GOTPCREL(%rip), %rax
+  leaq _foo2(%rip), %rax
 _bar1_ref:
-  mov _bar1@GOTPCREL(%rip), %rax
+  leaq _bar1(%rip), %rax
 _bar2_ref:
-  mov _bar2@GOTPCREL(%rip), %rax
+  leaq _bar2(%rip), %rax
 _baz1_ref:
-  mov _baz1@GOTPCREL(%rip), %rax
+  movq _baz1(%rip), %rax
 _baz2_ref:
-  mov _baz2@GOTPCREL(%rip), %rax
+  movq _baz2(%rip), %rax
 _qux1_ref:
-  mov _qux1@GOTPCREL(%rip), %rax
+  movq _qux1(%rip), %rax
 _qux2_ref:
-  mov _qux2@GOTPCREL(%rip), %rax
+  movq _qux2(%rip), %rax
+
+## _sub_str_a_b and _sub_str_b_a should not be folded: They contain relocations
+## against the same string symbols, but in a different order and hence
+## return different numbers.
+_sub_str_a_b:
+  leaq _foo2(%rip), %rdx
+  leaq _bar2(%rip), %rax
+  subq %rdx, %rax
+_sub_str_b_a:
+  leaq _bar2(%rip), %rdx
+  leaq _foo2(%rip), %rax
+  subq %rdx, %rax
+
+## Same with literals instead of strings.
+_sub_lit_a_b:
+  movq _baz2(%rip), %rax
+  subq _qux2(%rip), %rax
+_sub_lit_b_a:
+  movq _qux2(%rip), %rax
+  subq _baz2(%rip), %rax
+
 
 .globl _main
 _main:
   callq _foo1_ref
   callq _foo2_ref
   callq _bar1_ref
   callq _bar2_ref
   callq _baz1_ref
   callq _baz2_ref
   callq _qux1_ref
   callq _qux2_ref
+  callq _sub_str_a_b
+  callq _sub_str_b_a
+  callq _sub_lit_a_b
+  callq _sub_lit_b_a
 
 .subsections_via_symbols