Fix a few bugs with Mach-O corefile loading, plus perf

In ProcessMachCore::LoadBinariesViaMetadata(), if we did
load some binaries via metadata in the core file, don't
then search for a userland dyld in the corefile / kernel
and throw away that binary list.  Also fix a little bug
with correctly recognizing corefiles using a `main bin spec`
LC_NOTE that explicitly declare that this is a userland
corefile.

LocateSymbolFileMacOSX.cpp's Symbols::DownloadObjectAndSymbolFile
clarify the comments on how the force_lookup and how the
dbgshell_command local both have the same effect.

In PlatformDarwinKernel::LoadPlatformBinaryAndSetup, don't
log a message unless we actually found a kernel fileset.

Reorganize ObjectFileMachO::LoadCoreFileImages so that it delegates
binary searching to DynamicLoader::LoadBinaryWithUUIDAndAddress and
doesn't duplicate those searches.  For searches that fail, we would
perform them multiple times in both methods.  When we have the
mach-o segment vmaddrs for a binary, don't let LoadBinaryWithUUIDAndAddress
load the binary first at its mach-o header address in the Target;
we'll load the segments at the correct addresses individually later
in this method.

DynamicLoaderDarwin::ImageInfo::PutToLog fix a LLDB_LOG logging
formatter.

In DynamicLoader::LoadBinaryWithUUIDAndAddress, instead of using
Target::GetOrCreateModule as a way to find a binary already registered
in lldb's global module cache (and implicitly add it to the Target
image list), use ModuleList::GetSharedModule() which only searches
the global module cache, don't add it to the Target.  We may not
want to add an unstripped binary to the Target.

Add a call to Symbols::DownloadObjectAndSymbolFile() even if
"force_symbol_search" isn't set -- this will turn into a
DebugSymbols call / Spotlight search on a macOS system, which
we want.

Only set the Module's LoadAddress if the caller asked us to do that.

Differential Revision: https://reviews.llvm.org/D150928
rdar://109186357

Author: jasonmolenda

lldb/include/lldb/Target/DynamicLoader.h

