[PATCH] Switch to support::endian::read/write in X86-specific JIT and RuntimeDyld.

[Lang Hames]

================
Comment at: lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.cpp:307
@@ -303,1 +306,3 @@
+    auto Target =
+        reinterpret_cast<support::ulittle32_t *>(Section.Address + Offset);
     uint64_t FinalAddress = Section.LoadAddress + Offset;
----------------
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.

================
Comment at: lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.cpp:264-265
@@ -262,4 +263,4 @@
   case ELF::R_X86_64_64: {
-    uint64_t *Target = reinterpret_cast<uint64_t *>(Section.Address + Offset);
-    *Target = Value + Addend;
+    void *Target = Section.Address + Offset;
+    support::endian::write<uint64_t, support::little, 1>(Target, Value + Addend);
     DEBUG(dbgs() << "Writing " << format("%p", (Value + Addend)) << " at "
----------------
samsonov wrote:
> ributzka wrote:
> > What about using something like this instead?
> > auto *Target = reinterpret_cast<support::ulittle64_t *>(Section.Address + Offset);
> > *Target = Value + Addend;
> It would still be an unaligned write of ulittle_64_t, which is UB.
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> ;

================
Comment at: lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.cpp:264-265
@@ -262,4 +263,4 @@
   case ELF::R_X86_64_64: {
-    uint64_t *Target = reinterpret_cast<uint64_t *>(Section.Address + Offset);
-    *Target = Value + Addend;
+    void *Target = Section.Address + Offset;
+    support::endian::write<uint64_t, support::little, 1>(Target, Value + Addend);
     DEBUG(dbgs() << "Writing " << format("%p", (Value + Addend)) << " at "
----------------
ributzka wrote:
> lhames wrote:
> > samsonov wrote:
> > > ributzka wrote:
> > > > What about using something like this instead?
> > > > auto *Target = reinterpret_cast<support::ulittle64_t *>(Section.Address + Offset);
> > > > *Target = Value + Addend;
> > > It would still be an unaligned write of ulittle_64_t, which is UB.
> > 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> ;
> operator= is overloaded for support::ulittle64_t and should call support::endian::write<uint64_t, support::little, 1> for you.
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.

http://reviews.llvm.org/D5011