Page MenuHomePhabricator

Fix unaligned reads/writes in X86JIT and RuntimeDyldELF.
ClosedPublic

Authored by samsonov on Aug 21 2014, 1:07 PM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

samsonov updated this revision to Diff 12801.Aug 21 2014, 1:07 PM
samsonov retitled this revision from to Switch to support::endian::read/write in X86-specific JIT and RuntimeDyld..
samsonov updated this object.
samsonov edited the test plan for this revision. (Show Details)
samsonov added a reviewer: lhames.
samsonov added a subscriber: Unknown Object (MLST).
ributzka added inline comments.
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 added inline comments.Aug 21 2014, 2:43 PM
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 added inline comments.Aug 21 2014, 3:47 PM
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 updated this revision to Diff 12821.Aug 21 2014, 5:54 PM

Follow Juergen's suggestion.

lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.cpp
264–265 ↗(On Diff #12801)

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

Any more comments on this?

lhames added inline comments.Aug 25 2014, 11:39 AM
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 edited edge metadata.Aug 25 2014, 11:39 AM

Hi Alexey,

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

  • Lang.
samsonov updated this revision to Diff 12995.Aug 27 2014, 11:57 AM
samsonov edited edge metadata.

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

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 retitled this revision from Switch to support::endian::read/write in X86-specific JIT and RuntimeDyld. to Fix unaligned reads/writes in X86JIT and RuntimeDyldELF..Aug 27 2014, 4:14 PM
samsonov updated this object.
samsonov closed this revision.Aug 27 2014, 4:15 PM
samsonov updated this revision to Diff 13013.

Closed by commit rL216631 (authored by @samsonov).