[PATCH] Implementation of relocations: R_ARM_REL32, R_ARM_THM_JUMP11, R_ARM_PREL31 for ELF/ARM

[Leny Kholodov]

REPOSITORY
  rL LLVM

================
Comment at: lib/ReaderWriter/ELF/ARM/ARMRelocationHandler.cpp:83
@@ +82,3 @@
+
+  return llvm::SignExtend32<11>(imm11 << 1);
+}
----------------
denis-protivensky wrote:
> You have 12 bits after the shift, so it should be `SignExtend<12>`, no?
You are right. I've fixed it.

================
Comment at: lib/ReaderWriter/ELF/ARM/ARMRelocationHandler.cpp:96
@@ -86,1 +95,3 @@
+  case R_ARM_THM_JUMP11:
+    return readAddend_THM16_CALL(location);
   case R_ARM_CALL:
----------------
denis-protivensky wrote:
> 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.
Fixed.

================
Comment at: lib/ReaderWriter/ELF/ARM/ARMRelocationHandler.cpp:172
@@ +171,3 @@
+
+/// \brief R_ARM_PREL31 - ((S + A) | T) - P => S + A - P
+static void relocR_ARM_PREL31(uint8_t *location, uint64_t P, uint64_t S,
----------------
denis-protivensky wrote:
> The comment is wrong. You don't drop the `T` flag in calculations.
Fixed.

================
Comment at: lib/ReaderWriter/ELF/ARM/ARMRelocationHandler.cpp:187
@@ +186,3 @@
+      llvm::dbgs() << " P: 0x" << Twine::utohexstr(P);
+      llvm::dbgs() << " result: 0x" << Twine::utohexstr(result);
+      llvm::dbgs() << " rel31: 0x" << Twine::utohexstr(rel31) << "\n");
----------------
denis-protivensky wrote:
> Add `T` symbol output.
Fixed.

================
Comment at: lib/ReaderWriter/ELF/ARM/ARMRelocationHandler.cpp:260
@@ +259,3 @@
+
+  if (needVeneer)
+    return make_out_of_range_reloc_error();
----------------
denis-protivensky wrote:
> You didn't fix this and didn't provide any explanation why you left this code unchanged.
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): "..
In general the addressing range of these relocations is too small for them to reference external symbols and they are documented here for completeness. A linker is not required to generate trampoline sequences (or veneers) to extend the branching range of the jump relocations.". 




================
Comment at: lib/ReaderWriter/ELF/ARM/ARMRelocationHandler.cpp:273
@@ +272,3 @@
+
+  result = (result & 0x0FFE) >> 1; //we cut off first bit because it is always 1 according to p. 4.5.3
+
----------------
denis-protivensky wrote:
> This may be out of 80 symbols. Please, run it through `clang-format`.
Fixed.

================
Comment at: lib/ReaderWriter/ELF/ARM/ARMRelocationHandler.cpp:123
@@ +122,3 @@
+                                 uint16_t mask = 0xFFFF) {
+  assert(!(result & ~mask));
+  *reinterpret_cast<llvm::support::ulittle16_t *>(location) =
----------------
Bigcheese wrote:
> Is this checked before here? Bad input shouldn't assert, it should report a proper error.
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.

================
Comment at: lib/ReaderWriter/ELF/ARM/ARMRelocationHandler.cpp:446-450
@@ +445,7 @@
+  {
+    std::error_code error = relocR_ARM_THM_JUMP11(location, relocVAddress,
+                              targetVAddress, addend, addressesThumb);
+
+    if (error != std::error_code())
+      return error;
+
----------------
shankarke wrote:
> can be done in one line
> 
> if (std::error_code ec= function)
> return ec;
> 
> 
Will do.

http://reviews.llvm.org/D7565

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/