Two bug fixes for loading process save-core created core files, plus perf improvements

[jasonmolenda]

Summary

I had two bugs with lldb's mach-o corefile loader using the "all image infos" LC_NOTE, and while working on those, I was looking closely at the code and found several perf mistakes where I would re-do work multiple times. This patch unfortunately ends up doing several things at once.

1. If we load binaries in ObjectFileMachO via the "all image infos" LC_NOTE, ProcessMachCore::LoadBinariesViaMetadata() should detect that binaries were loaded by it, and not then scan for a dyld in the corefile, throw away the Target image list already constructed, and try to recreate it (and possibly not be successful). Also fix a mistake for the "main bin spec" LC_NOTE with userland process type in ProcessMachCore::LoadBinariesViaMetadata, noting that this is a userland dyld situation. It ProcessMachCore currently falls back to a scan, and finds dyld by exhaustive search, but that's unnecessary work.

2. Binaries would first be loaded into the Target at their mach-header vm address, then ObjectFileMachO would load each binary's segments at the correct vmaddrs. With binaries in the Darwin shared cache, the actual layout in memory has TEXT and DATA in discontiguous vm ranges. But when we load all the segments in the Target at the mach header address contiguously, we get "address 0x... maps to more than one section" warnings. The addresses for the file would then be set correctly on a segment-by-segment basis in ObjectFileMachO, but the warnings were displayed to the user. For this, I add a set_address_in_target bool argument to DynamicLoader::LoadBinaryWithUUIDAndAddress to control whether it sets the address of the binary it finds in the Target, or if it leaves this to the caller. In ObjectFileMachO::LoadCoreFileImages when a binary has per-segment vmaddrs, it sets a local set_load_address and we pass that in to our calls. Update other callers to all pass set_load_address=true (e.g. ProcessGDBRemote::LoadStubBinaries) to DynamicLoader::LoadBinaryWithUUIDAndAddress to preserve existing behavior for them.

3. Perf fix in ObjectFileMachO::LoadCoreFileImages where it would try to find the binary for a UUID multiple times as different approaches were unsuccessful. These attempts can be slow, and doing them multiple times can be more slow. Also a lot of code duplication if we have image.load_address or image.slide, but most lldb internal API take a uint64_t value and a "value is offset/slide" bool specifically to avoid this kind of duplication..

4. A previously committed perf fix https://reviews.llvm.org/D150621 was also driven by this investigation, but that patch was easily chiseled off into a separate patch, so I did that one already.

[bulbazord]

Bug fixes look good to me, the perf and code simplifications are also great to see. A few nits and I have one concern. Otherwise looks fine to me, I think.

lldb/include/lldb/Target/DynamicLoader.h

270–273

Just thinking aloud here, I wonder if we should construct some kind of "Options" struct to contain all of these bools?

lldb/source/Core/DynamicLoader.cpp

233–236

I think this technically changes behavior and probably not in a desirable way. Specifically, we previously only called DownloadObjectAndSymbolFile if force_symbol_search was true. Now you're passing it through which makes sense looking at the interface of DownloadObjectAndSymbolFile. However, peeking at the beginning of that function we see this code block:

// When dbgshell_command is empty, the user has not enabled the use of an
// external program to find the symbols, don't run it for them.
if (!force_lookup && dbgshell_command.empty())
  return false;

This means that even if force_lookup is false (or force_symbol_search as it's written here), if dbgshell_command is non-empty we'll still perform the lookup (assuming all the other checks pass) which is probably not what we intended performance-wise. That condition in the block above should probably be something like !force_lookup || dbgshell_command.empty() instead of &&.

267–269

You could use LLDB_LOG instead of LLDB_LOGF here and you'll get the file + function without explicitly writing it, but you'd have to change the formatting a bit since it uses Format and not Printf under the hood. Just a suggestion though.

276–278

Same here

285–287

Also here

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

7021

nit: this can be const

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

1000

nit: , /* set_address_in_target */ true); (or something like this)

Or you can create a const bool like above with force_symbol_search and notify.

1031

Same here

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

263–265

Same here

320–321

Same here

[jasonmolenda]

Thanks for the review comments, really helpful. I've also added a new block to DynamicLoader::LoadBinaryWithUUIDAndAddress to make the binary search system: (1) Target::GetOrCreateModule (lldb already has it), (2) Symbols::LocateExecutableObjectFile / Symbols::LocateExecutableSymbolFile (on a macOS system, this would be your Spotlight search), and then finally (3) Symbols::DownloadObjectAndSymbolFile with the force_symbol_search as passed in. I need to retest this with these different use cases to make sure it's all working correctly still, but looking pretty good now.

lldb/source/Core/DynamicLoader.cpp

