================ @@ -743,9 +743,28 @@ DebuggerSP Debugger::CreateInstance(lldb::LogOutputCallback log_callback, } void Debugger::HandleDestroyCallback() { - if (m_destroy_callback) { - m_destroy_callback(GetID(), m_destroy_callback_baton); - m_destroy_callback = nullptr; + std::lock_guard<std::recursive_mutex> guard(m_destroy_callback_mutex); + const lldb::user_id_t user_id = GetID(); + // This loop handles the case where callbacks are added/removed by existing + // callbacks during the loop, as the following: + // - Added callbacks will always be invoked. + // - Removed callbacks will never be invoked. That is *unless* the loop + // happens to invoke the said callbacks first, before they get removed. + // In this case, the callbacks gets invoked, and the removal return false. + // + // In the removal case, because the order of the container is random, it's + // wise to not depend on the order and instead implement logic inside the + // callbacks to decide if their work should be skipped. + while (m_destroy_callback_and_baton.size()) { + auto iter = m_destroy_callback_and_baton.begin(); + const lldb::destroy_callback_token_t &token = iter->first; + const lldb_private::DebuggerDestroyCallback &callback = iter->second.first; + void *const &baton = iter->second.second; + callback(user_id, baton); + // Using `token` to erase, because elements may have been added/removed, and + // that will cause error "invalid iterator access!" if `iter` is used + // instead. + m_destroy_callback_and_baton.erase(token); ---------------- jeffreytan81 wrote:
The biggest concern is that, this implementation completely changes the semantics -- now, client using this API has to re-register the callbacks after destroying debugger which is weird and not expected because there is no removeCallback() called from client. For example, the following user case won't work anymore: ``` void MyDestroyCallback() { ... } SBDebugger::AddDestroyCallbac(MyDestroyCallback); SBDebugger::Create(); ... SBDebugger::Destroy(); // Recreate another debugger SBDebugger::Create(); SBDebugger::Destroy(); // Now the MyDestroyCallback won't be called even user did not call RemoveDestroyCallback() which is not expected ``` There are several ways to handle this issue without clearing `m_destroy_callback_and_baton`. One simply way is making a copy of `m_destroy_callback_and_baton`, and calling callbacks from the copy (by checking if it still exists in original `m_destroy_callback_and_baton`). And at the end, checking there is no new entries in `m_destroy_callback_and_baton`, otherwise, getting the delta of the local copy and original copy, and redo the process in a loop. https://github.com/llvm/llvm-project/pull/89868 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits