Add support for mach-o corefile loading of a platform binary

[jasonmolenda]

Summary

This builds on the work in https://reviews.llvm.org/D133534 where support for a platform binary in live process debugging was added, and adds it to the Mach-O corefile loading as well. It has five changes:

1. Add a name field to DynamicLoader::LoadBinaryWithUUIDAndAddress when we have a name, but may end up reading the binary out of memory, so we can use the name instead of a generic placeholder.

2. In ObjectFileMachO::LoadCoreFileImages, check if the image address is a platform binary, load it and set the Platform/DynamicLoader as appropriate.

3. Clean up the different cases in LoadCoreFileImages that handle corefile binary loading so it is clearer to read.

4. Separate ProcessMachCore::DoLoadCore into half a dozen smaller helper methods. This method had grown quite large and it was difficult to trace the code flow through the different parts, and the complexity was unnecessary.

5. In the new ProcessMachCore::LoadBinariesViaMetadata method which handles LC_NOTE type hints about binaries, when we have a main bin spec LC_NOTE that is explicitly a kernel, don't load the binary here -- just save the address and note that this will be a DynamicLoaderDarwinKernel dynamic loader, and leave the loading up to DynamicLoaderDarwinKernel to do correctly.

The minor cleanups in ObjectFileMachO::LoadCoreFileImages are easy to read in the phabracator if you only read the new version of the code. The (seriously needed) separation of ObjectFileMachO::LoadCoreFileImages into separate methods has made the diff very difficult to read. In my own reviewing of the patch, I've settled on simply reading through the new version of the file. Most of it is simple code movement, but I did make some cleanups to how sections of code were working -- for instance in the exhaustive search for a dyld or kernel binary, when multiple images that appear to be a kernel are found, I handle this better now by allowing DynamicLoaderDarwinKernel to pick the kernel image. It's common that we get fragments in a corefile that look similar to a kernel and can confuse the exhaustive search algorithm.

Similar to the problem of testing this with a live process, to test this I'd need to construct a corefile with a Mach-O fileset with a kernel image in it. It would not be easy to construct a test case without a corefile writer that synthesized all of this together, a not small bit of code, and it would also need to modify a test binary's mach header to look enough like a kernel that lldb would be tricked into using it.

[DavidSpickett]

lldb/include/lldb/Target/DynamicLoader.h

231

Can you list what it might be constructed from? My guess:

• the filename if loading from a path
• <format>@<address> if loading from something in memory

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

420–421

I think the various log messages could use __FUNCTION__ instead but hey let's not make this diff any bigger :) .
(the one time I know you need to write out the name is inside a lambda)

[JDevlieghere]

LGTM

[jasonmolenda]

lldb/include/lldb/Target/DynamicLoader.h

231

Thanks, this should have been clearer. Yeah this name is only used when a file cannot be found on the debug host; it may construct a module out of live memory. And in that case, it needs to make up a name for it.

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

420–421

LOL I even updated the names of all the log messages to reflect their new methods, but of course you're right.

[jasonmolenda]

Update to address David's feedback.

lldb/include/lldb/Target/DynamicLoader.h

public:
   /// or with the slide applied, or at the file address unslid.
   ///
   /// Given an address, try to read the binary out of memory, get the UUID,
   /// find the file if possible and load it unslid, or add the memory module.
   ///
   /// \param[in] process
   ///     The process to add this binary to.
   ///
+  /// \param[in] name
+  ///     Name of the binary, if available.  If this method cannot find a
+  ///     matching binary on the debug host, it may create a memory module
+  ///     out of live memory, and the provided name will be used.  If an
+  ///     empty StringRef is provided, a name will be constructed for the module
+  ///     based on the address it is loaded at.
+  ///
   /// \param[in] uuid
   ///     UUID of the binary to be loaded.  UUID may be empty, and if a
   ///     load address is supplied, will read the binary from memory, get
   ///     a UUID and try to find a local binary.  There is a performance
   ///     cost to doing this, it is not preferable.
   ///
   /// \param[in] value
   ///     Address where the binary should be loaded, or read out of memory.
