Fixed the BreakpointLocationList to be able to do O(1) lookups on breakpoint

locations by ID. It used to be, worst case, O(N).

llvm-svn: 124914

lldb/include/lldb/Breakpoint/Breakpoint.h

@@ -232,7 +232,7 @@ public:
     ///    Returns a pointer to the new location.
     //------------------------------------------------------------------
     lldb::BreakpointLocationSP
-    AddLocation (Address &addr,
+    AddLocation (const Address &addr,
                  bool *new_location = NULL);
 
     //------------------------------------------------------------------
@@ -245,7 +245,7 @@ public:
     ///    in the shared pointer will be NULL if there is no location at that address.
     //------------------------------------------------------------------
     lldb::BreakpointLocationSP
-    FindLocationByAddress (Address &addr);
+    FindLocationByAddress (const Address &addr);
 
     //------------------------------------------------------------------
     /// Find a breakpoint location ID by Address.
@@ -257,7 +257,7 @@ public:
     ///    there is no breakpoint location at that address.
     //------------------------------------------------------------------
     lldb::break_id_t
-    FindLocationIDByAddress (Address &addr);
+    FindLocationIDByAddress (const Address &addr);
 
     //------------------------------------------------------------------
     /// Find a breakpoint location for a given breakpoint location ID.

lldb/include/lldb/Breakpoint/BreakpointLocation.h

@@ -312,7 +312,7 @@ public:
     InvokeCallback (StoppointCallbackContext *context);
 
 protected:
-    friend class Breakpoint;
+    friend class BreakpointLocationList;
     friend class CommandObjectBreakpointCommandAdd;
     friend class Process;
 
@@ -357,7 +357,7 @@ private:
 
     BreakpointLocation (lldb::break_id_t bid,
                         Breakpoint &owner,
-                        Address &addr,
+                        const Address &addr,
                         lldb::tid_t tid = LLDB_INVALID_THREAD_ID,
                         bool hardware = false);
 

lldb/include/lldb/Breakpoint/BreakpointLocationList.h

@@ -59,21 +59,7 @@ public:
     ///     pointer if the breakpoint doesn't exist.
     //------------------------------------------------------------------
     const lldb::BreakpointLocationSP
-    FindByAddress (Address &addr) const;
+    FindByAddress (const Address &addr) const;
-
-    //------------------------------------------------------------------
-    /// Returns a shared pointer to the breakpoint location with id \a
-    /// breakID.
-    ///
-    /// @param[in] breakID
-    ///     The breakpoint location ID to seek for.
-    ///
-    /// @result
-    ///     A shared pointer to the breakpoint.  May contain a NULL
-    ///     pointer if the breakpoint doesn't exist.
-    //------------------------------------------------------------------
-    lldb::BreakpointLocationSP
-    FindByID (lldb::break_id_t breakID);
 
     //------------------------------------------------------------------
     /// Returns a shared pointer to the breakpoint location with id
@@ -86,7 +72,7 @@ public:
     ///     A shared pointer to the breakpoint.  May contain a NULL
     ///     pointer if the breakpoint doesn't exist.
     //------------------------------------------------------------------
-    const lldb::BreakpointLocationSP
+    lldb::BreakpointLocationSP
     FindByID (lldb::break_id_t breakID) const;
 
     //------------------------------------------------------------------
@@ -100,7 +86,7 @@ public:
     ///     The ID of the breakpoint location, or LLDB_INVALID_BREAK_ID.
     //------------------------------------------------------------------
     lldb::break_id_t
-    FindIDByAddress (Address &addr);
+    FindIDByAddress (const Address &addr);
 
     //------------------------------------------------------------------
     /// Returns a breakpoint location list of the breakpoint locations
@@ -182,19 +168,6 @@ public:
     uint32_t
     GetHitCount () const;
 
-    //------------------------------------------------------------------
-    /// Removes the breakpoint location given by \b breakID from this
-    /// list.
-    ///
-    /// @param[in] breakID
-    ///     The breakpoint location index to remove.
-    ///
-    /// @result
-    ///     \b true if the breakpoint \a breakID was in the list.
-    //------------------------------------------------------------------
-    bool
-    Remove (lldb::break_id_t breakID);
-
     //------------------------------------------------------------------
     /// Enquires of the breakpoint location in this list with ID \a
     /// breakID whether we should stop.
@@ -262,28 +235,15 @@ protected:
     /// @result
     ///     Returns breakpoint location id.
     //------------------------------------------------------------------
-    virtual lldb::break_id_t
+    lldb::BreakpointLocationSP
-    Add (lldb::BreakpointLocationSP& bp_loc_sp);
+    Create (Breakpoint &owner, const Address &addr);
+//    Add (lldb::BreakpointLocationSP& bp_loc_sp);
 
     typedef std::vector<lldb::BreakpointLocationSP> collection;
     typedef std::map<lldb_private::Address,
                      lldb::BreakpointLocationSP,
                      Address::ModulePointerAndOffsetLessThanFunctionObject> addr_map;
 
-    // The breakpoint locations are stored in their Parent Breakpoint's location list by an
-    // index that is unique to this list, and not across all breakpoint location lists.
-    // This is only set in the Breakpoint's AddLocation method.
-    // There is another breakpoint location list, the owner's list in the BreakpointSite,
-    // but that should not reset the ID.  Unfortunately UserID's SetID method is public.
-    lldb::break_id_t
-    GetNextID();
-
-    collection::iterator
-    GetIDIterator(lldb::break_id_t breakID);
-
-    collection::const_iterator
-    GetIDConstIterator(lldb::break_id_t breakID) const;
-
     collection m_locations;
     addr_map m_address_to_location;
     mutable Mutex m_mutex;

lldb/source/Breakpoint/Breakpoint.cpp

@@ -81,33 +81,33 @@ Breakpoint::GetTarget () const
 }
 
 BreakpointLocationSP
