Page MenuHomePhabricator

Make RuntimeDyld::MemoryManager responsible for tracking EH frames.

Authored by lhames on May 3 2017, 2:13 PM.



RuntimeDyld is currently responsible for tracking allocated EH frames, but it makes more sense to have the RuntimeDyld::MemoryManager track them (since the frames are allocated through the memory manager, and written to memory owned by the memory manager). This patch moves the frame tracking into RTDyldMemoryManager, and changes the deregisterFrames method on RuntimeDyld::MemoryManager from:

void deregisterEHFrames(uint8_t *Addr, uint64_t LoadAddr, size_t Size);

to just

void deregisterEHFrames();

Separating this responsibility will allow ORC to continue to throw the RuntimeDyld instances away post-link (saving a few dozen bytes per lazy function) while properly deregistering frames when modules are unloaded.

This patch also updates ORC to call deregisterEHFrames when modules are unloaded. This fixes a bug where an exception that tears down the JIT can then unwind through dangling EH frames that have been deallocated but not deregistered, resulting in UB.

For people using SectionMemoryManager this should be pretty much a no-op. For people with custom allocators that override registerEHFrames/deregisterEHFrames, you will now be responsible for tracking allocated EH frames.

Diff Detail


Event Timeline

lhames created this revision.May 3 2017, 2:13 PM
lhames added a comment.May 3 2017, 2:20 PM

The bug that motivated this change is .

chfast added a subscriber: chfast.May 8 2017, 1:26 PM
chfast added inline comments.

How about emplace_back(LoadAddr, static_cast<uint32_t>(Size))?
BTW, why cast to uint32_t?


I'm not sure it will it more clear, but you write UnfinalizedEHFrames = {}.


Can you use size_t for Size? That might avoid the previous static_cast.


And there we have uint64_t? Can it be size_t as well?



lhames added a comment.May 8 2017, 2:08 PM

Will make the suggested changes and upload a new patch.


Yep - emplace_back will work.

The cast is there to suppress the implicit conversion warning. The situation with the Size argument is a bit weird: The method declares Size to be a size_t, but we want this to work even when we're cross-JITing from 32 to 64-bit, so we should probably fix it to uint64_t. I think I'll leave this as is for now and update the interface in a subsequent patch.


Oh - that's much neater.


Oddly enough (after the discussion above), this specific size member could be a size_t, since RTDyldMemoryManager's default implementation for register/deregister is local-only. (That's kind of broken in its own way, but that's an issue for another patch).



lhames added a comment.May 8 2017, 2:59 PM

Actually those push_backs should stay push_backs: To use emplace the structs would have to have non-default constructors, which seems like more effort than its worth for the conceptual neatness of emplace.

lhames updated this revision to Diff 98251.May 8 2017, 9:54 PM

Switch EHFrames::Size to size_t, discard now-redundant static_cast, use UnfinalizedEHFrames = {}.

chfast added inline comments.May 9 2017, 6:18 AM

The Size argument of type size_t is used here, but EHFrame struct expects uint64_t. Can we clean it up?


It's hard to see from the diff why this is declared twice.

lhames added inline comments.May 9 2017, 10:33 AM

As mentioned in the previous comment the correct size for the EHFrame struct is uint64_t. The interface should also change from size_t to uint64_t, but I think that warrants a separate patch.


You mean in OrcRemoteTargetClient.h and RTDyldMemoryManager.h? They're doing slightly different things. For backwards compatibility RTDyldMemoryManager is registering frames in the current process only. The RemoteClient version is dedicated to registering frames on a remote. That's why one uses JITTargetAddress/uint64_t and the other can get away with a pointer/size_t.

chfast accepted this revision.May 9 2017, 10:35 AM
This revision is now accepted and ready to land.May 9 2017, 10:35 AM
lhames closed this revision.May 9 2017, 2:46 PM

Thanks! Committed in r302589.