[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