This is an archive of the discontinued LLVM Phabricator instance.

[ELF] Don't shrink RelrSection
ClosedPublic

Authored by MaskRay on Sep 4 2019, 6:08 AM.

Details

Summary

Fixes PR43214.

The size of SHT_RELR may oscillate between 2 numbers (see D53003 for a
similar --pack-dyn-relocs=android issue). This can happen if the shrink
of SHT_RELR causes it to take more words to encode relocation offsets
(this can happen with thunks or segments with overlapping p_offset
ranges), and the expansion of SHT_RELR causes it to take fewer words to
encode relocation offsets.

To avoid the issue, add padding 1s to the end of the relocation section
if its size would decrease. Trailing 1s do not decode to more
relocations.

Diff Detail

Repository
rL LLVM

Event Timeline

MaskRay created this revision.Sep 4 2019, 6:08 AM
MaskRay updated this revision to Diff 218697.Sep 4 2019, 7:36 AM

Add a test

grimar added inline comments.Sep 4 2019, 7:44 AM
test/ELF/pack-dyn-relocs-relr-loop.s
11 ↗(On Diff #218697)

Just an idea, not sure how much is reasonable:

LLD sometimes calls log() and print().
The latter is used to test ICF:

  1. CHECK: selected section {{.*}}:(.text.f1)
  2. CHECK: removing identical section {{.*}}:(.text.f2)

You can probably add a log() call and -v and check that resize happened.

MaskRay updated this revision to Diff 218705.Sep 4 2019, 7:56 AM
MaskRay marked an inline comment as done.
MaskRay edited the summary of this revision. (Show Details)

Add log() as grimar suggested

peter.smith accepted this revision.Sep 4 2019, 9:04 AM

As far as I can tell this will definitely prevent the oscillation and has already seen success in the Android relocation side. As Rui mentioned on the PR, it would be useful to put a generic convergence limit on finalizeAddressDependentContent() to catch an infinite loop that we'd not thought possible. This could be done separately though.

This revision is now accepted and ready to land.Sep 4 2019, 9:04 AM

As far as I can tell this will definitely prevent the oscillation and has already seen success in the Android relocation side. As Rui mentioned on the PR, it would be useful to put a generic convergence limit on finalizeAddressDependentContent() to catch an infinite loop that we'd not thought possible. This could be done separately though.

Thanks! As grimar suggested, we may need some log() in these iterating loops as well.

This revision was automatically updated to reflect the committed changes.