From: Lang Hames Date: Tue, 30 Apr 2019 00:03:26 +0000 (+0000) Subject: [ORC] Allow JITDylib definition generators to return Errors. X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=9a1c76870e4143c86be6bfe1a89b5240860672cb;p=llvm [ORC] Allow JITDylib definition generators to return Errors. Background: A definition generator can be attached to a JITDylib to generate new definitions in response to queries. For example: a generator that forwards calls to dlsym can map symbols from a dynamic library into the JIT process on demand. If definition generation fails then the generator should be able to return an error. This allows the JIT API to distinguish between the case where a generator does not provide a definition, and the case where it was not able to determine whether it provided a definition due to an error. The immediate motivation for this is cross-process symbol lookups: If the remote-lookup generator is attached to a JITDylib early in the search list, and if a generator failure is misinterpreted as "no definition in this JITDylib" then lookup may continue and bind to a different definition in a later JITDylib, which is a bug. git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@359521 91177308-0d34-0410-b5e6-96231b3b80d8 --- diff --git a/include/llvm/ExecutionEngine/Orc/Core.h b/include/llvm/ExecutionEngine/Orc/Core.h index a1522af6b85..57144118932 100644 --- a/include/llvm/ExecutionEngine/Orc/Core.h +++ b/include/llvm/ExecutionEngine/Orc/Core.h @@ -418,7 +418,7 @@ public: ReexportsGenerator(JITDylib &SourceJD, bool MatchNonExported = false, SymbolPredicate Allow = SymbolPredicate()); - SymbolNameSet operator()(JITDylib &JD, const SymbolNameSet &Names); + Expected operator()(JITDylib &JD, const SymbolNameSet &Names); private: JITDylib &SourceJD; @@ -497,7 +497,7 @@ class JITDylib { friend class ExecutionSession; friend class MaterializationResponsibility; public: - using GeneratorFunction = std::function( JITDylib &Parent, const SymbolNameSet &Names)>; using AsynchronousSymbolQuerySet = @@ -595,7 +595,7 @@ public: /// Search the given JITDylib for the symbols in Symbols. If found, store /// the flags for each symbol in Flags. Returns any unresolved symbols. - SymbolFlagsMap lookupFlags(const SymbolNameSet &Names); + Expected lookupFlags(const SymbolNameSet &Names); /// Dump current JITDylib state to OS. void dump(raw_ostream &OS); @@ -608,8 +608,8 @@ public: /// and the query will not be applied. The Query is not failed and can be /// re-used in a subsequent lookup once the symbols have been added, or /// manually failed. - SymbolNameSet legacyLookup(std::shared_ptr Q, - SymbolNameSet Names); + Expected + legacyLookup(std::shared_ptr Q, SymbolNameSet Names); private: using AsynchronousSymbolQueryList = @@ -645,12 +645,12 @@ private: Error defineImpl(MaterializationUnit &MU); - SymbolNameSet lookupFlagsImpl(SymbolFlagsMap &Flags, - const SymbolNameSet &Names); + Expected lookupFlagsImpl(SymbolFlagsMap &Flags, + const SymbolNameSet &Names); - void lodgeQuery(std::shared_ptr &Q, - SymbolNameSet &Unresolved, bool MatchNonExported, - MaterializationUnitList &MUs); + Error lodgeQuery(std::shared_ptr &Q, + SymbolNameSet &Unresolved, bool MatchNonExported, + MaterializationUnitList &MUs); void lodgeQueryImpl(std::shared_ptr &Q, SymbolNameSet &Unresolved, bool MatchNonExported, diff --git a/include/llvm/ExecutionEngine/Orc/ExecutionUtils.h b/include/llvm/ExecutionEngine/Orc/ExecutionUtils.h index 6841fb198f1..ae3ab8c095f 100644 --- a/include/llvm/ExecutionEngine/Orc/ExecutionUtils.h +++ b/include/llvm/ExecutionEngine/Orc/ExecutionUtils.h @@ -239,7 +239,7 @@ public: return Load(nullptr, GlobalPrefix, std::move(Allow)); } - SymbolNameSet operator()(JITDylib &JD, const SymbolNameSet &Names); + Expected operator()(JITDylib &JD, const SymbolNameSet &Names); private: sys::DynamicLibrary Dylib; diff --git a/lib/ExecutionEngine/Orc/Core.cpp b/lib/ExecutionEngine/Orc/Core.cpp index 4aa763feb79..eda07c915c7 100644 --- a/lib/ExecutionEngine/Orc/Core.cpp +++ b/lib/ExecutionEngine/Orc/Core.cpp @@ -696,17 +696,20 @@ Expected buildSimpleReexportsAliasMap(JITDylib &SourceJD, const SymbolNameSet &Symbols) { auto Flags = SourceJD.lookupFlags(Symbols); - if (Flags.size() != Symbols.size()) { + if (!Flags) + return Flags.takeError(); + + if (Flags->size() != Symbols.size()) { SymbolNameSet Unresolved = Symbols; - for (auto &KV : Flags) + for (auto &KV : *Flags) Unresolved.erase(KV.first); return make_error(std::move(Unresolved)); } SymbolAliasMap Result; for (auto &Name : Symbols) { - assert(Flags.count(Name) && "Missing entry in flags map"); - Result[Name] = SymbolAliasMapEntry(Name, Flags[Name]); + assert(Flags->count(Name) && "Missing entry in flags map"); + Result[Name] = SymbolAliasMapEntry(Name, (*Flags)[Name]); } return Result; @@ -718,14 +721,17 @@ ReexportsGenerator::ReexportsGenerator(JITDylib &SourceJD, : SourceJD(SourceJD), MatchNonExported(MatchNonExported), Allow(std::move(Allow)) {} -SymbolNameSet ReexportsGenerator::operator()(JITDylib &JD, - const SymbolNameSet &Names) { +Expected +ReexportsGenerator::operator()(JITDylib &JD, const SymbolNameSet &Names) { orc::SymbolNameSet Added; orc::SymbolAliasMap AliasMap; auto Flags = SourceJD.lookupFlags(Names); - for (auto &KV : Flags) { + if (!Flags) + return Flags.takeError(); + + for (auto &KV : *Flags) { if (Allow && !Allow(KV.first)) continue; AliasMap[KV.first] = SymbolAliasMapEntry(KV.first, KV.second); @@ -1171,16 +1177,23 @@ Error JITDylib::remove(const SymbolNameSet &Names) { }); } -SymbolFlagsMap JITDylib::lookupFlags(const SymbolNameSet &Names) { - return ES.runSessionLocked([&, this]() { +Expected JITDylib::lookupFlags(const SymbolNameSet &Names) { + return ES.runSessionLocked([&, this]() -> Expected { SymbolFlagsMap Result; auto Unresolved = lookupFlagsImpl(Result, Names); - if (DefGenerator && !Unresolved.empty()) { - auto NewDefs = DefGenerator(*this, Unresolved); - if (!NewDefs.empty()) { - auto Unresolved2 = lookupFlagsImpl(Result, NewDefs); + if (!Unresolved) + return Unresolved.takeError(); + + if (DefGenerator && !Unresolved->empty()) { + auto NewDefs = DefGenerator(*this, *Unresolved); + if (!NewDefs) + return NewDefs.takeError(); + if (!NewDefs->empty()) { + auto Unresolved2 = lookupFlagsImpl(Result, *NewDefs); + if (!Unresolved2) + return Unresolved2.takeError(); (void)Unresolved2; - assert(Unresolved2.empty() && + assert(Unresolved2->empty() && "All fallback defs should have been found by lookupFlagsImpl"); } }; @@ -1188,8 +1201,8 @@ SymbolFlagsMap JITDylib::lookupFlags(const SymbolNameSet &Names) { }); } -SymbolNameSet JITDylib::lookupFlagsImpl(SymbolFlagsMap &Flags, - const SymbolNameSet &Names) { +Expected JITDylib::lookupFlagsImpl(SymbolFlagsMap &Flags, + const SymbolNameSet &Names) { SymbolNameSet Unresolved; for (auto &Name : Names) { @@ -1207,22 +1220,26 @@ SymbolNameSet JITDylib::lookupFlagsImpl(SymbolFlagsMap &Flags, return Unresolved; } -void JITDylib::lodgeQuery(std::shared_ptr &Q, - SymbolNameSet &Unresolved, bool MatchNonExported, - MaterializationUnitList &MUs) { +Error JITDylib::lodgeQuery(std::shared_ptr &Q, + SymbolNameSet &Unresolved, bool MatchNonExported, + MaterializationUnitList &MUs) { assert(Q && "Query can not be null"); lodgeQueryImpl(Q, Unresolved, MatchNonExported, MUs); if (DefGenerator && !Unresolved.empty()) { auto NewDefs = DefGenerator(*this, Unresolved); - if (!NewDefs.empty()) { - for (auto &D : NewDefs) + if (!NewDefs) + return NewDefs.takeError(); + if (!NewDefs->empty()) { + for (auto &D : *NewDefs) Unresolved.erase(D); - lodgeQueryImpl(Q, NewDefs, MatchNonExported, MUs); - assert(NewDefs.empty() && + lodgeQueryImpl(Q, *NewDefs, MatchNonExported, MUs); + assert(NewDefs->empty() && "All fallback defs should have been found by lookupImpl"); } } + + return Error::success(); } void JITDylib::lodgeQueryImpl( @@ -1294,8 +1311,9 @@ void JITDylib::lodgeQueryImpl( Unresolved.erase(Name); } -SymbolNameSet JITDylib::legacyLookup(std::shared_ptr Q, - SymbolNameSet Names) { +Expected +JITDylib::legacyLookup(std::shared_ptr Q, + SymbolNameSet Names) { assert(Q && "Query can not be null"); ES.runOutstandingMUs(); @@ -1304,22 +1322,28 @@ SymbolNameSet JITDylib::legacyLookup(std::shared_ptr Q, std::vector> MUs; SymbolNameSet Unresolved = std::move(Names); - ES.runSessionLocked([&, this]() { + auto Err = ES.runSessionLocked([&, this]() -> Error { ActionFlags = lookupImpl(Q, MUs, Unresolved); if (DefGenerator && !Unresolved.empty()) { assert(ActionFlags == None && "ActionFlags set but unresolved symbols remain?"); auto NewDefs = DefGenerator(*this, Unresolved); - if (!NewDefs.empty()) { - for (auto &D : NewDefs) + if (!NewDefs) + return NewDefs.takeError(); + if (!NewDefs->empty()) { + for (auto &D : *NewDefs) Unresolved.erase(D); - ActionFlags = lookupImpl(Q, MUs, NewDefs); - assert(NewDefs.empty() && + ActionFlags = lookupImpl(Q, MUs, *NewDefs); + assert(NewDefs->empty() && "All fallback defs should have been found by lookupImpl"); } } + return Error::success(); }); + if (Err) + return std::move(Err); + assert((MUs.empty() || ActionFlags == None) && "If action flags are set, there should be no work to do (so no MUs)"); @@ -1761,34 +1785,29 @@ void ExecutionSession::lookup( Unresolved, std::move(OnResolve), std::move(OnReady)); bool QueryIsFullyResolved = false; bool QueryIsFullyReady = false; - bool QueryFailed = false; - runSessionLocked([&]() { - for (auto &KV : SearchOrder) { - assert(KV.first && "JITDylibList entries must not be null"); - assert(!CollectedMUsMap.count(KV.first) && - "JITDylibList should not contain duplicate entries"); - - auto &JD = *KV.first; - auto MatchNonExported = KV.second; - JD.lodgeQuery(Q, Unresolved, MatchNonExported, CollectedMUsMap[&JD]); - } + auto LodgingErr = runSessionLocked([&]() -> Error { + auto LodgeQuery = [&]() -> Error { + for (auto &KV : SearchOrder) { + assert(KV.first && "JITDylibList entries must not be null"); + assert(!CollectedMUsMap.count(KV.first) && + "JITDylibList should not contain duplicate entries"); + + auto &JD = *KV.first; + auto MatchNonExported = KV.second; + if (auto Err = JD.lodgeQuery(Q, Unresolved, MatchNonExported, + CollectedMUsMap[&JD])) + return Err; + } - if (Unresolved.empty()) { - // Query lodged successfully. + if (!Unresolved.empty()) + return make_error(std::move(Unresolved)); - // Record whether this query is fully ready / resolved. We will use - // this to call handleFullyResolved/handleFullyReady outside the session - // lock. - QueryIsFullyResolved = Q->isFullyResolved(); - QueryIsFullyReady = Q->isFullyReady(); + return Error::success(); + }; - // Call the register dependencies function. - if (RegisterDependencies && !Q->QueryRegistrations.empty()) - RegisterDependencies(Q->QueryRegistrations); - } else { - // Query failed due to unresolved symbols. - QueryFailed = true; + if (auto Err = LodgeQuery()) { + // Query failed. // Disconnect the query from its dependencies. Q->detach(); @@ -1797,11 +1816,27 @@ void ExecutionSession::lookup( for (auto &KV : CollectedMUsMap) for (auto &MU : KV.second) KV.first->replace(std::move(MU)); + + return Err; } + + // Query lodged successfully. + + // Record whether this query is fully ready / resolved. We will use + // this to call handleFullyResolved/handleFullyReady outside the session + // lock. + QueryIsFullyResolved = Q->isFullyResolved(); + QueryIsFullyReady = Q->isFullyReady(); + + // Call the register dependencies function. + if (RegisterDependencies && !Q->QueryRegistrations.empty()) + RegisterDependencies(Q->QueryRegistrations); + + return Error::success(); }); - if (QueryFailed) { - Q->handleFailed(make_error(std::move(Unresolved))); + if (LodgingErr) { + Q->handleFailed(std::move(LodgingErr)); return; } else { if (QueryIsFullyResolved) diff --git a/lib/ExecutionEngine/Orc/ExecutionUtils.cpp b/lib/ExecutionEngine/Orc/ExecutionUtils.cpp index 195e9396671..deabb040670 100644 --- a/lib/ExecutionEngine/Orc/ExecutionUtils.cpp +++ b/lib/ExecutionEngine/Orc/ExecutionUtils.cpp @@ -193,8 +193,9 @@ DynamicLibrarySearchGenerator::Load(const char *FileName, char GlobalPrefix, std::move(Allow)); } -SymbolNameSet DynamicLibrarySearchGenerator:: -operator()(JITDylib &JD, const SymbolNameSet &Names) { +Expected +DynamicLibrarySearchGenerator::operator()(JITDylib &JD, + const SymbolNameSet &Names) { orc::SymbolNameSet Added; orc::SymbolMap NewSymbols; diff --git a/unittests/ExecutionEngine/Orc/CoreAPIsTest.cpp b/unittests/ExecutionEngine/Orc/CoreAPIsTest.cpp index 7b29f4c90da..ec167a1cdbd 100644 --- a/unittests/ExecutionEngine/Orc/CoreAPIsTest.cpp +++ b/unittests/ExecutionEngine/Orc/CoreAPIsTest.cpp @@ -216,7 +216,7 @@ TEST_F(CoreAPIsStandardTest, ChainedJITDylibLookup) { OnReadyRun = true; }); - JD2.legacyLookup(Q, JD.legacyLookup(Q, {Foo})); + cantFail(JD2.legacyLookup(Q, cantFail(JD.legacyLookup(Q, {Foo})))); EXPECT_TRUE(OnResolvedRun) << "OnResolved was not run for empty query"; EXPECT_TRUE(OnReadyRun) << "OnReady was not run for empty query"; @@ -260,7 +260,7 @@ TEST_F(CoreAPIsStandardTest, LookupFlagsTest) { SymbolNameSet Names({Foo, Bar, Baz}); - auto SymbolFlags = JD.lookupFlags(Names); + auto SymbolFlags = cantFail(JD.lookupFlags(Names)); EXPECT_EQ(SymbolFlags.size(), 2U) << "Returned symbol flags contains unexpected results"; @@ -273,6 +273,26 @@ TEST_F(CoreAPIsStandardTest, LookupFlagsTest) { << "Incorrect flags returned for Bar"; } +TEST_F(CoreAPIsStandardTest, LookupWithGeneratorFailure) { + + class BadGenerator { + public: + Expected operator()(JITDylib &, const SymbolNameSet &) { + return make_error("BadGenerator", inconvertibleErrorCode()); + } + }; + + JD.setGenerator(BadGenerator()); + + EXPECT_THAT_ERROR(JD.lookupFlags({Foo}).takeError(), Failed()) + << "Generator failure did not propagate through lookupFlags"; + + EXPECT_THAT_ERROR( + ES.lookup(JITDylibSearchList({{&JD, false}}), {Foo}).takeError(), + Failed()) + << "Generator failure did not propagate through lookup"; +} + TEST_F(CoreAPIsStandardTest, TestBasicAliases) { cantFail(JD.define(absoluteSymbols({{Foo, FooSym}, {Bar, BarSym}}))); cantFail(JD.define(symbolAliases({{Baz, {Foo, JITSymbolFlags::Exported}}, @@ -356,7 +376,7 @@ TEST_F(CoreAPIsStandardTest, TestReexportsGenerator) { JD.setGenerator(ReexportsGenerator(JD2, false, Filter)); - auto Flags = JD.lookupFlags({Foo, Bar, Baz}); + auto Flags = cantFail(JD.lookupFlags({Foo, Bar, Baz})); EXPECT_EQ(Flags.size(), 1U) << "Unexpected number of results"; EXPECT_EQ(Flags[Foo], FooSym.getFlags()) << "Unexpected flags for Foo"; diff --git a/unittests/ExecutionEngine/Orc/LegacyAPIInteropTest.cpp b/unittests/ExecutionEngine/Orc/LegacyAPIInteropTest.cpp index ea7479ff1c2..dca3eeb75fe 100644 --- a/unittests/ExecutionEngine/Orc/LegacyAPIInteropTest.cpp +++ b/unittests/ExecutionEngine/Orc/LegacyAPIInteropTest.cpp @@ -24,7 +24,7 @@ TEST_F(LegacyAPIsStandardTest, TestLambdaSymbolResolver) { auto Resolver = createSymbolResolver( [&](const SymbolNameSet &Symbols) { - auto FlagsMap = JD.lookupFlags(Symbols); + auto FlagsMap = cantFail(JD.lookupFlags(Symbols)); SymbolNameSet Result; for (auto &KV : FlagsMap) if (!KV.second.isStrong()) @@ -32,7 +32,7 @@ TEST_F(LegacyAPIsStandardTest, TestLambdaSymbolResolver) { return Result; }, [&](std::shared_ptr Q, SymbolNameSet Symbols) { - return JD.legacyLookup(std::move(Q), Symbols); + return cantFail(JD.legacyLookup(std::move(Q), Symbols)); }); auto RS = Resolver->getResponsibilitySet(SymbolNameSet({Bar, Baz}));