Author: Pavel Labath Date: 2024-07-02T10:08:03+02:00 New Revision: 01e96c86497ac9670e1168134870beb99cbd4d8f
URL: https://github.com/llvm/llvm-project/commit/01e96c86497ac9670e1168134870beb99cbd4d8f DIFF: https://github.com/llvm/llvm-project/commit/01e96c86497ac9670e1168134870beb99cbd4d8f.diff LOG: [lldb] Don't unregister a listener that's being destroyed (#97300) It's not necessary because the broadcasters hold a weak_ptr (*) to it, and will delete the weak_ptr next time they try to lock it. Doing this prevents recursion in RemoveListener, where the function can end up holding the only shared_ptr to a listener, and its destruction can trigger another call to RemoveListener -- which will mess up the state of the first instance. This is the same bug that we've have fixed in https://reviews.llvm.org/D23406, but it was effectively undone in https://reviews.llvm.org/D157556. With the addition of a primary listener, a fix like D23406 becomes unwieldy (and it has already shown itself to be fragile), which is why this patch attempts a different approach. Like in 2016, I don't know a good way to unit test this bug, since it depends on precise timing, but the thing I like about this approach is that it enables us to change the broadcaster mutex into a non-recursive one. While that doesn't prevent the bug from happening again, it will make it much easier to spot in the future, as the code will hang with a smoking gun (instead of crashing a little while later). I'm going to attempt that in a separate patch to minimize disruption. (*) Technically a broadcaster holds the *primary* listener as a shared_ptr, but that's still ok as it means that listener will not get destroyed until it is explicitly removed. Added: Modified: lldb/source/Utility/Listener.cpp Removed: ################################################################################ diff --git a/lldb/source/Utility/Listener.cpp b/lldb/source/Utility/Listener.cpp index 6a74c530ad257..5aacb4104e1cf 100644 --- a/lldb/source/Utility/Listener.cpp +++ b/lldb/source/Utility/Listener.cpp @@ -30,7 +30,7 @@ Listener::Listener(const char *name) Listener::~Listener() { Log *log = GetLog(LLDBLog::Object); - Clear(); + // Don't call Clear() from here as that can cause races. See #96750. LLDB_LOGF(log, "%p Listener::%s('%s')", static_cast<void *>(this), __FUNCTION__, m_name.c_str()); _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
