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

[Vincent Lee via Phabricator via llvm-commits]

thevinster accepted this revision.
thevinster added inline comments.
This revision is now accepted and ready to land.


================
Comment at: lld/MachO/ICF.cpp:163
+      // For section relocs, we compare the content at the section offset.
+      return isecA->getOffset(ra.addend) == isecB->getOffset(rb.addend);
   };
----------------
Do you think it's worth keeping the previous behavior here and do `getOffset(value + ra.addend)` or is that not needed here? 


================
Comment at: lld/test/MachO/icf-literals.s:80
+  leaq _foo2-32(%rip), %rax
+_foo1_pos_offset_ref:
+  leaq _foo1+4(%rip), %rax
----------------
int3 wrote:
> thevinster wrote:
> > 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?
> > 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.)
Got it. Okay thanks. 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124223/new/

https://reviews.llvm.org/D124223