This is an archive of the discontinued LLVM Phabricator instance.

[ELF] Check the Elf_Rel addends for dynamic relocations
ClosedPublic

Authored by arichardson on Apr 28 2021, 6:38 AM.

Details

Summary

There used to be many cases where addends for Elf_Rel were not emitted in
the final object file (mostly when building for MIPS64 since the input .o
files use RELA but the output uses REL). These cases have been fixed since,
but this patch adds a check to ensure that the written values are correct.
It is based on a previous patch that I added to the CHERI fork of LLD since
we were using MIPS64 as a baseline. The work has now almost entirely
shifted to RISC-V and Arm Morello (which use Elf_Rela), but I thought
it would be useful to upstream our local changes anyway.

This patch adds a (hidden) command line flag --check-dynamic-relocations
that can be used to enable these checks. It is also on by default in
assertions builds for targets that handle all dynamic relocations kinds
that LLD can emit in Target::getImplicitAddend(). Currently this is
enabled for ARM, MIPS, and I386.

Depends on D100490

Diff Detail

Event Timeline

arichardson created this revision.Apr 28 2021, 6:38 AM
arichardson requested review of this revision.Apr 28 2021, 6:38 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 28 2021, 6:38 AM

Thanks for splitting out. I'm happy with this. Will be good to wait for MaskRay to have a look.

MaskRay added inline comments.Apr 30 2021, 2:30 PM
lld/ELF/Arch/ARM.cpp
842

The value is not used.

ld.so implementations don't use addend for JUMP_SLOT/GLOB_DAT.

lld/ELF/OutputSections.cpp
571

perhaps use

dynamic relocation XXX at offset YYY[ against symbol ZZZ]

arichardson added inline comments.Jul 2 2021, 4:55 AM
lld/ELF/Arch/ARM.cpp
842

I agree that the value at that location is not really an addend since Elf_Rela also uses the in-memory value instead of of the r_addend field (FreeBSD does *where += (Elf_Addr)obj->relocbase; for all JUMP_SLOT values).
Maybe // These relocations do not have an implicit addend?

Improve comments

Add XFAIL to two tests that need a follow-up patch

MaskRay accepted this revision.Jul 2 2021, 8:21 PM

Thanks!

This revision is now accepted and ready to land.Jul 2 2021, 8:21 PM
This revision was landed with ongoing or failed builds.Jul 9 2021, 2:42 AM
This revision was automatically updated to reflect the committed changes.
arichardson marked an inline comment as done.