-Breakpoint::AddLocation (Address &addr, bool *new_location)
+Breakpoint::AddLocation (const Address &addr, bool *new_location)
 {
-    BreakpointLocationSP bp_loc_sp (m_locations.FindByAddress(addr));
-    if (bp_loc_sp)
-    {
-        if (new_location)
-            *new_location = false;
-        return bp_loc_sp;
-    }
-
-    bp_loc_sp.reset (new BreakpointLocation (m_locations.GetNextID(), *this, addr));
-    m_locations.Add (bp_loc_sp);
-    bp_loc_sp->ResolveBreakpointSite();
-
     if (new_location)
-        *new_location = true;
+        *new_location = false;
+    BreakpointLocationSP bp_loc_sp (m_locations.FindByAddress(addr));
+    if (!bp_loc_sp)
+	{
+		bp_loc_sp = m_locations.Create (*this, addr);
+		if (bp_loc_sp)
+		{
+	    	bp_loc_sp->ResolveBreakpointSite();
+
+		    if (new_location)
+	    	    *new_location = true;
+		}
+	}
     return bp_loc_sp;
 }
 
 BreakpointLocationSP
-Breakpoint::FindLocationByAddress (Address &addr)
+Breakpoint::FindLocationByAddress (const Address &addr)
 {
     return m_locations.FindByAddress(addr);
 }
 
 break_id_t
-Breakpoint::FindLocationIDByAddress (Address &addr)
+Breakpoint::FindLocationIDByAddress (const Address &addr)
 {
     return m_locations.FindIDByAddress(addr);
 }
@@ -127,7 +127,6 @@ Breakpoint::GetLocationAtIndex (uint32_t index)
 BreakpointLocationSP
 Breakpoint::GetLocationSP (BreakpointLocation *bp_loc_ptr)
 {
-    assert (bp_loc_ptr->GetBreakpoint().GetID() == GetID());
     return m_locations.FindByID(bp_loc_ptr->GetID());
 }
 
@@ -280,24 +279,25 @@ Breakpoint::ModulesChanged (ModuleList &module_list, bool load)
         // 3) If we don't see this module in our breakpoint location list, call ResolveInModules.
 
         ModuleList new_modules;  // We'll stuff the "unseen" modules in this list, and then resolve
