[llvm] [JIT] Fix crash in unit tests (PR #113492)
Kai Nacke via llvm-commits
llvm-commits at lists.llvm.org
Wed Oct 23 19:11:29 PDT 2024
================
@@ -59,12 +59,13 @@ class JITLinkRedirectableSymbolManager : public RedirectableSymbolManager,
ObjLinkingLayer.getExecutionSession().getTargetTriple())),
PtrJumpStubCreator(jitlink::getPointerJumpStubCreator(
ObjLinkingLayer.getExecutionSession().getTargetTriple())) {
+ ErrorAsOutParameter _(&Err);
+ ObjLinkingLayer.getExecutionSession().registerResourceManager(*this);
if (!AnonymousPtrCreator || !PtrJumpStubCreator)
Err = make_error<StringError>("Architecture not supported",
inconvertibleErrorCode());
if (Err)
return;
- ObjLinkingLayer.getExecutionSession().registerResourceManager(*this);
----------------
redstar wrote:
Without moving the line up, no resource manager is registered, and I get an assertion in the destructor:
```
~JITLinkRedirectableSymbolManager() {
ObjLinkingLayer.getExecutionSession().deregisterResourceManager(*this);
}
```
because of the check in the `else` branch:
```
void ExecutionSession::deregisterResourceManager(ResourceManager &RM) {
runSessionLocked([&] {
assert(!ResourceManagers.empty() && "No managers registered");
if (ResourceManagers.back() == &RM)
ResourceManagers.pop_back();
else {
auto I = llvm::find(ResourceManagers, &RM);
assert(I != ResourceManagers.end() && "RM not registered");
ResourceManagers.erase(I);
}
});
}
```
So yes, moving it up is necessary. And the `if (Err) return;` is then meaningless.
I'm running with assertions enabled, btw.
https://github.com/llvm/llvm-project/pull/113492
More information about the llvm-commits
mailing list