[PATCH] Implementation of PLT: handling of IFUNC calls (gnu_indirect_function).

[Denis Protivensky]

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/