This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by int3 on Apr 21 2022, 7:08 PM.

Details

Reviewers
thevinster
Group Reviewers
Restricted Project
Commits
rGc242e10c74d2: [lld-macho] Fix ICF crash when comparing symbol relocs
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.

Diff Detail

Event Timeline

int3 created this revision.Apr 21 2022, 7:08 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptApr 21 2022, 7:08 PM
int3 requested review of this revision.Apr 21 2022, 7:08 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 21 2022, 7:08 PM
thevinster added inline comments.
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 marked an inline comment as done.Apr 22 2022, 11:19 AM
int3 added inline comments.
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 accepted this revision.Apr 22 2022, 12:15 PM
thevinster added inline comments.
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.

This revision is now accepted and ready to land.Apr 22 2022, 12:15 PM
int3 marked 3 inline comments as done.Apr 22 2022, 12:24 PM
int3 added inline comments.
lld/MachO/ICF.cpp
163

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

This revision was automatically updated to reflect the committed changes.
int3 marked an inline comment as done.