[lldb] [mach-o corefiles] If we have LC_NOTE metadata and can't find a binary, don't fall back to an exhaustive scan

[jasonmolenda]

Summary

We have some corefiles which are intended to debug a standalone binary, and have an LC_NOTE mach-o load command with the UUID and/or address of that binary. If the search for that binary fails for any reason, we don't load the intended binary. ProcessMachCore then falls back to doing an exhaustive scan of the corefile memory pages looking for a darwin kernel or a userland dyld binary. And the case where this comes up most often, there is a darwin kernel. lldb starts doing a darwin kernel debug session and that's not at all what the user is intending to do.

This patch changes ProcessMachCore::LoadBinariesViaMetadata to return if any metadata record was found, even if we didn't find the binary, and ProcessMachCore::LoadBinariesAndSetDYLD will only start an exhaustive search if we had no metadata.

[bulbazord]

Patch looks good to me, one small suggestion about naming.

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

89–95

Since LoadBinariesViaMetadata can fail and we now care about the return value, what do you think about the name TryLoadBinariesViaMetadata? I think a name that indicates that it may fail to actually load would make it easier to read at a glance.

[JDevlieghere]

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

89–95

The same is true for the other methods though, so I don't know if that is really all that meaningful. IIUC, before this patch, all these methods conveyed whether they were successful by setting m_dyld_plugin_name. Now, LoadBinariesViaMetadata does something extra, which is conveyed by the boolean. I dislike the inconsistency, but I can't really think of a better way of doing this, I considered suggesting an out-parameter but that doesn't really improve anything.

[bulbazord]

LGTM

[jasonmolenda]

Rebase on current TOT sources, and add a comment to the ProcessMachCore::LoadBinariesViaMetadata prototype in the header documenting the meaning of the return value, I agree with Alex I should make this a little clearer.

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:
   void CreateMemoryRegions();
-  void LoadBinariesViaMetadata();
+
+  /// \return
+  ///   True if any metadata were found indicating the binary that should
+  ///   be loaded, regardless of whether the specified binary could be found.
+  ///   False if no metadata were present.
+  bool 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 };

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

for (uint32_t i = 0; i < num_sections; ++i) {
     }
   }
   if (!ranges_are_sorted) {
     m_core_aranges.Sort();
     m_core_range_infos.Sort();
   }
 }
 
-void ProcessMachCore::LoadBinariesViaMetadata() {
+bool 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;
 
+  // This will be set to true if we had a metadata hint
+  // specifying a UUID or address -- and we should not fall back
+  // to doing an exhaustive search.
+  bool found_binary_spec_in_metadata = false;
+
   if (core_objfile->GetCorefileMainBinaryInfo(objfile_binary_value,
                                               objfile_binary_value_is_offset,
                                               objfile_binary_uuid, type)) {
     if (log) {
       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);
     }
+    found_binary_spec_in_metadata = true;
 
     // 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;
       const bool allow_memory_image_last_resort = false;
       if (DynamicLoader::LoadBinaryWithUUIDAndAddress(
               this, llvm::StringRef(), objfile_binary_uuid,
               objfile_binary_value, objfile_binary_value_is_offset,
               force_symbol_search, notify, set_address_in_target,
               allow_memory_image_last_resort)) {
-        found_main_binary_definitively = true;
         m_dyld_plugin_name = DynamicLoaderStatic::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 (!found_main_binary_definitively) {
     std::string corefile_identifier = core_objfile->GetIdentifierString();
 
     // Search for UUID= and stext= strings in the identifier str.
     if (corefile_identifier.find("UUID=") != std::string::npos) {
       size_t p = corefile_identifier.find("UUID=") + strlen("UUID=");
       std::string uuid_str = corefile_identifier.substr(p, 36);
       ident_uuid.SetFromStringRef(uuid_str);
       if (log)
         log->Printf("Got a UUID from LC_IDENT/kern ver str LC_NOTE: %s",
                     ident_uuid.GetAsString().c_str());
+      found_binary_spec_in_metadata = true;
     }
     if (corefile_identifier.find("stext=") != std::string::npos) {
       size_t p = corefile_identifier.find("stext=") + strlen("stext=");
       if (corefile_identifier[p] == '0' && corefile_identifier[p + 1] == 'x') {
         ident_binary_addr =
             ::strtoul(corefile_identifier.c_str() + p, nullptr, 16);
         if (log)
           log->Printf("Got a load address from LC_IDENT/kern ver str "
                       "LC_NOTE: 0x%" PRIx64,
                       ident_binary_addr);
+        found_binary_spec_in_metadata = true;
       }
     }
 
     // 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::LoadBinariesViaMetadata: Found kernel binary via "
             "LC_IDENT/kern ver str LC_NOTE");
       m_mach_kernel_addr = ident_binary_addr;
-      found_main_binary_definitively = true;
+      found_binary_spec_in_metadata = 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;
       const bool allow_memory_image_last_resort = false;
       if (DynamicLoader::LoadBinaryWithUUIDAndAddress(
               this, llvm::StringRef(), ident_uuid, ident_binary_addr,
               value_is_offset, force_symbol_search, notify,
               set_address_in_target, allow_memory_image_last_resort)) {
-        found_main_binary_definitively = true;
+        found_binary_spec_in_metadata = true;
         m_dyld_plugin_name = DynamicLoaderStatic::GetPluginNameStatic();
       }
     }
-  }
 
   // Finally, load any binaries noted by "load binary" LC_NOTEs in the
   // corefile
   if (core_objfile->LoadCoreFileImages(*this)) {
-    found_main_binary_definitively = true;
+    found_binary_spec_in_metadata = 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();
+
+  return found_binary_spec_in_metadata;
 }
 
 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.
LLDB_LOGF(log,
               __FUNCTION__);
     m_mach_kernel_addr = better_kernel_address;
   }
 }
 
 void ProcessMachCore::LoadBinariesAndSetDYLD() {
   Log *log(GetLog(LLDBLog::DynamicLoader | LLDBLog::Process));
 
-  LoadBinariesViaMetadata();
-  if (m_dyld_plugin_name.empty())
+  bool found_binary_spec_in_metadata = LoadBinariesViaMetadata();
+  if (!found_binary_spec_in_metadata)
     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,