Non-recursive mutexes encourage better locking discipline and avoid bugs like #96750, where one can unexpectedly re-enter the critical section on the same thread, and interrupt a presumed-indivisible operation. In this case, the only needed fix was to remove locking from some BroadcastManager functions, which were only called from the Listener class (and the listener already locked those mutexes to preserve lock ordering). While doing that, I noticed we don't have unit tests for these functions, so I added one.
erase() invalidates the iterator, we must use the returned iterator to continue iterating. This does not actually make a difference with the current implementation of SmallVector (erase will effectively return the same pointer), but it avoids future confusion.
These are hardcoded strings that are already present in the data section of the binary, no need to immediately place them in the ConstString StringPools. Lots of code still calls `GetBroadcasterClass` and places the return value into a ConstString. Changing that would be a good follow-up. Additionally, calls to these functions are still wrapped in ConstStrings at the SBAPI layer. This is because we must guarantee the lifetime of all strings handed out publicly.
Instead of passing the data to BroadcastEventIfUnique to create an Event object on the behalf of the caller, the caller can create the Event up-front.
BroadcastEvent currently takes its EventData* param and shoves it into an Event object, which takes ownership of the pointer and places it into a shared_ptr to manage the lifetime. Instead of relying on `new` and passing raw pointers around, I think it would make more sense to create the shared_ptr up front.
If `unique` is true, I was redoing the uniqueness check for the secondary listeners, which is racy since a "duplicate" event could come in between deciding to send the event to the main listener and checking for the shadow listener, and then the two streams would get out of sync. There's not a good way to write a direct test for this, but the test_shadow_listeners test has been flakey and this is the only racy part of that system I can find. So the test would be that that test_shadow_listeners becomes not flakey.
Before the addition of the process "Shadow Listener" you could only have one Listener observing the Process Broadcaster. That was necessary because fetching the Process event is what switches the public process state, and for the execution control logic to be manageable you needed to keep other listeners from causing this to happen before the main process control engine was ready. Ismail added the notion of a "ShadowListener" - which allowed you ONE extra process listener. This patch inverts that setup by designating the first listener as primary - and giving it priority in fetching events. Differential Revision: https://reviews.llvm.org/D157556
Broadcasters don't need their names in the StringPool. It doesn't benefit from fast comparisons and doesn't benefit from uniqueness. Differential Revision: https://reviews.llvm.org/D152220
The code inside Broadcaster makes usage of iterators using olden C++ coding style. Hidden in this old style is a couple of N^2 loops: we iterate over a map (sequentially), removing the first element that matches some predicate. The search is _always_ done from the start of the map, which implies that, if the map has N elements and if all matches happen on the second half of the map, then we visit the first N/2 elements exactly N/2 * N/2 times. Ideally some of the code here would benefit from `std::map`s own "erase_if", but this is only available with C++20: https://en.cppreference.com/w/cpp/container/map/erase_if We spent quite some time trying to make these loops more elegant, but it is surprisingly tricky to do so. Differential Revision: https://reviews.llvm.org/D150219
We had some custom classes that were used as the predicate for `std::find_if`. It would be a lot simpler if we used lambdas instead. Differential Revision: https://reviews.llvm.org/D150168