233–236

Good catch. This is behavior I want, but it sure isn't real clear from the way this method is written. I will add some comments to make it clear that we want to do this external-program-lookup if either force_lookup is set, or if the user set dbgshell_command manually in their com.apple.DebugSymbols preferences.

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

7021

it's not clearly written, this is actually an output parameter from Module::SetLoadAddress. The obvious next question is - why is it initialized. It should not be, I'll remove the initialization.

[jasonmolenda]

Updating the patch to incorporate Alex's feedback, and a little further refinement with testing. I am still doing final testing now, and mostly interested in how to create a mach-o corefile that would trigger the failure that started this fix - where lldb found enough of a dyld in a modified-memory-only corefile that it cleared the binary list, but not enough to actually load the full list of binaries out of corefile memory. I think it may be tricky to get a corefile that meets that criteria, but I want to look into it a bit.

I suspect the actual code changes are done at this point though.

[bulbazord]

Looks like my concerns were addressed. Thanks!

lldb/include/lldb/Target/DynamicLoader.h

public:
   ///     Allow the search to do a possibly expensive external search for
   ///     the ObjectFile and/or SymbolFile.
   ///
   /// \param[in] notify
   ///     Whether ModulesDidLoad should be called when a binary has been added
   ///     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.
   ///
   /// On some systems (e.g. Darwin based systems), a set of libraries that are
   /// common to most processes may be put in a single region of memory and
   /// mapped into every process, this is called the shared cache, as a
   /// performance optimization.
   ///

lldb/source/Core/DynamicLoader.cpp

if (name.empty()) {
     snprintf(namebuf, sizeof(namebuf), "memory-image-0x%" PRIx64, addr);
     name = namebuf;
   }
   return process->ReadModuleFromMemory(FileSpec(name), 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();
   }
-
-  if (uuid.IsValid()) {
-    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()) {
+    // 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,
   // read it out of memory.
   if (!module_sp.get() && value != LLDB_INVALID_ADDRESS && !value_is_offset) {
     if (!memory_module_sp)
       memory_module_sp = ReadUnnamedMemoryModule(process, value, name);
     if (memory_module_sp)
       module_sp = memory_module_sp;
   }
 
   Log *log = GetLog(LLDBLog::DynamicLoader);
   if (module_sp.get()) {
     // Ensure the Target has an architecture set in case
     // we need it while processing this binary/eh_frame/debug info.
     if (!target.GetArchitecture().IsValid())
       target.SetArchitecture(module_sp->GetArchitecture());
     target.GetImages().AppendIfNeeded(module_sp, false);
 
     bool changed = false;
+    if (set_address_in_target) {
       if (module_sp->GetObjectFile()) {
         if (value != LLDB_INVALID_ADDRESS) {
-        LLDB_LOGF(log, "Loading binary UUID %s at %s 0x%" PRIx64,
+          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, "Loading binary UUID %s at file address",
+          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 {
         // In-memory image, load at its true address, offset 0.
-      LLDB_LOGF(log, "Loading binary UUID %s from memory at address 0x%" PRIx64,
+        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);
+        module_sp->SetLoadAddress(target, 0, true /* value_is_slide */,
+                                  changed);
+      }
     }
 
     if (notify) {
       ModuleList added_module;
       added_module.Append(module_sp, false);
       target.ModulesDidLoad(added_module);
     }
   } else {

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

 // Dump an image info structure to the file handle provided.
 void DynamicLoaderDarwin::ImageInfo::PutToLog(Log *log) const {
   if (!log)
     return;
   if (address == LLDB_INVALID_ADDRESS) {
     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);
   }
 }
 
 void DynamicLoaderDarwin::PrivateInitialize(Process *process) {
   DEBUG_PRINTF("DynamicLoaderDarwin::%s() process state = %s\n", __FUNCTION__,

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

if (process.GetTarget()
                                         true /* notify */)) {
       LLDB_LOGF(log,
                 "ObjectFileMachO::%s binary at 0x%" PRIx64
                 " is a platform binary, has been handled by a Platform plugin.",
                 __FUNCTION__, image.load_address);
       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 */);
-      }
+    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;
     }
 
-    // We have an address, that's the best way to discover the binary.
-    if (!module_sp && image.load_address != 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.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;
-      }
-    }
-
-    // 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) {
-      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
     // correct address/slide and notify/load scripting resources.
     if (module_sp) {
       added_modules.Append(module_sp, false /* notify */);
 
       // We have a list of segment load address
       if (image.segment_load_addresses.size() > 0) {
         if (log) {
           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();
           if (sectlist) {
             SectionSP sect_sp =
                 sectlist->FindSectionByName(std::get<0>(name_vmaddr_tuple));
             if (sect_sp) {
               process.GetTarget().SetSectionLoadAddress(
                   sect_sp, std::get<1>(name_vmaddr_tuple));
             }
           }
         }
-      } 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);
       }
     }
   }
   if (added_modules.GetSize() > 0) {
     process.GetTarget().ModulesDidLoad(added_modules);
     process.Flush();
     return true;
   }
   // Return true if the only binary we found was the platform binary,
   // and it was loaded outside the scope of this method.
   if (found_platform_binary)
     return true;
 
   // No binaries.
   return false;
 }

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