public:
   ///
   /// \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.
   ///
   /// \return
   ///     Returns a shared pointer for the Module that has been added.
-  static lldb::ModuleSP
-  LoadBinaryWithUUIDAndAddress(Process *process, 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);
 
   /// 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 (ModuleSP module_sp = FindModuleViaTarget(file)) {
     UpdateLoadedSections(module_sp, link_map_addr, base_addr,
                          base_addr_is_offset);
     return module_sp;
   }
 
   return nullptr;
 }
 
-static ModuleSP ReadUnnamedMemoryModule(Process *process, addr_t addr) {
+static ModuleSP ReadUnnamedMemoryModule(Process *process, addr_t addr,
+                                        llvm::StringRef name) {
   char namebuf[80];
+  if (name.empty()) {
     snprintf(namebuf, sizeof(namebuf), "memory-image-0x%" PRIx64, addr);
-  return process->ReadModuleFromMemory(FileSpec(namebuf), addr);
+    name = namebuf;
+  }
+  return process->ReadModuleFromMemory(FileSpec(name), addr);
 }
 
-ModuleSP DynamicLoader::LoadBinaryWithUUIDAndAddress(Process *process,
-                                                     UUID uuid, addr_t value,
-                                                     bool value_is_offset,
-                                                     bool force_symbol_search,
-                                                     bool notify) {
+ModuleSP DynamicLoader::LoadBinaryWithUUIDAndAddress(
+    Process *process, llvm::StringRef name, UUID uuid, addr_t value,
+    bool value_is_offset, bool force_symbol_search, bool notify) {
   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);
+    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;
if (force_symbol_search &&
       }
     }
   }
 
   // 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);
+      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.

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

ObjectFileMachO::GetCorefileAllImageInfos() {
   }
 
   return image_infos;
 }
 
 bool ObjectFileMachO::LoadCoreFileImages(lldb_private::Process &process) {
   MachOCorefileAllImageInfos image_infos = GetCorefileAllImageInfos();
   Log *log = GetLog(LLDBLog::DynamicLoader);
+  Status error;
 
+  bool found_platform_binary = false;
   ModuleList added_modules;
-  for (const MachOCorefileImageEntry &image : image_infos.all_image_infos) {
-    ModuleSP module_sp;
+  for (MachOCorefileImageEntry &image : image_infos.all_image_infos) {
+    ModuleSP module_sp, local_filesystem_module_sp;
 
-    if (!image.filename.empty()) {
-      Status error;
+    // If this is a platform binary, it has been loaded (or registered with
+    // the DynamicLoader to be loaded), we don't need to do any further
+    // processing.  We're not going to call ModulesDidLoad on this in this
+    // method, so notify==true.
+    if (process.GetTarget()
+            .GetDebugger()
+            .GetPlatformList()
+            .LoadPlatformBinaryAndSetup(&process, image.load_address,
+                                        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;
-      module_spec.GetFileSpec() = FileSpec(image.filename.c_str());
-      if (image.currently_executing) {
-        Symbols::DownloadObjectAndSymbolFile(module_spec, error, true);
-        if (FileSystem::Instance().Exists(module_spec.GetFileSpec())) {
-          process.GetTarget().GetOrCreateModule(module_spec, false);
-        }
-      }
-      module_sp =
-          process.GetTarget().GetOrCreateModule(module_spec, false, &error);
+        module_sp = process.GetTarget().GetOrCreateModule(module_spec, false);
         process.GetTarget().GetImages().AppendIfNeeded(module_sp,
                                                        false /* notify */);
-    } else {
-      if (image.load_address != LLDB_INVALID_ADDRESS) {
-        module_sp = DynamicLoader::LoadBinaryWithUUIDAndAddress(
-            &process, image.uuid, image.load_address,
-            false /* value_is_offset */, image.currently_executing,
-            false /* notify */);
-      } else if (image.slide != LLDB_INVALID_ADDRESS) {
-        module_sp = DynamicLoader::LoadBinaryWithUUIDAndAddress(
-            &process, image.uuid, image.slide, true /* value_is_offset */,
-            image.currently_executing, false /* notify */);
-      }
+      }
+    }
+
+    // We have an address, that's the best way to discover the binary.
+    if (!module_sp && image.load_address != LLDB_INVALID_ADDRESS) {
+          &process, image.filename, image.uuid, image.load_address,
+    }
+
+    // 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) {
+          &process, image.filename, image.uuid, image.slide,
+          true /* value_is_offset */, image.currently_executing,
+          false /* notify */);
+
+    // 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 */);
     }
 
-    if (module_sp.get()) {
-      // Will call ModulesDidLoad with all modules once they've all
-      // been added to the Target with load addresses.  Don't notify
-      // here, before the load address is set.
+    // 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());
         }
         for (auto name_vmaddr_tuple : image.segment_load_addresses) {
if (module_sp) {
       }
     }
   }
   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/Process/gdb-remote/ProcessGDBRemote.cpp

if (m_gdb_comm.GetStopReply(response)) {
       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;
           DynamicLoader::LoadBinaryWithUUIDAndAddress(
-              this, standalone_uuid, standalone_value,
+              this, llvm::StringRef(), standalone_uuid, standalone_value,
               standalone_value_is_offset, force_symbol_search, notify);
         }
       }
 
       // 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.
if (m_gdb_comm.GetStopReply(response)) {
                   .GetDebugger()
                   .GetPlatformList()
                   .LoadPlatformBinaryAndSetup(this, addr, notify))
             continue;
 
           const bool force_symbol_search = true;
           // Second manually load this binary into the Target.
           DynamicLoader::LoadBinaryWithUUIDAndAddress(
-              this, uuid, addr, value_is_slide, force_symbol_search, notify);
+              this, llvm::StringRef(), uuid, addr, value_is_slide,
+              force_symbol_search, notify);
         }
       }
 
       const StateType state = SetThreadStopInfo(response);
       if (state != eStateInvalid) {
         SetPrivateState(state);
       } else
         error.SetErrorStringWithFormat(

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

protected:
 
   lldb_private::ObjectFile *GetCoreObjectFile();
 
   lldb_private::Status
   DoGetMemoryRegionInfo(lldb::addr_t load_addr,
                         lldb_private::MemoryRegionInfo &region_info) override;
 
 private:
-  bool GetDynamicLoaderAddress(lldb::addr_t addr);
+  void CreateMemoryRegions();
+  void LoadBinariesViaMetadata();
+  void LoadBinariesViaExhaustiveSearch();
+  void LoadBinariesAndSetDYLD();
+  void CleanupMemoryRegionPermissions();
+
+  bool CheckAddressForDyldOrKernel(lldb::addr_t addr, lldb::addr_t &dyld,
+                                   lldb::addr_t &kernel);
 
   enum CorefilePreference { eUserProcessCorefile, eKernelCorefile };
 
   /// If a core file can be interpreted multiple ways, this establishes
   /// which style wins.
   ///
   /// If a core file contains both a kernel binary and a user-process
   /// dynamic loader, lldb needs to pick one over the other.  This could

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

 
 // Needed for the plug-in names for the dynamic loaders.
 #include "lldb/Host/SafeMachO.h"
 
 #include "Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.h"
 #include "Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderMacOSXDYLD.h"
 #include "Plugins/DynamicLoader/Static/DynamicLoaderStatic.h"
 #include "Plugins/ObjectFile/Mach-O/ObjectFileMachO.h"
+#include "Plugins/Platform/MacOSX/PlatformDarwinKernel.h"
 
 #include <memory>
 #include <mutex>
 
 using namespace lldb;
 using namespace lldb_private;
 
 LLDB_PLUGIN_DEFINE(ProcessMachCore)
ProcessMachCore::~ProcessMachCore() {
   Clear();
   // We need to call finalize on the process before destroying ourselves to
   // make sure all of the broadcaster cleanup goes as planned. If we destruct
   // this class, then Process::~Process() might have problems trying to fully
   // destroy the broadcaster.
   Finalize();
 }
 
-bool ProcessMachCore::GetDynamicLoaderAddress(lldb::addr_t addr) {
+bool ProcessMachCore::CheckAddressForDyldOrKernel(lldb::addr_t addr,
+                                                  addr_t &dyld,
+                                                  addr_t &kernel) {
   Log *log(GetLog(LLDBLog::DynamicLoader | LLDBLog::Process));
   llvm::MachO::mach_header header;
   Status error;
+  dyld = kernel = LLDB_INVALID_ADDRESS;
   if (DoReadMemory(addr, &header, sizeof(header), error) != sizeof(header))
     return false;
   if (header.magic == llvm::MachO::MH_CIGAM ||
       header.magic == llvm::MachO::MH_CIGAM_64) {
     header.magic = llvm::ByteSwap_32(header.magic);
     header.cputype = llvm::ByteSwap_32(header.cputype);
     header.cpusubtype = llvm::ByteSwap_32(header.cpusubtype);
     header.filetype = llvm::ByteSwap_32(header.filetype);
     header.ncmds = llvm::ByteSwap_32(header.ncmds);
     header.sizeofcmds = llvm::ByteSwap_32(header.sizeofcmds);
     header.flags = llvm::ByteSwap_32(header.flags);
   }
 
-  // TODO: swap header if needed...
-  // printf("0x%16.16" PRIx64 ": magic = 0x%8.8x, file_type= %u\n", vaddr,
-  // header.magic, header.filetype);
   if (header.magic == llvm::MachO::MH_MAGIC ||
       header.magic == llvm::MachO::MH_MAGIC_64) {
     // Check MH_EXECUTABLE to see if we can find the mach image that contains
     // the shared library list. The dynamic loader (dyld) is what contains the
     // list for user applications, and the mach kernel contains a global that
     // has the list of kexts to load
     switch (header.filetype) {
     case llvm::MachO::MH_DYLINKER:
-      // printf("0x%16.16" PRIx64 ": file_type = MH_DYLINKER\n", vaddr);
-      // Address of dyld "struct mach_header" in the core file
       LLDB_LOGF(log,
-                "ProcessMachCore::GetDynamicLoaderAddress found a user "
+                "ProcessMachCore::%s found a user "
                 "process dyld binary image at 0x%" PRIx64,
-                addr);
-      m_dyld_addr = addr;
+                __FUNCTION__, addr);
+      dyld = addr;
       return true;
 
     case llvm::MachO::MH_EXECUTE:
-      // printf("0x%16.16" PRIx64 ": file_type = MH_EXECUTE\n", vaddr);
       // Check MH_EXECUTABLE file types to see if the dynamic link object flag
       // is NOT set. If it isn't, then we have a mach_kernel.
       if ((header.flags & llvm::MachO::MH_DYLDLINK) == 0) {
         LLDB_LOGF(log,
-                  "ProcessMachCore::GetDynamicLoaderAddress found a mach "
+                  "ProcessMachCore::%s found a mach "
                   "kernel binary image at 0x%" PRIx64,
-                  addr);
+                  __FUNCTION__, addr);
         // Address of the mach kernel "struct mach_header" in the core file.
-        m_mach_kernel_addr = addr;
+        kernel = addr;
         return true;
       }
       break;
     }
   }
   return false;
 }
 
-// Process Control
-Status ProcessMachCore::DoLoadCore() {
-  Log *log(GetLog(LLDBLog::DynamicLoader | LLDBLog::Process));
-  Status error;
-  if (!m_core_module_sp) {
-    error.SetErrorString("invalid core module");
-    return error;
-  }
-
+void ProcessMachCore::CreateMemoryRegions() {
   ObjectFile *core_objfile = m_core_module_sp->GetObjectFile();
-  if (core_objfile == nullptr) {
-    error.SetErrorString("invalid core object file");
-    return error;
-  }
-
-  if (core_objfile->GetNumThreadContexts() == 0) {
-    error.SetErrorString("core file doesn't contain any LC_THREAD load "
-                         "commands, or the LC_THREAD architecture is not "
-                         "supported in this lldb");
-    return error;
-  }
-
   SectionList *section_list = core_objfile->GetSectionList();
-  if (section_list == nullptr) {
-    error.SetErrorString("core file has no sections");
-    return error;
-  }
-
   const uint32_t num_sections = section_list->GetNumSections(0);
-  if (num_sections == 0) {
-    error.SetErrorString("core file has no sections");
-    return error;
-  }
-
-  SetCanJIT(false);
-
-  llvm::MachO::mach_header header;
-  DataExtractor data(&header, sizeof(header),
-                     m_core_module_sp->GetArchitecture().GetByteOrder(),
-                     m_core_module_sp->GetArchitecture().GetAddressByteSize());
 
   bool ranges_are_sorted = true;
   addr_t vm_addr = 0;
   for (uint32_t i = 0; i < num_sections; ++i) {
     Section *section = section_list->GetSectionAtIndex(i).get();
     if (section && section->GetFileSize() > 0) {
       lldb::addr_t section_vm_addr = section->GetFileAddress();
       FileRange file_range(section->GetFileOffset(), section->GetFileSize());
if (section && section->GetFileSize() > 0) {
       m_core_range_infos.Append(VMRangeToPermissions::Entry(
           section_vm_addr, section->GetByteSize(), permissions));
     }
   }
   if (!ranges_are_sorted) {
     m_core_aranges.Sort();
     m_core_range_infos.Sort();
   }
+}
 
+void ProcessMachCore::LoadBinariesViaMetadata() {
+  Log *log(GetLog(LLDBLog::DynamicLoader | LLDBLog::Process));
+  ObjectFile *core_objfile = m_core_module_sp->GetObjectFile();
   bool found_main_binary_definitively = false;
 
   addr_t objfile_binary_value;
   bool objfile_binary_value_is_offset;
   UUID objfile_binary_uuid;
   ObjectFile::BinaryType type;
+
   if (core_objfile->GetCorefileMainBinaryInfo(objfile_binary_value,
                                               objfile_binary_value_is_offset,
                                               objfile_binary_uuid, type)) {
     if (log) {
-      log->Printf(
-          "ProcessMachCore::DoLoadCore: using binary hint from 'main bin spec' "
+      log->Printf("ProcessMachCore::LoadBinariesViaMetadata: using binary hint "
+                  "from 'main bin spec' "
                   "LC_NOTE with UUID %s value 0x%" PRIx64
                   " value is offset %d and type %d",
-          objfile_binary_uuid.GetAsString().c_str(), objfile_binary_value,
-          objfile_binary_value_is_offset, type);
+                  objfile_binary_uuid.GetAsString().c_str(),
+                  objfile_binary_value, objfile_binary_value_is_offset, type);
     }
+
+    // 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 {
       const bool force_symbol_search = true;
       const bool notify = true;
       if (DynamicLoader::LoadBinaryWithUUIDAndAddress(
-            this, objfile_binary_uuid, objfile_binary_value,
-            objfile_binary_value_is_offset, force_symbol_search, notify)) {
+              this, llvm::StringRef(), objfile_binary_uuid,
+              objfile_binary_value, objfile_binary_value_is_offset,
+              force_symbol_search, notify)) {
         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();
       }
-    if (type == ObjectFile::eBinaryTypeKernel) {
-      m_mach_kernel_addr = objfile_binary_value;
-      m_dyld_plugin_name = DynamicLoaderDarwinKernel::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("stext=") != std::string::npos) {
       }
     }
 
     // Search for a "Darwin Kernel" str indicating kernel; else treat as
     // standalone
     if (corefile_identifier.find("Darwin Kernel") != std::string::npos &&
         ident_uuid.IsValid() && ident_binary_addr != LLDB_INVALID_ADDRESS) {
       if (log)
-        log->Printf("ProcessMachCore::DoLoadCore: Found kernel binary via "
-                    "LC_IDENT/kern ver str LC_NOTE");
+        log->Printf(
+            "ProcessMachCore::LoadBinariesViaMetadata: Found kernel binary via "
       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;
       if (DynamicLoader::LoadBinaryWithUUIDAndAddress(
-              this, ident_uuid, ident_binary_addr, value_is_offset,
-              force_symbol_search, notify)) {
+              this, llvm::StringRef(), ident_uuid, ident_binary_addr,
+              value_is_offset, force_symbol_search, notify)) {
         found_main_binary_definitively = true;
         m_dyld_plugin_name = DynamicLoaderStatic::GetPluginNameStatic();
       }
     }
   }
 
-  bool did_load_extra_binaries = core_objfile->LoadCoreFileImages(*this);
-  // If we have a "all image infos" LC_NOTE, try to load all of the
-  // binaries listed, and set their Section load addresses in the Target.
-  if (found_main_binary_definitively == false && did_load_extra_binaries) {
-    m_dyld_plugin_name = DynamicLoaderDarwinKernel::GetPluginNameStatic();
-    found_main_binary_definitively = true;
-  }
+  // Finally, load any binaries noted by "load binary" LC_NOTEs in the
+  // corefile
+  core_objfile->LoadCoreFileImages(*this);
+
+  // LoadCoreFileImges may have set the dynamic loader; if we now have
+  // a dynamic loader, save its name so we don't un-set it later.
+  if (GetDynamicLoader())
+    m_dyld_plugin_name = GetDynamicLoader()->GetPluginName();
 
-  if (!found_main_binary_definitively &&
-      (m_dyld_addr == LLDB_INVALID_ADDRESS ||
-       m_mach_kernel_addr == LLDB_INVALID_ADDRESS)) {
-    // We need to locate the main executable in the memory ranges we have in
-    // the core file.  We need to search for both a user-process dyld binary
-    // and a kernel binary in memory; we must look at all the pages in the
-    // binary so we don't miss one or the other.  Step through all memory
-    // segments searching for a kernel binary and for a user process dyld --
-    // we'll decide which to prefer later if both are present.
+void ProcessMachCore::LoadBinariesViaExhaustiveSearch() {
+  Log *log(GetLog(LLDBLog::DynamicLoader | LLDBLog::Process));
+
+  // Search the pages of the corefile for dyld or mach kernel
+  // binaries.  There may be multiple things that look like a kernel
+  // in the corefile; disambiguating to the correct one can be difficult.
+
+  std::vector<addr_t> dylds_found;
+  std::vector<addr_t> kernels_found;
 
-    const size_t num_core_aranges = m_core_aranges.GetSize();
-    for (size_t i = 0; i < num_core_aranges; ++i) {
-      const VMRangeToFileOffset::Entry *entry =
-          m_core_aranges.GetEntryAtIndex(i);
-      lldb::addr_t section_vm_addr_start = entry->GetRangeBase();
-      lldb::addr_t section_vm_addr_end = entry->GetRangeEnd();
-      for (lldb::addr_t section_vm_addr = section_vm_addr_start;
-           section_vm_addr < section_vm_addr_end; section_vm_addr += 0x1000) {
-        GetDynamicLoaderAddress(section_vm_addr);
+    const VMRangeToFileOffset::Entry *entry = m_core_aranges.GetEntryAtIndex(i);
+      addr_t dyld, kernel;
+      if (CheckAddressForDyldOrKernel(section_vm_addr, dyld, kernel)) {
+        if (dyld != LLDB_INVALID_ADDRESS)
+          dylds_found.push_back(dyld);
+        if (kernel != LLDB_INVALID_ADDRESS)
+          kernels_found.push_back(dyld);
       }
     }
   }
 
-  if (!found_main_binary_definitively &&
-      m_mach_kernel_addr != LLDB_INVALID_ADDRESS) {
-    // In the case of multiple kernel images found in the core file via
-    // exhaustive search, we may not pick the correct one.  See if the
-    // DynamicLoaderDarwinKernel's search heuristics might identify the correct
-    // one. Most of the time, I expect the address from SearchForDarwinKernel()
-    // will be the same as the address we found via exhaustive search.
-
-    if (!GetTarget().GetArchitecture().IsValid() && m_core_module_sp.get()) {
-      GetTarget().SetArchitecture(m_core_module_sp->GetArchitecture());
-    }
+  // If we found more than one dyld mach-o header in the corefile,
+  // pick the first one.
+  if (dylds_found.size() > 0)
+    m_dyld_addr = dylds_found[0];
+  if (kernels_found.size() > 0)
+    m_mach_kernel_addr = kernels_found[0];
+
+  // Zero or one kernels found, we're done.
+  if (kernels_found.size() < 2)
+    return;
+
+  // one.
 
-    // SearchForDarwinKernel will end up calling back into this this class in
-    // the GetImageInfoAddress method which will give it the
-    // m_mach_kernel_addr/m_dyld_addr it already has.  Save that aside and set
-    // m_mach_kernel_addr/m_dyld_addr to an invalid address temporarily so
-    // DynamicLoaderDarwinKernel does a real search for the kernel using its
-    // own heuristics.
+  // SearchForDarwinKernel will call this class' GetImageInfoAddress method
+  // which will give it the addresses we already have.
+  // Save those aside and set
 
-    addr_t saved_mach_kernel_addr = m_mach_kernel_addr;
-    addr_t saved_user_dyld_addr = m_dyld_addr;
-    m_mach_kernel_addr = LLDB_INVALID_ADDRESS;
-    m_dyld_addr = LLDB_INVALID_ADDRESS;
 
-    addr_t better_kernel_address =
-        DynamicLoaderDarwinKernel::SearchForDarwinKernel(this);
 
-    m_mach_kernel_addr = saved_mach_kernel_addr;
-    m_dyld_addr = saved_user_dyld_addr;
 
-    if (better_kernel_address != LLDB_INVALID_ADDRESS) {
-      LLDB_LOGF(log, "ProcessMachCore::DoLoadCore: Using the kernel address "
-                     "from DynamicLoaderDarwinKernel");
-      m_mach_kernel_addr = better_kernel_address;
-    }
-  }
+    LLDB_LOGF(log,
+              "ProcessMachCore::%s: Using "
+              "the kernel address "
+              "from DynamicLoaderDarwinKernel",
+              __FUNCTION__);
 
+void ProcessMachCore::LoadBinariesAndSetDYLD() {
+  Log *log(GetLog(LLDBLog::DynamicLoader | LLDBLog::Process));
+
+  LoadBinariesViaMetadata();
+  if (m_dyld_plugin_name.empty())
+    LoadBinariesViaExhaustiveSearch();
+
   if (m_dyld_plugin_name.empty()) {
     // If we found both a user-process dyld and a kernel binary, we need to
     // decide which to prefer.
     if (GetCorefilePreference() == eKernelCorefile) {
       if (m_mach_kernel_addr != LLDB_INVALID_ADDRESS) {
         LLDB_LOGF(log,
-                  "ProcessMachCore::DoLoadCore: Using kernel corefile image "
+                  "ProcessMachCore::%s: Using kernel "
+                  "corefile image "
                   "at 0x%" PRIx64,
-                  m_mach_kernel_addr);
+                  __FUNCTION__, m_mach_kernel_addr);
         m_dyld_plugin_name = DynamicLoaderDarwinKernel::GetPluginNameStatic();
       } else if (m_dyld_addr != LLDB_INVALID_ADDRESS) {
         LLDB_LOGF(log,
-                  "ProcessMachCore::DoLoadCore: Using user process dyld "
+                  "ProcessMachCore::%s: Using user process dyld "
                   "image at 0x%" PRIx64,
-                  m_dyld_addr);
+                  __FUNCTION__, m_dyld_addr);
         m_dyld_plugin_name = DynamicLoaderMacOSXDYLD::GetPluginNameStatic();
       }
     } else {
       if (m_dyld_addr != LLDB_INVALID_ADDRESS) {
         LLDB_LOGF(log,
-                  "ProcessMachCore::DoLoadCore: Using user process dyld "
+                  "ProcessMachCore::%s: Using user process dyld "
                   "image at 0x%" PRIx64,
-                  m_dyld_addr);
+                  __FUNCTION__, m_dyld_addr);
         m_dyld_plugin_name = DynamicLoaderMacOSXDYLD::GetPluginNameStatic();
       } else if (m_mach_kernel_addr != LLDB_INVALID_ADDRESS) {
         LLDB_LOGF(log,
-                  "ProcessMachCore::DoLoadCore: Using kernel corefile image "
+                  "ProcessMachCore::%s: Using kernel "
+                  "corefile image "
                   "at 0x%" PRIx64,
-                  m_mach_kernel_addr);
+                  __FUNCTION__, m_mach_kernel_addr);
         m_dyld_plugin_name = DynamicLoaderDarwinKernel::GetPluginNameStatic();
       }
     }
   }
