llunak marked 2 inline comments as done. llunak added inline comments.
================ Comment at: lldb/source/Core/Debugger.cpp:1969 +llvm::ThreadPool &Debugger::GetThreadPool() { + static llvm::ThreadPool threadpool(llvm::optimal_concurrency()); + return threadpool; ---------------- clayborg wrote: > clayborg wrote: > > llunak wrote: > > > clayborg wrote: > > > > aganea wrote: > > > > > clayborg wrote: > > > > > > aganea wrote: > > > > > > > Ideally this should be explicitly created on the stack & passed > > > > > > > along on the callstack or in a context, like MLIR does: > > > > > > > https://github.com/llvm/llvm-project/blob/main/mlir/include/mlir/IR/MLIRContext.h#L48 > > > > > > We need one instance of the thread pool so we can't create on the > > > > > > stack. > > > > > > > > > > > > Static is not great because of the C++ global destructor chain > > > > > > could try and use if "main" exits and we still have threads > > > > > > running. I would do something like the "g_debugger_list_ptr" where > > > > > > you create a static variable in Debugger.cpp:105 which is a > > > > > > pointer, and we initialize it inside of Debugger::Initialize() like > > > > > > we do for "g_debugger_list_ptr". Then the thread pool will not > > > > > > cause shutdown crashes when "main" exits before other threads. > > > > > > > > > > > I meant having the `ThreadPool` in a context created by main() or the > > > > > lib caller, before getting here in library/plugin code, and passed > > > > > along. LLD does that too: > > > > > https://github.com/llvm/llvm-project/blob/main/lld/ELF/Driver.cpp#L94 > > > > > - the statics in `Debugger.cpp` seems like a workaround for that. In > > > > > any case, it's better than having the `static` here. > > > > In LLDB, the lldb_private::Debugger has static functions that hand out > > > > things that are needed for the debugger to do its work, so I like the > > > > static function here. We don't allow any llvm or clang code to make it > > > > through our public API (in lldb::SB* classes), so we do need code > > > > inside the debugger to be able to access global resources and the > > > > debugger class is where this should live. > > > > > > > > We can also just have code like: > > > > ``` > > > > // NOTE: intentional leak to avoid issues with C++ destructor chain > > > > static llvm::ThreadPool *g_thread_pool = nullptr; > > > > static llvm::once_flag g_once_flag; > > > > llvm::call_once(g_once_flag, []() { > > > > g_thread_pool = new llvm::ThreadPool(llvm::optimal_concurrency()); > > > > }); > > > > return *g_thread_pool; > > > > ``` > > > > > > > I've changed the code to initialize/cleanup from Debugger ctor/dtor. > > > > > You can't do this as there can be more than one debugger. This needs to be > > a true global and use a once_flag. > > > > Xcode creates one debugger per target window, that way each one has its own > > command interpreter etc; > Probably best to do it as described above where everything is contained in > this function due to multiple debuggers being possible, so we can't tie > anything to the lifetime of any debugger and we need to not crash when the > main thread exits first I see, I thought there could be just one Debugger instance. I'll use the call_once code from above then. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D123226/new/ https://reviews.llvm.org/D123226 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits