If we notice that a module with a given file path is replaced by another with the...

path on rerunning, evict the old module from the target module list, inform the breakpoints
about this so they can do something intelligent as well.

rdar://problem/11273043

llvm-svn: 157008

diff --git a/lldb/include/lldb/Breakpoint/Breakpoint.h b/lldb/include/lldb/Breakpoint/Breakpoint.h
index 782cf45..552c96d 100644 (file)
--- a/lldb/include/lldb/Breakpoint/Breakpoint.h
+++ b/lldb/include/lldb/Breakpoint/Breakpoint.h
@@ -203,11 +203,11 @@ public:
     /// Tell this breakpoint to scan a given module list and resolve any
     /// new locations that match the breakpoint's specifications.
     ///
-    /// @param[in] changedModules
+    /// @param[in] changed_modules
     ///    The list of modules to look in for new locations.
     //------------------------------------------------------------------
     void
-    ResolveBreakpointInModules (ModuleList &changedModules);
+    ResolveBreakpointInModules (ModuleList &changed_modules);
 
 
     //------------------------------------------------------------------
@@ -219,13 +219,29 @@ public:
     ///    The list of modules to look in for new locations.
     /// @param[in] load_event
     ///    If \b true then the modules were loaded, if \b false, unloaded.
+    /// @param[in] delete_locations
+    ///    If \b true then the modules were unloaded delete any locations in the changed modules.
     //------------------------------------------------------------------
     void
-    ModulesChanged (ModuleList &changedModules,
-                    bool load_event);
+    ModulesChanged (ModuleList &changed_modules,
+                    bool load_event,
+                    bool delete_locations = false);
 
 
     //------------------------------------------------------------------
+    /// Tells the breakpoint the old module \a old_module_sp has been
+    /// replaced by new_module_sp (usually because the underlying file has been
+    /// rebuilt, and the old version is gone.)
+    ///
+    /// @param[in] old_module_sp
+    ///    The old module that is going away.
+    /// @param[in] new_module_sp
+    ///    The new module that is replacing it.
+    //------------------------------------------------------------------
+    void
+    ModuleReplaced (lldb::ModuleSP old_module_sp, lldb::ModuleSP new_module_sp);
+    
+    //------------------------------------------------------------------
     // The next set of methods provide access to the breakpoint locations
     // for this breakpoint.
     //------------------------------------------------------------------

diff --git a/lldb/include/lldb/Breakpoint/BreakpointList.h b/lldb/include/lldb/Breakpoint/BreakpointList.h
index e5ccb8e..cf2584c 100644 (file)
--- a/lldb/include/lldb/Breakpoint/BreakpointList.h
+++ b/lldb/include/lldb/Breakpoint/BreakpointList.h
@@ -154,6 +154,9 @@ public:
     //------------------------------------------------------------------
     void
     UpdateBreakpoints (ModuleList &module_list, bool added);
+    
+    void
+    UpdateBreakpointsWhenModuleIsReplaced (lldb::ModuleSP old_module_sp, lldb::ModuleSP new_module_sp);
 
     void
     ClearAllBreakpointSites ();

diff --git a/lldb/include/lldb/Core/ModuleList.h b/lldb/include/lldb/Core/ModuleList.h
index afe6dcb..c3b9a63 100644 (file)
--- a/lldb/include/lldb/Core/ModuleList.h
+++ b/lldb/include/lldb/Core/ModuleList.h
@@ -355,6 +355,9 @@ public:
     size_t
     Remove (ModuleList &module_list);
     
+    bool
+    RemoveIfOrphaned (const Module *module_ptr);
+    
     size_t
     RemoveOrphans (bool mandatory);
 
@@ -419,6 +422,9 @@ public:
 
     static uint32_t
     RemoveOrphanSharedModules (bool mandatory);
+    
+    static bool
+    RemoveSharedModuleIfOrphaned (const Module *module_ptr);
 
 protected:
     //------------------------------------------------------------------

diff --git a/lldb/source/Breakpoint/Breakpoint.cpp b/lldb/source/Breakpoint/Breakpoint.cpp
index 62e1b24..5d0cbd4 100644 (file)
--- a/lldb/source/Breakpoint/Breakpoint.cpp
+++ b/lldb/source/Breakpoint/Breakpoint.cpp
@@ -315,7 +315,7 @@ Breakpoint::ClearAllBreakpointSites ()
 //----------------------------------------------------------------------
 
 void
-Breakpoint::ModulesChanged (ModuleList &module_list, bool load)
+Breakpoint::ModulesChanged (ModuleList &module_list, bool load, bool delete_locations)
 {
     if (load)
     {
@@ -395,11 +395,7 @@ Breakpoint::ModulesChanged (ModuleList &module_list, bool load)
     else
     {
         // Go through the currently set locations and if any have breakpoints in
-        // the module list, then remove their breakpoint sites.
-        // FIXME: Think about this...  Maybe it's better to delete the locations?
-        // Are we sure that on load-unload-reload the module pointer will remain
-        // the same?  Or do we need to do an equality on modules that is an
-        // "equivalence"???
+        // the module list, then remove their breakpoint sites, and their locations if asked to.
 
         BreakpointEventData *removed_locations_event;
         if (!IsInternal())
@@ -414,7 +410,9 @@ Breakpoint::ModulesChanged (ModuleList &module_list, bool load)
             if (m_filter_sp->ModulePasses (module_sp))
             {
                 size_t loc_idx = 0;
-                while (loc_idx < m_locations.GetSize())
+                size_t num_locations = m_locations.GetSize();
+                BreakpointLocationCollection locations_to_remove;
+                for (loc_idx = 0; loc_idx < num_locations; loc_idx++)
                 {
                     BreakpointLocationSP break_loc_sp (m_locations.GetByIndex(loc_idx));
                     SectionSP section_sp (break_loc_sp->GetAddress().GetSection());
@@ -429,9 +427,17 @@ Breakpoint::ModulesChanged (ModuleList &module_list, bool load)
                         {
                             removed_locations_event->GetBreakpointLocationCollection().Add(break_loc_sp);
                         }
-                        //m_locations.RemoveLocation  (break_loc_sp);
+                        if (delete_locations)
+                            locations_to_remove.Add (break_loc_sp);
+                            
                     }
-                    ++loc_idx;
+                }
+                
+                if (delete_locations)
+                {
+                    size_t num_locations_to_remove = locations_to_remove.GetSize();
+                    for (loc_idx = 0; loc_idx < num_locations_to_remove; loc_idx++)
+                        m_locations.RemoveLocation  (locations_to_remove.GetByIndex(loc_idx));
                 }
             }
         }
@@ -440,6 +446,23 @@ Breakpoint::ModulesChanged (ModuleList &module_list, bool load)
 }
 
 void
+Breakpoint::ModuleReplaced (ModuleSP old_module_sp, ModuleSP new_module_sp)
+{
+    ModuleList temp_list;
+    temp_list.Append (new_module_sp);
+    ModulesChanged (temp_list, true);
+
+    // TO DO: For now I'm just adding locations for the new module and removing the
+    // breakpoint locations that were in the old module.
+    // We should really go find the ones that are in the new module & if we can determine that they are "equivalent"
+    // carry over the options from the old location to the new.
+
+    temp_list.Clear();
+    temp_list.Append (old_module_sp);
+    ModulesChanged (temp_list, false, true);
+}
+
+void
 Breakpoint::Dump (Stream *)
 {
 }

diff --git a/lldb/source/Breakpoint/BreakpointList.cpp b/lldb/source/Breakpoint/BreakpointList.cpp
index e1aa2db..6a91bd6 100644 (file)
--- a/lldb/source/Breakpoint/BreakpointList.cpp
+++ b/lldb/source/Breakpoint/BreakpointList.cpp
@@ -210,6 +210,17 @@ BreakpointList::UpdateBreakpoints (ModuleList& module_list, bool added)
 }
 
 void
+BreakpointList::UpdateBreakpointsWhenModuleIsReplaced (ModuleSP old_module_sp, ModuleSP new_module_sp)
+{
+    Mutex::Locker locker(m_mutex);
+    bp_collection::iterator end = m_breakpoints.end();
+    bp_collection::iterator pos;
+    for (pos = m_breakpoints.begin(); pos != end; ++pos)
+        (*pos)->ModuleReplaced (old_module_sp, new_module_sp);
+
+}
+
+void
 BreakpointList::ClearAllBreakpointSites ()
 {
     Mutex::Locker locker(m_mutex);

diff --git a/lldb/source/Core/ModuleList.cpp b/lldb/source/Core/ModuleList.cpp
index c5a8955..8f852d4 100644 (file)
--- a/lldb/source/Core/ModuleList.cpp
+++ b/lldb/source/Core/ModuleList.cpp
@@ -110,6 +110,29 @@ ModuleList::Remove (const ModuleSP &module_sp)
     return false;
 }
 