+}
 
+void ProcessMachCore::CleanupMemoryRegionPermissions() {
   if (m_dyld_plugin_name != DynamicLoaderMacOSXDYLD::GetPluginNameStatic()) {
     // For non-user process core files, the permissions on the core file
     // segments are usually meaningless, they may be just "read", because we're
     // dealing with kernel coredumps or early startup coredumps and the dumper
     // is grabbing pages of memory without knowing what they are.  If they
     // aren't marked as "executable", that can break the unwinder which will
     // check a pc value to see if it is in an executable segment and stop the
     // backtrace early if it is not ("executable" and "unknown" would both be
     // fine, but "not executable" will break the unwinder).
     size_t core_range_infos_size = m_core_range_infos.GetSize();
     for (size_t i = 0; i < core_range_infos_size; i++) {
       VMRangeToPermissions::Entry *ent =
           m_core_range_infos.GetMutableEntryAtIndex(i);
       ent->data = lldb::ePermissionsReadable | lldb::ePermissionsExecutable;
     }
   }
+}
 
-  // Even if the architecture is set in the target, we need to override it to
-  // match the core file which is always single arch.
-  ArchSpec arch(m_core_module_sp->GetArchitecture());
-  if (arch.GetCore() == ArchSpec::eCore_x86_32_i486) {
-    arch = Platform::GetAugmentedArchSpec(GetTarget().GetPlatform().get(), "i386");
+// Process Control
+Status ProcessMachCore::DoLoadCore() {
+  Status error;
+  if (!m_core_module_sp) {
+    error.SetErrorString("invalid core module");
+    return error;
+  }
+
+  ObjectFile *core_objfile = m_core_module_sp->GetObjectFile();
+  if (core_objfile == nullptr) {
+    error.SetErrorString("invalid core object file");
+    return error;
+  }
+
+  if (core_objfile->GetNumThreadContexts() == 0) {
+    error.SetErrorString("core file doesn't contain any LC_THREAD load "
+                         "commands, or the LC_THREAD architecture is not "
+                         "supported in this lldb");
+    return error;
   }
+
+  SetCanJIT(false);
+
+  // The corefile's architecture is our best starting point.
+  ArchSpec arch(m_core_module_sp->GetArchitecture());
   if (arch.IsValid())
     GetTarget().SetArchitecture(arch);
 
+  CreateMemoryRegions();
+
+  LoadBinariesAndSetDYLD();
+
+  CleanupMemoryRegionPermissions();
+
   addr_t address_mask = core_objfile->GetAddressMask();
   if (address_mask != 0) {
     SetCodeAddressMask(address_mask);
     SetDataAddressMask(address_mask);
   }
   return error;
 }