[PATCH] D104016: [Orc][examples] Join ListenerThread on early exit in LLJITWithRemoteDebugging

[Stefan Gränitz via Phabricator via llvm-commits]

sgraenitz added a comment.

However, while investigating I found the threading issue that this patch aims to fix. @lhames Might the unjoined thread have caused the assertion failure in SymbolStringPool?

I cannot really see the deleted ExecutionSession being the reason for the assertion failure. RemoteTargetProcessControl does pass on its SymbolStringPool, but this is a shared_ptr and it shouldn't be deleted until both, TPC and ES are destroyed right? Also, I don't see where I hold a SymbolStringPtr in the example code.

And I am still not sure why the example did exit early in the first place. We don't have RPC timeouts or anything when talking to the subprocess?



================
Comment at: llvm/examples/OrcV2Examples/LLJITWithRemoteDebugging/RemoteJITUtils.cpp:88
+  // might have been deleted already.
+  consumeError(disconnect());
+}
----------------
I agree that this is a bit of a design issue, but it's not straightforward to fix. IMHO the conceptual goal in [[ https://github.com/llvm/llvm-project/blob/258f055ed93661900bc568350e09f467c0950486/llvm/examples/OrcV2Examples/LLJITWithRemoteDebugging/LLJITWithRemoteDebugging.cpp#L216 | LLJITWithRemoteDebugging.cpp ]] is valid:
```
// Create LLJIT and destroy it before disconnecting the target process.
```

Also, I think it makes sense to report RPC-related errors via ExecutionSession. The current LLJIT interface requires to hand over ownership of the ExecutionSession to the JIT and once it goes out of scope it gets deleted. This causes the dangling reference in the OrcRPCTargetProcessControlBase's ErrorReporter unique_function, which is really bad yes. This is why we need to avoid `reportError()` in the destructor here.

What is the proper solution? I could imagine LLJIT could only "borrow" ownership of the ExecutionSession. It requires a mechanism to hand it back upon destruction.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D104016/new/

https://reviews.llvm.org/D104016