+bool
+ModuleList::RemoveIfOrphaned (const Module *module_ptr)
+{
+    if (module_ptr)
+    {
+        Mutex::Locker locker(m_modules_mutex);
+        collection::iterator pos, end = m_modules.end();
+        for (pos = m_modules.begin(); pos != end; ++pos)
+        {
+            if (pos->get() == module_ptr)
+            {
+                if (pos->unique())
+                {
+                    pos = m_modules.erase (pos);
+                    return true;
+                }
+                else
+                    return false;
+            }
+        }
+    }
+    return false;
+}
 
 size_t
 ModuleList::RemoveOrphans (bool mandatory)
@@ -817,4 +840,10 @@ ModuleList::RemoveSharedModule (lldb::ModuleSP &module_sp)
     return GetSharedModuleList ().Remove (module_sp);
 }
 
+bool
+ModuleList::RemoveSharedModuleIfOrphaned (const Module *module_ptr)
+{
+    return GetSharedModuleList ().RemoveIfOrphaned (module_ptr);
+}
+
 

diff --git a/lldb/source/Target/Target.cpp b/lldb/source/Target/Target.cpp
index 7255b04..a1caed4 100644 (file)
--- a/lldb/source/Target/Target.cpp
+++ b/lldb/source/Target/Target.cpp
@@ -978,12 +978,7 @@ void
 Target::ModuleUpdated (ModuleSP &old_module_sp, ModuleSP &new_module_sp)
 {
     // A module is replacing an already added module
-    ModuleList module_list;
-    module_list.Append (old_module_sp);
-    ModulesDidUnload (module_list);
-    module_list.Clear ();
-    module_list.Append (new_module_sp);
-    ModulesDidLoad (module_list);
+    m_breakpoint_list.UpdateBreakpointsWhenModuleIsReplaced(old_module_sp, new_module_sp);
 }
 
 void
