[llvm] r257263 - [Orc][RuntimeDyld] Prevent duplicate calls to finalizeMemory on shared memory
David Blaikie via llvm-commits
llvm-commits at lists.llvm.org
Tue Jan 19 16:03:05 PST 2016
On Sat, Jan 16, 2016 at 1:12 PM, Lang Hames <lhames at gmail.com> wrote:
> Hi Dave,
>
> > Can, but don't have to?
>
> Yep. It's a matter of symbol dependence: If R1 contains a relocation to a
> symbol defined in R2, then relocating R1 requires relocating R2. If there
> is no symbol dependence, then there will be no recursive relocate calls.
>
> > What happens if one RuntimeDyld hasn't finalized its memory, but another
> that shares the same MM has? Could the first one end up trying to use its
> MM in a non-finalized when it had already been finalised?
>
> Ahh - I see your concern now. You've identified a new bug in this scheme.
> In practice this won't come up often because higher layers try to avoid
> pushing objects down (or compiling them at all) until they're needed, so
> you usually only have RuntimeDyld instances for the code you're asking for,
> plus any dependent code that's needed. The interface shouldn't require that
> though: You should be able to add unrelated objects with the same memory
> manager and not get premature finalisation.
>
> > To take your same scenario but instead of claiming the right to
> finalize, each RuntimeDyld instance gets what appears to be a separate
> MemoryManager, yet they are all reference-counting-finalizing decorators
> around the two real memory managers M1 and M2.
>
> Where does the ref count live? The user may pass the memory manager in in
> more than one place. E.g.:
>
> ObjectLayer.addObjectSet(JITStdLib, M1, NullResolver);
> CompileLayer.addModuleSet(UserCode, M1, CustomResolver);
>
> And I don't want to count on them doing the wrapping. If you add the
> ref-count to the MemoryManager itself it doesn't look much different to
> what I've got.
>
Slightly different in that MemoryManager wouldn't befriend RuntimeDyld,
which would probably be good from a design perspective (less
interdependence, etc).
One variant/detail I was thinking of would've been to have Orc layers take
MemoryManager by unique_ptr (so take ownership, rather than just an
external reference) which somewhat more accurately models the relationship
- given the side effects of finalizing memory, etc, sharing memory managers
seems a bit brittle. Then you could have a ref-counting MemoryManager that
had an "operator unique_ptr<MemoryManager>" so when passed down to a layer,
it would create the ref counter handle (& the original
RefCountingMemoryManager would own the ref count and decorate an existing,
plain, MemoryManager).
> I think the right way forward is to take my existing scheme and adapt the
> ObjectLinkingLayer so that it doesn't even create a RuntimeDyld instance
> until an object is actually needed. That way the only memory allocated by
> the memory manager will be the memory that needs to be finalised before we
> return from a symbol lookup. I'll see if I can work that up.
>
Thinking about it more, I think both our schemes are a bit brittle - they
don't really enshrine the contract well (these bugs demonstrate that) and
are still working around it. Having "finalize" not actually finalize seems
to demonstrate that there's something not-quite-right in the API.
Would it make sense to move MemoryManager finalization up the stack to
somewhere else? Since we've already established that there's no possibility
of putting stuff into a MemoryManager before you need it (otherwise it
might be prematurely finalized in your scheme, or hold finalization hostage
in my scheme) - what if finalization happened up at whatever layer was
about to return outside the scope of the MemoryManager in question? Does
any of this make sense? If not I can look more closely at the code/try to
work up a prototype.
But essentially what I'm imagining is something like this: A layer
guarantees to allocate all the memory in the MemoryManager you give it, and
it's up to you to finalize that before you go to use the memory. Maybe that
doesn't work with lazy things that never return that far (up to where the
MemoryManager is owned)? Not sure...
*ponders*
>
> - Lang.
>
> On Sat, Jan 16, 2016 at 11:45 AM, David Blaikie <dblaikie at gmail.com>
> wrote:
>
>>
>>
>> On Sat, Jan 16, 2016 at 11:25 AM, Lang Hames <lhames at gmail.com> wrote:
>>
>>> I can't see how a decorator would work here unfortunately.
>>>
>>> The issue is this: We want to finalise the memory before returning to
>>> the user, so that they can use whatever pointer is returned. However, we
>>> can't finalise memory until all RuntimeDyld instances that are sharing a
>>> given memory manager have applied their relocations, and relocation of one
>>> RuntimeDyld instance can trigger the relocation of another (and
>>> transitively another, and so on).
>>>
>>
>> Can, but don't have to?
>>
>> What happens if one RuntimeDyld hasn't finalized its memory, but another
>> that shares the same MM has? Could the first one end up trying to use its
>> MM in a non-finalized when it had already been finalized?
>>
>>
>>> Taking a concrete example: Imagine that you had five RuntimeDyld
>>> instances: R1..R5, and two memory managers: M1 and M2. R1, R2, and R4 use
>>> M1. R3 and R5 use M2. Each RuntimeDyld is chained such that relocating it
>>> will trigger relocation of the next one. When you relocate R1 you have the
>>> following situation:
>>>
>>> R1 -> R2 -> R3 -> R4 -> R5
>>> ^ ^
>>> | \ M2 should be finalised once we return here.
>>> \ M1 should be finalised when we return here.
>>>
>>> The scheme that this patch adopts is that RuntimeDyld instance checks
>>> its associated memory manager to see whether anyone else has claimed
>>> responsibility for finalising it yet. If nobody has, this instance claims
>>> responsibility and sets the flag. So before relocating, R1 will check M1
>>> and find that nobody else has claimed responsibility for finalising it yet,
>>> so R1 will claim that responsibility. R2 then checks M1 and finds that
>>> someone else has already claimed responsibility, so it does nothing. R3
>>> checks M2, finds that nobody has claimed responsibility for finalising it
>>> yet and so takes responsibility for that. R4 and R5 check M1 and M2
>>> respectively, both finding that responsibility has already been claimed, so
>>> they do nothing.
>>>
>>> Hope that makes sense?
>>>
>>
>> I think it does, but I'm still thinking a ref counted finalization using
>> decorators may still apply here.
>>
>> To take your same scenario but instead of claiming the right to finalize,
>> each RuntimeDyld instance gets what appears to be a separate MemoryManager,
>> yet they are all reference-counting-finalizing decorators around the two
>> real memory managers M1 and M2.
>>
>> When R5 finalizes its MM (a wrapper around M2) the ref count is decreased
>> (or increased, whichever way you look at it/implement it) by one, but
>> that's still not at zero (because there were two wrappers created, one for
>> R5 and one for R3) so it doesn't actually do anything (as is the case in
>> your current implementation). Then when R3 finalizes its MM (the other
>> wrapper around M2) the ref count is decremented again, dropping to zero and
>> causing the actual finalization to occur.
>>
>> Similarly for 4 (refcount 3->2), 2 (refcount 2->1), and 1 (refcount 1->0,
>> triggering finalization).
>>
>>
>>>
>>> - Lang.
>>>
>>>
>>> On Thu, Jan 14, 2016 at 3:25 PM, David Blaikie <dblaikie at gmail.com>
>>> wrote:
>>>
>>>>
>>>>
>>>> On Sat, Jan 9, 2016 at 11:50 AM, Lang Hames via llvm-commits <
>>>> llvm-commits at lists.llvm.org> wrote:
>>>>
>>>>> Author: lhames
>>>>> Date: Sat Jan 9 13:50:40 2016
>>>>> New Revision: 257263
>>>>>
>>>>> URL: http://llvm.org/viewvc/llvm-project?rev=257263&view=rev
>>>>> Log:
>>>>> [Orc][RuntimeDyld] Prevent duplicate calls to finalizeMemory on shared
>>>>> memory
>>>>> managers.
>>>>>
>>>>> Prior to this patch, recursive finalization (where finalization of one
>>>>> RuntimeDyld instance triggers finalization of another instance on
>>>>> which the
>>>>> first depends) could trigger memory access failures: When the inner
>>>>> (dependent)
>>>>> RuntimeDyld instance and its memory manager are finalized, memory
>>>>> allocated
>>>>> (but not yet relocated) by the outer instance is locked, and
>>>>> relocation in the
>>>>> outer instance fails with a memory access error.
>>>>>
>>>>> This patch adds a latch to the RuntimeDyld::MemoryManager base class
>>>>> that is
>>>>> checked by a new method:
>>>>> RuntimeDyld::finalizeWithMemoryManagerLocking, ensuring
>>>>> that shared memory managers are only finalized by the outermost
>>>>> RuntimeDyld
>>>>> instance.
>>>>>
>>>>> This allows ORC clients to supply the same memory manager to multiple
>>>>> calls to
>>>>> addModuleSet. In particular it enables the use of user-supplied memory
>>>>> managers
>>>>> with the CompileOnDemandLayer which must reuse the supplied memory
>>>>> manager for
>>>>> each function that is lazily compiled.
>>>>>
>>>>
>>>> Rather than having MemoryManager befriending RuntimeDyld, would it make
>>>> sense to have a MemoryManager ref counting decorator? If there are multiple
>>>> simultaneous users of a MemoryManager, is it a constraint that all clients
>>>> of the MemoryManager must finalize it before it can be truly finalized?
>>>> (it's not clear to me from this change if that is a constraint - I guess it
>>>> probably /isn't/? But then what happens to the other clients that never
>>>> participated in this finalize-with-locking - wouldn't they try to finalize
>>>> (or even do pre-finalization tasks) at some later point?)
>>>>
>>>> - Dave
>>>>
>>>>
>>>>>
>>>>>
>>>>> Modified:
>>>>> llvm/trunk/include/llvm/ExecutionEngine/Orc/ObjectLinkingLayer.h
>>>>> llvm/trunk/include/llvm/ExecutionEngine/RuntimeDyld.h
>>>>> llvm/trunk/lib/ExecutionEngine/RuntimeDyld/RuntimeDyld.cpp
>>>>> llvm/trunk/unittests/ExecutionEngine/Orc/ObjectLinkingLayerTest.cpp
>>>>> llvm/trunk/unittests/ExecutionEngine/Orc/OrcTestCommon.cpp
>>>>> llvm/trunk/unittests/ExecutionEngine/Orc/OrcTestCommon.h
>>>>>
>>>>> Modified:
>>>>> llvm/trunk/include/llvm/ExecutionEngine/Orc/ObjectLinkingLayer.h
>>>>> URL:
>>>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/ExecutionEngine/Orc/ObjectLinkingLayer.h?rev=257263&r1=257262&r2=257263&view=diff
>>>>>
>>>>> ==============================================================================
>>>>> --- llvm/trunk/include/llvm/ExecutionEngine/Orc/ObjectLinkingLayer.h
>>>>> (original)
>>>>> +++ llvm/trunk/include/llvm/ExecutionEngine/Orc/ObjectLinkingLayer.h
>>>>> Sat Jan 9 13:50:40 2016
>>>>> @@ -108,9 +108,7 @@ private:
>>>>>
>>>>> void Finalize() override {
>>>>> State = Finalizing;
>>>>> - RTDyld->resolveRelocations();
>>>>> - RTDyld->registerEHFrames();
>>>>> - MemMgr->finalizeMemory();
>>>>> + RTDyld->finalizeWithMemoryManagerLocking();
>>>>> State = Finalized;
>>>>> }
>>>>>
>>>>>
>>>>> Modified: llvm/trunk/include/llvm/ExecutionEngine/RuntimeDyld.h
>>>>> URL:
>>>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/ExecutionEngine/RuntimeDyld.h?rev=257263&r1=257262&r2=257263&view=diff
>>>>>
>>>>> ==============================================================================
>>>>> --- llvm/trunk/include/llvm/ExecutionEngine/RuntimeDyld.h (original)
>>>>> +++ llvm/trunk/include/llvm/ExecutionEngine/RuntimeDyld.h Sat Jan 9
>>>>> 13:50:40 2016
>>>>> @@ -95,7 +95,9 @@ public:
>>>>>
>>>>> /// \brief Memory Management.
>>>>> class MemoryManager {
>>>>> + friend class RuntimeDyld;
>>>>> public:
>>>>> + MemoryManager() : FinalizationLocked(false) {}
>>>>> virtual ~MemoryManager() {}
>>>>>
>>>>> /// Allocate a memory block of (at least) the given size suitable
>>>>> for
>>>>> @@ -153,6 +155,7 @@ public:
>>>>>
>>>>> private:
>>>>> virtual void anchor();
>>>>> + bool FinalizationLocked;
>>>>> };
>>>>>
>>>>> /// \brief Symbol resolution.
>>>>> @@ -241,6 +244,25 @@ public:
>>>>> this->ProcessAllSections = ProcessAllSections;
>>>>> }
>>>>>
>>>>> + /// Perform all actions needed to make the code owned by this
>>>>> RuntimeDyld
>>>>> + /// instance executable:
>>>>> + ///
>>>>> + /// 1) Apply relocations.
>>>>> + /// 2) Register EH frames.
>>>>> + /// 3) Update memory permissions*.
>>>>> + ///
>>>>> + /// * Finalization is potentially recursive**, and the 3rd step
>>>>> will only be
>>>>> + /// applied by the outermost call to finalize. This allows
>>>>> different
>>>>> + /// RuntimeDyld instances to share a memory manager without the
>>>>> innermost
>>>>> + /// finalization locking the memory and causing relocation fixup
>>>>> errors in
>>>>> + /// outer instances.
>>>>> + ///
>>>>> + /// ** Recursive finalization occurs when one RuntimeDyld instances
>>>>> needs the
>>>>> + /// address of a symbol owned by some other instance in order to
>>>>> apply
>>>>> + /// relocations.
>>>>> + ///
>>>>> + void finalizeWithMemoryManagerLocking();
>>>>> +
>>>>> private:
>>>>> // RuntimeDyldImpl is the actual class. RuntimeDyld is just the
>>>>> public
>>>>> // interface.
>>>>>
>>>>> Modified: llvm/trunk/lib/ExecutionEngine/RuntimeDyld/RuntimeDyld.cpp
>>>>> URL:
>>>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/ExecutionEngine/RuntimeDyld/RuntimeDyld.cpp?rev=257263&r1=257262&r2=257263&view=diff
>>>>>
>>>>> ==============================================================================
>>>>> --- llvm/trunk/lib/ExecutionEngine/RuntimeDyld/RuntimeDyld.cpp
>>>>> (original)
>>>>> +++ llvm/trunk/lib/ExecutionEngine/RuntimeDyld/RuntimeDyld.cpp Sat
>>>>> Jan 9 13:50:40 2016
>>>>> @@ -967,6 +967,17 @@ bool RuntimeDyld::hasError() { return Dy
>>>>>
>>>>> StringRef RuntimeDyld::getErrorString() { return
>>>>> Dyld->getErrorString(); }
>>>>>
>>>>> +void RuntimeDyld::finalizeWithMemoryManagerLocking() {
>>>>> + bool MemoryFinalizationLocked = MemMgr.FinalizationLocked;
>>>>> + MemMgr.FinalizationLocked = true;
>>>>> + resolveRelocations();
>>>>> + registerEHFrames();
>>>>> + if (!MemoryFinalizationLocked) {
>>>>> + MemMgr.finalizeMemory();
>>>>> + MemMgr.FinalizationLocked = false;
>>>>> + }
>>>>> +}
>>>>> +
>>>>> void RuntimeDyld::registerEHFrames() {
>>>>> if (Dyld)
>>>>> Dyld->registerEHFrames();
>>>>>
>>>>> Modified:
>>>>> llvm/trunk/unittests/ExecutionEngine/Orc/ObjectLinkingLayerTest.cpp
>>>>> URL:
>>>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/ExecutionEngine/Orc/ObjectLinkingLayerTest.cpp?rev=257263&r1=257262&r2=257263&view=diff
>>>>>
>>>>> ==============================================================================
>>>>> ---
>>>>> llvm/trunk/unittests/ExecutionEngine/Orc/ObjectLinkingLayerTest.cpp
>>>>> (original)
>>>>> +++
>>>>> llvm/trunk/unittests/ExecutionEngine/Orc/ObjectLinkingLayerTest.cpp Sat
>>>>> Jan 9 13:50:40 2016
>>>>> @@ -7,6 +7,7 @@
>>>>> //
>>>>>
>>>>> //===----------------------------------------------------------------------===//
>>>>>
>>>>> +#include "OrcTestCommon.h"
>>>>> #include "llvm/ExecutionEngine/ExecutionEngine.h"
>>>>> #include "llvm/ExecutionEngine/SectionMemoryManager.h"
>>>>> #include "llvm/ExecutionEngine/Orc/CompileUtils.h"
>>>>> @@ -21,6 +22,10 @@ using namespace llvm::orc;
>>>>>
>>>>> namespace {
>>>>>
>>>>> +class ObjectLinkingLayerExecutionTest : public testing::Test,
>>>>> + public OrcExecutionTest {
>>>>> +};
>>>>> +
>>>>> TEST(ObjectLinkingLayerTest, TestSetProcessAllSections) {
>>>>>
>>>>> class SectionMemoryManagerWrapper : public SectionMemoryManager {
>>>>> @@ -91,4 +96,81 @@ TEST(ObjectLinkingLayerTest, TestSetProc
>>>>> }
>>>>> }
>>>>>
>>>>> +
>>>>> +TEST_F(ObjectLinkingLayerExecutionTest, NoDuplicateFinalization) {
>>>>> +
>>>>> + if (!TM)
>>>>> + return;
>>>>> +
>>>>> + class SectionMemoryManagerWrapper : public SectionMemoryManager {
>>>>> + public:
>>>>> + int FinalizationCount = 0;
>>>>> + bool finalizeMemory(std::string *ErrMsg = 0) override {
>>>>> + ++FinalizationCount;
>>>>> + return SectionMemoryManager::finalizeMemory(ErrMsg);
>>>>> + }
>>>>> + };
>>>>> +
>>>>> + ObjectLinkingLayer<> ObjLayer;
>>>>> + SimpleCompiler Compile(*TM);
>>>>> +
>>>>> + // Create a pair of modules that will trigger recursive
>>>>> finalization:
>>>>> + // Module 1:
>>>>> + // int bar() { return 42; }
>>>>> + // Module 2:
>>>>> + // int bar();
>>>>> + // int foo() { return bar(); }
>>>>> +
>>>>> + ModuleBuilder MB1(getGlobalContext(), "", "dummy");
>>>>> + {
>>>>> + MB1.getModule()->setDataLayout(TM->createDataLayout());
>>>>> + Function *BarImpl = MB1.createFunctionDecl<int32_t(void)>("bar");
>>>>> + BasicBlock *BarEntry = BasicBlock::Create(getGlobalContext(),
>>>>> "entry",
>>>>> + BarImpl);
>>>>> + IRBuilder<> Builder(BarEntry);
>>>>> + IntegerType *Int32Ty = IntegerType::get(getGlobalContext(), 32);
>>>>> + Value *FourtyTwo = ConstantInt::getSigned(Int32Ty, 42);
>>>>> + Builder.CreateRet(FourtyTwo);
>>>>> + }
>>>>> +
>>>>> + auto Obj1 = Compile(*MB1.getModule());
>>>>> + std::vector<object::ObjectFile*> Obj1Set;
>>>>> + Obj1Set.push_back(Obj1.getBinary());
>>>>> +
>>>>> + ModuleBuilder MB2(getGlobalContext(), "", "dummy");
>>>>> + {
>>>>> + MB2.getModule()->setDataLayout(TM->createDataLayout());
>>>>> + Function *BarDecl = MB2.createFunctionDecl<int32_t(void)>("bar");
>>>>> + Function *FooImpl = MB2.createFunctionDecl<int32_t(void)>("foo");
>>>>> + BasicBlock *FooEntry = BasicBlock::Create(getGlobalContext(),
>>>>> "entry",
>>>>> + FooImpl);
>>>>> + IRBuilder<> Builder(FooEntry);
>>>>> + Builder.CreateRet(Builder.CreateCall(BarDecl));
>>>>> + }
>>>>> + auto Obj2 = Compile(*MB2.getModule());
>>>>> + std::vector<object::ObjectFile*> Obj2Set;
>>>>> + Obj2Set.push_back(Obj2.getBinary());
>>>>> +
>>>>> + auto Resolver =
>>>>> + createLambdaResolver(
>>>>> + [&](const std::string &Name) {
>>>>> + if (auto Sym = ObjLayer.findSymbol(Name, true))
>>>>> + return RuntimeDyld::SymbolInfo(Sym.getAddress(),
>>>>> Sym.getFlags());
>>>>> + return RuntimeDyld::SymbolInfo(nullptr);
>>>>> + },
>>>>> + [](const std::string &Name) {
>>>>> + return RuntimeDyld::SymbolInfo(nullptr);
>>>>> + });
>>>>> +
>>>>> + SectionMemoryManagerWrapper SMMW;
>>>>> + ObjLayer.addObjectSet(std::move(Obj1Set), &SMMW, &*Resolver);
>>>>> + auto H = ObjLayer.addObjectSet(std::move(Obj2Set), &SMMW,
>>>>> &*Resolver);
>>>>> + ObjLayer.emitAndFinalize(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)
>>>>> + << "Extra call to finalize";
>>>>> +}
>>>>> +
>>>>> }
>>>>>
>>>>> Modified: llvm/trunk/unittests/ExecutionEngine/Orc/OrcTestCommon.cpp
>>>>> URL:
>>>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/ExecutionEngine/Orc/OrcTestCommon.cpp?rev=257263&r1=257262&r2=257263&view=diff
>>>>>
>>>>> ==============================================================================
>>>>> --- llvm/trunk/unittests/ExecutionEngine/Orc/OrcTestCommon.cpp
>>>>> (original)
>>>>> +++ llvm/trunk/unittests/ExecutionEngine/Orc/OrcTestCommon.cpp Sat
>>>>> Jan 9 13:50:40 2016
>>>>> @@ -19,7 +19,7 @@ bool OrcExecutionTest::NativeTargetIniti
>>>>>
>>>>> ModuleBuilder::ModuleBuilder(LLVMContext &Context, StringRef Triple,
>>>>> StringRef Name)
>>>>> - : M(new Module(Name, Context)),
>>>>> - Builder(Context) {
>>>>> - M->setTargetTriple(Triple);
>>>>> + : M(new Module(Name, Context)) {
>>>>> + if (Triple != "")
>>>>> + M->setTargetTriple(Triple);
>>>>> }
>>>>>
>>>>> Modified: llvm/trunk/unittests/ExecutionEngine/Orc/OrcTestCommon.h
>>>>> URL:
>>>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/ExecutionEngine/Orc/OrcTestCommon.h?rev=257263&r1=257262&r2=257263&view=diff
>>>>>
>>>>> ==============================================================================
>>>>> --- llvm/trunk/unittests/ExecutionEngine/Orc/OrcTestCommon.h (original)
>>>>> +++ llvm/trunk/unittests/ExecutionEngine/Orc/OrcTestCommon.h Sat Jan
>>>>> 9 13:50:40 2016
>>>>> @@ -20,6 +20,7 @@
>>>>> #include "llvm/IR/LLVMContext.h"
>>>>> #include "llvm/IR/Module.h"
>>>>> #include "llvm/IR/TypeBuilder.h"
>>>>> +#include "llvm/Object/ObjectFile.h"
>>>>> #include "llvm/ExecutionEngine/ExecutionEngine.h"
>>>>> #include "llvm/ExecutionEngine/Orc/JITSymbol.h"
>>>>> #include "llvm/Support/TargetSelect.h"
>>>>> @@ -74,7 +75,6 @@ public:
>>>>>
>>>>> private:
>>>>> std::unique_ptr<Module> M;
>>>>> - IRBuilder<> Builder;
>>>>> };
>>>>>
>>>>> // Dummy struct type.
>>>>>
>>>>>
>>>>> _______________________________________________
>>>>> llvm-commits mailing list
>>>>> llvm-commits at lists.llvm.org
>>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>>>>
>>>>
>>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160119/2fcd5a91/attachment.html>
More information about the llvm-commits
mailing list