-                                   // them after the locations pass.  Have to do it this way because
+                                 // them after the locations pass.  Have to do it this way because
-                                   // resolving breakpoints will add new locations potentially.
+                                 // resolving breakpoints will add new locations potentially.
+
+        const size_t num_locs = m_locations.GetSize();
 
         for (size_t i = 0; i < module_list.GetSize(); i++)
         {
             bool seen = false;
             ModuleSP module_sp (module_list.GetModuleAtIndex (i));
-            Module *module = module_sp.get();
             if (!m_filter_sp->ModulePasses (module_sp))
                 continue;
 
-            for (size_t loc_idx = 0; loc_idx < m_locations.GetSize(); loc_idx++)
+            for (size_t loc_idx = 0; loc_idx < num_locs; loc_idx++)
             {
                 BreakpointLocationSP break_loc = m_locations.GetByIndex(loc_idx);
                 if (!break_loc->IsEnabled())
                     continue;
                 const Section *section = break_loc->GetAddress().GetSection();
-                if (section == NULL || section->GetModule() == module)
+                if (section == NULL || section->GetModule() == module_sp.get())
                 {
                     if (!seen)
                         seen = true;

lldb/source/Breakpoint/BreakpointLocation.cpp

@@ -32,7 +32,7 @@ BreakpointLocation::BreakpointLocation
 (
     break_id_t loc_id,
     Breakpoint &owner,
-    Address &addr,
+    const Address &addr,
     lldb::tid_t tid,
     bool hardware
 ) :

lldb/source/Breakpoint/BreakpointLocationList.cpp

@@ -23,8 +23,7 @@ using namespace lldb_private;
 BreakpointLocationList::BreakpointLocationList() :
     m_locations(),
     m_address_to_location (),
-    m_mutex (Mutex::eMutexTypeRecursive),
+    m_mutex (Mutex::eMutexTypeRecursive)
-    m_next_id (0)
 {
 }
 
@@ -32,17 +31,16 @@ BreakpointLocationList::~BreakpointLocationList()
 {
 }
 
-lldb::break_id_t
+BreakpointLocationSP
-BreakpointLocationList::Add (BreakpointLocationSP &bp_loc_sp)
+BreakpointLocationList::Create (Breakpoint &bp, const Address &addr)
 {
-    if (bp_loc_sp)
+    Mutex::Locker locker (m_mutex);
-    {
+    // The location ID is just the size of the location list + 1
-        Mutex::Locker locker (m_mutex);
+    lldb::break_id_t bp_loc_id = m_locations.size() + 1;
-        m_locations.push_back (bp_loc_sp);
+    BreakpointLocationSP bp_loc_sp (new BreakpointLocation (bp_loc_id, bp, addr));
-        m_address_to_location[bp_loc_sp->GetAddress()] = bp_loc_sp;
+    m_locations.push_back (bp_loc_sp);
-        return bp_loc_sp->GetID();
+    m_address_to_location[addr] = bp_loc_sp;
-    }
+    return bp_loc_sp;
-    return LLDB_INVALID_BREAK_ID;
 }
 
 bool
@@ -62,7 +60,7 @@ BreakpointLocationList::ShouldStop (StoppointCallbackContext *context, lldb::bre
 }
 
 lldb::break_id_t
-BreakpointLocationList::FindIDByAddress (Address &addr)
+BreakpointLocationList::FindIDByAddress (const Address &addr)
 {
     BreakpointLocationSP bp_loc_sp = FindByAddress (addr);
     if (bp_loc_sp)
@@ -72,95 +70,19 @@ BreakpointLocationList::FindIDByAddress (Address &addr)
     return LLDB_INVALID_BREAK_ID;
 }
 
-bool
-BreakpointLocationList::Remove (lldb::break_id_t break_id)
-{
-    Mutex::Locker locker (m_mutex);
-    collection::iterator pos = GetIDIterator(break_id);    // Predicate
-    if (pos != m_locations.end())
-    {
-        m_address_to_location.erase ((*pos)->GetAddress());
-        m_locations.erase(pos);
-        return true;
-    }
-    return false;
-}
-
-
-class BreakpointLocationIDMatches
-{
-public:
-    BreakpointLocationIDMatches (lldb::break_id_t break_id) :
-        m_break_id(break_id)
-    {
-    }
-
-    bool operator() (const BreakpointLocationSP &bp_loc_sp) const
-    {
-        return m_break_id == bp_loc_sp->GetID();
-    }
-
-private:
-   const lldb::break_id_t m_break_id;
-};
-
-class BreakpointLocationAddressMatches
-{
-public:
-    BreakpointLocationAddressMatches (Address& addr) :
-        m_addr(addr)
-    {
-    }
-
-    bool operator() (const BreakpointLocationSP& bp_loc_sp) const
-    {
-        return Address::CompareFileAddress(m_addr, bp_loc_sp->GetAddress()) == 0;
-    }
-
-private:
-   const Address &m_addr;
-};
-
-BreakpointLocationList::collection::iterator
-BreakpointLocationList::GetIDIterator (lldb::break_id_t break_id)
-{
-    Mutex::Locker locker (m_mutex);
-    return std::find_if (m_locations.begin(),
-                         m_locations.end(),
-                         BreakpointLocationIDMatches(break_id));
-}
-
-BreakpointLocationList::collection::const_iterator
-BreakpointLocationList::GetIDConstIterator (lldb::break_id_t break_id) const
-{
-    Mutex::Locker locker (m_mutex);
-    return std::find_if (m_locations.begin(),
-                         m_locations.end(),
-                         BreakpointLocationIDMatches(break_id));
-}
-
 BreakpointLocationSP
-BreakpointLocationList::FindByID (lldb::break_id_t break_id)
-{
-    Mutex::Locker locker (m_mutex);
-    BreakpointLocationSP stop_sp;
-    collection::iterator pos = GetIDIterator(break_id);
-    if (pos != m_locations.end())
-        stop_sp = *pos;
-
-    return stop_sp;
-}
-
-const BreakpointLocationSP
 BreakpointLocationList::FindByID (lldb::break_id_t break_id) const
 {
+    BreakpointLocationSP bp_loc_sp;
     Mutex::Locker locker (m_mutex);
-    BreakpointLocationSP stop_sp;
+    // We never remove a breakpoint locations, so the ID can be translated into
-    collection::const_iterator pos = GetIDConstIterator(break_id);
+    // the location index by subtracting 1
-    if (pos != m_locations.end())
+    uint32_t idx = break_id - 1;
-        stop_sp = *pos;
+    if (idx <= m_locations.size())
-
+    {
-    return stop_sp;
+        bp_loc_sp = m_locations[idx];
+    }
+    return bp_loc_sp;
 }
 
 size_t
@@ -193,18 +115,18 @@ BreakpointLocationList::FindInModule (Module *module,
 }
 
 const BreakpointLocationSP
-BreakpointLocationList::FindByAddress (Address &addr) const
+BreakpointLocationList::FindByAddress (const Address &addr) const
 {
     Mutex::Locker locker (m_mutex);
-    BreakpointLocationSP stop_sp;
+    BreakpointLocationSP bp_loc_sp;
     if (!m_locations.empty())
     {
         addr_map::const_iterator pos = m_address_to_location.find (addr);
         if (pos != m_address_to_location.end())
-            stop_sp = pos->second;
+            bp_loc_sp = pos->second;
     }
 
-    return stop_sp;
+    return bp_loc_sp;
 }
 
 void
@@ -226,22 +148,22 @@ BreakpointLocationSP
 BreakpointLocationList::GetByIndex (uint32_t i)
 {
     Mutex::Locker locker (m_mutex);
-    BreakpointLocationSP stop_sp;
+    BreakpointLocationSP bp_loc_sp;
     if (i < m_locations.size())
-        stop_sp = m_locations[i];
+        bp_loc_sp = m_locations[i];
 
-    return stop_sp;
+    return bp_loc_sp;
 }
 
 const BreakpointLocationSP
 BreakpointLocationList::GetByIndex (uint32_t i) const
 {
     Mutex::Locker locker (m_mutex);
-    BreakpointLocationSP stop_sp;
+    BreakpointLocationSP bp_loc_sp;
     if (i < m_locations.size())
-        stop_sp = m_locations[i];
+        bp_loc_sp = m_locations[i];
 
-    return stop_sp;
+    return bp_loc_sp;
 }
 
 void
@@ -291,12 +213,6 @@ BreakpointLocationList::GetNumResolvedLocations() const
     return resolve_count;
 }
 
-break_id_t
-BreakpointLocationList::GetNextID()
-{
-    return ++m_next_id;
-}
-
 void
 BreakpointLocationList::GetDescription (Stream *s, lldb::DescriptionLevel level)
 {