From 6ef4990daa1da215b25b1802f5d03cf1044f72bf Mon Sep 17 00:00:00 2001 From: Lang Hames Date: Tue, 3 Dec 2024 14:34:18 +1100 Subject: [PATCH] Re-apply "[ORC] Track all dependencies on symbols that aren't..." with fixes. This reapplies 427fb5cc5ac, which was reverted in 08c1a6b3e18 due to bot failures. The fix was to remove an incorrect assertion: In IL_emit, during the initial worklist loop, an EDU can have all of its dependencies removed without becoming ready (because it may still have implicit dependencies that will be added back during the subsequent propagateExtraEmitDeps operation). The EDU will be marked Ready at the end of IL_emit if its Dependencies set is empty at that point. Prior to that we can only assert that it's either Emitted or Ready (which is already covered by other assertions). --- llvm/lib/ExecutionEngine/Orc/Core.cpp | 15 +++-- .../ExecutionEngine/Orc/CoreAPIsTest.cpp | 65 +++++++++++++++++++ 2 files changed, 73 insertions(+), 7 deletions(-) diff --git a/llvm/lib/ExecutionEngine/Orc/Core.cpp b/llvm/lib/ExecutionEngine/Orc/Core.cpp index f226e81cc02a6d6..85022870164136e 100644 --- a/llvm/lib/ExecutionEngine/Orc/Core.cpp +++ b/llvm/lib/ExecutionEngine/Orc/Core.cpp @@ -938,7 +938,6 @@ Error JITDylib::resolve(MaterializationResponsibility &MR, auto &MI = MII->second; for (auto &Q : MI.takeQueriesMeeting(SymbolState::Resolved)) { Q->notifySymbolMetRequiredState(Name, ResolvedSym); - Q->removeQueryDependence(*this, Name); if (Q->isComplete()) CompletedQueries.insert(std::move(Q)); } @@ -1207,9 +1206,8 @@ void JITDylib::MaterializingInfo::removeQuery( PendingQueries, [&Q](const std::shared_ptr &V) { return V.get() == &Q; }); - assert(I != PendingQueries.end() && - "Query is not attached to this MaterializingInfo"); - PendingQueries.erase(I); + if (I != PendingQueries.end()) + PendingQueries.erase(I); } JITDylib::AsynchronousSymbolQueryList @@ -2615,6 +2613,12 @@ void ExecutionSession::OL_completeLookup( LLVM_DEBUG(dbgs() << "matched, symbol already in required state\n"); Q->notifySymbolMetRequiredState(Name, SymI->second.getSymbol()); + + // If this symbol is in anything other than the Ready state then + // we need to track the dependence. + if (SymI->second.getState() != SymbolState::Ready) + Q->addQueryDependence(JD, Name); + return true; } @@ -3165,7 +3169,6 @@ void ExecutionSession::IL_makeEDUEmitted( Q->notifySymbolMetRequiredState(SymbolStringPtr(Sym), Entry.getSymbol()); if (Q->isComplete()) Queries.insert(Q); - Q->removeQueryDependence(JD, SymbolStringPtr(Sym)); } } @@ -3317,8 +3320,6 @@ ExecutionSession::IL_emit(MaterializationResponsibility &MR, auto &DepMI = DepJD->MaterializingInfos[SymbolStringPtr(Dep)]; assert(DepMI.DefiningEDU && "Emitted symbol does not have a defining EDU"); - assert(!DepMI.DefiningEDU->Dependencies.empty() && - "Emitted symbol has empty dependencies (should be ready)"); assert(DepMI.DependantEDUs.empty() && "Already-emitted symbol has dependant EDUs?"); auto &DepEDUInfo = EDUInfos[DepMI.DefiningEDU.get()]; diff --git a/llvm/unittests/ExecutionEngine/Orc/CoreAPIsTest.cpp b/llvm/unittests/ExecutionEngine/Orc/CoreAPIsTest.cpp index a907dfcf2cec5b5..8ae05c4ddc59aed 100644 --- a/llvm/unittests/ExecutionEngine/Orc/CoreAPIsTest.cpp +++ b/llvm/unittests/ExecutionEngine/Orc/CoreAPIsTest.cpp @@ -518,6 +518,71 @@ TEST_F(CoreAPIsStandardTest, TestTrivialCircularDependency) { << "Self-dependency prevented symbol from being marked ready"; } +TEST_F(CoreAPIsStandardTest, TestBasicQueryDependenciesReporting) { + // Test that dependencies are reported as expected. + + bool DependenciesCallbackRan = false; + + std::unique_ptr FooR; + std::unique_ptr BarR; + + cantFail(JD.define(std::make_unique( + SymbolFlagsMap({{Foo, FooSym.getFlags()}}), + [&](std::unique_ptr R) { + FooR = std::move(R); + }))); + + cantFail(JD.define(std::make_unique( + SymbolFlagsMap({{Bar, BarSym.getFlags()}}), + [&](std::unique_ptr R) { + BarR = std::move(R); + }))); + + cantFail(JD.define(std::make_unique( + SymbolFlagsMap({{Baz, BazSym.getFlags()}}), + [&](std::unique_ptr R) { + cantFail(R->notifyResolved({{Baz, BazSym}})); + cantFail(R->notifyEmitted({})); + }))); + + // First issue a lookup for Foo and Bar so that we can put them + // into the required states for the test lookup below. + ES.lookup( + LookupKind::Static, makeJITDylibSearchOrder(&JD), + SymbolLookupSet({Foo, Bar}), SymbolState::Resolved, + [](Expected Result) { + EXPECT_THAT_EXPECTED(std::move(Result), Succeeded()); + }, + NoDependenciesToRegister); + + cantFail(FooR->notifyResolved({{Foo, FooSym}})); + cantFail(FooR->notifyEmitted({})); + + cantFail(BarR->notifyResolved({{Bar, BarSym}})); + + ES.lookup( + LookupKind::Static, makeJITDylibSearchOrder(&JD), + SymbolLookupSet({Foo, Bar, Baz}), SymbolState::Resolved, + [](Expected Result) { + EXPECT_THAT_EXPECTED(std::move(Result), Succeeded()); + }, + [&](const SymbolDependenceMap &Dependencies) { + EXPECT_EQ(Dependencies.size(), 1U) + << "Expect dependencies on only one JITDylib"; + EXPECT_TRUE(Dependencies.count(&JD)) + << "Expect dependencies on JD only"; + auto &Deps = Dependencies.begin()->second; + EXPECT_EQ(Deps.size(), 2U); + EXPECT_TRUE(Deps.count(Bar)); + EXPECT_TRUE(Deps.count(Baz)); + DependenciesCallbackRan = true; + }); + + cantFail(BarR->notifyEmitted({})); + + EXPECT_TRUE(DependenciesCallbackRan); +} + TEST_F(CoreAPIsStandardTest, TestCircularDependenceInOneJITDylib) { // Test that a circular symbol dependency between three symbols in a JITDylib // does not prevent any symbol from becoming 'ready' once all symbols are