[lld-macho] Fix ICF crash when comparing symbol relocs

[int3]

Summary

Previously, when encountering a symbol reloc located in a literal section, we
would look up the contents of the literal at the symbol value + addend offset
within the literal section. However, it seems that this offset is not guaranteed
to be valid. Instead, we should use just the symbol value to retrieve the
literal's contents, and compare the addend values separately. ld64 seems to do
this.

[thevinster]

lld/test/MachO/icf-literals.s

80

Is there reason why we need to also have the pos_offset cases? It looks like it's exercising the same logic as the neg_offset right? Also, would it be possible to also have a test case for the section relocs comparison?

[int3]

lld/test/MachO/icf-literals.s

80

Is there reason why we need to also have the pos_offset cases?

As the comment on line 83 explains, I'm doing this to demonstrate that _foo2+4 should not be treated the same as _bar1, even though they refer to the same location in the input object file

Also, would it be possible to also have a test case for the section relocs comparison?

This is already covered in icf-arm64.s -- it's not done in this file because ICF can't really do much deduplication of section relocs on x86 due to the embedded addends making the section hashes different. (arm64 uses an explicit ADDEND relocation instead of embedded addends and thus avoids this problem.)

[thevinster]

lld/MachO/ICF.cpp

163

Do you think it's worth keeping the previous behavior here and do getOffset(value + ra.addend) or is that not needed here?

lld/test/MachO/icf-literals.s

80

Got it. Okay thanks.

[int3]

lld/MachO/ICF.cpp

163

valueA == valueB == 0 in this case so it isn't necessary. I'll add an assert

lld/MachO/ICF.cpp

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 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) ==
-           isecB->getOffset(valueB + rb.addend);
+    if (ra.referent.is<Symbol *>())
+      // For symbol relocs, we compare the contents at the symbol address. We
+      // don't do `getOffset(value + addend)` because value + addend may not be
+      // a valid offset in the literal section.
+      return isecA->getOffset(valueA) == isecB->getOffset(valueB) &&
+             ra.addend == rb.addend;
+    else {
+      assert(valueA == 0 && valueB == 0);
+      // For section relocs, we compare the content at the section offset.
+      return isecA->getOffset(ra.addend) == isecB->getOffset(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().
 bool ICF::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   _foo2_neg_offset_ref
+# CHECK-NEXT: callq   _foo2_neg_offset_ref
+# CHECK-NEXT: callq   _foo2_pos_offset_ref
+# CHECK-NEXT: callq   _foo2_pos_offset_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
 # CHECK-NEXT: [[#%.16x,QUX:]]     l     O __TEXT,__literals _qux2
 # CHECK-NEXT: [[#%.16x,FOO_REF:]] l     F __TEXT,__text _foo1_ref
 # CHECK-NEXT: [[#%.16x,FOO_REF:]] l     F __TEXT,__text _foo2_ref
+# CHECK-NEXT: [[#%.16x,FOO_NEG:]] l     F __TEXT,__text _foo1_neg_offset_ref
+# CHECK-NEXT: [[#%.16x,FOO_NEG]]  l     F __TEXT,__text _foo2_neg_offset_ref
+# CHECK-NEXT: [[#%.16x,FOO_POS:]] l     F __TEXT,__text _foo1_pos_offset_ref
+# CHECK-NEXT: [[#%.16x,FOO_POS]]  l     F __TEXT,__text _foo2_pos_offset_ref
 # CHECK-NEXT: [[#%.16x,BAR_REF:]] l     F __TEXT,__text _bar1_ref
 # CHECK-NEXT: [[#%.16x,BAR_REF:]] l     F __TEXT,__text _bar2_ref
 # CHECK-NEXT: [[#%.16x,BAZ_REF:]] l     F __TEXT,__text _baz1_ref
 # CHECK-NEXT: [[#%.16x,BAZ_REF:]] l     F __TEXT,__text _baz2_ref
 # CHECK-NEXT: [[#%.16x,QUX_REF:]] l     F __TEXT,__text _qux1_ref
 # CHECK-NEXT: [[#%.16x,QUX_REF:]] l     F __TEXT,__text _qux2_ref
 
 ## _foo1 vs _bar1: same section, different offsets

 _qux2:
   .quad 0xbeef
 
 .text
 _foo1_ref:
   leaq _foo1(%rip), %rax
 _foo2_ref:
   leaq _foo2(%rip), %rax
+_foo1_neg_offset_ref:
+## Check that we can correctly handle `_foo1-32` even though it points outside
+## the __cstring section.
+  leaq _foo1-32(%rip), %rax
+_foo2_neg_offset_ref:
+  leaq _foo2-32(%rip), %rax
+_foo1_pos_offset_ref:
+  leaq _foo1+4(%rip), %rax
+_foo2_pos_offset_ref:
+## Although `_foo2+4` points at _bar1 in the input object file, we shouldn't
+## dedup references to _foo2+4 with references to _bar1.
+  leaq _foo2+4(%rip), %rax
 _bar1_ref:
   leaq _bar1(%rip), %rax
 _bar2_ref:
   leaq _bar2(%rip), %rax
 _baz1_ref:
   movq _baz1(%rip), %rax
 _baz2_ref:
   movq _baz2(%rip), %rax
_sub_lit_b_a:
   movq _qux2(%rip), %rax
   subq _baz2(%rip), %rax
 
 
 .globl _main
 _main:
   callq _foo1_ref
   callq _foo2_ref
+  callq _foo1_neg_offset_ref
+  callq _foo2_neg_offset_ref
+  callq _foo1_pos_offset_ref
+  callq _foo2_pos_offset_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