This is an archive of the discontinued LLVM Phabricator instance.

Ensure that Elf_Rel addends are always written for dynamic relocations
ClosedPublic

Authored by arichardson on Feb 2 2018, 7:09 AM.

Details

Summary

This follows up on r321889 where writing of Elf_Rel addends was partially
moved to RelocationBaseSection. This patch ensures that the addends are
always written to the output section when a input section uses RELA but the
output is REL.

Diff Detail

Repository
rLLD LLVM Linker

Event Timeline

arichardson created this revision.Feb 2 2018, 7:09 AM
arichardson added inline comments.
test/ELF/convert-rel-rela-addends.s
103 ↗(On Diff #132582)

This ugly hack is needed because LLD will reject R_X86_64_32 relocations in shared objects but 16 and 64 bit is fine. Not sure why this is the case as the dynamic linker can just write the correct value to data section.

grimar added inline comments.Feb 2 2018, 7:24 AM
test/ELF/convert-rel-rela-addends.s
103 ↗(On Diff #132582)

We do not allow 32 bits relocations because they might overflow in runtime.

May be I am missing something, but I do not understand why we allow 16 bit relocations in dso. I think it is a bug.
Though bfd allows them too, gold rejects them and that is what I would expect.

Usually we don't mix i386 and x64 test cases in a single test, so I would split it probably.

arichardson added inline comments.Feb 2 2018, 7:29 AM
test/ELF/convert-rel-rela-addends.s
103 ↗(On Diff #132582)

Ah that makes sense, thanks. I can split it into two tests, one checking REL-> RELA and the other checking that RELA->REL is also correct.

ruiu added a comment.Feb 2 2018, 12:44 PM

I don't think I fully understand the code. Can you write more comments so that first-time readers of your code can understand how your code is supposed to work?

ELF/SyntheticSections.h
331

Basically we should avoid friend whenever possible. If you need to access something that is currently private, please make it public.

372

Just like above, please avoid writing functions with default parameters (and function overloading) as well. I prefer not to hide arguments.

  • Removed fried and renamed a method with the same name as the member but completely different semantics
  • Removed default parameters
  • Added comments
ruiu added inline comments.Feb 6 2018, 4:48 PM
ELF/SyntheticSections.cpp
1215

Do I understand it correctly that, when you call the second addReloc (by the way it is better to give the two functions different name), if (!Config->IsRela) part won't be executed? If so, can this function just be

if (Reloc.Type == Target->RelativeRel)
  ++NumRelativeRelocs;
Relocs.push_back(Reloc);

?

1233–1235

I agree with you that this is a nice optimization. Please remove this TODO and just say that we skip addend 0 to reduce the number of dynamic relocations.

ELF/SyntheticSections.h
319–320

nit: add blank lines before and after this block so that it is obvious that this comment describes getRelaAddend.

331–332

This TODO should be a new code review request rather than a TODO in source code.

371

input -> output?

test/ELF/convert-rel-rela-addends.s
110 ↗(On Diff #132734)

Please make sure the last byte of a text file is '\n'.

arichardson marked 5 inline comments as done.
  • rebase
  • address comments
  • use mips for tests to avoid depending on llvm-mc changes
ELF/SyntheticSections.cpp
1215

Since the UseSymVA flag is always zero and and the addend is zero it will not be doing anything. I avoid calling this function now to skip unnecessary checks.

arichardson marked 3 inline comments as done.
  • Split out an unrelated change to D43161
  • made the diff smaller

Shrink patch even more to address only the issue in the test case

  • Updated comments to Rafael's suggestions
This revision was not accepted when it landed; it landed in state Needs Review.Feb 16 2018, 2:04 AM
This revision was not accepted when it landed; it landed in state Needs Review.
This revision was automatically updated to reflect the committed changes.
This revision was automatically updated to reflect the committed changes.