This is an archive of the discontinued LLVM Phabricator instance.

[LLD][ELF] Use more specific method to calculate DT_PLTRELSZ
ClosedPublic

Authored by peter.smith on Nov 20 2018, 8:44 AM.

Details

Summary

The DT_PLTRELSZ dynamic tag is calculated using the size of the OutputSection containing the In.RelaPlt InputSection. This will work for the default no linker script case and the majority of linker scripts. Unfortunately it doesn't work for some 'almost' sensible linker scripts. It is permitted by ELF to have a single OutputSection containing both In.RelaDyn, In.RelaPlt and In.RelaIPlt. It is also permissible for the range of memory [DT_RELA, DT_RELA + DT_RELASZ) and the range [DT_JMPREL, DT_JMPREL + DT_JMPRELSZ) to overlap as long as the the latter range is at the end (see comment in glibc dynamic loader) https://code.woboq.org/userspace/glibc/elf/dynamic-link.h.html#108).

To support a linker script containing something like: .rela.dyn : { *(.rela.dyn) *(.rela.plt) } use the InputSections to determine DT_JMPRELSZ rather than the OutputSection size. I've left the calculation of the .rela.dyn to be the size of the OutputSection as that is the most literal reading of ELF and the smallest change. This affects all targets, I've added tests for AArch64, x86 (cited in PR) and ARM (as the irelative relocations do not go in the .rel.plt).

There will be no change to any project using the default linker script or the more modern

.rela.dyn       : { *(.rela.dyn) }
.rela.plt         : { *(.rela.plt) *(.rela.iplt) }

fixes pr39678 (https://bugs.llvm.org/show_bug.cgi?id=39678)

I considered giving an error message rather than trying to support this case but the logic of working out when there is an error is no simpler. It also risked faulting existing programs.

Diff Detail

Event Timeline

peter.smith created this revision.Nov 20 2018, 8:44 AM

It feels like some corner case. Though it still a bug we could fix.
I have a few suggestions/questions below.

ELF/SyntheticSections.cpp
1255

DT_JMPTREL -> DT_JMPREL

1267

I truncated this to

Entries.push_back({DT_PLTRELSZ, [=] {
                     size_t Size = In.RelaPlt->getSize();
                     if (In.RelaIplt->getParent() == In.RelaPlt->getParent())
                       Size += In.RelaIplt->getSize();
                     return Size;
                   }});

And all your tests pass.
That will break the case when !Config->AndroidPackDynReloc (so, In.RelaIplt is placed to .rel.dyn),
but do we really need to support it? I guess such script is a rare case and perhaps not worth to support all corner cases? I am not sure.

1339–1351

So I suggest to inline addPltRelSize here and just update the existent comment above.

ELF/SyntheticSections.h
465 ↗(On Diff #174783)

Perhaps I would not create the additional method, because it is very specific.
I think it can be inlined, see how DT_PPC64_GLINK is handled for example.

test/ELF/arm-combined-dynrel-ifunc.s
13

I would just include the comment here instead of referencing it. No need to save space so hard in test cases I think:)
The same applies to another test.

Thanks very much for the comments. I agree that this is a somewhat niche use case, most likely embedded systems related where someone has built their own loader. I've updated the diff with the following changes:

  • The ARM test passed with your updated changes because it was always being skipped! The REQUIRES: ARM should have been REQUIRES: arm. There are a couple of other tests that have that pattern that I'll fix up later today.
  • I've simplified the test for the non-Android Arm case in a simpler way (just check the In.RelaIplt name isn't .rel.dyn)
  • I've inlined the function and updated the comments in the test as suggested.
grimar added inline comments.Nov 21 2018, 2:51 AM
ELF/SyntheticSections.cpp
1347

Seems it can be In.RelaIplt->Name == In.RelaPlt.
It feels like a slightly cleaner way probably?

1347

I mean In.RelaIplt->Name == In.RelaPlt->Name

Agreed, I've made the change to In.RelaIplt->Name == In.RelaPlt->Name.

grimar accepted this revision.Nov 21 2018, 3:01 AM

LGTM.

This revision is now accepted and ready to land.Nov 21 2018, 3:01 AM

Thanks, I'll wait a bit before committing to see if Rui has any comments.

ruiu added inline comments.Nov 26 2018, 4:12 PM
ELF/SyntheticSections.cpp
1343

It seems this lambda doesn't capture any variable, so this can be [], no? It might be better to write this as a regular (outline) function, as this seems indented a bit too much.

test/ELF/aarch64-combined-dynrel-ifunc.s
1

Needs a colon after REQUIRES?

Thanks for the comments. I've made the lambda a free function and have made sure REQUIRES has a colon after it in all the tests.

ruiu accepted this revision.Nov 27 2018, 9:31 AM

LGTM

This revision was automatically updated to reflect the committed changes.

Hi @peter.smith! The tests arm-combined-dynrel.s and arm-combined-dynrel-ifunc.s do not run because of inaccurate REQUIRES. Could you fix them, please?

Hi @peter.smith! The tests arm-combined-dynrel.s and arm-combined-dynrel-ifunc.s do not run because of inaccurate REQUIRES. Could you fix them, please?

I'll take a look. It may be later on this week before I can get to it.