From ab3dba86f951a1bdfe01d3102e2850e607af791a Mon Sep 17 00:00:00 2001 From: Lang Hames Date: Tue, 9 May 2017 21:32:18 +0000 Subject: [PATCH] [ExecutionEngine] Make RuntimeDyld::MemoryManager responsible for tracking EH frames. RuntimeDyld was previously 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: 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. Reviewed in https://reviews.llvm.org/D32829 git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@302589 91177308-0d34-0410-b5e6-96231b3b80d8 --- .../Orc/CompileOnDemandLayer.h | 13 +++++++++ .../Orc/OrcRemoteTargetClient.h | 29 ++++++++++++------- .../Orc/RTDyldObjectLinkingLayer.h | 4 +++ .../ExecutionEngine/RTDyldMemoryManager.h | 16 +++++----- include/llvm/ExecutionEngine/RuntimeDyld.h | 3 +- lib/ExecutionEngine/Orc/OrcMCJITReplacement.h | 5 ++-- .../RuntimeDyld/RTDyldMemoryManager.cpp | 12 ++++++++ .../RuntimeDyld/RuntimeDyld.cpp | 4 ++- .../RuntimeDyld/RuntimeDyldELF.cpp | 12 -------- .../RuntimeDyld/RuntimeDyldELF.h | 2 -- .../RuntimeDyld/RuntimeDyldImpl.h | 2 +- .../RuntimeDyld/Targets/RuntimeDyldCOFFI386.h | 1 - .../Targets/RuntimeDyldCOFFThumb.h | 1 - .../Targets/RuntimeDyldCOFFX86_64.h | 3 -- tools/lli/RemoteJITUtils.h | 5 ++-- tools/llvm-rtdyld/llvm-rtdyld.cpp | 3 +- .../Orc/ObjectTransformLayerTest.cpp | 2 +- .../Orc/RTDyldObjectLinkingLayerTest.cpp | 29 ++++++++++--------- 18 files changed, 83 insertions(+), 63 deletions(-) diff --git a/include/llvm/ExecutionEngine/Orc/CompileOnDemandLayer.h b/include/llvm/ExecutionEngine/Orc/CompileOnDemandLayer.h index 7e7f7358938..1bb911d09cf 100644 --- a/include/llvm/ExecutionEngine/Orc/CompileOnDemandLayer.h +++ b/include/llvm/ExecutionEngine/Orc/CompileOnDemandLayer.h @@ -172,6 +172,11 @@ private: return nullptr; } + void removeModulesFromBaseLayer(BaseLayerT &BaseLayer) { + for (auto &BLH : BaseLayerHandles) + BaseLayer.removeModuleSet(BLH); + } + std::unique_ptr ExternalSymbolResolver; std::unique_ptr> MemMgr; std::unique_ptr StubsMgr; @@ -204,6 +209,11 @@ public: CreateIndirectStubsManager(std::move(CreateIndirectStubsManager)), CloneStubsIntoPartitions(CloneStubsIntoPartitions) {} + ~CompileOnDemandLayer() { + while (!LogicalDylibs.empty()) + removeModuleSet(LogicalDylibs.begin()); + } + /// @brief Add a module to the compile-on-demand layer. template @@ -239,6 +249,7 @@ public: /// This will remove all modules in the layers below that were derived from /// the module represented by H. void removeModuleSet(ModuleSetHandleT H) { + H->removeModulesFromBaseLayer(BaseLayer); LogicalDylibs.erase(H); } @@ -478,6 +489,8 @@ private: return 0; } + LD.BaseLayerHandles.push_back(PartH); + return CalledAddr; } diff --git a/include/llvm/ExecutionEngine/Orc/OrcRemoteTargetClient.h b/include/llvm/ExecutionEngine/Orc/OrcRemoteTargetClient.h index 02f59d6a831..a19c30631c5 100644 --- a/include/llvm/ExecutionEngine/Orc/OrcRemoteTargetClient.h +++ b/include/llvm/ExecutionEngine/Orc/OrcRemoteTargetClient.h @@ -144,16 +144,16 @@ public: void registerEHFrames(uint8_t *Addr, uint64_t LoadAddr, size_t Size) override { - UnfinalizedEHFrames.push_back( - std::make_pair(LoadAddr, static_cast(Size))); + UnfinalizedEHFrames.push_back({LoadAddr, Size}); } - void deregisterEHFrames(uint8_t *Addr, uint64_t LoadAddr, - size_t Size) override { - auto Err = Client.deregisterEHFrames(LoadAddr, Size); - // FIXME: Add error poll. - assert(!Err && "Failed to register remote EH frames."); - (void)Err; + void deregisterEHFrames() override { + for (auto &Frame : RegisteredEHFrames) { + auto Err = Client.deregisterEHFrames(Frame.Addr, Frame.Size); + // FIXME: Add error poll. + assert(!Err && "Failed to register remote EH frames."); + (void)Err; + } } void notifyObjectLoaded(RuntimeDyld &Dyld, @@ -320,7 +320,7 @@ public: Unfinalized.clear(); for (auto &EHFrame : UnfinalizedEHFrames) { - if (auto Err = Client.registerEHFrames(EHFrame.first, EHFrame.second)) { + if (auto Err = Client.registerEHFrames(EHFrame.Addr, EHFrame.Size)) { // FIXME: Replace this once finalizeMemory can return an Error. handleAllErrors(std::move(Err), [&](ErrorInfoBase &EIB) { if (ErrMsg) { @@ -331,7 +331,8 @@ public: return false; } } - UnfinalizedEHFrames.clear(); + RegisteredEHFrames = std::move(UnfinalizedEHFrames); + UnfinalizedEHFrames = {}; return false; } @@ -387,7 +388,13 @@ public: ResourceIdMgr::ResourceId Id; std::vector Unmapped; std::vector Unfinalized; - std::vector> UnfinalizedEHFrames; + + struct EHFrame { + JITTargetAddress Addr; + uint64_t Size; + }; + std::vector UnfinalizedEHFrames; + std::vector RegisteredEHFrames; }; /// Remote indirect stubs manager. diff --git a/include/llvm/ExecutionEngine/Orc/RTDyldObjectLinkingLayer.h b/include/llvm/ExecutionEngine/Orc/RTDyldObjectLinkingLayer.h index babcc7f26aa..5b3426afe58 100644 --- a/include/llvm/ExecutionEngine/Orc/RTDyldObjectLinkingLayer.h +++ b/include/llvm/ExecutionEngine/Orc/RTDyldObjectLinkingLayer.h @@ -120,6 +120,10 @@ private: buildInitialSymbolTable(PFC->Objects); } + ~ConcreteLinkedObjectSet() override { + MemMgr->deregisterEHFrames(); + } + void setHandle(ObjSetHandleT H) { PFC->Handle = H; } diff --git a/include/llvm/ExecutionEngine/RTDyldMemoryManager.h b/include/llvm/ExecutionEngine/RTDyldMemoryManager.h index 5638717790b..74535fe948f 100644 --- a/include/llvm/ExecutionEngine/RTDyldMemoryManager.h +++ b/include/llvm/ExecutionEngine/RTDyldMemoryManager.h @@ -69,13 +69,8 @@ public: /// Deregister EH frames in the current proces. static void deregisterEHFramesInProcess(uint8_t *Addr, size_t Size); - void registerEHFrames(uint8_t *Addr, uint64_t LoadAddr, size_t Size) override { - registerEHFramesInProcess(Addr, Size); - } - - void deregisterEHFrames(uint8_t *Addr, uint64_t LoadAddr, size_t Size) override { - deregisterEHFramesInProcess(Addr, Size); - } + void registerEHFrames(uint8_t *Addr, uint64_t LoadAddr, size_t Size) override; + void deregisterEHFrames() override; /// This method returns the address of the specified function or variable in /// the current process. @@ -139,6 +134,13 @@ public: /// MCJIT or RuntimeDyld. Use getSymbolAddress instead. virtual void *getPointerToNamedFunction(const std::string &Name, bool AbortOnFailure = true); + +private: + struct EHFrame { + uint8_t *Addr; + size_t Size; + }; + std::vector EHFrames; }; // Create wrappers for C Binding types (see CBindingWrapping.h). diff --git a/include/llvm/ExecutionEngine/RuntimeDyld.h b/include/llvm/ExecutionEngine/RuntimeDyld.h index 13a5f9922c5..9470866dc0d 100644 --- a/include/llvm/ExecutionEngine/RuntimeDyld.h +++ b/include/llvm/ExecutionEngine/RuntimeDyld.h @@ -150,8 +150,7 @@ public: /// be the case for local execution) these two values will be the same. virtual void registerEHFrames(uint8_t *Addr, uint64_t LoadAddr, size_t Size) = 0; - virtual void deregisterEHFrames(uint8_t *addr, uint64_t LoadAddr, - size_t Size) = 0; + virtual void deregisterEHFrames() = 0; /// This method is called when object loading is complete and section page /// permissions can be applied. It is up to the memory manager implementation diff --git a/lib/ExecutionEngine/Orc/OrcMCJITReplacement.h b/lib/ExecutionEngine/Orc/OrcMCJITReplacement.h index a5100a56bcf..a27573f93b9 100644 --- a/lib/ExecutionEngine/Orc/OrcMCJITReplacement.h +++ b/lib/ExecutionEngine/Orc/OrcMCJITReplacement.h @@ -94,9 +94,8 @@ class OrcMCJITReplacement : public ExecutionEngine { return ClientMM->registerEHFrames(Addr, LoadAddr, Size); } - void deregisterEHFrames(uint8_t *Addr, uint64_t LoadAddr, - size_t Size) override { - return ClientMM->deregisterEHFrames(Addr, LoadAddr, Size); + void deregisterEHFrames() override { + return ClientMM->deregisterEHFrames(); } void notifyObjectLoaded(RuntimeDyld &RTDyld, diff --git a/lib/ExecutionEngine/RuntimeDyld/RTDyldMemoryManager.cpp b/lib/ExecutionEngine/RuntimeDyld/RTDyldMemoryManager.cpp index de73fbde8eb..99e84b7496d 100644 --- a/lib/ExecutionEngine/RuntimeDyld/RTDyldMemoryManager.cpp +++ b/lib/ExecutionEngine/RuntimeDyld/RTDyldMemoryManager.cpp @@ -134,6 +134,18 @@ void RTDyldMemoryManager::deregisterEHFramesInProcess(uint8_t *Addr, #endif +void RTDyldMemoryManager::registerEHFrames(uint8_t *Addr, uint64_t LoadAddr, + size_t Size) { + registerEHFramesInProcess(Addr, Size); + EHFrames.push_back({Addr, Size}); +} + +void RTDyldMemoryManager::deregisterEHFrames() { + for (auto &Frame : EHFrames) + deregisterEHFramesInProcess(Frame.Addr, Frame.Size); + EHFrames.clear(); +} + static int jit_noop() { return 0; } diff --git a/lib/ExecutionEngine/RuntimeDyld/RuntimeDyld.cpp b/lib/ExecutionEngine/RuntimeDyld/RuntimeDyld.cpp index df9d2ceba32..e9a4b71c903 100644 --- a/lib/ExecutionEngine/RuntimeDyld/RuntimeDyld.cpp +++ b/lib/ExecutionEngine/RuntimeDyld/RuntimeDyld.cpp @@ -73,7 +73,9 @@ namespace llvm { void RuntimeDyldImpl::registerEHFrames() {} -void RuntimeDyldImpl::deregisterEHFrames() {} +void RuntimeDyldImpl::deregisterEHFrames() { + MemMgr.deregisterEHFrames(); +} #ifndef NDEBUG static void dumpSectionMemory(const SectionEntry &S, StringRef State) { diff --git a/lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.cpp b/lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.cpp index 2725d8ad0e7..660843765b3 100644 --- a/lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.cpp +++ b/lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.cpp @@ -221,22 +221,10 @@ void RuntimeDyldELF::registerEHFrames() { uint64_t EHFrameLoadAddr = Sections[EHFrameSID].getLoadAddress(); size_t EHFrameSize = Sections[EHFrameSID].getSize(); MemMgr.registerEHFrames(EHFrameAddr, EHFrameLoadAddr, EHFrameSize); - RegisteredEHFrameSections.push_back(EHFrameSID); } UnregisteredEHFrameSections.clear(); } -void RuntimeDyldELF::deregisterEHFrames() { - for (int i = 0, e = RegisteredEHFrameSections.size(); i != e; ++i) { - SID EHFrameSID = RegisteredEHFrameSections[i]; - uint8_t *EHFrameAddr = Sections[EHFrameSID].getAddress(); - uint64_t EHFrameLoadAddr = Sections[EHFrameSID].getLoadAddress(); - size_t EHFrameSize = Sections[EHFrameSID].getSize(); - MemMgr.deregisterEHFrames(EHFrameAddr, EHFrameLoadAddr, EHFrameSize); - } - RegisteredEHFrameSections.clear(); -} - std::unique_ptr llvm::RuntimeDyldELF::create(Triple::ArchType Arch, RuntimeDyld::MemoryManager &MemMgr, diff --git a/lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.h b/lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.h index 84dd810101f..fb5da6dd8bb 100644 --- a/lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.h +++ b/lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.h @@ -152,7 +152,6 @@ private: // in a table until we receive a request to register all unregistered // EH frame sections with the memory manager. SmallVector UnregisteredEHFrameSections; - SmallVector RegisteredEHFrameSections; // Map between GOT relocation value and corresponding GOT offset std::map GOTOffsetMap; @@ -180,7 +179,6 @@ public: StubMap &Stubs) override; bool isCompatibleFile(const object::ObjectFile &Obj) const override; void registerEHFrames() override; - void deregisterEHFrames() override; Error finalizeLoad(const ObjectFile &Obj, ObjSectionToIDMap &SectionMap) override; }; diff --git a/lib/ExecutionEngine/RuntimeDyld/RuntimeDyldImpl.h b/lib/ExecutionEngine/RuntimeDyld/RuntimeDyldImpl.h index f5cc883d98f..18c23c5a2a5 100644 --- a/lib/ExecutionEngine/RuntimeDyld/RuntimeDyldImpl.h +++ b/lib/ExecutionEngine/RuntimeDyld/RuntimeDyldImpl.h @@ -515,7 +515,7 @@ public: virtual void registerEHFrames(); - virtual void deregisterEHFrames(); + void deregisterEHFrames(); virtual Error finalizeLoad(const ObjectFile &ObjImg, ObjSectionToIDMap &SectionMap) { diff --git a/lib/ExecutionEngine/RuntimeDyld/Targets/RuntimeDyldCOFFI386.h b/lib/ExecutionEngine/RuntimeDyld/Targets/RuntimeDyldCOFFI386.h index 0398413e153..6aa1a2bdb92 100644 --- a/lib/ExecutionEngine/RuntimeDyld/Targets/RuntimeDyldCOFFI386.h +++ b/lib/ExecutionEngine/RuntimeDyld/Targets/RuntimeDyldCOFFI386.h @@ -217,7 +217,6 @@ public: } void registerEHFrames() override {} - void deregisterEHFrames() override {} }; } diff --git a/lib/ExecutionEngine/RuntimeDyld/Targets/RuntimeDyldCOFFThumb.h b/lib/ExecutionEngine/RuntimeDyld/Targets/RuntimeDyldCOFFThumb.h index 8c6af0bd9c6..318afa21a88 100644 --- a/lib/ExecutionEngine/RuntimeDyld/Targets/RuntimeDyldCOFFThumb.h +++ b/lib/ExecutionEngine/RuntimeDyld/Targets/RuntimeDyldCOFFThumb.h @@ -316,7 +316,6 @@ public: } void registerEHFrames() override {} - void deregisterEHFrames() override {} }; } diff --git a/lib/ExecutionEngine/RuntimeDyld/Targets/RuntimeDyldCOFFX86_64.h b/lib/ExecutionEngine/RuntimeDyld/Targets/RuntimeDyldCOFFX86_64.h index 109beb36f1e..26e73989d7e 100644 --- a/lib/ExecutionEngine/RuntimeDyld/Targets/RuntimeDyldCOFFX86_64.h +++ b/lib/ExecutionEngine/RuntimeDyld/Targets/RuntimeDyldCOFFX86_64.h @@ -194,9 +194,6 @@ public: } UnregisteredEHFrameSections.clear(); } - void deregisterEHFrames() override { - // Stub - } Error finalizeLoad(const ObjectFile &Obj, ObjSectionToIDMap &SectionMap) override { // Look for and record the EH frame section IDs. diff --git a/tools/lli/RemoteJITUtils.h b/tools/lli/RemoteJITUtils.h index 89a51420256..3c82f73ff07 100644 --- a/tools/lli/RemoteJITUtils.h +++ b/tools/lli/RemoteJITUtils.h @@ -118,9 +118,8 @@ public: MemMgr->registerEHFrames(Addr, LoadAddr, Size); } - void deregisterEHFrames(uint8_t *Addr, uint64_t LoadAddr, - size_t Size) override { - MemMgr->deregisterEHFrames(Addr, LoadAddr, Size); + void deregisterEHFrames() override { + MemMgr->deregisterEHFrames(); } bool finalizeMemory(std::string *ErrMsg = nullptr) override { diff --git a/tools/llvm-rtdyld/llvm-rtdyld.cpp b/tools/llvm-rtdyld/llvm-rtdyld.cpp index 75345de5028..ba130ce80be 100644 --- a/tools/llvm-rtdyld/llvm-rtdyld.cpp +++ b/tools/llvm-rtdyld/llvm-rtdyld.cpp @@ -175,8 +175,7 @@ public: void registerEHFrames(uint8_t *Addr, uint64_t LoadAddr, size_t Size) override {} - void deregisterEHFrames(uint8_t *Addr, uint64_t LoadAddr, - size_t Size) override {} + void deregisterEHFrames() override {} void preallocateSlab(uint64_t Size) { std::string Err; diff --git a/unittests/ExecutionEngine/Orc/ObjectTransformLayerTest.cpp b/unittests/ExecutionEngine/Orc/ObjectTransformLayerTest.cpp index 96214a368dc..362c143c54e 100644 --- a/unittests/ExecutionEngine/Orc/ObjectTransformLayerTest.cpp +++ b/unittests/ExecutionEngine/Orc/ObjectTransformLayerTest.cpp @@ -304,7 +304,7 @@ TEST(ObjectTransformLayerTest, Main) { return nullptr; } void registerEHFrames(uint8_t *, uint64_t, size_t) override {} - void deregisterEHFrames(uint8_t *, uint64_t, size_t) override {} + void deregisterEHFrames() override {} bool finalizeMemory(std::string *) override { return false; } }; diff --git a/unittests/ExecutionEngine/Orc/RTDyldObjectLinkingLayerTest.cpp b/unittests/ExecutionEngine/Orc/RTDyldObjectLinkingLayerTest.cpp index de99c022fb9..c13a75a5cbf 100644 --- a/unittests/ExecutionEngine/Orc/RTDyldObjectLinkingLayerTest.cpp +++ b/unittests/ExecutionEngine/Orc/RTDyldObjectLinkingLayerTest.cpp @@ -90,7 +90,8 @@ TEST(RTDyldObjectLinkingLayerTest, TestSetProcessAllSections) { Objs.push_back(OwningObj.getBinary()); bool DebugSectionSeen = false; - SectionMemoryManagerWrapper SMMW(DebugSectionSeen); + auto SMMW = + std::make_shared(DebugSectionSeen); auto Resolver = createLambdaResolver( [](const std::string &Name) { @@ -102,7 +103,7 @@ TEST(RTDyldObjectLinkingLayerTest, TestSetProcessAllSections) { { // Test with ProcessAllSections = false (the default). - auto H = ObjLayer.addObjectSet(Objs, &SMMW, &*Resolver); + auto H = ObjLayer.addObjectSet(Objs, SMMW, &*Resolver); ObjLayer.emitAndFinalize(H); EXPECT_EQ(DebugSectionSeen, false) << "Unexpected debug info section"; @@ -112,7 +113,7 @@ TEST(RTDyldObjectLinkingLayerTest, TestSetProcessAllSections) { { // Test with ProcessAllSections = true. ObjLayer.setProcessAllSections(true); - auto H = ObjLayer.addObjectSet(Objs, &SMMW, &*Resolver); + auto H = ObjLayer.addObjectSet(Objs, SMMW, &*Resolver); ObjLayer.emitAndFinalize(H); EXPECT_EQ(DebugSectionSeen, true) << "Expected debug info section not seen"; @@ -178,14 +179,15 @@ TEST_F(RTDyldObjectLinkingLayerExecutionTest, NoDuplicateFinalization) { return JITSymbol(nullptr); }); - SectionMemoryManagerWrapper SMMW; - ObjLayer.addObjectSet(std::move(Obj1Set), &SMMW, &*Resolver); - auto H = ObjLayer.addObjectSet(std::move(Obj2Set), &SMMW, &*Resolver); + auto SMMW = std::make_shared(); + ObjLayer.addObjectSet(std::move(Obj1Set), SMMW, &*Resolver); + auto H = ObjLayer.addObjectSet(std::move(Obj2Set), SMMW, &*Resolver); ObjLayer.emitAndFinalize(H); - + ObjLayer.removeObjectSet(H); + // Finalization of module 2 should trigger finalization of module 1. // Verify that finalize on SMMW is only called once. - EXPECT_EQ(SMMW.FinalizationCount, 1) + EXPECT_EQ(SMMW->FinalizationCount, 1) << "Extra call to finalize"; } @@ -238,14 +240,15 @@ TEST_F(RTDyldObjectLinkingLayerExecutionTest, NoPrematureAllocation) { std::vector Obj2Set; Obj2Set.push_back(Obj2.getBinary()); - SectionMemoryManagerWrapper SMMW; + auto SMMW = std::make_shared(); NullResolver NR; - auto H = ObjLayer.addObjectSet(std::move(Obj1Set), &SMMW, &NR); - ObjLayer.addObjectSet(std::move(Obj2Set), &SMMW, &NR); + auto H = ObjLayer.addObjectSet(std::move(Obj1Set), SMMW, &NR); + ObjLayer.addObjectSet(std::move(Obj2Set), SMMW, &NR); ObjLayer.emitAndFinalize(H); - + ObjLayer.removeObjectSet(H); + // Only one call to needsToReserveAllocationSpace should have been made. - EXPECT_EQ(SMMW.NeedsToReserveAllocationSpaceCount, 1) + EXPECT_EQ(SMMW->NeedsToReserveAllocationSpaceCount, 1) << "More than one call to needsToReserveAllocationSpace " "(multiple unrelated objects loaded prior to finalization)"; } -- 2.50.1