@@ -256,11 +256,21 @@ class DynamicLoader : public PluginInterface {
   ///     to the Target.  The caller may prefer to batch up these when loading
   ///     multiple binaries.
   ///
+  /// \param[in] set_address_in_target
+  ///     Whether the address of the binary should be set in the Target if it
+  ///     is added.  The caller may want to set the section addresses
+  ///     individually, instead of loading the binary the entire based on the
+  ///     start address or slide.  The caller is responsible for setting the
+  ///     load address for the binary or its segments in the Target if it passes
+  ///     true.
+  ///
   /// \return
   ///     Returns a shared pointer for the Module that has been added.
-  static lldb::ModuleSP LoadBinaryWithUUIDAndAddress(
-      Process *process, llvm::StringRef name, UUID uuid, lldb::addr_t value,
-      bool value_is_offset, bool force_symbol_search, bool notify);
+  static lldb::ModuleSP
+  LoadBinaryWithUUIDAndAddress(Process *process, llvm::StringRef name,
+                               UUID uuid, lldb::addr_t value,
+                               bool value_is_offset, bool force_symbol_search,
+                               bool notify, bool set_address_in_target);
 
   /// Get information about the shared cache for a process, if possible.
   ///

lldb/source/Core/DynamicLoader.cpp

@@ -187,38 +187,60 @@ static ModuleSP ReadUnnamedMemoryModule(Process *process, addr_t addr,
 
 ModuleSP DynamicLoader::LoadBinaryWithUUIDAndAddress(
     Process *process, llvm::StringRef name, UUID uuid, addr_t value,
-    bool value_is_offset, bool force_symbol_search, bool notify) {
+    bool value_is_offset, bool force_symbol_search, bool notify,
+    bool set_address_in_target) {
   ModuleSP memory_module_sp;
   ModuleSP module_sp;
   PlatformSP platform_sp = process->GetTarget().GetPlatform();
   Target &target = process->GetTarget();
   Status error;
-  ModuleSpec module_spec;
-  module_spec.GetUUID() = uuid;
 
   if (!uuid.IsValid() && !value_is_offset) {
     memory_module_sp = ReadUnnamedMemoryModule(process, value, name);
 
     if (memory_module_sp)
       uuid = memory_module_sp->GetUUID();
   }
+  ModuleSpec module_spec;
+  module_spec.GetUUID() = uuid;
+  FileSpec name_filespec(name);
+  if (FileSystem::Instance().Exists(name_filespec))
+    module_spec.GetFileSpec() = name_filespec;
 
   if (uuid.IsValid()) {
-    ModuleSpec module_spec;
-    module_spec.GetUUID() = uuid;
-
+    // Has lldb already seen a module with this UUID?
     if (!module_sp)
-      module_sp = target.GetOrCreateModule(module_spec, false, &error);
+      error = ModuleList::GetSharedModule(module_spec, module_sp, nullptr,
+                                          nullptr, nullptr);
+
+    // Can lldb's symbol/executable location schemes
+    // find an executable and symbol file.
+    if (!module_sp) {
+      FileSpecList search_paths = Target::GetDefaultDebugFileSearchPaths();
+      module_spec.GetSymbolFileSpec() =
+          Symbols::LocateExecutableSymbolFile(module_spec, search_paths);
+      ModuleSpec objfile_module_spec =
+          Symbols::LocateExecutableObjectFile(module_spec);
+      module_spec.GetFileSpec() = objfile_module_spec.GetFileSpec();
+      if (FileSystem::Instance().Exists(module_spec.GetFileSpec()) &&
+          FileSystem::Instance().Exists(module_spec.GetSymbolFileSpec())) {
+        module_sp = std::make_shared<Module>(module_spec);
+      }
+    }
 
     // If we haven't found a binary, or we don't have a SymbolFile, see
     // if there is an external search tool that can find it.
-    if (force_symbol_search &&
-        (!module_sp || !module_sp->GetSymbolFileFileSpec())) {
-      Symbols::DownloadObjectAndSymbolFile(module_spec, error, true);
+    if (!module_sp || !module_sp->GetSymbolFileFileSpec()) {
+      Symbols::DownloadObjectAndSymbolFile(module_spec, error,
+                                           force_symbol_search);
       if (FileSystem::Instance().Exists(module_spec.GetFileSpec())) {
         module_sp = std::make_shared<Module>(module_spec);
       }
     }
+
+    // If we only found the executable, create a Module based on that.
+    if (!module_sp && FileSystem::Instance().Exists(module_spec.GetFileSpec()))
+      module_sp = std::make_shared<Module>(module_spec);
   }
 
   // If we couldn't find the binary anywhere else, as a last resort,
@@ -239,25 +261,34 @@ ModuleSP DynamicLoader::LoadBinaryWithUUIDAndAddress(
     target.GetImages().AppendIfNeeded(module_sp, false);
 
     bool changed = false;
-    if (module_sp->GetObjectFile()) {
-      if (value != LLDB_INVALID_ADDRESS) {
-        LLDB_LOGF(log, "Loading binary UUID %s at %s 0x%" PRIx64,
-                  uuid.GetAsString().c_str(),
-                  value_is_offset ? "offset" : "address", value);
-        module_sp->SetLoadAddress(target, value, value_is_offset, changed);
+    if (set_address_in_target) {
+      if (module_sp->GetObjectFile()) {
+        if (value != LLDB_INVALID_ADDRESS) {
+          LLDB_LOGF(log,
+                    "DynamicLoader::LoadBinaryWithUUIDAndAddress Loading "
+                    "binary UUID %s at %s 0x%" PRIx64,
+                    uuid.GetAsString().c_str(),
+                    value_is_offset ? "offset" : "address", value);
+          module_sp->SetLoadAddress(target, value, value_is_offset, changed);
+        } else {
+          // No address/offset/slide, load the binary at file address,
+          // offset 0.
+          LLDB_LOGF(log,
+                    "DynamicLoader::LoadBinaryWithUUIDAndAddress Loading "
+                    "binary UUID %s at file address",
+                    uuid.GetAsString().c_str());
+          module_sp->SetLoadAddress(target, 0, true /* value_is_slide */,
+                                    changed);
+        }
       } else {
-        // No address/offset/slide, load the binary at file address,
-        // offset 0.
-        LLDB_LOGF(log, "Loading binary UUID %s at file address",
-                  uuid.GetAsString().c_str());
+        // In-memory image, load at its true address, offset 0.
+        LLDB_LOGF(log,
+                  "DynamicLoader::LoadBinaryWithUUIDAndAddress Loading binary "
+                  "UUID %s from memory at address 0x%" PRIx64,
+                  uuid.GetAsString().c_str(), value);
         module_sp->SetLoadAddress(target, 0, true /* value_is_slide */,
                                   changed);
       }
-    } else {
-      // In-memory image, load at its true address, offset 0.
-      LLDB_LOGF(log, "Loading binary UUID %s from memory at address 0x%" PRIx64,
-                uuid.GetAsString().c_str(), value);
-      module_sp->SetLoadAddress(target, 0, true /* value_is_slide */, changed);
     }
 
     if (notify) {

lldb/source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderDarwin.cpp

@@ -808,7 +808,7 @@ void DynamicLoaderDarwin::ImageInfo::PutToLog(Log *log) const {
     LLDB_LOG(log, "uuid={1} path='{2}' (UNLOADED)", uuid.GetAsString(),
              file_spec.GetPath());
   } else {
-    LLDB_LOG(log, "address={0:x+16} uuid={2} path='{3}'", address,
+    LLDB_LOG(log, "address={0:x+16} uuid={1} path='{2}'", address,
              uuid.GetAsString(), file_spec.GetPath());
     for (uint32_t i = 0; i < segments.size(); ++i)
       segments[i].PutToLog(log, slide);

lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp

@@ -6878,62 +6878,22 @@ bool ObjectFileMachO::LoadCoreFileImages(lldb_private::Process &process) {
       continue;
     }
 
-    // If this binary is currently executing, we want to force a
-    // possibly expensive search for the binary and its dSYM.
-    if (image.currently_executing && image.uuid.IsValid()) {
-      ModuleSpec module_spec;
-      module_spec.GetUUID() = image.uuid;
-      Symbols::DownloadObjectAndSymbolFile(module_spec, error, true);
-      if (FileSystem::Instance().Exists(module_spec.GetFileSpec())) {
-        module_sp = process.GetTarget().GetOrCreateModule(module_spec, false);
-        process.GetTarget().GetImages().AppendIfNeeded(module_sp,
-                                                       false /* notify */);
-      }
-    }
-
-    // We have an address, that's the best way to discover the binary.
-    if (!module_sp && image.load_address != LLDB_INVALID_ADDRESS) {
-      module_sp = DynamicLoader::LoadBinaryWithUUIDAndAddress(
-          &process, image.filename, image.uuid, image.load_address,
-          false /* value_is_offset */, image.currently_executing,
-          false /* notify */);
-      if (module_sp) {
-        // We've already set the load address in the Target,
-        // don't do any more processing on this module.
-        added_modules.Append(module_sp, false /* notify */);
-        continue;
-      }
+    bool value_is_offset = image.load_address == LLDB_INVALID_ADDRESS;
+    uint64_t value = value_is_offset ? image.slide : image.load_address;
+    if (value_is_offset && value == LLDB_INVALID_ADDRESS) {
+      // We have neither address nor slide; so we will find the binary
+      // by UUID and load it at slide/offset 0.
+      value = 0;
     }
 
-    // If we have a slide, we need to find the original binary
-    // by UUID, then we can apply the slide value.
-    if (!module_sp && image.uuid.IsValid() &&
-        image.slide != LLDB_INVALID_ADDRESS) {
+    // We have either a UUID, or we have a load address which
+    // and can try to read load commands and find a UUID.
+    if (image.uuid.IsValid() ||
+        (!value_is_offset && value != LLDB_INVALID_ADDRESS)) {
+      const bool set_load_address = image.segment_load_addresses.size() == 0;
       module_sp = DynamicLoader::LoadBinaryWithUUIDAndAddress(
-          &process, image.filename, image.uuid, image.slide,
-          true /* value_is_offset */, image.currently_executing,
-          false /* notify */);
-      if (module_sp) {
-        // We've already set the load address in the Target,
-        // don't do any more processing on this module.
-        added_modules.Append(module_sp, false /* notify */);
-        continue;
-      }
-    }
-
-    // Try to find the binary by UUID or filename on the local
-    // filesystem or in lldb's global module cache.
-    if (!module_sp) {
-      Status error;
-      ModuleSpec module_spec;
-      if (image.uuid.IsValid())
-        module_spec.GetUUID() = image.uuid;
-      if (!image.filename.empty())
-        module_spec.GetFileSpec() = FileSpec(image.filename.c_str());
-      module_sp =
-          process.GetTarget().GetOrCreateModule(module_spec, false, &error);
-      process.GetTarget().GetImages().AppendIfNeeded(module_sp,
-                                                     false /* notify */);
+          &process, image.filename, image.uuid, value, value_is_offset,
+          image.currently_executing, false /* notify */, set_load_address);
     }
 
     // We have a ModuleSP to load in the Target.  Load it at the
@@ -6947,7 +6907,8 @@ bool ObjectFileMachO::LoadCoreFileImages(lldb_private::Process &process) {
           std::string uuidstr = image.uuid.GetAsString();
           log->Printf("ObjectFileMachO::LoadCoreFileImages adding binary '%s' "
                       "UUID %s with section load addresses",
-                      image.filename.c_str(), uuidstr.c_str());
+                      module_sp->GetFileSpec().GetPath().c_str(),
+                      uuidstr.c_str());
         }
         for (auto name_vmaddr_tuple : image.segment_load_addresses) {
           SectionList *sectlist = module_sp->GetObjectFile()->GetSectionList();
@@ -6960,39 +6921,17 @@ bool ObjectFileMachO::LoadCoreFileImages(lldb_private::Process &process) {
             }
           }
         }
-      } else if (image.load_address != LLDB_INVALID_ADDRESS) {
-        if (log) {
-          std::string uuidstr = image.uuid.GetAsString();
-          log->Printf("ObjectFileMachO::LoadCoreFileImages adding binary '%s' "
-                      "UUID %s with load address 0x%" PRIx64,
-                      image.filename.c_str(), uuidstr.c_str(),
-                      image.load_address);
-        }
-        const bool address_is_slide = false;
-        bool changed = false;
-        module_sp->SetLoadAddress(process.GetTarget(), image.load_address,
-                                  address_is_slide, changed);
-      } else if (image.slide != 0) {
-        if (log) {
-          std::string uuidstr = image.uuid.GetAsString();
-          log->Printf("ObjectFileMachO::LoadCoreFileImages adding binary '%s' "
-                      "UUID %s with slide amount 0x%" PRIx64,
-                      image.filename.c_str(), uuidstr.c_str(), image.slide);
-        }
-        const bool address_is_slide = true;
-        bool changed = false;
-        module_sp->SetLoadAddress(process.GetTarget(), image.slide,
-                                  address_is_slide, changed);
       } else {
         if (log) {
           std::string uuidstr = image.uuid.GetAsString();
           log->Printf("ObjectFileMachO::LoadCoreFileImages adding binary '%s' "
-                      "UUID %s at its file address, no slide applied",
-                      image.filename.c_str(), uuidstr.c_str());
+                      "UUID %s with %s 0x%" PRIx64,
+                      module_sp->GetFileSpec().GetPath().c_str(),
+                      uuidstr.c_str(),
+                      value_is_offset ? "slide" : "load address", value);
         }
-        const bool address_is_slide = true;
-        bool changed = false;
-        module_sp->SetLoadAddress(process.GetTarget(), 0, address_is_slide,
+        bool changed;
+        module_sp->SetLoadAddress(process.GetTarget(), value, value_is_offset,
                                   changed);
       }
     }

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

@@ -952,14 +952,14 @@ bool PlatformDarwinKernel::LoadPlatformBinaryAndSetup(Process *process,
 
   addr_t actual_address = find_kernel_in_macho_fileset(process, input_addr);
 
+  if (actual_address == LLDB_INVALID_ADDRESS)
+    return false;
+
   LLDB_LOGF(log,
             "PlatformDarwinKernel::%s check address 0x%" PRIx64 " for "
             "a macho fileset, got back kernel address 0x%" PRIx64,
             __FUNCTION__, input_addr, actual_address);
 
-  if (actual_address == LLDB_INVALID_ADDRESS)
-    return false;
-
   // We have a xnu kernel binary, this is a kernel debug session.
   // Set the Target's Platform to be PlatformDarwinKernel, and the
   // Process' DynamicLoader to be DynamicLoaderDarwinKernel.

lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp

@@ -995,9 +995,11 @@ void ProcessGDBRemote::LoadStubBinaries() {
     if (standalone_uuid.IsValid()) {
       const bool force_symbol_search = true;
       const bool notify = true;
+      const bool set_address_in_target = true;
       DynamicLoader::LoadBinaryWithUUIDAndAddress(
           this, "", standalone_uuid, standalone_value,
-          standalone_value_is_offset, force_symbol_search, notify);
+          standalone_value_is_offset, force_symbol_search, notify,
+          set_address_in_target);
     }
   }
 
@@ -1025,10 +1027,11 @@ void ProcessGDBRemote::LoadStubBinaries() {
         continue;
 
       const bool force_symbol_search = true;
+      const bool set_address_in_target = true;
       // Second manually load this binary into the Target.
-      DynamicLoader::LoadBinaryWithUUIDAndAddress(this, llvm::StringRef(), uuid,
-                                                  addr, value_is_slide,
-                                                  force_symbol_search, notify);
+      DynamicLoader::LoadBinaryWithUUIDAndAddress(
+          this, llvm::StringRef(), uuid, addr, value_is_slide,
+          force_symbol_search, notify, set_address_in_target);
     }
   }
 }

lldb/source/Plugins/Process/mach-core/ProcessMachCore.cpp

@@ -252,20 +252,20 @@ void ProcessMachCore::LoadBinariesViaMetadata() {
       m_mach_kernel_addr = objfile_binary_value;
       m_dyld_plugin_name = DynamicLoaderDarwinKernel::GetPluginNameStatic();
       found_main_binary_definitively = true;
+    } else if (type == ObjectFile::eBinaryTypeUser) {
+      m_dyld_addr = objfile_binary_value;
+      m_dyld_plugin_name = DynamicLoaderMacOSXDYLD::GetPluginNameStatic();
     } else {
       const bool force_symbol_search = true;
       const bool notify = true;
+      const bool set_address_in_target = true;
       if (DynamicLoader::LoadBinaryWithUUIDAndAddress(
               this, llvm::StringRef(), objfile_binary_uuid,
               objfile_binary_value, objfile_binary_value_is_offset,
-              force_symbol_search, notify)) {
+              force_symbol_search, notify, set_address_in_target)) {
         found_main_binary_definitively = true;
         m_dyld_plugin_name = DynamicLoaderStatic::GetPluginNameStatic();
       }
-      if (type == ObjectFile::eBinaryTypeUser) {
-        m_dyld_addr = objfile_binary_value;
-        m_dyld_plugin_name = DynamicLoaderMacOSXDYLD::GetPluginNameStatic();
-      }
     }
   }
 
@@ -314,9 +314,11 @@ void ProcessMachCore::LoadBinariesViaMetadata() {
       const bool value_is_offset = false;
       const bool force_symbol_search = true;
       const bool notify = true;
+      const bool set_address_in_target = true;
       if (DynamicLoader::LoadBinaryWithUUIDAndAddress(
               this, llvm::StringRef(), ident_uuid, ident_binary_addr,
-              value_is_offset, force_symbol_search, notify)) {
+              value_is_offset, force_symbol_search, notify,
+              set_address_in_target)) {
         found_main_binary_definitively = true;
         m_dyld_plugin_name = DynamicLoaderStatic::GetPluginNameStatic();
       }
@@ -325,7 +327,10 @@ void ProcessMachCore::LoadBinariesViaMetadata() {
 
   // Finally, load any binaries noted by "load binary" LC_NOTEs in the
   // corefile
-  core_objfile->LoadCoreFileImages(*this);
+  if (core_objfile->LoadCoreFileImages(*this)) {
+    found_main_binary_definitively = true;
+    m_dyld_plugin_name = DynamicLoaderStatic::GetPluginNameStatic();
+  }
 
   // LoadCoreFileImges may have set the dynamic loader, e.g. in
   // PlatformDarwinKernel::LoadPlatformBinaryAndSetup().