bool PlatformDarwinKernel::LoadPlatformBinaryAndSetup(Process *process,
   Log *log =
       GetLog(LLDBLog::Platform | LLDBLog::DynamicLoader | LLDBLog::Process);
 
   if (!process)
     return false;
 
   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.
 
   PlatformSP platform_sp =
       process->GetTarget().GetDebugger().GetPlatformList().Create(
           PlatformDarwinKernel::GetPluginNameStatic());
   if (platform_sp)

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

void ProcessGDBRemote::LoadStubBinaries() {
   bool standalone_value_is_offset;
   if (m_gdb_comm.GetProcessStandaloneBinary(standalone_uuid, standalone_value,
                                             standalone_value_is_offset)) {
     ModuleSP module_sp;
 
     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);
     }
   }
 
   // The remote stub may know about a list of binaries to
   // force load into the process -- a firmware type situation
   // where multiple binaries are present in virtual memory,
   // and we are only given the addresses of the binaries.
   // Not intended for use with userland debugging, when we use
for (addr_t addr : bin_addrs) {
       // Platform to be used in this Process and Target.
       if (GetTarget()
               .GetDebugger()
               .GetPlatformList()
               .LoadPlatformBinaryAndSetup(this, addr, notify))
         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);
     }
   }
 }
 
 void ProcessGDBRemote::MaybeLoadExecutableModule() {
   ModuleSP module_sp = GetTarget().GetExecutableModule();
   if (!module_sp)
     return;

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

if (core_objfile->GetCorefileMainBinaryInfo(objfile_binary_value,
 
     // If this is the xnu kernel, don't load it now.  Note the correct
     // DynamicLoader plugin to use, and the address of the kernel, and
     // let the DynamicLoader handle the finding & loading of the binary.
     if (type == ObjectFile::eBinaryTypeKernel) {
       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();
-      }
     }
   }
 
   // This checks for the presence of an LC_IDENT string in a core file;
   // LC_IDENT is very obsolete and should not be used in new code, but if the
   // load command is present, let's use the contents.
   UUID ident_uuid;
   addr_t ident_binary_addr = LLDB_INVALID_ADDRESS;
if (corefile_identifier.find("Darwin Kernel") != std::string::npos &&
       m_mach_kernel_addr = ident_binary_addr;
       found_main_binary_definitively = true;
     } else if (ident_uuid.IsValid()) {
       // We have no address specified, only a UUID.  Load it at the file
       // address.
       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();
       }
     }
   }
 
   // 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().
   // If we now have a dynamic loader, save its name so we don't 
   // un-set it later.
   if (m_dyld_up)
     m_dyld_plugin_name = GetDynamicLoader()->GetPluginName();
 }

lldb/source/Symbol/LocateSymbolFileMacOSX.cpp

 }
 
 bool Symbols::DownloadObjectAndSymbolFile(ModuleSpec &module_spec,
                                           Status &error, bool force_lookup,
                                           bool copy_executable) {
   const UUID *uuid_ptr = module_spec.GetUUIDPtr();
   const FileSpec *file_spec_ptr = module_spec.GetFileSpecPtr();
 
+  // If \a dbgshell_command is set, the user has specified
+  // forced symbol lookup via that command.  We'll get the
+  // path back from GetDsymForUUIDExecutable() later.
   llvm::StringRef dbgshell_command = GetDbgShellCommand();
 
-  // When dbgshell_command is empty, the user has not enabled the use of an
-  // external program to find the symbols, don't run it for them.
+  // If forced lookup isn't set, by the user's \a dbgshell_command or
+  // by the \a force_lookup argument, exit this method.
   if (!force_lookup && dbgshell_command.empty())
     return false;
 
-  // We need a UUID or valid (existing FileSpec.
+  // We need a UUID or valid existing FileSpec.
   if (!uuid_ptr &&
       (!file_spec_ptr || !FileSystem::Instance().Exists(*file_spec_ptr)))
     return false;
 
   // We need a dsymForUUID binary or an equivalent executable/script.
   FileSpec dsymForUUID_exe_spec = GetDsymForUUIDExecutable();
   if (!dsymForUUID_exe_spec)
     return false;