@@ -1287,76 +1282,112 @@ Target::ReadPointerFromMemory (const Address& addr,
 ModuleSP
 Target::GetSharedModule (const ModuleSpec &module_spec, Error *error_ptr)
 {
-    // Don't pass in the UUID so we can tell if we have a stale value in our list
-    ModuleSP old_module_sp; // This will get filled in if we have a new version of the library
-    bool did_create_module = false;
     ModuleSP module_sp;
 
     Error error;
 
-    // If there are image search path entries, try to use them first to acquire a suitable image.
-    if (m_image_search_paths.GetSize())
-    {
-        ModuleSpec transformed_spec (module_spec);
-        if (m_image_search_paths.RemapPath (module_spec.GetFileSpec().GetDirectory(), transformed_spec.GetFileSpec().GetDirectory()))
-        {
-            transformed_spec.GetFileSpec().GetFilename() = module_spec.GetFileSpec().GetFilename();
-            error = ModuleList::GetSharedModule (transformed_spec, 
-                                                 module_sp, 
-                                                 &GetExecutableSearchPaths(),
-                                                 &old_module_sp, 
-                                                 &did_create_module);
-        }
-    }
+    // First see if we already have this module in our module list.  If we do, then we're done, we don't need
+    // to consult the shared modules list.  But only do this if we are passed a UUID.
     
+    if (module_spec.GetUUID().IsValid())
+        module_sp = m_images.FindFirstModule(module_spec);
+        
     if (!module_sp)
     {
-        // If we have a UUID, we can check our global shared module list in case
-        // we already have it. If we don't have a valid UUID, then we can't since
-        // the path in "module_spec" will be a platform path, and we will need to
-        // let the platform find that file. For example, we could be asking for
-        // "/usr/lib/dyld" and if we do not have a UUID, we don't want to pick
-        // the local copy of "/usr/lib/dyld" since our platform could be a remote
-        // platform that has its own "/usr/lib/dyld" in an SDK or in a local file
-        // cache.
-        if (module_spec.GetUUID().IsValid())
+        ModuleSP old_module_sp; // This will get filled in if we have a new version of the library
+        bool did_create_module = false;
+    
+        // If there are image search path entries, try to use them first to acquire a suitable image.
+        if (m_image_search_paths.GetSize())
         {
-            // We have a UUID, it is OK to check the global module list...
-            error = ModuleList::GetSharedModule (module_spec,
-                                                 module_sp, 
-                                                 &GetExecutableSearchPaths(),
-                                                 &old_module_sp, 
-                                                 &did_create_module);
+            ModuleSpec transformed_spec (module_spec);
+            if (m_image_search_paths.RemapPath (module_spec.GetFileSpec().GetDirectory(), transformed_spec.GetFileSpec().GetDirectory()))
+            {
+                transformed_spec.GetFileSpec().GetFilename() = module_spec.GetFileSpec().GetFilename();
+                error = ModuleList::GetSharedModule (transformed_spec, 
+                                                     module_sp, 
+                                                     &GetExecutableSearchPaths(),
+                                                     &old_module_sp, 
+                                                     &did_create_module);
+            }
         }
-
+        
         if (!module_sp)
         {
-            // The platform is responsible for finding and caching an appropriate
-            // module in the shared module cache.
-            if (m_platform_sp)
+            // If we have a UUID, we can check our global shared module list in case
+            // we already have it. If we don't have a valid UUID, then we can't since
+            // the path in "module_spec" will be a platform path, and we will need to
+            // let the platform find that file. For example, we could be asking for
+            // "/usr/lib/dyld" and if we do not have a UUID, we don't want to pick
+            // the local copy of "/usr/lib/dyld" since our platform could be a remote
+            // platform that has its own "/usr/lib/dyld" in an SDK or in a local file
+            // cache.
+            if (module_spec.GetUUID().IsValid())
             {
-                FileSpec platform_file_spec;        
-                error = m_platform_sp->GetSharedModule (module_spec, 
-                                                        module_sp, 
-                                                        &GetExecutableSearchPaths(),
-                                                        &old_module_sp, 
-                                                        &did_create_module);
+                // We have a UUID, it is OK to check the global module list...
+                error = ModuleList::GetSharedModule (module_spec,
+                                                     module_sp, 
+                                                     &GetExecutableSearchPaths(),
+                                                     &old_module_sp, 
+                                                     &did_create_module);
             }
-            else
+
+            if (!module_sp)
             {
-                error.SetErrorString("no platform is currently set");
+                // The platform is responsible for finding and caching an appropriate
+                // module in the shared module cache.
+                if (m_platform_sp)
+                {
+                    FileSpec platform_file_spec;        
+                    error = m_platform_sp->GetSharedModule (module_spec, 
+                                                            module_sp, 
+                                                            &GetExecutableSearchPaths(),
+                                                            &old_module_sp, 
+                                                            &did_create_module);
+                }
+                else
+                {
+                    error.SetErrorString("no platform is currently set");
+                }
             }
         }
-    }
 
-    // If a module hasn't been found yet, use the unmodified path.
-    if (module_sp)
-    {
-        m_images.Append (module_sp);
-        if (did_create_module)
+        // We found a module that wasn't in our target list.  Let's make sure that there wasn't an equivalent
+        // module in the list already, and if there was, let's remove it.
+        if (module_sp)
         {
+            // GetSharedModule is not guaranteed to find the old shared module, for instance
+            // in the common case where you pass in the UUID, it is only going to find the one
+            // module matching the UUID.  In fact, it has no good way to know what the "old module"
+            // relevant to this target is, since there might be many copies of a module with this file spec
+            // in various running debug sessions, but only one of them will belong to this target.
+            // So let's remove the UUID from the module list, and look in the target's module list.
+            // Only do this if there is SOMETHING else in the module spec...
+            if (!old_module_sp)
+            {
+                if (module_spec.GetUUID().IsValid() && !module_spec.GetFileSpec().GetFilename().IsEmpty() && !module_spec.GetFileSpec().GetDirectory().IsEmpty())
+                {
+                    ModuleSpec module_spec_copy(module_spec.GetFileSpec());
+                    module_spec_copy.GetUUID().Clear();
+                    
+                    ModuleList found_modules;
+                    size_t num_found = m_images.FindModules (module_spec_copy, found_modules);
+                    if (num_found == 1)
+                    {
+                        old_module_sp = found_modules.GetModuleAtIndex(0);
+                    }
+                }
+            }
+            
+            m_images.Append (module_sp);
             if (old_module_sp && m_images.GetIndexForModule (old_module_sp.get()) != LLDB_INVALID_INDEX32)
+            {
                 ModuleUpdated(old_module_sp, module_sp);
+                m_images.Remove (old_module_sp);
+                Module *old_module_ptr = old_module_sp.get();
+                old_module_sp.reset();
+                ModuleList::RemoveSharedModuleIfOrphaned (old_module_ptr);
+            }
             else
                 ModuleAdded(module_sp);
         }