[PATCH] Implementation of PLT: handling of IFUNC calls (gnu_indirect_function).
Denis Protivensky
dprotivensky at accesssoftek.com
Fri Mar 20 02:07:22 PDT 2015
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?
REPOSITORY
rL LLVM
================
Comment at: lib/ReaderWriter/ELF/ARM/ARMRelocationHandler.cpp:384
@@ +383,3 @@
+static void relocR_ARM_ALU_PC_GN_NC(uint8_t* location, uint32_t result) {
+ assert(lshift < 32 && lshift % 2 == 0);
+
----------------
That's better to be made `static_assert`
================
Comment at: lib/ReaderWriter/ELF/ARM/ARMRelocationHandler.cpp:386
@@ +385,3 @@
+
+ static const uint32_t rshift = 32 - lshift;
+
----------------
`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.
================
Comment at: lib/ReaderWriter/ELF/ARM/ARMRelocationHandler.cpp:387
@@ +386,3 @@
+ static const uint32_t rshift = 32 - lshift;
+
+ result = ((result >> lshift) & 0xFF) | ((rshift / 2) << 8);
----------------
Remove this empty line.
================
Comment at: lib/ReaderWriter/ELF/ARM/ARMRelocationHandler.cpp:396
@@ +395,3 @@
+ int64_t A) {
+ int32_t result = (uint32_t)((S + A) - P);
+
----------------
The cast type is wrong. Should be ` int32_t result = (int32_t)((S + A) - P);`
================
Comment at: lib/ReaderWriter/ELF/ARM/ARMRelocationHandler.cpp:414
@@ +413,3 @@
+ int64_t A) {
+ int32_t result = (uint32_t)((S + A) - P);
+
----------------
The cast is wrong as above.
================
Comment at: lib/ReaderWriter/ELF/ARM/ARMRelocationHandler.cpp:432
@@ +431,3 @@
+ int64_t A) {
+ int32_t result = (uint32_t)((S + A) - P);
+
----------------
The cast is wrong as above.
================
Comment at: lib/ReaderWriter/ELF/ARM/ARMRelocationHandler.cpp:438
@@ +437,3 @@
+ const uint32_t mask = 0xFFF;
+ result = result & mask;
+
----------------
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:
`applyArmReloc(location, (uint32_t)result & mask, mask);`
================
Comment at: lib/ReaderWriter/ELF/ARM/ARMRelocationPass.cpp:50
@@ +49,3 @@
+// .got values
+static const uint8_t ARMGotAtomContent[4] = {0, 0, 0, 0};
+
----------------
I think this code is already in. You should rebase to the actual sources.
================
Comment at: lib/ReaderWriter/ELF/ARM/ARMRelocationPass.cpp:122
@@ +121,3 @@
+/// \brief Atoms that are used by ARM dynamic linking
+class ARMGOTAtom : public GOTAtom {
+public:
----------------
This code is also in the trunk already.
================
Comment at: lib/ReaderWriter/ELF/ARM/ARMRelocationPass.cpp:174
@@ -128,3 +173,3 @@
case R_ARM_JUMP24:
case R_ARM_THM_JUMP24:
static_cast<Derived *>(this)->handleVeneer(atom, ref);
----------------
Why don't you handle these two relocs on a subject of IFUNC?
================
Comment at: lib/ReaderWriter/ELF/ARM/ARMRelocationPass.cpp:282
@@ +281,3 @@
+ std::error_code handleIFUNC(const Reference &ref) {
+ auto target = dyn_cast_or_null<const DefinedAtom>(ref.target());
+ if (target && target->contentType() == DefinedAtom::typeResolver) {
----------------
We came to using `dyn_cast` and assuming that `ref.target()` cannot be null.
http://reviews.llvm.org/D7833
EMAIL PREFERENCES
http://reviews.llvm.org/settings/panel/emailpreferences/
More information about the llvm-commits
mailing list