[Lldb-commits] [lldb] r269377 - Fix some long standing issues that caused tests to be flaky.

Greg Clayton via lldb-commits lldb-commits at lists.llvm.org
Thu May 12 15:58:53 PDT 2016


Author: gclayton
Date: Thu May 12 17:58:52 2016
New Revision: 269377

URL: http://llvm.org/viewvc/llvm-project?rev=269377&view=rev
Log:
Fix some long standing issues that caused tests to be flaky. 

The main issues were:
- Listeners recently were converted over to used by getting a shared pointer to a listener. And when they listened to broadcasters they would get a strong reference added to them meaning the listeners would never go away. This caused memory usage to increase and would cause performance issue if many steps were done.
- The lldb_private::Process private state thread had an issue where if a "stop" contol signal was attempted to be sent to that thread, it could end up not responding in 2 seconds and end up getting cancelled which might cause us to cancel a thread that had a mutex locked and it would deadlock the test.

This change makes broadcasters hold onto weak references to listeners. It also fixes some bad threading code that had races inside of it by making the m_events_mutex be non-recursive and getting rid of fragile use of a Predicate<bool> to say that new events are available, and replacing it with using the m_events_mutex with a new m_events_condition to control access to the events in a safer way.

The private state thread now uses a safer way to communicate that the control event has been received by the private state thread: it makes a EventDataReceipt instance that it attaches to the event that sends the control to the private state thread and used this to synchronize the fact that the private state thread has received the event instead of using a Predicate<bool> to convey the info. When the signal event is received, it will pull the event off of the queue in the private state thread and cause the EventData::DoOnRemoval() to be called, which will signal that the event has been received. This cleans up the signal delivery notification so it doesn't rely on a member variable of the process class to convey the info.

std::shared_ptr<EventDataReceipt> event_receipt_sp(new EventDataReceipt());
m_private_state_control_broadcaster.BroadcastEvent(signal, event_receipt_sp);

<rdar://problem/26256353> Listeners are being kept around longer than they should be due to recent changs
<rdar://problem/26256258> Private process state thread can be cancelled and cause deadlocks in test suite


Modified:
    lldb/trunk/include/lldb/Core/Broadcaster.h
    lldb/trunk/include/lldb/Core/Event.h
    lldb/trunk/include/lldb/Core/Listener.h
    lldb/trunk/include/lldb/Target/Process.h
    lldb/trunk/include/lldb/lldb-forward.h
    lldb/trunk/source/Core/Broadcaster.cpp
    lldb/trunk/source/Core/Event.cpp
    lldb/trunk/source/Core/Listener.cpp
    lldb/trunk/source/Host/posix/HostThreadPosix.cpp
    lldb/trunk/source/Target/Process.cpp

Modified: lldb/trunk/include/lldb/Core/Broadcaster.h
URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/include/lldb/Core/Broadcaster.h?rev=269377&r1=269376&r2=269377&view=diff
==============================================================================
--- lldb/trunk/include/lldb/Core/Broadcaster.h (original)
+++ lldb/trunk/include/lldb/Core/Broadcaster.h Thu May 12 17:58:52 2016
@@ -12,6 +12,8 @@
 
 // C Includes
 // C++ Includes
+#include <functional>
+#include <list>
 #include <map>
 #include <string>
 #include <vector>
@@ -99,10 +101,10 @@ public:
     ~BroadcasterManager() = default;
 
     uint32_t
-    RegisterListenerForEvents (lldb::ListenerSP listener_sp, BroadcastEventSpec event_spec);
+    RegisterListenerForEvents (const lldb::ListenerSP &listener_sp, BroadcastEventSpec event_spec);
     
     bool
-    UnregisterListenerForEvents (lldb::ListenerSP listener_sp, BroadcastEventSpec event_spec);
+    UnregisterListenerForEvents (const lldb::ListenerSP &listener_sp, BroadcastEventSpec event_spec);
     
     lldb::ListenerSP
     GetListenerForEventSpec (BroadcastEventSpec event_spec) const;
@@ -111,7 +113,7 @@ public:
     SignUpListenersForBroadcaster (Broadcaster &broadcaster);
     
     void
-    RemoveListener (lldb::ListenerSP listener_sp);
+    RemoveListener (const lldb::ListenerSP &listener_sp);
 
     void
     RemoveListener (Listener *listener);
