[lldb] Add flag to DynamicLoader::LoadBinaryWithUUIDAndAddress to control whether we fall back to reading the binary out of memory

[jasonmolenda]

Summary

DynamicLoader::LoadBinaryWithUUIDAndAddress() tries a few different ways of finding a binary, and if it can't find an actual binary (and symbol file) on the local computer, it will try to read a binary out of memory, with the thought that there may be a symbol table in memory so we'll get some symbol names. It will create a placeholder name of memory-image-0x.... It seemed like a good idea when I wrote this method, but in practice for binaries outside of userland binaries in a corefile, this hasn't been helpful.

This patch adds a flag to LoadBinaryWithUUIDAndAddress to control if we should create a memory image module, or not. The only case where we still enable this behavior is in ObjectFileMachO::LoadCoreFileImages() when a binary has segment load addresses. This is the case with the "all image infos" LC_NOTE that lldb creates for userland corefiles with binaries that typically have some symbols in their in-memory symbol tables.

The other type of userland corefiles (gcore-created) will not use this LC_NOTE but an entirely different codepath -- through the Darwin userland DynamicLoader plugin -- is taken in those cases, so we'll still read them out of memory.

This patch makes firmware/standalone binary loading not create these memory-image-0x... placeholder binaries with no symbols in them, the scenario where it is more annoying than useful.

The test case TestMultipleBinaryCorefile.py tests this standalone-binary corefile scenario, so it needed adjusting to the new setup where the binary image is not added to the target. I also cleaned up the fake dsym-for-uuid.sh it creates to return a properly formatted DBGError message for https://reviews.llvm.org/D157160

[JDevlieghere]

lldb/include/lldb/Target/DynamicLoader.h

267

Nit: seems like either allow_memory_image_last_resort or use_memory_image_last_resort would convey the same thing.

lldb/test/API/macosx/lc-note/multiple-binary-corefile/TestMultipleBinaryCorefile.py

38–39

Out of curiosity what's the point of doing self.runCmd("script print ... vs printing the same thing directly? Does this run in another context or something? Do we save the command output somewhere?

122–123

I know this was moved and this goes beyond the scope of this patch, but we should be using the just-built llvm-dwarfdump instead of whatever happens to be on the system. Obviously no big deal for extracting the --uuid but it's relatively easy to hook up and what we do for other llvm tools such as dsymutil. objdump, etc.

[bulbazord]

I don't have any objections other than the questions Jonas has asked. LGTM otherwise

lldb/test/API/macosx/lc-note/multiple-binary-corefile/TestMultipleBinaryCorefile.py

38–39

I'm also curious. Printing directly is probably less expensive than running a print command through the lldb python script interpreter.

[jasonmolenda]

lldb/include/lldb/Target/DynamicLoader.h

267

Yes, good suggestion, my name wasn't very good here.

lldb/test/API/macosx/lc-note/multiple-binary-corefile/TestMultipleBinaryCorefile.py

38–39

In the past, when I had an SB API test print() directly, the trace output is buffered differently than STDOUT, so I've done this hack in a few places to get the print to happen exactly when the test is executing. Doing a if self.TraceOn(): self.runCmd("bt"); print("got here") will not print the two next to each other, iirc. I can switch to a print and confirm my memory of this.

122–123

I wasn't sure how do find & run that in the build dir; it's used in a few Shell tests but none in API.

[jasonmolenda]

Incorporate Jonas' feedback about the naming of the new DynamicLoader method argument.

[jasonmolenda]

lldb/test/API/macosx/lc-note/multiple-binary-corefile/TestMultipleBinaryCorefile.py

38–39

I switched this test to just print directly and works fine with lldb-dotest -t, I must have misremembered. I'll remove a few places where I do this in this test.

[jasonmolenda]

Fix my script print's to just print, I can't repo a buffering problem that I thought I had seen in the past.

[JDevlieghere]

LGTM

lldb/include/lldb/Target/DynamicLoader.h

public:
   /// \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.
   ///
+  /// \param[in] allow_memory_image_last_resort
+  ///     If no better binary image can be found, allow reading the binary
+  ///     out of memory, if possible, and create the Module based on that.
+  ///     May be slow to read a binary out of memory, and for unusual
+  ///     environments, may be no symbols mapped in memory at all.
+  ///
   /// \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, bool set_address_in_target);
+  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, bool allow_memory_image_last_resort);
 
   /// 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()) {
     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 set_address_in_target) {
+    bool set_address_in_target, bool allow_memory_image_last_resort) {
   ModuleSP memory_module_sp;
   ModuleSP module_sp;
   PlatformSP platform_sp = process->GetTarget().GetPlatform();
   Target &target = process->GetTarget();
   Status error;
 
   if (!uuid.IsValid() && !value_is_offset) {
     memory_module_sp = ReadUnnamedMemoryModule(process, value, name);
if (uuid.IsValid()) {
 
     // 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 (allow_memory_image_last_resort && !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()) {

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

if (value_is_offset && value == LLDB_INVALID_ADDRESS) {
       value = 0;
     }
 
     // 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;
+      const bool notify = false;
+      // Userland Darwin binaries will have segment load addresses via
+      // the `all image infos` LC_NOTE.
+      const bool allow_memory_image_last_resort =
+          image.segment_load_addresses.size();
       module_sp = DynamicLoader::LoadBinaryWithUUIDAndAddress(
           &process, image.filename, image.uuid, value, value_is_offset,
-          image.currently_executing, false /* notify */, set_load_address);
+          image.currently_executing, notify, set_load_address,
+          allow_memory_image_last_resort);
     }
 
     // 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

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

void ProcessGDBRemote::LoadStubBinaries() {
   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;
+      const bool allow_memory_image_last_resort = false;
       DynamicLoader::LoadBinaryWithUUIDAndAddress(
           this, "", standalone_uuid, standalone_value,
           standalone_value_is_offset, force_symbol_search, notify,
-          set_address_in_target);
+          set_address_in_target, allow_memory_image_last_resort);
     }
   }
 
   // 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) {
       if (GetTarget()
               .GetDebugger()
               .GetPlatformList()
               .LoadPlatformBinaryAndSetup(this, addr, notify))
         continue;
 
       const bool force_symbol_search = true;
       const bool set_address_in_target = true;
+      const bool allow_memory_image_last_resort = false;
       // Second manually load this binary into the Target.
       DynamicLoader::LoadBinaryWithUUIDAndAddress(
           this, llvm::StringRef(), uuid, addr, value_is_slide,
-          force_symbol_search, notify, set_address_in_target);
+          force_symbol_search, notify, set_address_in_target,
+          allow_memory_image_last_resort);
     }
   }
 }
 
 void ProcessGDBRemote::MaybeLoadExecutableModule() {
   ModuleSP module_sp = GetTarget().GetExecutableModule();
   if (!module_sp)
     return;

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

if (type == ObjectFile::eBinaryTypeKernel) {
       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)) {
+              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
if (corefile_identifier.find("Darwin Kernel") != std::string::npos &&
       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;
+      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)) {
+              set_address_in_target, allow_memory_image_last_resort)) {
         found_main_binary_definitively = true;
         m_dyld_plugin_name = DynamicLoaderStatic::GetPluginNameStatic();
       }
     }
   }
 
   // Finally, load any binaries noted by "load binary" LC_NOTEs in the
   // corefile

lldb/test/API/macosx/lc-note/multiple-binary-corefile/TestMultipleBinaryCorefile.py

-"""Test corefiles with "main bin spec"/"load binary" with only addrs work."""
+"""Test corefiles with "main bin spec"/"load binary" with only vmaddrs works."""
 
 
 import os
 import re
 import subprocess
 
 import lldb
 from lldbsuite.test.decorators import *
def initial_setup(self):
             self.corefile,
             self.aout_exe,
             self.aout_slide,
             self.libone_exe,
             self.libone_slide,
             self.libtwo_exe,
             self.libtwo_slide,
         )
+        if self.TraceOn():
+            print("Creating corefile with command %s")
         call(cmd, shell=True)
 
     def load_corefile_and_test(self):
         target = self.dbg.CreateTarget("")
         err = lldb.SBError()
         if self.TraceOn():
-            self.runCmd("script print('loading corefile %s')" % self.corefile)
+            print("loading corefile %s" % self.corefile)
         process = target.LoadCore(self.corefile)
         self.assertEqual(process.IsValid(), True)
         if self.TraceOn():
-            self.runCmd("script print('image list after loading corefile:')")
+            print("image list after loading corefile:")
             self.runCmd("image list")
 
