[llvm] r251273 - [Orc] In the CompileOnDemand layer, wrap module ownership in order to preserve
David Blaikie via llvm-commits
llvm-commits at lists.llvm.org
Wed Nov 11 10:49:01 PST 2015
On Sun, Oct 25, 2015 at 11:37 PM, Lang Hames via llvm-commits <
llvm-commits at lists.llvm.org> wrote:
> Author: lhames
> Date: Mon Oct 26 01:37:04 2015
> New Revision: 251273
>
> URL: http://llvm.org/viewvc/llvm-project?rev=251273&view=rev
> Log:
> [Orc] In the CompileOnDemand layer, wrap module ownership in order to
> preserve
> the module pointer type passed in by the user.
>
> The previous ownership scheme, where the user pointer was always moved
> into a
> std::shared_ptr, breaks if the user passes in a raw pointer.
>
> Discovered while working on the Orc C API, which should be landing shortly.
> I expect to include a test-case with that.
>
This seems like it'd warrant a unit test by itself - I've attached a
possible example.
The change you've made allows arbitrary ownership mechanisms - and while
testing that the raw pointer case works is difficulty (since there's no
hook/obvious way to observe that the Module's dtor is /not/ run by the
layer when it's passed a raw pointer), since this change generalizes to any
ownership, I've tested that this new functionality works by creating a
custom smart pointer (in fact a thin wrapper around a shared_ptr) and
testing that it is handled correctly. This test wouldn't even compile
without your change (since there would be no way to construct a shared_ptr
from this custom smart pointer type), and the test verifies that not only
does it compile, but it behaves as expected.
Feel free to commit the patch/let me know if it looks reasonable, etc.
Maybe there are some other changes/improvements I could make to it.
- Dave
>
> Modified:
> llvm/trunk/include/llvm/ExecutionEngine/Orc/CompileOnDemandLayer.h
>
> Modified:
> llvm/trunk/include/llvm/ExecutionEngine/Orc/CompileOnDemandLayer.h
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/ExecutionEngine/Orc/CompileOnDemandLayer.h?rev=251273&r1=251272&r2=251273&view=diff
>
> ==============================================================================
> --- llvm/trunk/include/llvm/ExecutionEngine/Orc/CompileOnDemandLayer.h
> (original)
> +++ llvm/trunk/include/llvm/ExecutionEngine/Orc/CompileOnDemandLayer.h Mon
> Oct 26 01:37:04 2015
> @@ -62,8 +62,31 @@ private:
>
> typedef typename BaseLayerT::ModuleSetHandleT BaseLayerModuleSetHandleT;
>
> + class ModuleOwner {
> + public:
> + ModuleOwner() = default;
> + ModuleOwner(const ModuleOwner&) = delete;
> + ModuleOwner& operator=(const ModuleOwner&) = delete;
> + virtual ~ModuleOwner() { }
> + virtual Module& getModule() const = 0;
> + };
> +
> + template <typename ModulePtrT>
> + class ModuleOwnerImpl : public ModuleOwner {
> + public:
> + ModuleOwnerImpl(ModulePtrT ModulePtr) :
> ModulePtr(std::move(ModulePtr)) {}
> + Module& getModule() const override { return *ModulePtr; }
> + private:
> + ModulePtrT ModulePtr;
> + };
> +
> + template <typename ModulePtrT>
> + std::unique_ptr<ModuleOwner> wrapOwnership(ModulePtrT ModulePtr) {
> + return
> llvm::make_unique<ModuleOwnerImpl<ModulePtrT>>(std::move(ModulePtr));
> + }
> +
> struct LogicalModuleResources {
> - std::shared_ptr<Module> SourceModule;
> + std::unique_ptr<ModuleOwner> SourceModuleOwner;
> std::set<const Function*> StubsToClone;
> std::unique_ptr<IndirectStubsMgrT> StubsMgr;
>
> @@ -71,13 +94,13 @@ private:
>
> // Explicit move constructor to make MSVC happy.
> LogicalModuleResources(LogicalModuleResources &&Other)
> - : SourceModule(std::move(Other.SourceModule)),
> + : SourceModuleOwner(std::move(Other.SourceModuleOwner)),
> StubsToClone(std::move(Other.StubsToClone)),
> StubsMgr(std::move(Other.StubsMgr)) {}
>
> // Explicit move assignment to make MSVC happy.
> LogicalModuleResources& operator=(LogicalModuleResources &&Other) {
> - SourceModule = std::move(Other.SourceModule);
> + SourceModuleOwner = std::move(Other.SourceModuleOwner);
> StubsToClone = std::move(Other.StubsToClone);
> StubsMgr = std::move(Other.StubsMgr);
> }
> @@ -92,6 +115,8 @@ private:
>
> };
>
> +
> +
> struct LogicalDylibResources {
> typedef std::function<RuntimeDyld::SymbolInfo(const std::string&)>
> SymbolResolverFtor;
> @@ -146,8 +171,7 @@ public:
>
> // Process each of the modules in this module set.
> for (auto &M : Ms)
> - addLogicalModule(LogicalDylibs.back(),
> - std::shared_ptr<Module>(std::move(M)));
> + addLogicalModule(LogicalDylibs.back(), std::move(M));
>
> return std::prev(LogicalDylibs.end());
> }
> @@ -181,29 +205,32 @@ public:
>
> private:
>
> - void addLogicalModule(CODLogicalDylib &LD, std::shared_ptr<Module>
> SrcM) {
> + template <typename ModulePtrT>
> + void addLogicalModule(CODLogicalDylib &LD, ModulePtrT SrcMPtr) {
>
> // Bump the linkage and rename any anonymous/privote members in SrcM
> to
> // ensure that everything will resolve properly after we partition
> SrcM.
> - makeAllSymbolsExternallyAccessible(*SrcM);
> + makeAllSymbolsExternallyAccessible(*SrcMPtr);
>
> // Create a logical module handle for SrcM within the logical dylib.
> auto LMH = LD.createLogicalModule();
> auto &LMResources = LD.getLogicalModuleResources(LMH);
>
> - LMResources.SourceModule = SrcM;
> + LMResources.SourceModuleOwner = wrapOwnership(std::move(SrcMPtr));
> +
> + Module &SrcM = LMResources.SourceModuleOwner->getModule();
>
> // Create the GlobalValues module.
> - const DataLayout &DL = SrcM->getDataLayout();
> - auto GVsM = llvm::make_unique<Module>((SrcM->getName() +
> ".globals").str(),
> - SrcM->getContext());
> + const DataLayout &DL = SrcM.getDataLayout();
> + auto GVsM = llvm::make_unique<Module>((SrcM.getName() +
> ".globals").str(),
> + SrcM.getContext());
> GVsM->setDataLayout(DL);
>
> // Create function stubs.
> ValueToValueMapTy VMap;
> {
> typename IndirectStubsMgrT::StubInitsMap StubInits;
> - for (auto &F : *SrcM) {
> + for (auto &F : SrcM) {
> // Skip declarations.
> if (F.isDeclaration())
> continue;
> @@ -215,7 +242,7 @@ private:
> // Create a callback, associate it with the stub for the function,
> // and set the compile action to compile the partition containing
> the
> // function.
> - auto CCInfo =
> CompileCallbackMgr.getCompileCallback(SrcM->getContext());
> + auto CCInfo =
> CompileCallbackMgr.getCompileCallback(SrcM.getContext());
> StubInits[mangle(F.getName(), DL)] =
> std::make_pair(CCInfo.getAddress(),
> JITSymbolBase::flagsFromGlobalValue(F));
> @@ -234,12 +261,12 @@ private:
> }
>
> // Clone global variable decls.
> - for (auto &GV : SrcM->globals())
> + for (auto &GV : SrcM.globals())
> if (!GV.isDeclaration() && !VMap.count(&GV))
> cloneGlobalVariableDecl(*GVsM, GV, &VMap);
>
> // And the aliases.
> - for (auto &A : SrcM->aliases())
> + for (auto &A : SrcM.aliases())
> if (!VMap.count(&A))
> cloneGlobalAliasDecl(*GVsM, A, VMap);
>
> @@ -276,12 +303,12 @@ private:
> });
>
> // Clone the global variable initializers.
> - for (auto &GV : SrcM->globals())
> + for (auto &GV : SrcM.globals())
> if (!GV.isDeclaration())
> moveGlobalVariableInitializer(GV, VMap, &Materializer);
>
> // Clone the global alias initializers.
> - for (auto &A : SrcM->aliases()) {
> + for (auto &A : SrcM.aliases()) {
> auto *NewA = cast<GlobalAlias>(VMap[&A]);
> assert(NewA && "Alias not cloned?");
> Value *Init = MapValue(A.getAliasee(), VMap, RF_None, nullptr,
> @@ -323,7 +350,7 @@ private:
> LogicalModuleHandle LMH,
> Function &F) {
> auto &LMResources = LD.getLogicalModuleResources(LMH);
> - Module &SrcM = *LMResources.SourceModule;
> + Module &SrcM = LMResources.SourceModuleOwner->getModule();
>
> // If F is a declaration we must already have compiled it.
> if (F.isDeclaration())
> @@ -361,7 +388,7 @@ private:
> LogicalModuleHandle LMH,
> const PartitionT &Part) {
> auto &LMResources = LD.getLogicalModuleResources(LMH);
> - Module &SrcM = *LMResources.SourceModule;
> + Module &SrcM = LMResources.SourceModuleOwner->getModule();
>
> // Create the module.
> std::string NewName = SrcM.getName();
>
>
> _______________________________________________
> 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/20151111/2bf47c44/attachment.html>
-------------- next part --------------
commit f769062da18d449229475460cea17128c3915e96
Author: David Blaikie <dblaikie at gmail.com>
Date: Wed Nov 4 12:02:01 2015 -0800
Orc: Add unit test coverage for ownership changes to CompileOnDemandLayer made in r251273
diff --git unittests/ExecutionEngine/Orc/CompileOnDemandLayerTest.cpp unittests/ExecutionEngine/Orc/CompileOnDemandLayerTest.cpp
index 49f4cc1..52dec89 100644
--- unittests/ExecutionEngine/Orc/CompileOnDemandLayerTest.cpp
+++ unittests/ExecutionEngine/Orc/CompileOnDemandLayerTest.cpp
@@ -39,7 +39,7 @@ public:
}
std::error_code createStubs(const StubInitsMap &StubInits) override {
- llvm_unreachable("Not implemented");
+ return std::error_code();
}
JITSymbol findStub(StringRef Name, bool ExportedStubsOnly) override {
@@ -80,4 +80,37 @@ TEST(CompileOnDemandLayerTest, FindSymbol) {
<< "CompileOnDemand::findSymbol should call findSymbol in the base layer.";
}
+TEST(CompileOnDemandLayerTest, ModuleCustomOwnership) {
+ struct ModuleHolder {
+ std::shared_ptr<llvm::Module> M =
+ std::make_shared<Module>("stub", llvm::getGlobalContext());
+ ModuleHolder() = default;
+ ModuleHolder(ModuleHolder &&RHS) : M(std::move(RHS.M)) {}
+ Module &operator*() const { return *M; }
+ Module *operator->() const { return M.get(); }
+ };
+ auto MockBaseLayer = createMockBaseLayer<int>(
+ DoNothingAndReturn<int>(0), DoNothingAndReturn<void>(),
+ DoNothingAndReturn<JITSymbol>(nullptr),
+ DoNothingAndReturn<JITSymbol>(nullptr));
+
+ typedef decltype(MockBaseLayer) MockBaseLayerT;
+ DummyCallbackManager CallbackMgr;
+
+ std::weak_ptr<llvm::Module> WeakMod;
+ {
+ llvm::orc::CompileOnDemandLayer<MockBaseLayerT> COD(
+ MockBaseLayer, [](Function &F) { return std::set<Function *>{&F}; },
+ CallbackMgr, [] { return llvm::make_unique<DummyStubsManager>(); },
+ true);
+
+ std::vector<ModuleHolder> Modules;
+ Modules.emplace_back();
+ WeakMod = Modules.back().M;
+ COD.addModuleSet(std::move(Modules), nullptr,
+ static_cast<RuntimeDyld::SymbolResolver *>(nullptr));
+ EXPECT_FALSE(WeakMod.expired());
+ }
+ EXPECT_TRUE(WeakMod.expired());
+}
}
More information about the llvm-commits
mailing list