[all-commits] [llvm/llvm-project] 01e96c: [lldb] Don't unregister a listener that's being de...

[Pavel Labath via All-commits]

  Branch: refs/heads/main
  Home:   https://github.com/llvm/llvm-project
  Commit: 01e96c86497ac9670e1168134870beb99cbd4d8f
      https://github.com/llvm/llvm-project/commit/01e96c86497ac9670e1168134870beb99cbd4d8f
  Author: Pavel Labath <pavel at labath.sk>
  Date:   2024-07-02 (Tue, 02 Jul 2024)

  Changed paths:
    M lldb/source/Utility/Listener.cpp

  Log Message:
  -----------
  [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.



To unsubscribe from these emails, change your notification settings at https://github.com/llvm/llvm-project/settings/notifications