-        self.assertEqual(target.GetNumModules(), 3)
+        ## We don't have libone.dylib in the global module cache or from
+        ## dsymForUUID, and lldb will not read the binary out of memory.
+        self.assertEqual(target.GetNumModules(), 2)
         fspec = target.GetModuleAtIndex(0).GetFileSpec()
         self.assertEqual(fspec.GetFilename(), self.aout_exe_basename)
 
-        # libone.dylib was never loaded into lldb, see that we added a memory module.
         fspec = target.GetModuleAtIndex(1).GetFileSpec()
-        self.assertIn("memory-image", fspec.GetFilename())
-
-        dwarfdump_uuid_regex = re.compile("UUID: ([-0-9a-fA-F]+) \(([^\(]+)\) .*")
-        dwarfdump_cmd_output = subprocess.check_output(
-            ('/usr/bin/dwarfdump --uuid "%s"' % self.libone_exe), shell=True
-        ).decode("utf-8")
-        libone_uuid = None
-        for line in dwarfdump_cmd_output.splitlines():
-            match = dwarfdump_uuid_regex.search(line)
-            if match:
-                libone_uuid = match.group(1)
-
-        memory_image_uuid = target.GetModuleAtIndex(1).GetUUIDString()
-        self.assertEqual(libone_uuid, memory_image_uuid)
-
-        fspec = target.GetModuleAtIndex(2).GetFileSpec()
         self.assertEqual(fspec.GetFilename(), self.libtwo_exe_basename)
 
         # Executables "always" have this base address
         aout_load = (
             target.GetModuleAtIndex(0)
             .GetObjectFileHeaderAddress()
             .GetLoadAddress(target)
         )
         self.assertEqual(aout_load, 0x100000000 + self.aout_slide)
 
         # Value from Makefile
-        libone_load = (
-            target.GetModuleAtIndex(1)
-            .GetObjectFileHeaderAddress()
-            .GetLoadAddress(target)
-        )
-        self.assertEqual(libone_load, self.libone_slide)
-
-        # Value from Makefile
         libtwo_load = (
-            target.GetModuleAtIndex(2)
+            target.GetModuleAtIndex(1)
             .GetObjectFileHeaderAddress()
             .GetLoadAddress(target)
         )
         self.assertEqual(libtwo_load, self.libtwo_slide)
 
         self.dbg.DeleteTarget(target)
         self.dbg.Clear()
 
def test_corefile_binaries_dsymforuuid(self):
         libtwo_uuid = None
         for line in dwarfdump_cmd_output.splitlines():
             match = dwarfdump_uuid_regex.search(line)
             if match:
                 libtwo_uuid = match.group(1)
         self.assertNotEqual(
             libtwo_uuid, None, "Could not get uuid of built libtwo.dylib"
         )
