This diff includes implementation of following relocations: R_ARM_REL32, R_ARM_THM_JUMP11, R_ARM_PREL31.
Details
- Reviewers
Bigcheese - Group Reviewers
lld - Commits
- rGe458ab457749: [ARM] Implement relocations: R_ARM_REL32, R_ARM_THM_JUMP11, R_ARM_PREL31
rLLD232464: [ARM] Implement relocations: R_ARM_REL32, R_ARM_THM_JUMP11, R_ARM_PREL31
rL232464: [ARM] Implement relocations: R_ARM_REL32, R_ARM_THM_JUMP11, R_ARM_PREL31
Diff Detail
- Repository
- rL LLVM
Event Timeline
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.
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. |
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): ".. |
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. |
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. |
lib/ReaderWriter/ELF/ARM/ARMRelocationHandler.cpp | ||
---|---|---|
133 ↗ | (On Diff #20033) | Fixed. |