[llvm] ca6f584 - [ORC] Fix symbol dependence propagation algorithm in ObjectLinkingLayer.
Lang Hames via llvm-commits
llvm-commits at lists.llvm.org
Tue Feb 11 12:57:01 PST 2020
Author: Lang Hames
Date: 2020-02-11T12:56:41-08:00
New Revision: ca6f58486ffda249a4330f5a9e20266ea8a40806
URL: https://github.com/llvm/llvm-project/commit/ca6f58486ffda249a4330f5a9e20266ea8a40806
DIFF: https://github.com/llvm/llvm-project/commit/ca6f58486ffda249a4330f5a9e20266ea8a40806.diff
LOG: [ORC] Fix symbol dependence propagation algorithm in ObjectLinkingLayer.
ObjectLinkingLayer was not correctly propagating dependencies through local
symbols within an object. This could cause symbol lookup to return before a
searched-for symbol is ready if the following conditions are met:
(1) The definition of the symbol being searched for transitively depends on a
local symbol within the same object, and that local symbol in turn
transitively depends on an external symbol provided by some other module
in the JIT.
(2) Concurrent compilation is enabled.
(3) Thread scheduling causes the lookup of the searched-for symbol to return
before all transitive dependencies of the looked-up symbol are emitted.
This bug was found by inspection and has not been observed in practice.
A jitlink test case has been added to verify that symbol dependencies are
correctly propagated through local symbol definitions.
Added:
llvm/test/ExecutionEngine/JITLink/X86/LocalDependencyPropagation.s
Modified:
llvm/lib/ExecutionEngine/Orc/ObjectLinkingLayer.cpp
Removed:
################################################################################
diff --git a/llvm/lib/ExecutionEngine/Orc/ObjectLinkingLayer.cpp b/llvm/lib/ExecutionEngine/Orc/ObjectLinkingLayer.cpp
index 2572b7f4878d..6575b0a6ccb2 100644
--- a/llvm/lib/ExecutionEngine/Orc/ObjectLinkingLayer.cpp
+++ b/llvm/lib/ExecutionEngine/Orc/ObjectLinkingLayer.cpp
@@ -84,6 +84,12 @@ class ObjectLinkingLayerJITLinkContext final : public JITLinkContext {
}
};
+ for (auto &KV : InternalNamedSymbolDeps) {
+ SymbolDependenceMap InternalDeps;
+ InternalDeps[&MR.getTargetJITDylib()] = std::move(KV.second);
+ MR.addDependencies(KV.first, InternalDeps);
+ }
+
ES.lookup(LookupKind::Static, SearchOrder, std::move(LookupSet),
SymbolState::Resolved, std::move(OnResolve),
[this](const SymbolDependenceMap &Deps) {
@@ -168,16 +174,18 @@ class ObjectLinkingLayerJITLinkContext final : public JITLinkContext {
// link graph to build the symbol dependence graph.
Config.PrePrunePasses.push_back(
[this](LinkGraph &G) { return externalizeWeakAndCommonSymbols(G); });
- Config.PostPrunePasses.push_back(
- [this](LinkGraph &G) { return computeNamedSymbolDependencies(G); });
Layer.modifyPassConfig(MR, TT, Config);
+ Config.PostPrunePasses.push_back(
+ [this](LinkGraph &G) { return computeNamedSymbolDependencies(G); });
+
return Error::success();
}
private:
- using AnonToNamedDependenciesMap = DenseMap<const Symbol *, SymbolNameSet>;
+ using JITLinkSymbolSet = DenseSet<const Symbol *>;
+ using LocalToNamedDependenciesMap = DenseMap<const Symbol *, JITLinkSymbolSet>;
Error externalizeWeakAndCommonSymbols(LinkGraph &G) {
auto &ES = Layer.getExecutionSession();
@@ -206,90 +214,107 @@ class ObjectLinkingLayerJITLinkContext final : public JITLinkContext {
Error computeNamedSymbolDependencies(LinkGraph &G) {
auto &ES = MR.getTargetJITDylib().getExecutionSession();
- auto AnonDeps = computeAnonDeps(G);
+ auto LocalDeps = computeLocalDeps(G);
for (auto *Sym : G.defined_symbols()) {
- // Skip anonymous and non-global atoms: we do not need dependencies for
- // these.
+ // Skip local symbols: we do not track dependencies for these.
if (Sym->getScope() == Scope::Local)
continue;
+ assert(Sym->hasName() &&
+ "Defined non-local jitlink::Symbol should have a name");
- auto SymName = ES.intern(Sym->getName());
- SymbolNameSet &SymDeps = NamedSymbolDeps[SymName];
+ SymbolNameSet ExternalSymDeps, InternalSymDeps;
+ // Find internal and external named symbol dependencies.
for (auto &E : Sym->getBlock().edges()) {
auto &TargetSym = E.getTarget();
- if (TargetSym.getScope() != Scope::Local)
- SymDeps.insert(ES.intern(TargetSym.getName()));
- else {
+ if (TargetSym.getScope() != Scope::Local) {
+ if (TargetSym.isExternal())
+ ExternalSymDeps.insert(ES.intern(TargetSym.getName()));
+ else if (&TargetSym != Sym)
+ InternalSymDeps.insert(ES.intern(TargetSym.getName()));
+ } else {
assert(TargetSym.isDefined() &&
- "Anonymous/local symbols must be defined");
- auto I = AnonDeps.find(&TargetSym);
- if (I != AnonDeps.end())
- for (auto &S : I->second)
- SymDeps.insert(S);
+ "local symbols must be defined");
+ auto I = LocalDeps.find(&TargetSym);
+ if (I != LocalDeps.end())
+ for (auto &S : I->second) {
+ assert(S->hasName() &&
+ "LocalDeps should only contain named values");
+ if (S->isExternal())
+ ExternalSymDeps.insert(ES.intern(S->getName()));
+ else if (S != Sym)
+ InternalSymDeps.insert(ES.intern(S->getName()));
+ }
}
}
+
+ if (ExternalSymDeps.empty() && InternalSymDeps.empty())
+ continue;
+
+ auto SymName = ES.intern(Sym->getName());
+ if (!ExternalSymDeps.empty())
+ ExternalNamedSymbolDeps[SymName] = std::move(ExternalSymDeps);
+ if (!InternalSymDeps.empty())
+ InternalNamedSymbolDeps[SymName] = std::move(InternalSymDeps);
}
return Error::success();
}
- AnonToNamedDependenciesMap computeAnonDeps(LinkGraph &G) {
+ LocalToNamedDependenciesMap computeLocalDeps(LinkGraph &G) {
+ LocalToNamedDependenciesMap DepMap;
- auto &ES = MR.getTargetJITDylib().getExecutionSession();
- AnonToNamedDependenciesMap DepMap;
-
- // For all anonymous symbols:
+ // For all local symbols:
// (1) Add their named dependencies.
// (2) Add them to the worklist for further iteration if they have any
- // depend on any other anonymous symbols.
+ // depend on any other local symbols.
struct WorklistEntry {
- WorklistEntry(Symbol *Sym, DenseSet<Symbol *> SymAnonDeps)
- : Sym(Sym), SymAnonDeps(std::move(SymAnonDeps)) {}
+ WorklistEntry(Symbol *Sym, DenseSet<Symbol *> LocalDeps)
+ : Sym(Sym), LocalDeps(std::move(LocalDeps)) {}
Symbol *Sym = nullptr;
- DenseSet<Symbol *> SymAnonDeps;
+ DenseSet<Symbol *> LocalDeps;
};
std::vector<WorklistEntry> Worklist;
for (auto *Sym : G.defined_symbols())
- if (!Sym->hasName()) {
+ if (Sym->getScope() == Scope::Local) {
auto &SymNamedDeps = DepMap[Sym];
- DenseSet<Symbol *> SymAnonDeps;
+ DenseSet<Symbol *> LocalDeps;
for (auto &E : Sym->getBlock().edges()) {
auto &TargetSym = E.getTarget();
- if (TargetSym.hasName())
- SymNamedDeps.insert(ES.intern(TargetSym.getName()));
+ if (TargetSym.getScope() != Scope::Local)
+ SymNamedDeps.insert(&TargetSym);
else {
assert(TargetSym.isDefined() &&
- "Anonymous symbols must be defined");
- SymAnonDeps.insert(&TargetSym);
+ "local symbols must be defined");
+ LocalDeps.insert(&TargetSym);
}
}
- if (!SymAnonDeps.empty())
- Worklist.push_back(WorklistEntry(Sym, std::move(SymAnonDeps)));
+ if (!LocalDeps.empty())
+ Worklist.push_back(WorklistEntry(Sym, std::move(LocalDeps)));
}
- // Loop over all anonymous symbols with anonymous dependencies, propagating
- // their respective *named* dependencies. Iterate until we hit a stable
+ // Loop over all local symbols with local dependencies, propagating
+ // their respective non-local dependencies. Iterate until we hit a stable
// state.
bool Changed;
do {
Changed = false;
for (auto &WLEntry : Worklist) {
auto *Sym = WLEntry.Sym;
- auto &SymNamedDeps = DepMap[Sym];
- auto &SymAnonDeps = WLEntry.SymAnonDeps;
+ auto &NamedDeps = DepMap[Sym];
+ auto &LocalDeps = WLEntry.LocalDeps;
- for (auto *TargetSym : SymAnonDeps) {
+ for (auto *TargetSym : LocalDeps) {
auto I = DepMap.find(TargetSym);
if (I != DepMap.end())
for (const auto &S : I->second)
- Changed |= SymNamedDeps.insert(S).second;
+ Changed |= NamedDeps.insert(S).second;
}
}
} while (Changed);
@@ -298,7 +323,7 @@ class ObjectLinkingLayerJITLinkContext final : public JITLinkContext {
}
void registerDependencies(const SymbolDependenceMap &QueryDeps) {
- for (auto &NamedDepsEntry : NamedSymbolDeps) {
+ for (auto &NamedDepsEntry : ExternalNamedSymbolDeps) {
auto &Name = NamedDepsEntry.first;
auto &NameDeps = NamedDepsEntry.second;
SymbolDependenceMap SymbolDeps;
@@ -323,7 +348,8 @@ class ObjectLinkingLayerJITLinkContext final : public JITLinkContext {
ObjectLinkingLayer &Layer;
MaterializationResponsibility MR;
std::unique_ptr<MemoryBuffer> ObjBuffer;
- DenseMap<SymbolStringPtr, SymbolNameSet> NamedSymbolDeps;
+ DenseMap<SymbolStringPtr, SymbolNameSet> ExternalNamedSymbolDeps;
+ DenseMap<SymbolStringPtr, SymbolNameSet> InternalNamedSymbolDeps;
};
ObjectLinkingLayer::Plugin::~Plugin() {}
diff --git a/llvm/test/ExecutionEngine/JITLink/X86/LocalDependencyPropagation.s b/llvm/test/ExecutionEngine/JITLink/X86/LocalDependencyPropagation.s
new file mode 100644
index 000000000000..e44e39f67fae
--- /dev/null
+++ b/llvm/test/ExecutionEngine/JITLink/X86/LocalDependencyPropagation.s
@@ -0,0 +1,31 @@
+# REQUIRES: asserts
+# RUN: llvm-mc -triple=x86_64-apple-macosx10.9 -filetype=obj -o %t %s
+# RUN: llvm-jitlink -debug-only=orc -noexec -define-abs _external_func=0x1 \
+# RUN: -entry=_foo %t 2>&1 | FileCheck %s
+#
+# Verify that symbol dependencies are correctly propagated through local
+# symbols: _baz depends on _foo indirectly via the local symbol _bar. We expect
+# _baz to depend on _foo, and _foo on _external_func.
+
+# CHECK-DAG: In <main> adding dependencies for _foo: { (<main>, { _external_func }) }
+# CHECK-DAG: In <main> adding dependencies for _baz: { (<main>, { _foo }) }
+
+ .section __TEXT,__text,regular,pure_instructions
+
+ .globl _foo
+ .p2align 4, 0x90
+_foo:
+ jmp _external_func
+
+ .p2align 4, 0x90
+_bar:
+
+ jmp _foo
+
+ .globl _baz
+ .p2align 4, 0x90
+_baz:
+
+ jmp _bar
+
+.subsections_via_symbols
More information about the llvm-commits
mailing list