Address comment

Author: George Hu

lldb/include/lldb/Core/ModuleList.h

@@ -478,7 +478,7 @@ class ModuleList {
   GetSharedModule(const ModuleSpec &module_spec, lldb::ModuleSP &module_sp,
                   llvm::SmallVectorImpl<lldb::ModuleSP> *old_modules,
                   bool *did_create_ptr, bool always_create = false,
-                  bool allow_locate_callback = true);
+                  bool invoke_locate_callback = true);
 
   static bool RemoveSharedModule(lldb::ModuleSP &module_sp);
 

lldb/include/lldb/Core/ModuleSpec.h

@@ -16,6 +16,7 @@
 #include "lldb/Utility/Iterable.h"
 #include "lldb/Utility/Stream.h"
 #include "lldb/Utility/UUID.h"
+#include "lldb/lldb-forward.h"
 
 #include "llvm/Support/Chrono.h"
 
@@ -127,15 +128,9 @@ class ModuleSpec {
 
   lldb::DataBufferSP GetData() const { return m_data; }
 
-  Target *GetTargetPtr() {
-    auto locked = m_target.lock();
-    return locked.get();
-  }
+  lldb::TargetSP GetTargetSP() { return m_target.lock(); }
 
-  const Target *GetTargetPtr() const {
-    auto locked = m_target.lock();
-    return locked.get();
-  }
+  lldb::TargetSP GetTargetSP() const { return m_target.lock(); }
 
   void SetTarget(std::shared_ptr<Target> target) { m_target = target; }
 

lldb/source/Core/ModuleList.cpp

@@ -1033,7 +1033,7 @@ Status
 ModuleList::GetSharedModule(const ModuleSpec &module_spec, ModuleSP &module_sp,
                             llvm::SmallVectorImpl<lldb::ModuleSP> *old_modules,
                             bool *did_create_ptr, bool always_create,
-                            bool allow_locate_callback) {
+                            bool invoke_locate_callback) {
   ModuleList &shared_module_list = GetSharedModuleList();
   std::lock_guard<std::recursive_mutex> guard(
       shared_module_list.m_modules_mutex);
@@ -1089,18 +1089,16 @@ ModuleList::GetSharedModule(const ModuleSpec &module_spec, ModuleSP &module_sp,
   if (module_sp)
     return error;
 
-  // Try target's platform locate module callback before second attempt
-  if (allow_locate_callback) {
-    ModuleSpec module_spec_copy(module_spec);
-    Target *target = module_spec_copy.GetTargetPtr();
-    if (target && target->IsValid()) {
-      Platform *platform = target->GetPlatform().get();
-      if (platform) {
+  // Try target's platform locate module callback before second attempt.
+  if (invoke_locate_callback) {
+    TargetSP target_sp = module_spec.GetTargetSP();
+    if (target_sp && target_sp->IsValid()) {
+      if (PlatformSP platform_sp = target_sp->GetPlatform()) {
         FileSpec symbol_file_spec;
-        platform->CallLocateModuleCallbackIfSet(
+        platform_sp->CallLocateModuleCallbackIfSet(
             module_spec, module_sp, symbol_file_spec, did_create_ptr);
         if (module_sp) {
-          // Success! The callback found a module
+          // The callback found a module.
           return error;
         }
       }
@@ -1134,13 +1132,12 @@ ModuleList::GetSharedModule(const ModuleSpec &module_spec, ModuleSP &module_sp,
     module_sp.reset();
   }
 
-  // Get module search paths from the target if available
-  ModuleSpec module_spec_copy(module_spec);
-  Target *target = module_spec_copy.GetTargetPtr();
+  // Get module search paths from the target if available.
+  lldb::TargetSP target_sp = module_spec.GetTargetSP();
   FileSpecList module_search_paths;
   FileSpecList *module_search_paths_ptr = nullptr;
-  if (target && target->IsValid()) {
-    module_search_paths = target->GetExecutableSearchPaths();
+  if (target_sp && target_sp->IsValid()) {
+    module_search_paths = target_sp->GetExecutableSearchPaths();
     module_search_paths_ptr = &module_search_paths;
   }
 

lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.cpp

@@ -350,16 +350,13 @@ Status PlatformDarwin::GetSharedModule(
                                       old_modules, did_create_ptr);
 
     const FileSpec &platform_file = module_spec.GetFileSpec();
-    // Get module search paths from the target if available
-    ModuleSpec module_spec_copy(module_spec);
-    Target *target = module_spec_copy.GetTargetPtr();
+    // Get module search paths from the target if available.
+    TargetSP target_sp = module_spec.GetTargetSP();
     FileSpecList module_search_paths;
-    FileSpecList *module_search_paths_ptr = nullptr;
-    if (target) {
-      module_search_paths = target->GetExecutableSearchPaths();
-      module_search_paths_ptr = &module_search_paths;
+    if (target_sp) {
+      module_search_paths = target_sp->GetExecutableSearchPaths();
     }
-    if (!module_sp && module_search_paths_ptr && platform_file) {
+    if (!module_sp && !module_search_paths.IsEmpty() && platform_file) {
       // We can try to pull off part of the file path up to the bundle
       // directory level and try any module search paths...
       FileSpec bundle_directory;
@@ -382,10 +379,10 @@ Status PlatformDarwin::GetSharedModule(
           const size_t bundle_directory_len =
               bundle_directory.GetPath(bundle_dir, sizeof(bundle_dir));
           char new_path[PATH_MAX];
-          size_t num_module_search_paths = module_search_paths_ptr->GetSize();
+          size_t num_module_search_paths = module_search_paths.GetSize();
           for (size_t i = 0; i < num_module_search_paths; ++i) {
             const size_t search_path_len =
-                module_search_paths_ptr->GetFileSpecAtIndex(i).GetPath(
+                module_search_paths.GetFileSpecAtIndex(i).GetPath(
                     new_path, sizeof(new_path));
             if (search_path_len < sizeof(new_path)) {
               snprintf(new_path + search_path_len,
@@ -1311,17 +1308,14 @@ lldb_private::Status PlatformDarwin::FindBundleBinaryInExecSearchPaths(
     const ModuleSpec &module_spec, Process *process, ModuleSP &module_sp,
     llvm::SmallVectorImpl<ModuleSP> *old_modules, bool *did_create_ptr) {
   const FileSpec &platform_file = module_spec.GetFileSpec();
-  ModuleSpec module_spec_copy(module_spec);
-  Target *target = module_spec_copy.GetTargetPtr();
+  TargetSP target_sp = module_spec.GetTargetSP();
   FileSpecList module_search_paths;
-  FileSpecList *module_search_paths_ptr = nullptr;
-  if (target) {
-    module_search_paths = target->GetExecutableSearchPaths();
-    module_search_paths_ptr = &module_search_paths;
+  if (target_sp) {
+    module_search_paths = target_sp->GetExecutableSearchPaths();
   }
-  // See if the file is present in any of the module_search_paths_ptr
+  // See if the file is present in any of the module_search_paths
   // directories.
-  if (!module_sp && module_search_paths_ptr && platform_file) {
+  if (!module_sp && !module_search_paths.IsEmpty() && platform_file) {
     // create a vector of all the file / directory names in platform_file e.g.
     // this might be
     // /System/Library/PrivateFrameworks/UIFoundation.framework/UIFoundation
@@ -1335,21 +1329,21 @@ lldb_private::Status PlatformDarwin::FindBundleBinaryInExecSearchPaths(
     std::reverse(path_parts.begin(), path_parts.end());
     const size_t path_parts_size = path_parts.size();
 
-    size_t num_module_search_paths = module_search_paths_ptr->GetSize();
+    size_t num_module_search_paths = module_search_paths.GetSize();
     for (size_t i = 0; i < num_module_search_paths; ++i) {
       Log *log_verbose = GetLog(LLDBLog::Host);
       LLDB_LOGF(
           log_verbose,
           "PlatformRemoteDarwinDevice::GetSharedModule searching for binary in "
           "search-path %s",
-          module_search_paths_ptr->GetFileSpecAtIndex(i).GetPath().c_str());
+          module_search_paths.GetFileSpecAtIndex(i).GetPath().c_str());
       // Create a new FileSpec with this module_search_paths_ptr plus just the
       // filename ("UIFoundation"), then the parent dir plus filename
       // ("UIFoundation.framework/UIFoundation") etc - up to four names (to
       // handle "Foo.framework/Contents/MacOS/Foo")
 
       for (size_t j = 0; j < 4 && j < path_parts_size - 1; ++j) {
-        FileSpec path_to_try(module_search_paths_ptr->GetFileSpecAtIndex(i));
+        FileSpec path_to_try(module_search_paths.GetFileSpecAtIndex(i));
 
         // Add the components backwards.  For
         // .../PrivateFrameworks/UIFoundation.framework/UIFoundation path_parts

lldb/source/Target/Platform.cpp

@@ -764,15 +764,14 @@ Status Platform::ResolveExecutable(const ModuleSpec &module_spec,
   for (const ArchSpec &arch : GetSupportedArchitectures(process_host_arch)) {
     resolved_module_spec.GetArchitecture() = arch;
 
-    // Call locate module callback first, then fallback to standard path
+    // Call locate module callback first, then fallback to standard path.
     FileSpec symbol_file_spec;
     CallLocateModuleCallbackIfSet(resolved_module_spec, exe_module_sp,
                                   symbol_file_spec, nullptr);
 
-    if (!exe_module_sp) {
+    if (!exe_module_sp)
       error = ModuleList::GetSharedModule(resolved_module_spec, exe_module_sp,
                                           nullptr, nullptr);
-    }
 
     error = ModuleList::GetSharedModule(resolved_module_spec, exe_module_sp,
                                         nullptr, nullptr);