[PATCH] Implementation of PLT: handling of IFUNC calls (gnu_indirect_function).
Denis Protivensky
dprotivensky at accesssoftek.com
Tue Mar 3 04:44:48 PST 2015
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.
REPOSITORY
rL LLVM
================
Comment at: lib/ReaderWriter/ELF/ARM/ARMLinkingContext.h:35
@@ +34,3 @@
+
+ bool isDynamicRelocation(const Reference &r) const override {
+ if (r.kindNamespace() != Reference::KindNamespace::ELF)
----------------
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.
================
Comment at: lib/ReaderWriter/ELF/ARM/ARMLinkingContext.h:66
@@ +65,3 @@
+
+ bool isRelativeReloc(const Reference &r) const override {
+ if (r.kindNamespace() != Reference::KindNamespace::ELF)
----------------
The same as for `isDynamicRelocation` - may be removed.
================
Comment at: lib/ReaderWriter/ELF/ARM/ARMRelocationHandler.cpp:312
@@ -311,1 +311,3 @@
+static uint32_t formALU_PC_GN(uint32_t val, uint32_t lshift) {
+ assert(lshift < 32 && lshift % 2 == 0);
----------------
This is better to be implemented as a template function taking count of bits in `lshift` as a template parameter.
Also, I'd rework this function to:
- calculate shifted `result` value
- do what current function does
- write the reloc with `applyArmReloc`
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.
================
Comment at: lib/ReaderWriter/ELF/ARM/ARMRelocationHandler.cpp:348
@@ +347,3 @@
+
+/// \brief R_ARM_LDR_PC_G2 - ((S + A) | T) - P => S + A - P
+static void relocR_ARM_LDR_PC_G2(uint8_t *location, uint64_t P, uint64_t S,
----------------
There's no `T` bit in the documentation, so please, update the comment here.
================
Comment at: lib/ReaderWriter/ELF/ARM/ARMRelocationHandler.cpp:351
@@ +350,3 @@
+ int64_t A) {
+ uint32_t result = (uint32_t)((S + A) - P);
+ const uint32_t mask = 0xFFF;
----------------
For all these three group relocations:
the offset you receive when making calculations, it may be negative.
The reference ("Group relocations", p.32) says how you should handle this case.
Since you don't handle it, you need to put a check and fire `llvm_unreachable` in that case, so we may later implement proper handling if needed.
================
Comment at: lib/ReaderWriter/ELF/ARM/ARMRelocationHandler.cpp:425
@@ -373,1 +424,3 @@
break;
+ case R_ARM_ALU_PC_G0_NC:
+ relocR_ARM_ALU_PC_G0_NC(location, relocVAddress, targetVAddress, addend);
----------------
For these, you don't handle initial addend calculation as for other relocation types, so it's inconsistent.
================
Comment at: lib/ReaderWriter/ELF/ARM/ARMRelocationPass.cpp:169
@@ +168,3 @@
+ case R_ARM_CALL:
+ case R_ARM_THM_CALL:
+ static_cast<Derived *>(this)->handleIFUNC(ref);
----------------
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.
================
Comment at: lib/ReaderWriter/ELF/ARM/ARMRelocationPass.cpp:250
@@ +249,3 @@
+
+ ga->_name = "__got_ifunc_";
+ ga->_name += da->name();
----------------
You shouldn't pollute the symbol table with this sort of needless symbols. Just put
```#ifndef NDEBUG
#endif```
guard around.
================
Comment at: test/elf/ARM/rel-ifunc.test:7
@@ +6,3 @@
+
+# CHECK: Contents of section .plt:
+# CHECK: 400080 00c68fe2 00ca8ce2 78ffbce5
----------------
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.
================
Comment at: test/elf/ARM/rel-ifunc.test:10
@@ +9,3 @@
+# CHECK-NEXT: Contents of section .text:
+# CHECK: 40013c 80b500af fff7ccff fff79cef 00231846
+# CHECK: Contents of section .got.plt:
----------------
You should add `.rel.plt` section output, and it should point at the `.got.plt` symbol you specified below.
================
Comment at: test/elf/ARM/rel-ifunc.test:12
@@ +11,3 @@
+# CHECK: Contents of section .got.plt:
+# CHECK: 401000 ad004000
+# CHECK: SYMBOL TABLE:
----------------
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).
http://reviews.llvm.org/D7833
EMAIL PREFERENCES
http://reviews.llvm.org/settings/panel/emailpreferences/
More information about the llvm-commits
mailing list