<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Sun, Oct 25, 2015 at 11:37 PM, Lang Hames via llvm-commits <span dir="ltr"><<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Author: lhames<br>
Date: Mon Oct 26 01:37:04 2015<br>
New Revision: 251273<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=251273&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=251273&view=rev</a><br>
Log:<br>
[Orc] In the CompileOnDemand layer, wrap module ownership in order to preserve<br>
the module pointer type passed in by the user.<br>
<br>
The previous ownership scheme, where the user pointer was always moved into a<br>
std::shared_ptr, breaks if the user passes in a raw pointer.<br>
<br>
Discovered while working on the Orc C API, which should be landing shortly.<br>
I expect to include a test-case with that.<br></blockquote><div><br></div><div>This seems like it'd warrant a unit test by itself - I've attached a possible example.<br><br>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.<br><br>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.<br><br>- Dave</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Modified:<br>
llvm/trunk/include/llvm/ExecutionEngine/Orc/CompileOnDemandLayer.h<br>
<br>
Modified: llvm/trunk/include/llvm/ExecutionEngine/Orc/CompileOnDemandLayer.h<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/ExecutionEngine/Orc/CompileOnDemandLayer.h?rev=251273&r1=251272&r2=251273&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/ExecutionEngine/Orc/CompileOnDemandLayer.h?rev=251273&r1=251272&r2=251273&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/include/llvm/ExecutionEngine/Orc/CompileOnDemandLayer.h (original)<br>
+++ llvm/trunk/include/llvm/ExecutionEngine/Orc/CompileOnDemandLayer.h Mon Oct 26 01:37:04 2015<br>
@@ -62,8 +62,31 @@ private:<br>
<br>
typedef typename BaseLayerT::ModuleSetHandleT BaseLayerModuleSetHandleT;<br>
<br>
+ class ModuleOwner {<br>
+ public:<br>
+ ModuleOwner() = default;<br>
+ ModuleOwner(const ModuleOwner&) = delete;<br>
+ ModuleOwner& operator=(const ModuleOwner&) = delete;<br>
+ virtual ~ModuleOwner() { }<br>
+ virtual Module& getModule() const = 0;<br>
+ };<br>
+<br>
+ template <typename ModulePtrT><br>
+ class ModuleOwnerImpl : public ModuleOwner {<br>
+ public:<br>
+ ModuleOwnerImpl(ModulePtrT ModulePtr) : ModulePtr(std::move(ModulePtr)) {}<br>
+ Module& getModule() const override { return *ModulePtr; }<br>
+ private:<br>
+ ModulePtrT ModulePtr;<br>
+ };<br>
+<br>
+ template <typename ModulePtrT><br>
+ std::unique_ptr<ModuleOwner> wrapOwnership(ModulePtrT ModulePtr) {<br>
+ return llvm::make_unique<ModuleOwnerImpl<ModulePtrT>>(std::move(ModulePtr));<br>
+ }<br>
+<br>
struct LogicalModuleResources {<br>
- std::shared_ptr<Module> SourceModule;<br>
+ std::unique_ptr<ModuleOwner> SourceModuleOwner;<br>
std::set<const Function*> StubsToClone;<br>
std::unique_ptr<IndirectStubsMgrT> StubsMgr;<br>
<br>
@@ -71,13 +94,13 @@ private:<br>
<br>
// Explicit move constructor to make MSVC happy.<br>
LogicalModuleResources(LogicalModuleResources &&Other)<br>
- : SourceModule(std::move(Other.SourceModule)),<br>
+ : SourceModuleOwner(std::move(Other.SourceModuleOwner)),<br>
StubsToClone(std::move(Other.StubsToClone)),<br>
StubsMgr(std::move(Other.StubsMgr)) {}<br>
<br>
// Explicit move assignment to make MSVC happy.<br>
LogicalModuleResources& operator=(LogicalModuleResources &&Other) {<br>
- SourceModule = std::move(Other.SourceModule);<br>
+ SourceModuleOwner = std::move(Other.SourceModuleOwner);<br>
StubsToClone = std::move(Other.StubsToClone);<br>
StubsMgr = std::move(Other.StubsMgr);<br>
}<br>
@@ -92,6 +115,8 @@ private:<br>
<br>
};<br>
<br>
+<br>
+<br>
struct LogicalDylibResources {<br>
typedef std::function<RuntimeDyld::SymbolInfo(const std::string&)><br>
SymbolResolverFtor;<br>
@@ -146,8 +171,7 @@ public:<br>
<br>
// Process each of the modules in this module set.<br>
for (auto &M : Ms)<br>
- addLogicalModule(LogicalDylibs.back(),<br>
- std::shared_ptr<Module>(std::move(M)));<br>
+ addLogicalModule(LogicalDylibs.back(), std::move(M));<br>
<br>
return std::prev(LogicalDylibs.end());<br>
}<br>
@@ -181,29 +205,32 @@ public:<br>
<br>
private:<br>
<br>
- void addLogicalModule(CODLogicalDylib &LD, std::shared_ptr<Module> SrcM) {<br>
+ template <typename ModulePtrT><br>
+ void addLogicalModule(CODLogicalDylib &LD, ModulePtrT SrcMPtr) {<br>
<br>
// Bump the linkage and rename any anonymous/privote members in SrcM to<br>
// ensure that everything will resolve properly after we partition SrcM.<br>
- makeAllSymbolsExternallyAccessible(*SrcM);<br>
+ makeAllSymbolsExternallyAccessible(*SrcMPtr);<br>
<br>
// Create a logical module handle for SrcM within the logical dylib.<br>
auto LMH = LD.createLogicalModule();<br>
auto &LMResources = LD.getLogicalModuleResources(LMH);<br>
<br>
- LMResources.SourceModule = SrcM;<br>
+ LMResources.SourceModuleOwner = wrapOwnership(std::move(SrcMPtr));<br>
+<br>
+ Module &SrcM = LMResources.SourceModuleOwner->getModule();<br>
<br>
// Create the GlobalValues module.<br>
- const DataLayout &DL = SrcM->getDataLayout();<br>
- auto GVsM = llvm::make_unique<Module>((SrcM->getName() + ".globals").str(),<br>
- SrcM->getContext());<br>
+ const DataLayout &DL = SrcM.getDataLayout();<br>
+ auto GVsM = llvm::make_unique<Module>((SrcM.getName() + ".globals").str(),<br>
+ SrcM.getContext());<br>
GVsM->setDataLayout(DL);<br>
<br>
// Create function stubs.<br>
ValueToValueMapTy VMap;<br>
{<br>
typename IndirectStubsMgrT::StubInitsMap StubInits;<br>
- for (auto &F : *SrcM) {<br>
+ for (auto &F : SrcM) {<br>
// Skip declarations.<br>
if (F.isDeclaration())<br>
continue;<br>
@@ -215,7 +242,7 @@ private:<br>
// Create a callback, associate it with the stub for the function,<br>
// and set the compile action to compile the partition containing the<br>
// function.<br>
- auto CCInfo = CompileCallbackMgr.getCompileCallback(SrcM->getContext());<br>
+ auto CCInfo = CompileCallbackMgr.getCompileCallback(SrcM.getContext());<br>
StubInits[mangle(F.getName(), DL)] =<br>
std::make_pair(CCInfo.getAddress(),<br>
JITSymbolBase::flagsFromGlobalValue(F));<br>
@@ -234,12 +261,12 @@ private:<br>
}<br>
<br>
// Clone global variable decls.<br>
- for (auto &GV : SrcM->globals())<br>
+ for (auto &GV : SrcM.globals())<br>
if (!GV.isDeclaration() && !VMap.count(&GV))<br>
cloneGlobalVariableDecl(*GVsM, GV, &VMap);<br>
<br>
// And the aliases.<br>
- for (auto &A : SrcM->aliases())<br>
+ for (auto &A : SrcM.aliases())<br>
if (!VMap.count(&A))<br>
cloneGlobalAliasDecl(*GVsM, A, VMap);<br>
<br>
@@ -276,12 +303,12 @@ private:<br>
});<br>
<br>
// Clone the global variable initializers.<br>
- for (auto &GV : SrcM->globals())<br>
+ for (auto &GV : SrcM.globals())<br>
if (!GV.isDeclaration())<br>
moveGlobalVariableInitializer(GV, VMap, &Materializer);<br>
<br>
// Clone the global alias initializers.<br>
- for (auto &A : SrcM->aliases()) {<br>
+ for (auto &A : SrcM.aliases()) {<br>
auto *NewA = cast<GlobalAlias>(VMap[&A]);<br>
assert(NewA && "Alias not cloned?");<br>
Value *Init = MapValue(A.getAliasee(), VMap, RF_None, nullptr,<br>
@@ -323,7 +350,7 @@ private:<br>
LogicalModuleHandle LMH,<br>
Function &F) {<br>
auto &LMResources = LD.getLogicalModuleResources(LMH);<br>
- Module &SrcM = *LMResources.SourceModule;<br>
+ Module &SrcM = LMResources.SourceModuleOwner->getModule();<br>
<br>
// If F is a declaration we must already have compiled it.<br>
if (F.isDeclaration())<br>
@@ -361,7 +388,7 @@ private:<br>
LogicalModuleHandle LMH,<br>
const PartitionT &Part) {<br>
auto &LMResources = LD.getLogicalModuleResources(LMH);<br>
- Module &SrcM = *LMResources.SourceModule;<br>
+ Module &SrcM = LMResources.SourceModuleOwner->getModule();<br>
<br>
// Create the module.<br>
std::string NewName = SrcM.getName();<br>
<br>
<br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
</blockquote></div><br></div></div>