@@ -170,8 +172,7 @@ private:
     class ListenerMatchesAndSharedBits
     {
     public:
-        ListenerMatchesAndSharedBits (BroadcastEventSpec broadcaster_spec, 
-                                                   const lldb::ListenerSP listener_sp) :
+        explicit ListenerMatchesAndSharedBits (BroadcastEventSpec broadcaster_spec, const lldb::ListenerSP listener_sp) :
             m_broadcaster_spec (broadcaster_spec),
             m_listener_sp (listener_sp)
         {
@@ -194,7 +195,7 @@ private:
     class ListenerMatches
     {
     public:
-        ListenerMatches (const lldb::ListenerSP in_listener_sp) :
+        explicit ListenerMatches (const lldb::ListenerSP in_listener_sp) :
             m_listener_sp (in_listener_sp)
         {
         }
@@ -331,6 +332,12 @@ public:
     }
 
     void
+    BroadcastEvent(uint32_t event_type, const lldb::EventDataSP &event_data_sp)
+    {
+        m_broadcaster_sp->BroadcastEvent(event_type, event_data_sp);
+    }
+
+    void
     BroadcastEvent(uint32_t event_type, EventData *event_data = nullptr)
     {
         m_broadcaster_sp->BroadcastEvent(event_type, event_data);
@@ -349,7 +356,7 @@ public:
     }
 
     virtual void
-    AddInitialEventsToListener (lldb::ListenerSP listener_sp, uint32_t requested_events);
+    AddInitialEventsToListener (const lldb::ListenerSP &listener_sp, uint32_t requested_events);
 
     //------------------------------------------------------------------
     /// Listen for any events specified by \a event_mask.
@@ -374,7 +381,7 @@ public:
     ///     The actual event bits that were acquired by \a listener.
     //------------------------------------------------------------------
     uint32_t
-    AddListener (lldb::ListenerSP listener_sp, uint32_t event_mask)
+    AddListener (const lldb::ListenerSP &listener_sp, uint32_t event_mask)
     {
         return m_broadcaster_sp->AddListener(listener_sp, event_mask);
     }
@@ -454,7 +461,7 @@ public:
     /// @see uint32_t Broadcaster::AddListener (Listener*, uint32_t)
     //------------------------------------------------------------------
     bool
-    RemoveListener (lldb::ListenerSP listener_sp, uint32_t event_mask = UINT32_MAX)
+    RemoveListener (const lldb::ListenerSP &listener_sp, uint32_t event_mask = UINT32_MAX)
     {
         return m_broadcaster_sp->RemoveListener(listener_sp, event_mask);
     }
@@ -481,7 +488,7 @@ public:
     /// @see uint32_t Broadcaster::AddListener (Listener*, uint32_t)
     //------------------------------------------------------------------
     bool
-    HijackBroadcaster (lldb::ListenerSP listener_sp, uint32_t event_mask = UINT32_MAX)
+    HijackBroadcaster (const lldb::ListenerSP &listener_sp, uint32_t event_mask = UINT32_MAX)
     {
         return m_broadcaster_sp->HijackBroadcaster(listener_sp, event_mask);
     }
@@ -542,13 +549,16 @@ protected:
         BroadcastEvent(uint32_t event_type, EventData *event_data = nullptr);
 
         void
+        BroadcastEvent(uint32_t event_type, const lldb::EventDataSP &event_data_sp);
+
+        void
         BroadcastEventIfUnique(uint32_t event_type, EventData *event_data = nullptr);
 
         void
         Clear();
 
         uint32_t
-        AddListener (lldb::ListenerSP listener_sp, uint32_t event_mask);
+        AddListener (const lldb::ListenerSP &listener_sp, uint32_t event_mask);
 
         const char *
         GetBroadcasterName () const
@@ -581,10 +591,13 @@ protected:
         EventTypeHasListeners (uint32_t event_type);
 
         bool
-        RemoveListener (lldb::ListenerSP listener_sp, uint32_t event_mask = UINT32_MAX);
+        RemoveListener (lldb_private::Listener *listener, uint32_t event_mask = UINT32_MAX);
+
+        bool
+        RemoveListener (const lldb::ListenerSP &listener_sp, uint32_t event_mask = UINT32_MAX);
         
         bool
-        HijackBroadcaster (lldb::ListenerSP listener_sp, uint32_t event_mask = UINT32_MAX);
+        HijackBroadcaster (const lldb::ListenerSP &listener_sp, uint32_t event_mask = UINT32_MAX);
         
         bool
         IsHijackedForEvent (uint32_t event_mask);
@@ -602,9 +615,13 @@ protected:
         //------------------------------------------------------------------
         //
         //------------------------------------------------------------------
-        typedef std::vector< std::pair<lldb::ListenerSP,uint32_t> > collection;
+        typedef std::list< std::pair<lldb::ListenerWP,uint32_t> > collection;
         typedef std::map<uint32_t, std::string> event_names_map;
-        
+
+        void
+        ListenerIterator (std::function <bool (const lldb::ListenerSP &listener_sp, uint32_t &event_mask)> const &callback);
+
+
         Broadcaster &m_broadcaster;                     ///< The broadcsater that this implements
         event_names_map m_event_names;                  ///< Optionally define event names for readability and logging for each event bit
         collection m_listeners;                         ///< A list of Listener / event_mask pairs that are listening to this broadcaster.

Modified: lldb/trunk/include/lldb/Core/Event.h
URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/include/lldb/Core/Event.h?rev=269377&r1=269376&r2=269377&view=diff
==============================================================================
--- lldb/trunk/include/lldb/Core/Event.h (original)
+++ lldb/trunk/include/lldb/Core/Event.h Thu May 12 17:58:52 2016
@@ -114,6 +114,48 @@ private:
     DISALLOW_COPY_AND_ASSIGN (EventDataBytes);
 };
 
+class EventDataReceipt : public EventData
+{
+public:
+    EventDataReceipt() :
+        EventData(),
+        m_predicate(false)
+    {
+    }
+
+    ~EventDataReceipt() override
+    {
+    }
+
+    static const ConstString &
+    GetFlavorString ()
+    {
+        static ConstString g_flavor("Process::ProcessEventData");
+        return g_flavor;
+    }
+
+    const ConstString &
+    GetFlavor () const override
+    {
+        return GetFlavorString();
+    }
+
+    bool
+    WaitForEventReceived (const TimeValue *abstime = nullptr, bool *timed_out = nullptr)
+    {
+        return m_predicate.WaitForValueEqualTo(true, abstime, timed_out);
+    }
+
+private:
+    Predicate<bool> m_predicate;
+    
+    void
+    DoOnRemoval (Event *event_ptr)  override
+    {
+        m_predicate.SetValue(true, eBroadcastAlways);
+    }
+};
+
 //----------------------------------------------------------------------
 // lldb::Event
 //----------------------------------------------------------------------
@@ -126,8 +168,12 @@ class Event
 public:
     Event(Broadcaster *broadcaster, uint32_t event_type, EventData *data = nullptr);
 
+    Event(Broadcaster *broadcaster, uint32_t event_type, const lldb::EventDataSP &event_data_sp);
+
     Event(uint32_t event_type, EventData *data = nullptr);
 
+    Event(uint32_t event_type, const lldb::EventDataSP &event_data_sp);
+
     ~Event ();
 
     void
@@ -136,19 +182,19 @@ public:
     EventData *
     GetData ()
     {
-        return m_data_ap.get();
+        return m_data_sp.get();
     }
 
     const EventData *
     GetData () const
     {
-        return m_data_ap.get();
+        return m_data_sp.get();
     }
     
     void
     SetData (EventData *new_data)
     {
-        m_data_ap.reset (new_data);
+        m_data_sp.reset (new_data);
     }
 
     uint32_t
@@ -186,7 +232,7 @@ public:
     void
     Clear()
     {
-        m_data_ap.reset();
+        m_data_sp.reset();
     }
 
 private:
@@ -206,9 +252,9 @@ private:
         m_broadcaster_wp = broadcaster->GetBroadcasterImpl();
     }
 
