Fix unaligned reads/writes in X86JIT and RuntimeDyldELF.

[samsonov]

Summary

Introduce support::ulittleX_t::ref type to Support/Endian.h and use it in x86 JIT
to enforce correct endianness and fix unaligned accesses.

[ributzka]

lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.cpp

264–265 ↗

(On Diff #12801)

What about using something like this instead?
auto *Target = reinterpret_cast<support::ulittle64_t *>(Section.Address + Offset);
*Target = Value + Addend;

[samsonov]

lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.cpp

264–265 ↗

(On Diff #12801)

It would still be an unaligned write of ulittle_64_t, which is UB.

[ributzka]

lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.cpp

264–265 ↗

(On Diff #12801)

operator= is overloaded for support::ulittle64_t and should call support::endian::write<uint64_t, support::little, 1> for you.

[samsonov]

Follow Juergen's suggestion.

lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.cpp

264–265 ↗

(On Diff #12801)

Oh, right, my bad =/
Thanks for pointing this out.

[samsonov]

Any more comments on this?

[lhames]

lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.cpp

307 ↗

(On Diff #12821)

Unfortunately this isn't safe: The remaining pointer arithmetic will be carried out with the type of Placeholder being ulittle32_t*, and sizeof(ulittle32_t) != sizeof(char).

I think the best thing to do is leave the pointer types as they are and introduce read and write methods to RuntimeDyldELF along the same lines as the ones you added to the JIT.

264–265 ↗

(On Diff #12801)

This might be a good argument for adding some extra types to Endian.h that take the address of the storage. Then we could write something like:

ulittle8_ref(Address) = <expr> ;

264–265 ↗

(On Diff #12801)

Juergen - I believe you're right. I had misread your original code.

Samsonov - The assignment in Juergen's suggested code should go to the explicit assignment operator for ulittle64_t, no?

Though in either case, I'd prefer to provide a convenience wrapper along the lines I described. Having explicit reinterpret_casts everywhere would be awful.

[lhames]

Hi Alexey,

Sorry - I failed to hit submit on my last comment!

• Lang.

[samsonov]

Introduce support::ulittleX_t::ref and use it to simplify code.

lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.cpp

307 ↗

(On Diff #12821)

Not sure I understand this - we don't do any pointer arithmetic with Placeholder - we just do a single dereference of that pointer.

264–265 ↗

(On Diff #12801)

Done

[lhames]

LGTM. Thanks Alexey!

lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.cpp

307 ↗

(On Diff #12821)

Oh right - I missed the dereference on placeholder. Sorry about that: this is all good. :)

[samsonov]

Closed by commit rL216631 (authored by @samsonov).

llvm/trunk/include/llvm/Support/Endian.h

struct packed_endian_specific_integral {
   void operator=(value_type newValue) {
     endian::write<value_type, endian, alignment>(
       (void*)Value.buffer, newValue);
   }
 
 private:
   AlignedCharArray<PickAlignment<value_type, alignment>::value,
                    sizeof(value_type)> Value;
+
+public:
+  struct ref {
+    explicit ref(void *Ptr) : Ptr(Ptr) {}
+
+    operator value_type() const {
+      return endian::read<value_type, endian, alignment>(Ptr);
+    }
+
+    void operator=(value_type NewValue) {
+      endian::write<value_type, endian, alignment>(Ptr, NewValue);
+    }
+
+  private:
+    void *Ptr;
   };
+};
+
 } // end namespace detail
 
 typedef detail::packed_endian_specific_integral
                   <uint8_t, little, unaligned>  ulittle8_t;
 typedef detail::packed_endian_specific_integral
                   <uint16_t, little, unaligned> ulittle16_t;
 typedef detail::packed_endian_specific_integral
                   <uint32_t, little, unaligned> ulittle32_t;

llvm/trunk/lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.cpp

 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/Triple.h"
 #include "llvm/ExecutionEngine/ObjectBuffer.h"
 #include "llvm/ExecutionEngine/ObjectImage.h"
 #include "llvm/Object/ELFObjectFile.h"
 #include "llvm/Object/ObjectFile.h"
 #include "llvm/Support/ELF.h"
+#include "llvm/Support/Endian.h"
 #include "llvm/Support/MemoryBuffer.h"
 
 using namespace llvm;
 using namespace llvm::object;
 
 #define DEBUG_TYPE "dyld"
 
 namespace {
void RuntimeDyldELF::resolveX86_64Relocation(const SectionEntry &Section,
                                              uint64_t Offset, uint64_t Value,
                                              uint32_t Type, int64_t Addend,
                                              uint64_t SymOffset) {
   switch (Type) {
   default:
     llvm_unreachable("Relocation type not implemented yet!");
     break;
   case ELF::R_X86_64_64: {
-    uint64_t *Target = reinterpret_cast<uint64_t *>(Section.Address + Offset);
-    *Target = Value + Addend;
+    support::ulittle64_t::ref(Section.Address + Offset) = Value + Addend;
     DEBUG(dbgs() << "Writing " << format("%p", (Value + Addend)) << " at "
-                 << format("%p\n", Target));
+                 << format("%p\n", Section.Address + Offset));
     break;
   }
   case ELF::R_X86_64_32:
   case ELF::R_X86_64_32S: {
     Value += Addend;
     assert((Type == ELF::R_X86_64_32 && (Value <= UINT32_MAX)) ||
            (Type == ELF::R_X86_64_32S &&
             ((int64_t)Value <= INT32_MAX && (int64_t)Value >= INT32_MIN)));
     uint32_t TruncatedAddr = (Value & 0xFFFFFFFF);
-    uint32_t *Target = reinterpret_cast<uint32_t *>(Section.Address + Offset);
-    *Target = TruncatedAddr;
+    support::ulittle32_t::ref(Section.Address + Offset) = TruncatedAddr;
     DEBUG(dbgs() << "Writing " << format("%p", TruncatedAddr) << " at "
-                 << format("%p\n", Target));
+                 << format("%p\n", Section.Address + Offset));
     break;
   }
   case ELF::R_X86_64_GOTPCREL: {
     // findGOTEntry returns the 'G + GOT' part of the relocation calculation
     // based on the load/target address of the GOT (not the current/local addr).
     uint64_t GOTAddr = findGOTEntry(Value, SymOffset);
-    uint32_t *Target = reinterpret_cast<uint32_t *>(Section.Address + Offset);
     uint64_t FinalAddress = Section.LoadAddress + Offset;
     // The processRelocationRef method combines the symbol offset and the addend
     // and in most cases that's what we want.  For this relocation type, we need
     // the raw addend, so we subtract the symbol offset to get it.
     int64_t RealOffset = GOTAddr + Addend - SymOffset - FinalAddress;
     assert(RealOffset <= INT32_MAX && RealOffset >= INT32_MIN);
     int32_t TruncOffset = (RealOffset & 0xFFFFFFFF);
-    *Target = TruncOffset;
+    support::ulittle32_t::ref(Section.Address + Offset) = TruncOffset;
     break;
   }
   case ELF::R_X86_64_PC32: {
     // Get the placeholder value from the generated object since
     // a previous relocation attempt may have overwritten the loaded version
-    uint32_t *Placeholder =
-        reinterpret_cast<uint32_t *>(Section.ObjAddress + Offset);
-    uint32_t *Target = reinterpret_cast<uint32_t *>(Section.Address + Offset);
+    support::ulittle32_t::ref Placeholder(
+        (void *)(Section.ObjAddress + Offset));
     uint64_t FinalAddress = Section.LoadAddress + Offset;
-    int64_t RealOffset = *Placeholder + Value + Addend - FinalAddress;
+    int64_t RealOffset = Placeholder + Value + Addend - FinalAddress;
     assert(RealOffset <= INT32_MAX && RealOffset >= INT32_MIN);
     int32_t TruncOffset = (RealOffset & 0xFFFFFFFF);
-    *Target = TruncOffset;
+    support::ulittle32_t::ref(Section.Address + Offset) = TruncOffset;
     break;
   }
   case ELF::R_X86_64_PC64: {
     // Get the placeholder value from the generated object since
     // a previous relocation attempt may have overwritten the loaded version
-    uint64_t *Placeholder =
-        reinterpret_cast<uint64_t *>(Section.ObjAddress + Offset);
-    uint64_t *Target = reinterpret_cast<uint64_t *>(Section.Address + Offset);
+    support::ulittle64_t::ref Placeholder(
+        (void *)(Section.ObjAddress + Offset));
     uint64_t FinalAddress = Section.LoadAddress + Offset;
-    *Target = *Placeholder + Value + Addend - FinalAddress;
+    support::ulittle64_t::ref(Section.Address + Offset) =
+        Placeholder + Value + Addend - FinalAddress;
     break;
   }
   }
 }
 
 void RuntimeDyldELF::resolveX86Relocation(const SectionEntry &Section,
                                           uint64_t Offset, uint32_t Value,
                                           uint32_t Type, int32_t Addend) {
   switch (Type) {
   case ELF::R_386_32: {
     // Get the placeholder value from the generated object since
     // a previous relocation attempt may have overwritten the loaded version
-    uint32_t *Placeholder =
-        reinterpret_cast<uint32_t *>(Section.ObjAddress + Offset);
-    uint32_t *Target = reinterpret_cast<uint32_t *>(Section.Address + Offset);
-    *Target = *Placeholder + Value + Addend;
+    support::ulittle32_t::ref Placeholder(
+        (void *)(Section.ObjAddress + Offset));
+    support::ulittle32_t::ref(Section.Address + Offset) =
+        Placeholder + Value + Addend;
     break;
   }
   case ELF::R_386_PC32: {
     // Get the placeholder value from the generated object since
     // a previous relocation attempt may have overwritten the loaded version
-    uint32_t *Placeholder =
-        reinterpret_cast<uint32_t *>(Section.ObjAddress + Offset);
-    uint32_t *Target = reinterpret_cast<uint32_t *>(Section.Address + Offset);
+    support::ulittle32_t::ref Placeholder(
+        (void *)(Section.ObjAddress + Offset));
     uint32_t FinalAddress = ((Section.LoadAddress + Offset) & 0xFFFFFFFF);
-    uint32_t RealOffset = *Placeholder + Value + Addend - FinalAddress;
-    *Target = RealOffset;
+    uint32_t RealOffset = Placeholder + Value + Addend - FinalAddress;
+    support::ulittle32_t::ref(Section.Address + Offset) = RealOffset;
     break;
   }
   default:
     // There are other relocation types, but it appears these are the
     // only ones currently used by the LLVM ELF object writer
     llvm_unreachable("Relocation type not implemented yet!");
     break;
   }

llvm/trunk/lib/Target/X86/X86JITInfo.cpp

 //===----------------------------------------------------------------------===//
 
 #include "X86JITInfo.h"
 #include "X86Relocations.h"
 #include "X86Subtarget.h"
 #include "X86TargetMachine.h"
 #include "llvm/IR/Function.h"
 #include "llvm/Support/Compiler.h"
+#include "llvm/Support/Endian.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/Valgrind.h"
 #include <cstdlib>
 #include <cstring>
 using namespace llvm;
 
 #define DEBUG_TYPE "jit"
 
 // Determine the platform we're running on
 #if defined (__x86_64__) || defined (_M_AMD64) || defined (_M_X64)
 # define X86_64_JIT
 #elif defined(__i386__) || defined(i386) || defined(_M_IX86)
 # define X86_32_JIT
 #endif
 
+// x86 is little-endian, and we can do unaligned memory accesses.
+template<typename value_type>
+static value_type read_x86(const void *memory) {
+  return support::endian::read<value_type, support::little, 1>(memory);
+}
+
+template<typename value_type>
+static void write_x86(void *memory, value_type value) {
+  support::endian::write<value_type, support::little, 1>(memory, value);
+}
+
 void X86JITInfo::replaceMachineCodeForFunction(void *Old, void *New) {
-  unsigned char *OldByte = (unsigned char *)Old;
-  *OldByte++ = 0xE9;                // Emit JMP opcode.
-  unsigned *OldWord = (unsigned *)OldByte;
+  unsigned char *OldPtr = static_cast<unsigned char*>(Old);
+  write_x86<unsigned char>(OldPtr++, 0xE9); // Emit JMP opcode.
   unsigned NewAddr = (intptr_t)New;
-  unsigned OldAddr = (intptr_t)OldWord;
-  *OldWord = NewAddr - OldAddr - 4; // Emit PC-relative addr of New code.
+  unsigned OldAddr = (intptr_t)OldPtr;
+  write_x86<unsigned>(
+      OldPtr, NewAddr - OldAddr - 4); // Emit PC-relative addr of New code.
 
   // X86 doesn't need to invalidate the processor cache, so just invalidate
   // Valgrind's cache directly.
   sys::ValgrindDiscardTranslations(Old, 5);
 }
 
 
 /// JITCompilerFunction - This contains the address of the JIT function used to
LLVM_LIBRARY_VISIBILITY void LLVMX86CompilationCallback2(intptr_t *StackPtr,
   intptr_t *RetAddrLoc = &StackPtr[1];
   // We are reading raw stack data here. Tell MemorySanitizer that it is
   // sufficiently initialized.
   __msan_unpoison(RetAddrLoc, sizeof(*RetAddrLoc));
   assert(*RetAddrLoc == RetAddr &&
          "Could not find return address on the stack!");
 
   // It's a stub if there is an interrupt marker after the call.
-  bool isStub = ((unsigned char*)RetAddr)[0] == 0xCE;
+  unsigned char *RetAddrPtr = (unsigned char*)RetAddr;
+  bool isStub = read_x86<unsigned char>(RetAddrPtr) == 0xCE;
 
   // The call instruction should have pushed the return value onto the stack...
 #if defined (X86_64_JIT)
-  RetAddr--;     // Backtrack to the reference itself...
+  RetAddrPtr--;  // Backtrack to the reference itself...
 #else
-  RetAddr -= 4;  // Backtrack to the reference itself...
+  RetAddrPtr -= 4;  // Backtrack to the reference itself...
 #endif
 
 #if 0
-  DEBUG(dbgs() << "In callback! Addr=" << (void*)RetAddr
+  DEBUG(dbgs() << "In callback! Addr=" << RetAddrPtr
                << " ESP=" << (void*)StackPtr
                << ": Resolving call to function: "
-               << TheVM->getFunctionReferencedName((void*)RetAddr) << "\n");
+               << TheVM->getFunctionReferencedName(RetAddrPtr) << "\n");
 #endif
 
   // Sanity check to make sure this really is a call instruction.
 #if defined (X86_64_JIT)
-  assert(((unsigned char*)RetAddr)[-2] == 0x41 &&"Not a call instr!");
-  assert(((unsigned char*)RetAddr)[-1] == 0xFF &&"Not a call instr!");
+  assert(read_x86<unsigned char>(RetAddrPtr - 2) == 0x41 &&
+         "Not a call instr!");
+  assert(read_x86<unsigned char>(RetAddrPtr - 1) == 0xFF &&
+         "Not a call instr!");
 #else
-  assert(((unsigned char*)RetAddr)[-1] == 0xE8 &&"Not a call instr!");
+  assert(read_x86<unsigned char>(RetAddrPtr - 1) == 0xE8 &&
+         "Not a call instr!");
 #endif
 
-  intptr_t NewVal = (intptr_t)JITCompilerFunction((void*)RetAddr);
+  intptr_t NewVal = (intptr_t)JITCompilerFunction(RetAddrPtr);
 
   // Rewrite the call target... so that we don't end up here every time we
   // execute the call.
 #if defined (X86_64_JIT)
   assert(isStub &&
          "X86-64 doesn't support rewriting non-stub lazy compilation calls:"
          " the call instruction varies too much.");
 #else
-  *(intptr_t *)RetAddr = (intptr_t)(NewVal-RetAddr-4);
+  write_x86<intptr_t>(RetAddrPtr, NewVal - (intptr_t)RetAddrPtr - 4);
 #endif
 
   if (isStub) {
     // If this is a stub, rewrite the call into an unconditional branch
     // instruction so that two return addresses are not pushed onto the stack
     // when the requested function finally gets called.  This also makes the
     // 0xCE byte (interrupt) dead, so the marker doesn't effect anything.
 #if defined (X86_64_JIT)
     // If the target address is within 32-bit range of the stub, use a
     // PC-relative branch instead of loading the actual address.  (This is
     // considerably shorter than the 64-bit immediate load already there.)
     // We assume here intptr_t is 64 bits.
-    intptr_t diff = NewVal-RetAddr+7;
+    intptr_t diff = NewVal - (intptr_t)RetAddrPtr + 7;
     if (diff >= -2147483648LL && diff <= 2147483647LL) {
-      *(unsigned char*)(RetAddr-0xc) = 0xE9;
-      *(intptr_t *)(RetAddr-0xb) = diff & 0xffffffff;
+      write_x86<unsigned char>(RetAddrPtr - 0xC, 0xE9);
+      write_x86<intptr_t>(RetAddrPtr - 0xB, diff & 0xffffffff);
     } else {
-      *(intptr_t *)(RetAddr - 0xa) = NewVal;
-      ((unsigned char*)RetAddr)[0] = (2 | (4 << 3) | (3 << 6));
+      write_x86<intptr_t>(RetAddrPtr - 0xA, NewVal);
+      write_x86<unsigned char>(RetAddrPtr, (2 | (4 << 3) | (3 << 6)));
     }
-    sys::ValgrindDiscardTranslations((void*)(RetAddr-0xc), 0xd);
+    sys::ValgrindDiscardTranslations(RetAddrPtr - 0xC, 0xd);
 #else
-    ((unsigned char*)RetAddr)[-1] = 0xE9;
-    sys::ValgrindDiscardTranslations((void*)(RetAddr-1), 5);
+    write_x86<unsigned char>(RetAddrPtr - 1, 0xE9);
+    sys::ValgrindDiscardTranslations(RetAddrPtr - 1, 5);
 #endif
   }
 
   // Change the return address to reexecute the call instruction...
 #if defined (X86_64_JIT)
   *RetAddrLoc -= 0xd;
 #else
   *RetAddrLoc -= 5;