From a944589cc51ab4a693a52fa5188280910c148531 Mon Sep 17 00:00:00 2001 From: Lang Hames Date: Wed, 21 Feb 2018 21:55:57 +0000 Subject: [PATCH] [ORC] Switch to shared_ptr ownership for SymbolSources in VSOs. This makes it easy to free a SymbolSource (and any related resources) when the last reference in a VSO is dropped. llvm-svn: 325727 --- llvm/include/llvm/ExecutionEngine/JITSymbol.h | 11 +-- llvm/include/llvm/ExecutionEngine/Orc/Core.h | 27 +++--- llvm/lib/ExecutionEngine/Orc/Core.cpp | 107 ++++++++++++--------- .../unittests/ExecutionEngine/Orc/CoreAPIsTest.cpp | 4 +- 4 files changed, 79 insertions(+), 70 deletions(-) diff --git a/llvm/include/llvm/ExecutionEngine/JITSymbol.h b/llvm/include/llvm/ExecutionEngine/JITSymbol.h index 0ce16dc..86ab173 100644 --- a/llvm/include/llvm/ExecutionEngine/JITSymbol.h +++ b/llvm/include/llvm/ExecutionEngine/JITSymbol.h @@ -52,13 +52,11 @@ public: Common = 1U << 2, Absolute = 1U << 3, Exported = 1U << 4, - NotMaterialized = 1U << 5, - Materializing = 1U << 6 + NotMaterialized = 1U << 5 }; static JITSymbolFlags stripTransientFlags(JITSymbolFlags Orig) { - return static_cast(Orig.Flags & - ~(NotMaterialized | Materializing)); + return static_cast(Orig.Flags & ~NotMaterialized); } /// @brief Default-construct a JITSymbolFlags instance. @@ -81,11 +79,6 @@ public: /// callable). bool isMaterialized() const { return !(Flags & NotMaterialized); } - /// @brief Returns true if this symbol is in the process of being - /// materialized. This is generally only of interest as an - /// implementation detail to JIT infrastructure. - bool isMaterializing() const { return Flags & Materializing; } - /// @brief Returns true if the Weak flag is set. bool isWeak() const { return (Flags & Weak) == Weak; diff --git a/llvm/include/llvm/ExecutionEngine/Orc/Core.h b/llvm/include/llvm/ExecutionEngine/Orc/Core.h index d6f9fd35..6ed2135 100644 --- a/llvm/include/llvm/ExecutionEngine/Orc/Core.h +++ b/llvm/include/llvm/ExecutionEngine/Orc/Core.h @@ -201,7 +201,7 @@ public: using SetDefinitionsResult = std::map; - using SourceWorkMap = std::map; + using SourceWorkMap = std::map, SymbolNameSet>; struct LookupResult { SourceWorkMap MaterializationWork; @@ -231,7 +231,8 @@ public: Error define(SymbolMap NewSymbols); /// @brief Adds the given symbols to the mapping as lazy symbols. - Error defineLazy(const SymbolFlagsMap &NewSymbols, SymbolSource &Source); + Error defineLazy(const SymbolFlagsMap &NewSymbols, + std::shared_ptr Source); /// @brief Add the given symbol/address mappings to the dylib, but do not /// mark the symbols as finalized yet. @@ -265,32 +266,37 @@ private: class MaterializationInfo { public: MaterializationInfo(JITSymbolFlags Flags, - std::shared_ptr Query); + std::shared_ptr Query); JITSymbolFlags getFlags() const; JITTargetAddress getAddress() const; - void query(SymbolStringPtr Name, - std::shared_ptr Query); - void resolve(SymbolStringPtr Name, JITEvaluatedSymbol Sym); + void replaceWithSource(VSO &V, SymbolStringPtr Name, + JITSymbolFlags NewFlags, + std::shared_ptr NewSource); + std::shared_ptr + query(SymbolStringPtr Name, std::shared_ptr Query); + void resolve(VSO &V, SymbolStringPtr Name, JITEvaluatedSymbol Sym); void finalize(); private: JITSymbolFlags Flags; JITTargetAddress Address = 0; + std::shared_ptr Source; std::vector> PendingResolution; std::vector> PendingFinalization; }; class SymbolTableEntry { public: - SymbolTableEntry(JITSymbolFlags Flags, SymbolSource &Source); + SymbolTableEntry(JITSymbolFlags Flags, + std::shared_ptr Source); SymbolTableEntry(JITEvaluatedSymbol Sym); SymbolTableEntry(SymbolTableEntry &&Other); ~SymbolTableEntry(); JITSymbolFlags getFlags() const; void replaceWithSource(VSO &V, SymbolStringPtr Name, JITSymbolFlags Flags, - SymbolSource &NewSource); - SymbolSource *query(SymbolStringPtr Name, - std::shared_ptr Query); + std::shared_ptr NewSource); + std::shared_ptr + query(SymbolStringPtr Name, std::shared_ptr Query); void resolve(VSO &V, SymbolStringPtr Name, JITEvaluatedSymbol Sym); void finalize(); @@ -298,7 +304,6 @@ private: JITSymbolFlags Flags; union { JITTargetAddress Address; - SymbolSource *Source; std::unique_ptr MatInfo; }; }; diff --git a/llvm/lib/ExecutionEngine/Orc/Core.cpp b/llvm/lib/ExecutionEngine/Orc/Core.cpp index 37133b7..b7707ad 100644 --- a/llvm/lib/ExecutionEngine/Orc/Core.cpp +++ b/llvm/lib/ExecutionEngine/Orc/Core.cpp @@ -65,8 +65,8 @@ void AsynchronousSymbolQuery::notifySymbolFinalized() { } VSO::MaterializationInfo::MaterializationInfo( - JITSymbolFlags Flags, std::shared_ptr Query) - : Flags(std::move(Flags)), PendingResolution({std::move(Query)}) {} + JITSymbolFlags Flags, std::shared_ptr Source) + : Flags(std::move(Flags)), Source(std::move(Source)) {} JITSymbolFlags VSO::MaterializationInfo::getFlags() const { return Flags; } @@ -74,17 +74,38 @@ JITTargetAddress VSO::MaterializationInfo::getAddress() const { return Address; } -void VSO::MaterializationInfo::query( +void VSO::MaterializationInfo::replaceWithSource( + VSO &V, SymbolStringPtr Name, JITSymbolFlags NewFlags, + std::shared_ptr NewSource) { + assert(Address == 0 && PendingResolution.empty() && + PendingFinalization.empty() && + "Cannot replace source during or after materialization"); + Source->discard(V, Name); + Flags = std::move(NewFlags); + Source = std::move(NewSource); +} + +std::shared_ptr VSO::MaterializationInfo::query( SymbolStringPtr Name, std::shared_ptr Query) { - if (Address != 0) { - Query->setDefinition(Name, JITEvaluatedSymbol(Address, Flags)); - PendingFinalization.push_back(std::move(Query)); - } else + if (Address == 0) { PendingResolution.push_back(std::move(Query)); + auto S = std::move(Source); + Source = nullptr; + return S; + } + + Query->setDefinition(Name, JITEvaluatedSymbol(Address, Flags)); + PendingFinalization.push_back(std::move(Query)); + return nullptr; } -void VSO::MaterializationInfo::resolve(SymbolStringPtr Name, +void VSO::MaterializationInfo::resolve(VSO &V, SymbolStringPtr Name, JITEvaluatedSymbol Sym) { + if (Source) { + Source->discard(V, Name); + Source = nullptr; + } + // FIXME: Sanity check flags? Flags = Sym.getFlags(); Address = Sym.getAddress(); @@ -102,9 +123,10 @@ void VSO::MaterializationInfo::finalize() { } VSO::SymbolTableEntry::SymbolTableEntry(JITSymbolFlags Flags, - SymbolSource &Source) + std::shared_ptr Source) : Flags(JITSymbolFlags::FlagNames(Flags | JITSymbolFlags::NotMaterialized)), - Source(&Source) { + MatInfo( + llvm::make_unique(Flags, std::move(Source))) { // FIXME: Assert flag sanity. } @@ -115,68 +137,56 @@ VSO::SymbolTableEntry::SymbolTableEntry(JITEvaluatedSymbol Sym) VSO::SymbolTableEntry::SymbolTableEntry(SymbolTableEntry &&Other) : Flags(Other.Flags), Address(0) { - if (Flags.isMaterializing()) - MatInfo = std::move(Other.MatInfo); + if (Flags.isMaterialized()) + Address = Other.Address; else - Source = Other.Source; + MatInfo = std::move(Other.MatInfo); } VSO::SymbolTableEntry::~SymbolTableEntry() { - assert(!Flags.isMaterializing() && - "Symbol table entry destroyed while symbol was being materialized"); + if (!Flags.isMaterialized()) + MatInfo.std::unique_ptr::~unique_ptr(); } JITSymbolFlags VSO::SymbolTableEntry::getFlags() const { return Flags; } -void VSO::SymbolTableEntry::replaceWithSource(VSO &V, SymbolStringPtr Name, - JITSymbolFlags Flags, - SymbolSource &NewSource) { - assert(!this->Flags.isMaterializing() && - "Attempted to replace symbol with lazy definition during " - "materialization"); - if (!this->Flags.isMaterialized()) - Source->discard(V, Name); - this->Flags = Flags; - this->Source = &NewSource; +void VSO::SymbolTableEntry::replaceWithSource( + VSO &V, SymbolStringPtr Name, JITSymbolFlags NewFlags, + std::shared_ptr NewSource) { + bool ReplaceExisting = !Flags.isMaterialized(); + Flags = NewFlags; + if (ReplaceExisting) + MatInfo->replaceWithSource(V, Name, Flags, std::move(NewSource)); + else + new (&MatInfo) std::unique_ptr( + llvm::make_unique(Flags, std::move(NewSource))); } -SymbolSource * +std::shared_ptr VSO::SymbolTableEntry::query(SymbolStringPtr Name, std::shared_ptr Query) { - if (Flags.isMaterializing()) { - MatInfo->query(std::move(Name), std::move(Query)); - return nullptr; - } else if (Flags.isMaterialized()) { + if (Flags.isMaterialized()) { Query->setDefinition(std::move(Name), JITEvaluatedSymbol(Address, Flags)); Query->notifySymbolFinalized(); return nullptr; - } - SymbolSource *S = Source; - new (&MatInfo) std::unique_ptr( - llvm::make_unique(Flags, std::move(Query))); - Flags |= JITSymbolFlags::Materializing; - return S; + } else + return MatInfo->query(std::move(Name), std::move(Query)); } void VSO::SymbolTableEntry::resolve(VSO &V, SymbolStringPtr Name, JITEvaluatedSymbol Sym) { - if (Flags.isMaterializing()) - MatInfo->resolve(std::move(Name), std::move(Sym)); - else { - // If there's a layer for this symbol. - if (!Flags.isMaterialized()) - Source->discard(V, Name); - + if (Flags.isMaterialized()) { // FIXME: Should we assert flag state here (flags must match except for // materialization state, overrides must be legal) or in the caller // in VSO? Flags = Sym.getFlags(); Address = Sym.getAddress(); - } + } else + MatInfo->resolve(V, std::move(Name), std::move(Sym)); } void VSO::SymbolTableEntry::finalize() { - if (Flags.isMaterializing()) { + if (!Flags.isMaterialized()) { auto TmpMatInfo = std::move(MatInfo); MatInfo.std::unique_ptr::~unique_ptr(); // FIXME: Assert flag sanity? @@ -243,7 +253,8 @@ Error VSO::define(SymbolMap NewSymbols) { return Err; } -Error VSO::defineLazy(const SymbolFlagsMap &NewSymbols, SymbolSource &Source) { +Error VSO::defineLazy(const SymbolFlagsMap &NewSymbols, + std::shared_ptr Source) { Error Err = Error::success(); for (auto &KV : NewSymbols) { auto I = Symbols.find(KV.first); @@ -255,7 +266,7 @@ Error VSO::defineLazy(const SymbolFlagsMap &NewSymbols, SymbolSource &Source) { // Discard weaker definitions. if (LinkageResult == ExistingDefinitionIsStronger) - Source.discard(*this, KV.first); + Source->discard(*this, KV.first); // Report duplicate definition errors. if (LinkageResult == DuplicateDefinition) { @@ -326,7 +337,7 @@ VSO::LookupResult VSO::lookup(std::shared_ptr Query, // Forward the query to the given SymbolTableEntry, and if it return a // layer to perform materialization with, add that to the // MaterializationWork map. - if (auto *Source = SymI->second.query(SymI->first, Query)) + if (auto Source = SymI->second.query(SymI->first, Query)) MaterializationWork[Source].insert(SymI->first); } diff --git a/llvm/unittests/ExecutionEngine/Orc/CoreAPIsTest.cpp b/llvm/unittests/ExecutionEngine/Orc/CoreAPIsTest.cpp index e859362..a79f31f 100644 --- a/llvm/unittests/ExecutionEngine/Orc/CoreAPIsTest.cpp +++ b/llvm/unittests/ExecutionEngine/Orc/CoreAPIsTest.cpp @@ -160,7 +160,7 @@ TEST(CoreAPIsTest, LookupFlagsTest) { cantFail(V.define(std::move(InitialDefs))); SymbolFlagsMap InitialLazyDefs({{Bar, BarFlags}}); - cantFail(V.defineLazy(InitialLazyDefs, *Source)); + cantFail(V.defineLazy(InitialLazyDefs, Source)); SymbolNameSet Names({Foo, Bar, Baz}); @@ -217,7 +217,7 @@ TEST(CoreAPIsTest, AddAndMaterializeLazySymbol) { {{Foo, JITSymbolFlags::Exported}, {Bar, static_cast(JITSymbolFlags::Exported | JITSymbolFlags::Weak)}}); - cantFail(V.defineLazy(InitialSymbols, *Source)); + cantFail(V.defineLazy(InitialSymbols, Source)); SymbolMap BarOverride; BarOverride[Bar] = JITEvaluatedSymbol(FakeBarAddr, JITSymbolFlags::Exported); -- 2.7.4