-    Broadcaster::BroadcasterImplWP   m_broadcaster_wp;  // The broadcaster that sent this event
-    uint32_t        m_type;         // The bit describing this event
-    std::unique_ptr<EventData> m_data_ap;         // User specific data for this event
+    Broadcaster::BroadcasterImplWP m_broadcaster_wp; // The broadcaster that sent this event
+    uint32_t m_type; // The bit describing this event
+    lldb::EventDataSP m_data_sp; // User specific data for this event
 
 
     DISALLOW_COPY_AND_ASSIGN (Event);

Modified: lldb/trunk/include/lldb/Core/Listener.h
URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/include/lldb/Core/Listener.h?rev=269377&r1=269376&r2=269377&view=diff
==============================================================================
--- lldb/trunk/include/lldb/Core/Listener.h (original)
+++ lldb/trunk/include/lldb/Core/Listener.h Thu May 12 17:58:52 2016
@@ -20,8 +20,8 @@
 // Other libraries and framework includes
 // Project includes
 #include "lldb/lldb-private.h"
-#include "lldb/Host/Predicate.h"
 #include "lldb/Core/Broadcaster.h"
+#include "lldb/Host/Condition.h"
 #include "lldb/Core/Event.h"
 
 namespace lldb_private {
@@ -150,7 +150,8 @@ private:
     typedef std::vector<lldb::BroadcasterManagerWP> broadcaster_manager_collection;
 
     bool
-    FindNextEventInternal(Broadcaster *broadcaster,   // nullptr for any broadcaster
+    FindNextEventInternal(Mutex::Locker& lock,
+                          Broadcaster *broadcaster,   // nullptr for any broadcaster
                           const ConstString *sources, // nullptr for any event
                           uint32_t num_sources,
                           uint32_t event_type_mask,
@@ -177,7 +178,7 @@ private:
     Mutex m_broadcasters_mutex; // Protects m_broadcasters
     event_collection m_events;
     Mutex m_events_mutex; // Protects m_broadcasters and m_events
-    Predicate<bool> m_cond_wait;
+    Condition m_events_condition;
     broadcaster_manager_collection m_broadcaster_managers;
 
     void

Modified: lldb/trunk/include/lldb/Target/Process.h
URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/include/lldb/Target/Process.h?rev=269377&r1=269376&r2=269377&view=diff
==============================================================================
--- lldb/trunk/include/lldb/Target/Process.h (original)
+++ lldb/trunk/include/lldb/Target/Process.h Thu May 12 17:58:52 2016
@@ -3344,7 +3344,6 @@ protected:
     Broadcaster                 m_private_state_broadcaster;  // This broadcaster feeds state changed events into the private state thread's listener.
     Broadcaster                 m_private_state_control_broadcaster; // This is the control broadcaster, used to pause, resume & stop the private state thread.
     lldb::ListenerSP            m_private_state_listener_sp;     // This is the listener for the private state thread.
-    Predicate<bool>             m_private_state_control_wait; /// This Predicate is used to signal that a control operation is complete.
     HostThread                  m_private_state_thread; ///< Thread ID for the thread that watches internal state events
     ProcessModID                m_mod_id;               ///< Tracks the state of the process over stops and other alterations.
     uint32_t                    m_process_unique_id;    ///< Each lldb_private::Process class that is created gets a unique integer ID that increments with each new instance

Modified: lldb/trunk/include/lldb/lldb-forward.h
URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/include/lldb/lldb-forward.h?rev=269377&r1=269376&r2=269377&view=diff
==============================================================================
--- lldb/trunk/include/lldb/lldb-forward.h (original)
+++ lldb/trunk/include/lldb/lldb-forward.h Thu May 12 17:58:52 2016
@@ -332,6 +332,7 @@ namespace lldb {
     typedef std::shared_ptr<lldb_private::DynamicLoader> DynamicLoaderSP;
     typedef std::unique_ptr<lldb_private::DynamicLoader> DynamicLoaderUP;
     typedef std::shared_ptr<lldb_private::Event> EventSP;
+    typedef std::shared_ptr<lldb_private::EventData> EventDataSP;
     typedef std::shared_ptr<lldb_private::ExecutionContextRef> ExecutionContextRefSP;
     typedef std::shared_ptr<lldb_private::ExpressionVariable> ExpressionVariableSP;
     typedef std::shared_ptr<lldb_private::File> FileSP;
@@ -352,6 +353,7 @@ namespace lldb {
     typedef std::unique_ptr<lldb_private::SystemRuntime> SystemRuntimeUP;
     typedef std::shared_ptr<lldb_private::LineTable> LineTableSP;
     typedef std::shared_ptr<lldb_private::Listener> ListenerSP;
+    typedef std::weak_ptr<lldb_private::Listener> ListenerWP;
     typedef std::shared_ptr<lldb_private::LogChannel> LogChannelSP;
     typedef std::shared_ptr<lldb_private::MemoryHistory> MemoryHistorySP;
     typedef std::shared_ptr<lldb_private::Module> ModuleSP;

Modified: lldb/trunk/source/Core/Broadcaster.cpp
URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Core/Broadcaster.cpp?rev=269377&r1=269376&r2=269377&view=diff
==============================================================================
--- lldb/trunk/source/Core/Broadcaster.cpp (original)
+++ lldb/trunk/source/Core/Broadcaster.cpp Thu May 12 17:58:52 2016
@@ -61,6 +61,33 @@ Broadcaster::CheckInWithManager ()
 }
 
 void
+Broadcaster::BroadcasterImpl::ListenerIterator (std::function <bool (const lldb::ListenerSP &listener_sp, uint32_t &event_mask)> const &callback)
+{
+    // Private iterator that should be used by everyone except BroadcasterImpl::RemoveListener().
+    // We have weak pointers to our listeners which means that at any point the listener can
+    // expire which means we will need to take it out of our list. To take care of this, we
+    // iterate and check that the weak pointer can be made into a valid shared pointer before
+    // we call the callback. If the weak pointer has expired, we remove it from our list.
+    collection::iterator pos = m_listeners.begin();
+    while (pos != m_listeners.end())
+    {
+        lldb::ListenerSP curr_listener_sp(pos->first.lock());
+        if (curr_listener_sp)
+        {
+            if (callback(curr_listener_sp, pos->second))
+                ++pos;  // Keep iterating
+            else
+                return; // Done iterating
+        }
+        else
+        {
+            // The listener has been expired. Remove this entry.
+            pos = m_listeners.erase(pos);
+        }
+    }
+}
+
+void
 Broadcaster::BroadcasterImpl::Clear()
 {
     Mutex::Locker listeners_locker(m_listeners_mutex);
@@ -68,10 +95,11 @@ Broadcaster::BroadcasterImpl::Clear()
     // Make sure the listener forgets about this broadcaster. We do
     // this in the broadcaster in case the broadcaster object initiates
     // the removal.
-    
-    collection::iterator pos, end = m_listeners.end();
-    for (pos = m_listeners.begin(); pos != end; ++pos)
-        pos->first->BroadcasterWillDestruct (&m_broadcaster);
+
+    ListenerIterator([this](const lldb::ListenerSP &curr_listener_sp, uint32_t &curr_event_mask) -> bool {
+        curr_listener_sp->BroadcasterWillDestruct (&m_broadcaster);
+        return true; // Keep iterating
+    });
     
     m_listeners.clear();
 }
@@ -114,57 +142,45 @@ Broadcaster::BroadcasterImpl::GetEventNa
 }
 
 void
-Broadcaster::AddInitialEventsToListener (lldb::ListenerSP listener_sp, uint32_t requested_events)
+Broadcaster::AddInitialEventsToListener (const lldb::ListenerSP &listener_sp, uint32_t requested_events)
 {
 }
 
 uint32_t
-Broadcaster::BroadcasterImpl::AddListener (lldb::ListenerSP listener_sp, uint32_t event_mask)
+Broadcaster::BroadcasterImpl::AddListener (const lldb::ListenerSP &listener_sp, uint32_t event_mask)
 {
     if (!listener_sp)
         return 0;
 
     Mutex::Locker locker(m_listeners_mutex);
-    collection::iterator pos, end = m_listeners.end();
 
-    collection::iterator existing_pos = end;
     // See if we already have this listener, and if so, update its mask
-    uint32_t taken_event_types = 0;
-    for (pos = m_listeners.begin(); pos != end; ++pos)
-    {
-        if (pos->first == listener_sp)
-            existing_pos = pos;
-    // For now don't descriminate on who gets what
-    // FIXME: Implement "unique listener for this bit" mask
-    //  taken_event_types |= pos->second;
-    }
 
-    // Each event bit in a Broadcaster object can only be used
-    // by one listener
-    uint32_t available_event_types = ~taken_event_types & event_mask;
+    bool handled = false;
 
-    if (available_event_types)
-    {
-        // If we didn't find our listener, add it
-        if (existing_pos == end)
+    ListenerIterator([this, &listener_sp, &handled, event_mask](const lldb::ListenerSP &curr_listener_sp, uint32_t &curr_event_mask) -> bool {
+        if (curr_listener_sp == listener_sp)
         {
-            // Grant a new listener the available event bits
-            m_listeners.push_back(std::make_pair(listener_sp, available_event_types));
-        }
-        else
-        {
-            // Grant the existing listener the available event bits
-            existing_pos->second |= available_event_types;
+            handled = true;
+            curr_event_mask |= event_mask;
+            m_broadcaster.AddInitialEventsToListener (listener_sp, event_mask);
+            return false; // Stop iterating
         }
+        return true; // Keep iterating
+    });
+
+    if (!handled)
+    {
+        // Grant a new listener the available event bits
+        m_listeners.push_back(std::make_pair(lldb::ListenerWP(listener_sp), event_mask));
 
         // Individual broadcasters decide whether they have outstanding data when a
         // listener attaches, and insert it into the listener with this method.
-
-        m_broadcaster.AddInitialEventsToListener (listener_sp, available_event_types);
+        m_broadcaster.AddInitialEventsToListener (listener_sp, event_mask);
     }
 
     // Return the event bits that were granted to the listener
-    return available_event_types;
+    return event_mask;
 }
 
 bool
@@ -178,36 +194,63 @@ Broadcaster::BroadcasterImpl::EventTypeH
     if (m_listeners.empty())
         return false;
 
-    collection::iterator pos, end = m_listeners.end();
-    for (pos = m_listeners.begin(); pos != end; ++pos)
-    {
-        if (pos->second & event_type)
-            return true;
-    }
-    return false;
+    bool result = false;
+    ListenerIterator([this, event_type, &result](const lldb::ListenerSP &curr_listener_sp, uint32_t &curr_event_mask) -> bool {
+
+        if (curr_event_mask & event_type)
+        {
+            result = true;
+            return false; // Stop iterating
+        }
+        else
+        {
+            return true; // Keep iterating
+        }
+    });
+    return result;
 }
 
 bool
-Broadcaster::BroadcasterImpl::RemoveListener (lldb::ListenerSP listener_sp, uint32_t event_mask)
+Broadcaster::BroadcasterImpl::RemoveListener (lldb_private::Listener *listener, uint32_t event_mask)
 {
-    Mutex::Locker locker(m_listeners_mutex);
-    collection::iterator pos, end = m_listeners.end();
-    // See if we already have this listener, and if so, update its mask
-    for (pos = m_listeners.begin(); pos != end; ++pos)
+    if (listener)
     {
-        if (pos->first == listener_sp)
+        Mutex::Locker locker(m_listeners_mutex);
+        collection::iterator pos = m_listeners.begin();
+        // See if we already have this listener, and if so, update its mask
+        while (pos != m_listeners.end())
         {
-            // Relinquish all event bits in "event_mask"
-            pos->second &= ~event_mask;
-            // If all bits have been relinquished then remove this listener
-            if (pos->second == 0)
-                m_listeners.erase (pos);
-            return true;
+            lldb::ListenerSP curr_listener_sp(pos->first.lock());
+            if (curr_listener_sp)
+            {
+                if (curr_listener_sp.get() == listener)
+                {
+                    // Relinquish all event bits in "event_mask"
+                    pos->second &= ~event_mask;
+                    // If all bits have been relinquished then remove this listener
+                    if (pos->second == 0)
+                        m_listeners.erase (pos);
+                    return true;
+                }
+                ++pos;
+            }
+            else
+            {
+                // The listener has been destroyed since we couldn't turn the std::weak_ptr
+                // into a valid shared pointer, so we can remove it.
+                pos = m_listeners.erase (pos);
+            }
         }
     }
     return false;
 }
 
+bool
+Broadcaster::BroadcasterImpl::RemoveListener (const lldb::ListenerSP &listener_sp, uint32_t event_mask)
+{
+    return RemoveListener (listener_sp.get(), event_mask);
+}
+
 void
 Broadcaster::BroadcasterImpl::BroadcastEvent (EventSP &event_sp)
 {
@@ -232,7 +275,7 @@ Broadcaster::BroadcasterImpl::PrivateBro
 
     const uint32_t event_type = event_sp->GetType();
 
-    Mutex::Locker event_types_locker(m_listeners_mutex);
+    Mutex::Locker locker(m_listeners_mutex);
 
     ListenerSP hijacking_listener_sp;
 
@@ -263,20 +306,16 @@ Broadcaster::BroadcasterImpl::PrivateBro
     }
     else
     {
-        collection::iterator pos, end = m_listeners.end();
 
-        // Iterate through all listener/mask pairs
-        for (pos = m_listeners.begin(); pos != end; ++pos)
-        {
-            // If the listener's mask matches any bits that we just set, then
-            // put the new event on its event queue.
-            if (event_type & pos->second)
+        ListenerIterator([this, unique, event_type, &event_sp](const lldb::ListenerSP &curr_listener_sp, uint32_t &curr_event_mask) -> bool {
+
+            if (event_type & curr_event_mask)
             {
-                if (unique && pos->first->PeekAtNextEventForBroadcasterWithType (&m_broadcaster, event_type))
-                    continue;
-                pos->first->AddEvent (event_sp);
+                if (!unique || curr_listener_sp->PeekAtNextEventForBroadcasterWithType (&m_broadcaster, event_type) == nullptr)
+                    curr_listener_sp->AddEvent (event_sp);
             }
-        }
+            return true; // Keep iterating
+        });
     }
 }
 
@@ -288,6 +327,13 @@ Broadcaster::BroadcasterImpl::BroadcastE
 }
 
 void
+Broadcaster::BroadcasterImpl::BroadcastEvent (uint32_t event_type, const lldb::EventDataSP &event_data_sp)
+{
+    EventSP event_sp (new Event (event_type, event_data_sp));
+    PrivateBroadcastEvent (event_sp, false);
+}
+
+void
 Broadcaster::BroadcasterImpl::BroadcastEventIfUnique (uint32_t event_type, EventData *event_data)
 {
     EventSP event_sp (new Event (event_type, event_data));
@@ -295,9 +341,9 @@ Broadcaster::BroadcasterImpl::BroadcastE
 }
 
 bool
-Broadcaster::BroadcasterImpl::HijackBroadcaster (ListenerSP listener_sp, uint32_t event_mask)
+Broadcaster::BroadcasterImpl::HijackBroadcaster (const lldb::ListenerSP &listener_sp, uint32_t event_mask)
 {
-    Mutex::Locker event_types_locker(m_listeners_mutex);
+    Mutex::Locker locker(m_listeners_mutex);
 
     Log *log(lldb_private::GetLogIfAnyCategoriesSet (LIBLLDB_LOG_EVENTS));
     if (log)
@@ -312,7 +358,7 @@ Broadcaster::BroadcasterImpl::HijackBroa
 bool
 Broadcaster::BroadcasterImpl::IsHijackedForEvent (uint32_t event_mask)
 {
-    Mutex::Locker event_types_locker(m_listeners_mutex);
+    Mutex::Locker locker(m_listeners_mutex);
 
     if (!m_hijacking_listeners.empty())
         return (event_mask & m_hijacking_masks.back()) != 0;
@@ -335,7 +381,7 @@ Broadcaster::BroadcasterImpl::GetHijacki
 void
 Broadcaster::BroadcasterImpl::RestoreBroadcaster ()
 {
-    Mutex::Locker event_types_locker(m_listeners_mutex);
+    Mutex::Locker locker(m_listeners_mutex);
 
     if (!m_hijacking_listeners.empty())
     {
@@ -391,7 +437,7 @@ BroadcasterManager::MakeBroadcasterManag
 }
 
 uint32_t
-BroadcasterManager::RegisterListenerForEvents (ListenerSP listener_sp, BroadcastEventSpec event_spec)
+BroadcasterManager::RegisterListenerForEvents (const lldb::ListenerSP &listener_sp, BroadcastEventSpec event_spec)
 {
     Mutex::Locker locker(m_manager_mutex);
     
@@ -415,7 +461,7 @@ BroadcasterManager::RegisterListenerForE
 }
 
 bool
-BroadcasterManager::UnregisterListenerForEvents (ListenerSP listener_sp, BroadcastEventSpec event_spec)
+BroadcasterManager::UnregisterListenerForEvents (const lldb::ListenerSP &listener_sp, BroadcastEventSpec event_spec)
 {
     Mutex::Locker locker(m_manager_mutex);
     bool removed_some = false;
@@ -495,7 +541,7 @@ BroadcasterManager::RemoveListener(Liste
 }
 
 void
-BroadcasterManager::RemoveListener (ListenerSP listener_sp)
+BroadcasterManager::RemoveListener (const lldb::ListenerSP &listener_sp)
 {
     Mutex::Locker locker(m_manager_mutex);
     ListenerMatches predicate (listener_sp);

Modified: lldb/trunk/source/Core/Event.cpp
URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Core/Event.cpp?rev=269377&r1=269376&r2=269377&view=diff
==============================================================================
--- lldb/trunk/source/Core/Event.cpp (original)
+++ lldb/trunk/source/Core/Event.cpp Thu May 12 17:58:52 2016
@@ -26,15 +26,30 @@ using namespace lldb;
 using namespace lldb_private;
 
 Event::Event (Broadcaster *broadcaster, uint32_t event_type, EventData *data) :
-    m_broadcaster_wp (broadcaster->GetBroadcasterImpl()),
-    m_type (event_type),
-    m_data_ap (data)
+    m_broadcaster_wp(broadcaster->GetBroadcasterImpl()),
+    m_type(event_type),
+    m_data_sp(data)
+{
+}
+
+Event::Event (Broadcaster *broadcaster, uint32_t event_type, const EventDataSP &event_data_sp) :
+    m_broadcaster_wp(broadcaster->GetBroadcasterImpl()),
+    m_type(event_type),
+    m_data_sp(event_data_sp)
 {
 }
 
 Event::Event(uint32_t event_type, EventData *data) :
-    m_type (event_type),
-    m_data_ap (data)
+    m_broadcaster_wp(),
+    m_type(event_type),
+    m_data_sp(data)
+{
+}
+
+Event::Event(uint32_t event_type, const EventDataSP &event_data_sp) :
+    m_broadcaster_wp(),
+    m_type(event_type),
+    m_data_sp(event_data_sp)
 {
 }
 
@@ -69,10 +84,10 @@ Event::Dump (Stream *s) const
         s->Printf("%p Event: broadcaster = NULL, type = 0x%8.8x, data = ",
                   static_cast<const void*>(this), m_type);
 
-    if (m_data_ap)
+    if (m_data_sp)
     {
         s->PutChar('{');
-        m_data_ap->Dump (s);
+        m_data_sp->Dump (s);
         s->PutChar('}');
     }
     else
@@ -82,8 +97,8 @@ Event::Dump (Stream *s) const
 void
 Event::DoOnRemoval ()
 {
-    if (m_data_ap)
-        m_data_ap->DoOnRemoval (this);
+    if (m_data_sp)
+        m_data_sp->DoOnRemoval (this);
 }
 
 EventData::EventData() = default;

Modified: lldb/trunk/source/Core/Listener.cpp
URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Core/Listener.cpp?rev=269377&r1=269376&r2=269377&view=diff
==============================================================================
--- lldb/trunk/source/Core/Listener.cpp (original)
+++ lldb/trunk/source/Core/Listener.cpp Thu May 12 17:58:52 2016
@@ -45,8 +45,7 @@ Listener::Listener(const char *name) :
     m_broadcasters(),
     m_broadcasters_mutex (Mutex::eMutexTypeRecursive),
     m_events (),
-    m_events_mutex (Mutex::eMutexTypeRecursive),
-    m_cond_wait()
+    m_events_mutex (Mutex::eMutexTypeNormal)
 {
     Log *log(lldb_private::GetLogIfAllCategoriesSet (LIBLLDB_LOG_OBJECT));
     if (log != nullptr)
@@ -74,11 +73,10 @@ Listener::Clear()
     {
         Broadcaster::BroadcasterImplSP broadcaster_sp(pos->first.lock());
         if (broadcaster_sp)
-            broadcaster_sp->RemoveListener (this->shared_from_this(), pos->second.event_mask);
+            broadcaster_sp->RemoveListener (this, pos->second.event_mask);
     }
     m_broadcasters.clear();
-    m_cond_wait.SetValue (false, eBroadcastNever);
-    m_broadcasters.clear();
+
     Mutex::Locker event_locker(m_events_mutex);
     m_events.clear();
     size_t num_managers = m_broadcaster_managers.size();
@@ -193,9 +191,6 @@ Listener::BroadcasterWillDestruct (Broad
             else
                 ++pos;
         }
-
-        if (m_events.empty())
-            m_cond_wait.SetValue (false, eBroadcastNever);
     }
 }
 
@@ -221,12 +216,9 @@ Listener::AddEvent (EventSP &event_sp)
                      static_cast<void*>(this), m_name.c_str(),
                      static_cast<void*>(event_sp.get()));
 
-    // Scope for "locker"
-    {
-        Mutex::Locker locker(m_events_mutex);
-        m_events.push_back (event_sp);
-    }
-    m_cond_wait.SetValue (true, eBroadcastAlways);
+    Mutex::Locker locker(m_events_mutex);
+    m_events.push_back (event_sp);
+    m_events_condition.Broadcast();
 }
 
 class EventBroadcasterMatches
@@ -293,6 +285,7 @@ private:
 bool
 Listener::FindNextEventInternal
 (
+    Mutex::Locker& lock,
     Broadcaster *broadcaster,   // nullptr for any broadcaster
     const ConstString *broadcaster_names, // nullptr for any event
     uint32_t num_broadcaster_names,
@@ -300,10 +293,10 @@ Listener::FindNextEventInternal
     EventSP &event_sp,
     bool remove)
 {
+    // NOTE: callers of this function must lock m_events_mutex using a Mutex::Locker
+    // and pass the locker as the first argument. m_events_mutex is no longer recursive.
     Log *log(lldb_private::GetLogIfAllCategoriesSet (LIBLLDB_LOG_EVENTS));
 
-    Mutex::Locker lock(m_events_mutex);
-
     if (m_events.empty())
         return false;
 
@@ -333,20 +326,12 @@ Listener::FindNextEventInternal
         if (remove)
         {
             m_events.erase(pos);
-
-            if (m_events.empty())
-                m_cond_wait.SetValue (false, eBroadcastNever);
-        }
-
-        // Unlock the event queue here.  We've removed this event and are about to return
-        // it so it should be okay to get the next event off the queue here - and it might
-        // be useful to do that in the "DoOnRemoval".
-        lock.Unlock();
-
-        // Don't call DoOnRemoval if you aren't removing the event...
-        if (remove)
+            // Unlock the event queue here.  We've removed this event and are about to return
+            // it so it should be okay to get the next event off the queue here - and it might
+            // be useful to do that in the "DoOnRemoval".
+            lock.Unlock();
             event_sp->DoOnRemoval();
-
+        }
         return true;
     }
 
@@ -357,8 +342,9 @@ Listener::FindNextEventInternal
 Event *
 Listener::PeekAtNextEvent ()
 {
+    Mutex::Locker lock(m_events_mutex);
     EventSP event_sp;
-    if (FindNextEventInternal(nullptr, nullptr, 0, 0, event_sp, false))
+    if (FindNextEventInternal(lock, nullptr, nullptr, 0, 0, event_sp, false))
         return event_sp.get();
     return nullptr;
 }
@@ -366,8 +352,9 @@ Listener::PeekAtNextEvent ()
 Event *
 Listener::PeekAtNextEventForBroadcaster (Broadcaster *broadcaster)
 {
+    Mutex::Locker lock(m_events_mutex);
     EventSP event_sp;
-    if (FindNextEventInternal(broadcaster, nullptr, 0, 0, event_sp, false))
+    if (FindNextEventInternal(lock, broadcaster, nullptr, 0, 0, event_sp, false))
         return event_sp.get();
     return nullptr;
 }
@@ -375,8 +362,9 @@ Listener::PeekAtNextEventForBroadcaster
 Event *
 Listener::PeekAtNextEventForBroadcasterWithType (Broadcaster *broadcaster, uint32_t event_type_mask)
 {
+    Mutex::Locker lock(m_events_mutex);
     EventSP event_sp;
-    if (FindNextEventInternal(broadcaster, nullptr, 0, event_type_mask, event_sp, false))
+    if (FindNextEventInternal(lock, broadcaster, nullptr, 0, event_type_mask, event_sp, false))
         return event_sp.get();
     return nullptr;
 }
@@ -388,7 +376,8 @@ Listener::GetNextEventInternal(Broadcast
                                uint32_t event_type_mask,
                                EventSP &event_sp)
 {
-    return FindNextEventInternal (broadcaster, broadcaster_names, num_broadcaster_names, event_type_mask, event_sp, true);
+    Mutex::Locker lock(m_events_mutex);
+    return FindNextEventInternal (lock, broadcaster, broadcaster_names, num_broadcaster_names, event_type_mask, event_sp, true);
 }
 
 bool
@@ -418,52 +407,40 @@ Listener::WaitForEventsInternal(const Ti
                                 EventSP &event_sp)
 {
     Log *log(lldb_private::GetLogIfAllCategoriesSet (LIBLLDB_LOG_EVENTS));
-    bool timed_out = false;
-
     if (log != nullptr)
         log->Printf ("%p Listener::WaitForEventsInternal (timeout = { %p }) for %s",
                      static_cast<void*>(this), static_cast<const void*>(timeout),
                      m_name.c_str());
 
+    Mutex::Locker lock(m_events_mutex);
+
     while (true)
     {
-        // Note, we don't want to lock the m_events_mutex in the call to GetNextEventInternal, since the DoOnRemoval
-        // code might require that new events be serviced.  For instance, the Breakpoint Command's 
-        if (GetNextEventInternal (broadcaster, broadcaster_names, num_broadcaster_names, event_type_mask, event_sp))
-                return true;
-
-        {
-            // Reset condition value to false, so we can wait for new events to be
-            // added that might meet our current filter
-            // But first poll for any new event that might satisfy our condition, and if so consume it,
-            // otherwise wait.
-
-            Mutex::Locker event_locker(m_events_mutex);
-            const bool remove = false;
-            if (FindNextEventInternal (broadcaster, broadcaster_names, num_broadcaster_names, event_type_mask, event_sp, remove))
-                continue;
-            else
-                m_cond_wait.SetValue (false, eBroadcastNever);
-        }
-
-        if (m_cond_wait.WaitForValueEqualTo (true, timeout, &timed_out))
-            continue;
-
-        else if (timed_out)
+        if (FindNextEventInternal (lock, broadcaster, broadcaster_names, num_broadcaster_names, event_type_mask, event_sp, true))
         {
-            log = lldb_private::GetLogIfAllCategoriesSet (LIBLLDB_LOG_EVENTS);
-            if (log != nullptr)
-                log->Printf ("%p Listener::WaitForEventsInternal() timed out for %s",
-                             static_cast<void*>(this), m_name.c_str());
-            break;
+            return true;
         }
         else
         {
-            log = lldb_private::GetLogIfAllCategoriesSet (LIBLLDB_LOG_EVENTS);
-            if (log != nullptr)
-                log->Printf ("%p Listener::WaitForEventsInternal() unknown error for %s",
-                             static_cast<void*>(this), m_name.c_str());
-            break;
+            bool timed_out = false;
+            if (m_events_condition.Wait(m_events_mutex, timeout, &timed_out) != 0)
+            {
+                if (timed_out)
+                {
+                    log = lldb_private::GetLogIfAllCategoriesSet (LIBLLDB_LOG_EVENTS);
+                    if (log != nullptr)
+                        log->Printf ("%p Listener::WaitForEventsInternal() timed out for %s",
+                                     static_cast<void*>(this), m_name.c_str());
+                }
+                else
+                {
+                    log = lldb_private::GetLogIfAllCategoriesSet (LIBLLDB_LOG_EVENTS);
+                    if (log != nullptr)
+                        log->Printf ("%p Listener::WaitForEventsInternal() unknown error for %s",
+                                     static_cast<void*>(this), m_name.c_str());
+                }
+                return false;
+            }
         }
     }
 

Modified: lldb/trunk/source/Host/posix/HostThreadPosix.cpp
URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Host/posix/HostThreadPosix.cpp?rev=269377&r1=269376&r2=269377&view=diff
==============================================================================
--- lldb/trunk/source/Host/posix/HostThreadPosix.cpp (original)
+++ lldb/trunk/source/Host/posix/HostThreadPosix.cpp Thu May 12 17:58:52 2016
@@ -53,13 +53,16 @@ Error
 HostThreadPosix::Cancel()
 {
     Error error;
+    if (IsJoinable())
+    {
 #ifndef __ANDROID__
-    int err = ::pthread_cancel(m_thread);
-    error.SetError(err, eErrorTypePOSIX);
+        assert(false && "someone is calling HostThread::Cancel()");
+        int err = ::pthread_cancel(m_thread);
+        error.SetError(err, eErrorTypePOSIX);
 #else
-    error.SetErrorString("HostThreadPosix::Cancel() not supported on Android");
+        error.SetErrorString("HostThreadPosix::Cancel() not supported on Android");
 #endif
-
+    }
     return error;
 }
 
@@ -67,8 +70,11 @@ Error
 HostThreadPosix::Detach()
 {
     Error error;
-    int err = ::pthread_detach(m_thread);
-    error.SetError(err, eErrorTypePOSIX);
+    if (IsJoinable())
+    {
+        int err = ::pthread_detach(m_thread);
+        error.SetError(err, eErrorTypePOSIX);
+    }
     Reset();
     return error;
 }

Modified: lldb/trunk/source/Target/Process.cpp
URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Target/Process.cpp?rev=269377&r1=269376&r2=269377&view=diff
==============================================================================
--- lldb/trunk/source/Target/Process.cpp (original)
+++ lldb/trunk/source/Target/Process.cpp Thu May 12 17:58:52 2016
@@ -726,7 +726,6 @@ Process::Process(lldb::TargetSP target_s
     m_private_state_broadcaster(nullptr, "lldb.process.internal_state_broadcaster"),
     m_private_state_control_broadcaster(nullptr, "lldb.process.internal_state_control_broadcaster"),
     m_private_state_listener_sp (Listener::MakeListener("lldb.process.internal_state_listener")),
-    m_private_state_control_wait(),
     m_mod_id (),
     m_process_unique_id(0),
     m_thread_index_id (0),
@@ -4109,44 +4108,46 @@ Process::ControlPrivateStateThread (uint
     // Signal the private state thread. First we should copy this is case the
     // thread starts exiting since the private state thread will NULL this out
     // when it exits
-    HostThread private_state_thread(m_private_state_thread);
-    if (private_state_thread.IsJoinable())
     {
-        TimeValue timeout_time;
-        bool timed_out;
-
-        m_private_state_control_broadcaster.BroadcastEvent(signal, nullptr);
-
-        timeout_time = TimeValue::Now();
-        timeout_time.OffsetWithSeconds(2);
-        if (log)
-            log->Printf ("Sending control event of type: %d.", signal);
-        m_private_state_control_wait.WaitForValueEqualTo (true, &timeout_time, &timed_out);
-        m_private_state_control_wait.SetValue (false, eBroadcastNever);
-
-        if (signal == eBroadcastInternalStateControlStop)
+        HostThread private_state_thread(m_private_state_thread);
+        if (private_state_thread.IsJoinable())
         {
-            if (timed_out)
-            {
-                Error error = private_state_thread.Cancel();
-                if (log)
-                    log->Printf ("Timed out responding to the control event, cancel got error: \"%s\".", error.AsCString());
+            if (log)
+                log->Printf ("Sending control event of type: %d.", signal);
+            // Send the control event and wait for the receipt or for the private state
+            // thread to exit
+            std::shared_ptr<EventDataReceipt> event_receipt_sp(new EventDataReceipt());
+            m_private_state_control_broadcaster.BroadcastEvent(signal, event_receipt_sp);
+
+            bool receipt_received = false;
+            while (!receipt_received)
+            {
+                bool timed_out = false;
+                TimeValue timeout_time;
+                timeout_time = TimeValue::Now();
+                timeout_time.OffsetWithSeconds(2);
+                // Check for a receipt for 2 seconds and then check if the private state
+                // thread is still around.
+                receipt_received = event_receipt_sp->WaitForEventReceived (&timeout_time, &timed_out);
+                if (!receipt_received)
+                {
+                    // Check if the private state thread is still around. If it isn't then we are done waiting
+                    if (!m_private_state_thread.IsJoinable())
+                        break; // Private state thread exited, we are done
+                }
             }
-            else
+
+            if (signal == eBroadcastInternalStateControlStop)
             {
-                if (log)
-                    log->Printf ("The control event killed the private state thread without having to cancel.");
+                thread_result_t result = NULL;
+                private_state_thread.Join(&result);
             }
-
-            thread_result_t result = NULL;
-            private_state_thread.Join(&result);
-            m_private_state_thread.Reset();
         }
-    }
-    else
-    {
-        if (log)
-            log->Printf ("Private state thread already dead, no need to signal it to stop.");
+        else
+        {
+            if (log)
+                log->Printf ("Private state thread already dead, no need to signal it to stop.");
+        }
     }
 }
 
@@ -4311,7 +4312,6 @@ thread_result_t
 Process::RunPrivateStateThread (bool is_secondary_thread)
 {
     bool control_only = true;
-    m_private_state_control_wait.SetValue (false, eBroadcastNever);
 
     Log *log(lldb_private::GetLogIfAllCategoriesSet (LIBLLDB_LOG_PROCESS));
     if (log)
@@ -4346,7 +4346,6 @@ Process::RunPrivateStateThread (bool is_
                 break;
             }
 
-            m_private_state_control_wait.SetValue (true, eBroadcastAlways);
             continue;
         }
         else if (event_sp->GetType() == eBroadcastBitInterrupt)
@@ -4442,7 +4441,6 @@ Process::RunPrivateStateThread (bool is_
     // try to change it on the way out.
     if (!is_secondary_thread)
         m_public_run_lock.SetStopped();
-    m_private_state_control_wait.SetValue (true, eBroadcastAlways);
     m_private_state_thread.Reset();
     return NULL;
 }




More information about the lldb-commits mailing list