This diff includes implementation of linking calls to ifunc functions. It provides ifunc entries in PLT and corresponding relocations (R_ARM_ALU_PC_G0_NC, R_ARM_ALU_PC_G1_NC, R_ARM_LDR_PC_G2 for link-time and R_ARM_IRELATIVE for run-time).
Details
- Reviewers
denis-protivensky Bigcheese • shankar.easwaran - Group Reviewers
lld - Commits
- rG28074d6e9ba1: [ARM] Implementation of PLT: handling of IFUNC calls (gnu_indirect_function)
rLLD233277: [ARM] Implementation of PLT: handling of IFUNC calls (gnu_indirect_function)
rL233277: [ARM] Implementation of PLT: handling of IFUNC calls (gnu_indirect_function)
Diff Detail
- Repository
- rL LLVM
Event Timeline
Tests for relocations R_ARM_ALU_PC_G0_NC, R_ARM_ALU_PC_G1_NC, R_ARM_LDR_PC_G2 have been added. All of them were created in manual mode by editing YAML files. Unfortunately, I couldn't find the way how I could generate these relocations with gcc/clang compilation.
The idea of group relocations is that they allow splitting an address up among couple of instructions without making access to the memory with load instruction. It's the optimization, that won't work for offsets more than 2^28 bits in the way it's used (maybe we should add comments into TODO section about that?)
So the three separated tests for group relocations don't really test anything. They should be tested as a group in one test case:
- you point all three relocs at the same function address
- you then show offsets in comments to the content of .rel.plt section as it's done for other tests. Example:
# CHECK: Contents of section .text: # CHECK: 400084 80b500af fff7f4bf 03461846 80bd00bf # ^^ offset = -0x18
- you then show that summing up these offsets gives the address of the function you're pointing at (again in comments):
# call site offset PC(thm) _Z1fv addr # 0x400088 + (-0x18) + 0x4 = 0x400074
I think in this case it's at least understandable what's happening. And the current edition of these three tests just shows that something is generated by lld, but there's no way to check it.
lib/ReaderWriter/ELF/ARM/ARMLinkingContext.h | ||
---|---|---|
35 | We don't handle dynamic linking yet. Moreover, we don't use any of the relocations in the switch block, so I doubt if this code is needed. You shouldn't add the code that you cannot state is working. Please, remove this function. | |
66 | The same as for isDynamicRelocation - may be removed. | |
lib/ReaderWriter/ELF/ARM/ARMRelocationHandler.cpp | ||
324 | This is better to be implemented as a template function taking count of bits in lshift as a template parameter.
So the prototype would be: template <uint32_t lshift> static void reloc_R_ARM_ALU_PC_GN_NC(uint8_t *location, uint32_t result); The calling function should calculate the result by the formula, dump values in the log, and call this function in the end. | |
360 | There's no T bit in the documentation, so please, update the comment here. | |
363 | For all these three group relocations: | |
437 | For these, you don't handle initial addend calculation as for other relocation types, so it's inconsistent. | |
lib/ReaderWriter/ELF/ARM/ARMRelocationPass.cpp | ||
171 | There are more reloc types that can target IFUNC entries. Imagine that someone takes the address of memcpy, so it goes through the ldr instruction (using R_ARM_ABS32 reloc, for example). Other platforms handle most types of relocations with this function just not to miss anything. | |
252 | You shouldn't pollute the symbol table with this sort of needless symbols. Just put #ifndef NDEBUG #endif guard around. | |
test/elf/ARM/rel-ifunc.test | ||
8 | I'd really want to see the explanation of where these three instructions in .plt section point at. I think they should point at the same .got entry where value from .rel.plt section points. | |
11 | You should add .rel.plt section output, and it should point at the .got.plt symbol you specified below. | |
13 | You'd better show where this address ad004000 is pointing at. This should be somewhere in the .text section defining a function to be substituted (you may add it to the symbol table output for simplicity). |
- unused methods ARMLinkingContext::isDynamicRelocation and ARMLinkingContext::isRelativeReloc have been removed;
- imlementations of relocations R_ARM_ALU_PC_G0_NC & R_ARM_ALU_PC_G1_NC have been generalized (see relocR_ARM_ALU_PC_GN_NC function);
- initial addends have been ignored because they are read during the relocation applying, for group relocation reference.addend() have been used;
- for negative offsets in group relocations llvm_unreachable has been inserted (full implementation will be done in further patch);
- additional relocs have been processed with ifunc;
- global vars scope was changed to static;
- separate tests for relocations were removed (because of useless);
- test for ifunc has been changed according to the Denis's comment;
- comments were fixed.
You removed tests for separate group relocations, but didn't add one which tests all the three at a time. Can you please do that?
lib/ReaderWriter/ELF/ARM/ARMRelocationHandler.cpp | ||
---|---|---|
326 | That's better to be made static_assert | |
328 | static doesn't make sense. Statics use locks in C++11, so why introduce unneeded internal complexity? If you want to have compile-time variable here, use enum { rshift = 32 - lshift }; but I think that regular variable is enough. | |
329 | Remove this empty line. | |
338 | The cast type is wrong. Should be int32_t result = (int32_t)((S + A) - P); | |
356 | The cast is wrong as above. | |
374 | The cast is wrong as above. | |
380 | The result is converted to unsigned while making & operation (because mask is unsigned) then back to signed when assigned. You're also modifying the result value before logging. So I'd propose to move this operation into applyArmReloc: | |
lib/ReaderWriter/ELF/ARM/ARMRelocationPass.cpp | ||
52 | I think this code is already in. You should rebase to the actual sources. | |
124 | This code is also in the trunk already. | |
167 | Why don't you handle these two relocs on a subject of IFUNC? | |
279 | We came to using dyn_cast and assuming that ref.target() cannot be null. |
- small fixes for group relocations (static_assert, using of static specifier, formatting, types conversion);
- ifunc handling for all available relocations;
- rebasing with master;
- new complex test has been added for three group relocs (R_ARM_ALU_PC_G0_NC, R_ARM_ALU_PC_G1_NC, R_ARM_LDR_PC_G2).
I have no commit permission. I would really appreciate if someone could send this patch on my behalf.
- generation of veneers for non jump instructions has been disabled;
- tests have been fixes;
- missing break has been added to the relocations processing switch.
We don't handle dynamic linking yet. Moreover, we don't use any of the relocations in the switch block, so I doubt if this code is needed. You shouldn't add the code that you cannot state is working. Please, remove this function.