clayborg added inline comments.
================
Comment at: lldb/source/Core/Debugger.cpp:761
m_instance_name.SetString(llvm::formatv("debugger_{0}", GetID()).str());
+ g_threadpool =
std::make_unique<llvm::ThreadPool>(llvm::optimal_concurrency());
if (log_callback)
----------------
clayborg wrote:
>
actually probably best to do it the way I originally described all inside of
"llvm::ThreadPool &Debugger::GetThreadPool()" since multiple debuggers can
exist within the same process.
================
Comment at: lldb/source/Core/Debugger.cpp:1969
+llvm::ThreadPool &Debugger::GetThreadPool() {
+ static llvm::ThreadPool threadpool(llvm::optimal_concurrency());
+ return threadpool;
----------------
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
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
