This is an archive of the discontinued LLVM Phabricator instance.

Implementation of relocations: R_ARM_REL32, R_ARM_THM_JUMP11, R_ARM_PREL31 for ELF/ARM
ClosedPublic

Authored by lenykholodov on Feb 11 2015, 9:28 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

lenykholodov retitled this revision from to Implementation of relocations: R_ARM_REL32, R_ARM_THM_JUMP11, R_ARM_PREL31 for ELF/ARM.
lenykholodov updated this object.
lenykholodov edited the test plan for this revision. (Show Details)
lenykholodov added a reviewer: lld.
lenykholodov set the repository for this revision to rL LLVM.
lenykholodov added a project: lld.
lenykholodov added a subscriber: Unknown Object (MLST).
shankarke added inline comments.
lib/ReaderWriter/ELF/ARM/ARMRelocationHandler.cpp
245–246 ↗(On Diff #19766)

Why isnt a veneer being created for this ?

446–450 ↗(On Diff #19766)

can be done in one line

if (std::error_code ec= function)
return ec;

looks fine other than what I mention inline.

lib/ReaderWriter/ELF/ARM/ARMRelocationHandler.cpp
121 ↗(On Diff #19766)

applyThumbReloc

123 ↗(On Diff #19766)

Is this checked before here? Bad input shouldn't assert, it should report a proper error.

lib/ReaderWriter/ELF/ARM/ARMRelocationHandler.cpp
121 ↗(On Diff #19766)

If the function fixes up 16-bit Thumb instructions only, this should be reflected in its name.

168 ↗(On Diff #19766)

I'd propose not to modify the `result``` variable after calculating address by the formula, so the logged value stays consistent with other handlers.

You can always introduce new variable with more readable name like `rel31``` and store the masked address in it.

258 ↗(On Diff #19766)

Subtracting 2 from the shifted address is wrong. This means that you subtract 4 bytes from the original address value. Why should you do that?

  • R_ARM_THM_JUMP11 addend fix;
  • function name change: applyThmReloc -> applyThumb16Reloc for thumb16 instruction;
  • minor code formatting changes.
Bigcheese accepted this revision.Feb 13 2015, 12:37 PM
Bigcheese edited edge metadata.

lgtm

This revision is now accepted and ready to land.Feb 13 2015, 12:37 PM

Couple of functional errors:

  • error in use of SignExtend
  • veneer handling for THM_JUMP11 reloc is not fixed

Others are code style changes, but please be careful to address (fix or comment) all of them.

lib/ReaderWriter/ELF/ARM/ARMRelocationHandler.cpp
83 ↗(On Diff #19886)

You have 12 bits after the shift, so it should be SignExtend<12>, no?

96 ↗(On Diff #19886)

I'd ask you to name readAddend method to correspond to the reloc name (readAddend_THM_JUMP11). Otherwise it's confusing, and moreover, THM16 suffix is never used (neither in documentation, nor in the code). The way when more generalized name may be used is when you have couple of relocs specifying related instructions.

172 ↗(On Diff #19886)

The comment is wrong. You don't drop the T flag in calculations.

187 ↗(On Diff #19886)

Add T symbol output.

260 ↗(On Diff #19886)

You didn't fix this and didn't provide any explanation why you left this code unchanged.

273 ↗(On Diff #19886)

This may be out of 80 symbols. Please, run it through clang-format.

lenykholodov added inline comments.Feb 16 2015, 7:21 AM
lib/ReaderWriter/ELF/ARM/ARMRelocationHandler.cpp
83 ↗(On Diff #19886)

You are right. I've fixed it.

96 ↗(On Diff #19886)

Fixed.

172 ↗(On Diff #19886)

Fixed.

187 ↗(On Diff #19886)

Fixed.

260 ↗(On Diff #19886)

I've checked documentation again and decided to remove veneer processing for this relocation. Jump11 won't have such case so this check may be removed.

"ELF for the ARM Architecture" documentation paper (p.4.6.1.5. Static Thumb16 relocations): "..
In general the addressing range of these relocations is too small for them to reference external symbols and they are documented here for completeness. A linker is not required to generate trampoline sequences (or veneers) to extend the branching range of the jump relocations.".

273 ↗(On Diff #19886)

Fixed.

123 ↗(On Diff #19766)

This assert only for self checking. Input parameter result has been already masked before. This assert checks that there is no unmasked bits in result.

446–450 ↗(On Diff #19766)

Will do.

lenykholodov edited edge metadata.

Fixes:

  • veneer processing for jump11 was removed (according to p.4.6.1.5. Static Thumb16 relocations);
  • SignExtend for jump11 addend reading has been fixed (from 11 bits to 12 bits);
  • formatting changes & comments corrections.
lib/ReaderWriter/ELF/ARM/ARMRelocationHandler.cpp
133 ↗(On Diff #20033)

I asked you to run the code through clang-format so there are no things like wrong indentations.

Formatting changes after clang-format.

lenykholodov added inline comments.Feb 17 2015, 4:09 AM
lib/ReaderWriter/ELF/ARM/ARMRelocationHandler.cpp
133 ↗(On Diff #20033)

Fixed.

I don't have commit permissions. Could anyone commit this diff in my behalf?

  • rebase with master changes;
  • changes in code formatting.

Since you don't have permissions, I'll merge this patch for you.

This revision was automatically updated to reflect the committed changes.