+        dwarfdump_cmd_output = subprocess.check_output(
+            ('/usr/bin/dwarfdump --uuid "%s"' % self.aout_exe), shell=True
+        ).decode("utf-8")
+        aout_uuid = None
+        for line in dwarfdump_cmd_output.splitlines():
+            match = dwarfdump_uuid_regex.search(line)
+            if match:
+                aout_uuid = match.group(1)
+        self.assertNotEqual(aout_uuid, None, "Could not get uuid of built a.out")
 
         ###  Create our dsym-for-uuid shell script which returns aout_exe
         shell_cmds = [
             "#! /bin/sh",
             "# the last argument is the uuid",
             "while [ $# -gt 1 ]",
             "do",
             "  shift",
             "done",
-            "ret=0",
             'echo "<?xml version=\\"1.0\\" encoding=\\"UTF-8\\"?>"',
             'echo "<!DOCTYPE plist PUBLIC \\"-//Apple//DTD PLIST 1.0//EN\\" \\"http://www.apple.com/DTDs/PropertyList-1.0.dtd\\">"',
             'echo "<plist version=\\"1.0\\">"',
+            'echo "  <dict>"',
+            'echo "    <key>$1</key>"',
+            'echo "    <dict>"',
             "",
-            'if [ "$1" != "%s" ]' % (libtwo_uuid),
+            'if [ "$1" != "%s" -a "$1" != "%s" ]' % (libtwo_uuid, aout_uuid),
             "then",
-            '  echo "<key>DBGError</key><string>not found</string>"',
+            '  echo "      <key>DBGError</key>"',
+            '  echo "      <string>not found by $0</string>"',
+            '  echo "    </dict>"',
+            '  echo "  </dict>"',
             '  echo "</plist>"',
-            "  exit 1",
+            "  exit 0",
             "fi",
+            #  UUID matches libtwo.dylib
+            'if [ "$1" = "%s" ]' % (libtwo_uuid),
+            "then",
             "  uuid=%s" % libtwo_uuid,
             "  bin=%s" % self.libtwo_exe,
             "  dsym=%s.dSYM/Contents/Resources/DWARF/%s"
             % (self.libtwo_exe, os.path.basename(self.libtwo_exe)),
-            'echo "<dict><key>$uuid</key><dict>"',
+            "fi",
+            #  UUID matches a.out
+            'if [ "$1" = "%s" ]' % (aout_uuid),
+            "then",
+            "  uuid=%s" % aout_uuid,
+            "  bin=%s" % self.aout_exe,
+            "  dsym=%s.dSYM/Contents/Resources/DWARF/%s"
+            % (self.aout_exe, os.path.basename(self.aout_exe)),
+            "fi",
             "",
-            'echo "<key>DBGDSYMPath</key><string>$dsym</string>"',
-            'echo "<key>DBGSymbolRichExecutable</key><string>$bin</string>"',
-            'echo "</dict></dict></plist>"',
-            "exit $ret",
+            'echo "      <key>DBGDSYMPath</key>"',
+            'echo "      <string>$dsym</string>"',
+            'echo "      <key>DBGSymbolRichExecutable</key>"',
+            'echo "      <string>$bin</string>"',
+            'echo "    </dict>"',
+            'echo "  </dict>"',
+            'echo "</plist>"',
+            "exit 0",
         ]
 
         with open(dsym_for_uuid, "w") as writer:
             for l in shell_cmds:
                 writer.write(l + "\n")
 
         os.chmod(dsym_for_uuid, 0o755)
 
         # Register TWO of our binaries, but require dsymForUUID to find the third.
         target = self.dbg.CreateTarget(self.aout_exe, "", "", False, lldb.SBError())
         self.dbg.DeleteTarget(target)
 
         if self.TraceOn():
-            self.runCmd("script print('Global image list, before loading corefile:')")
+            print("Global image list, before loading corefile:")
             self.runCmd("image list -g")
 
         self.load_corefile_and_test()
 
     @skipIf(archs=no_match(["x86_64", "arm64", "arm64e", "aarch64"]))
     @skipIfRemote
     @skipUnlessDarwin
     def test_corefile_binaries_preloaded(self):
         self.initial_setup()
 
         if self.TraceOn():
             self.runCmd("log enable lldb dyld host")
             self.addTearDownHook(lambda: self.runCmd("log disable lldb dyld host"))
 
         # Register all three binaries in lldb's global module
         # cache, then throw the Targets away.
         target = self.dbg.CreateTarget(self.aout_exe, "", "", False, lldb.SBError())
         self.dbg.DeleteTarget(target)
         target = self.dbg.CreateTarget(self.libtwo_exe, "", "", False, lldb.SBError())
         self.dbg.DeleteTarget(target)
 
         if self.TraceOn():
-            self.runCmd("script print('Global image list, before loading corefile:')")
+            print("Global image list, before loading corefile:")
             self.runCmd("image list -g")
 
         self.load_corefile_and_test()

lldb/test/API/macosx/lc-note/multiple-binary-corefile/create-multibin-corefile.cpp

 #include <sys/stat.h>
 #include <sys/types.h>
 #include <sys/uio.h>
 #include <unistd.h>
 #include <uuid/uuid.h>
 #include <vector>
 
 // Given a list of binaries, and optional slides to be applied,
-// create a corefile whose memory is those binaries laid at at
+// create a corefile whose memory is those binaries laid down at
 // their slid addresses.
 //
 // Add a 'main bin spec' LC_NOTE for the first binary, and
 // 'load binary' LC_NOTEs for any additional binaries, and
 // these LC_NOTEs will ONLY have the vmaddr of the binary - no
 // UUID, no slide, no filename.
 //
 // Test that lldb can use the load addresses, find the UUIDs,