Add optional to debugserver's "tell me about all binaries in the process" packet, to limit it to just load addresses and names

[jasonmolenda]

Summary

debugserver has a memory use problem when a process has many (1000+) binaries loaded in it. lldb sends the jGetLoadedDynamicLibrariesInfos packet to get a JSON description of those binaries -- load address, filepath, mach-o header, mach-o load commands. This can result in such a large packet that it will temporarily cause debugserver's memory use to balloon up for the duration of the packet processes. I made a first crack at this problem a year ago ( https://reviews.llvm.org/D122848 and https://reviews.llvm.org/D122882 ) to reduce unnecessary copies while creating this response, but inevitably there will be more binaries and we'll be back here again.

This patch adds a new option to the jGetLoadedDynamicLibrariesInfos fetch-all-binaries packet to only report the mach-o load addresses and the filepaths -- to skip the mach-o header & load command scan and JSONification. The default behavior is unchanged.

In a future patch, I will change DynamicLoaderMacOS to request binaries in batches of maybe 300 per request. When it attaches to a new process, it will ask for the load addresses of the binaries, and get the full information in these batches. On notification of new binaries being loaded, it will already have the load addresses and can skip this first request.

As long as I was updating this code, I also removed support for our pre-iOS 10/macOS 10.12 way of querying binaries loaded in a process. We can't build debugserver with a deployment target that old any more; it is not testable.

This patch does have a regression on API/macosx/unregistered-macho/TestUnregisteredMacho.py that I wrote last summer; I have the patch generate JSON for a binary that it had an address for, but could not read the mach-o headers. This test case is specifically testing the address of something that is not a mach-o in memory and testing that nothin is returned. I need to revisit why I wrote this before I fix it by changing my patch or removing the test, but the reason for the test failure is obvious, it's a minor revision either way.

[bulbazord]

This looks good to me.

This patch does have a regression on API/macosx/unregistered-macho/TestUnregisteredMacho.py that I wrote last summer; I have the patch generate JSON for a binary that it had an address for, but could not read the mach-o headers. This test case is specifically testing the address of something that is not a mach-o in memory and testing that nothin is returned. I need to revisit why I wrote this before I fix it by changing my patch or removing the test, but the reason for the test failure is obvious, it's a minor revision either way.

I assume you plan on addressing this failure before landing this change or otherwise disabling the test?

lldb/tools/debugserver/source/MacOSX/MachProcess.mm

930–931

Why do we still want to add information about this dylib if the mach header isn't valid?

lldb/tools/debugserver/source/RNBRemote.cpp

5932–5933

Should probably update this comment with "report_load_commands":false as well.

[jasonmolenda]

Thanks for the feedback. Testing now & will update the patch in a bit.

lldb/tools/debugserver/source/MacOSX/MachProcess.mm

930–931

Yeah, I was being too clever here, it's not clear what I'm doing (and it's the cause of the test failure I was looking at). I tried to overload "could not read the header/load commands" for both "it failed" and "lldb told us to not read them". I'll pass down the bool for whether we're including the header/loadcmds to make it a lot clearer.

lldb/tools/debugserver/source/RNBRemote.cpp

5932–5933

Good catch.

[jasonmolenda]

Update to address Alex's comments, fix the testsuite failure.

[jasonmolenda]

Update the patch to stop checking for the mod_date key in the JSON reply for each binary. This value is always zero with dyld for the past five+ years. It increases the size of the packet unnecessarily, but lldb is currently checking that the field is included for this to be a valid response. I was getting annoyed at this required field and how I couldn't remove this unnecessary field, and then I decided to start the process by removing lldb's requiring of it.

Change debugserver to hardcode returning mod_date 0 and a comment that this should be removed in the near future.

[bulbazord]

I have no problem with removing mod_date since you're only removing the requirement in the Darwin-specific code. Also, be sure to remove the portion of the commit message that says this introduces a regression since you've fixed the test now. It would be confusing for future code archeologists.

LGTM!

lldb/tools/debugserver/source/MacOSX/MachProcess.mm

1141

nit: Could you add a comment specifying what the bool argument is for?

return FormatDynamicLibrariesIntoJSON(image_infos, /* report_load_commands = */ true);

[jasonmolenda]

Updating to reflect what I committed.

lldb/docs/lldb-gdb-remote.txt

 
 //----------------------------------------------------------------------
 // "jGetLoadedDynamicLibrariesInfos"
 //
 // BRIEF
 //  This packet asks the remote debug stub to send the details about libraries
 //  being added/removed from the process as a performance optimization.
 //
-//  There are three ways this packet can be used.  All three return a dictionary of
+//  There are two ways this packet can be used.  Both return a dictionary of
 //  binary images formatted the same way.
 //
-//  On OS X 10.11, iOS 9, tvOS 9, watchOS 2 and earlier, the packet is used like
-//       jGetLoadedDynamicLibrariesInfos:{"image_count":1,"image_list_address":140734800075128}
-//  where the image_list_address is an array of {void* load_addr, void* mod_date, void* pathname}
-//  in the inferior process memory (and image_count is the number of elements in this array).
-//  lldb is using information from the dyld_all_image_infos structure to make these requests to
-//  debugserver.  This use is not supported on macOS 10.12, iOS 10, tvOS 10, watchOS 3 or newer.
-//
-//  On macOS 10.12, iOS 10, tvOS 10, watchOS 3 and newer, there are two calls.  One requests information
-//  on all shared libraries:
+//  One requests information on all shared libraries:
 //       jGetLoadedDynamicLibrariesInfos:{"fetch_all_solibs":true}
+//  with an optional `"report_load_commands":false` which can be added, asking
+//  that only the dyld SPI information (load addresses, filenames) be returned.
+//  The default behavior is that debugserver scans the mach-o header and load 
+//  commands of each binary, and returns it in the JSON reply.
+//
 //  And the second requests information about a list of shared libraries, given their load addresses:
 //       jGetLoadedDynamicLibrariesInfos:{"solib_addresses":[8382824135,3258302053,830202858503]}
 //
 //  The second call is both a performance optimization (instead of having lldb read the mach-o header/load commands
 //  out of memory with generic read packets) but also adds additional information in the form of the
 //  filename of the shared libraries (which is not available in the mach-o header/load commands.)
 //
 //  An example using the OS X 10.11 style call:

lldb/tools/debugserver/source/DNB.h

ThreadInfo::QoS DNBGetRequestedQoSForThread(nub_process_t pid, nub_thread_t tid,
                                             uint64_t dti_qos_class_index);
 nub_addr_t DNBGetPThreadT(nub_process_t pid, nub_thread_t tid);
 nub_addr_t DNBGetDispatchQueueT(nub_process_t pid, nub_thread_t tid);
 nub_addr_t
 DNBGetTSDAddressForThread(nub_process_t pid, nub_thread_t tid,
                           uint64_t plo_pthread_tsd_base_address_offset,
                           uint64_t plo_pthread_tsd_base_offset,
                           uint64_t plo_pthread_tsd_entry_size);
-JSONGenerator::ObjectSP DNBGetLoadedDynamicLibrariesInfos(
-    nub_process_t pid, nub_addr_t image_list_address, nub_addr_t image_count);
-JSONGenerator::ObjectSP DNBGetAllLoadedLibrariesInfos(nub_process_t pid);
+JSONGenerator::ObjectSP
+DNBGetAllLoadedLibrariesInfos(nub_process_t pid, bool report_load_commands);
 JSONGenerator::ObjectSP
 DNBGetLibrariesInfoForAddresses(nub_process_t pid,
                                 std::vector<uint64_t> &macho_addresses);
 JSONGenerator::ObjectSP DNBGetSharedCacheInfo(nub_process_t pid);
 
 //
 // Breakpoint functions
 nub_bool_t DNBBreakpointSet(nub_process_t pid, nub_addr_t addr, nub_size_t size,

lldb/tools/debugserver/source/DNB.cpp

   if (GetProcessSP(pid, procSP)) {
     return procSP->GetTSDAddressForThread(
         tid, plo_pthread_tsd_base_address_offset, plo_pthread_tsd_base_offset,
         plo_pthread_tsd_entry_size);
   }
   return INVALID_NUB_ADDRESS;
 }
 
-JSONGenerator::ObjectSP DNBGetLoadedDynamicLibrariesInfos(
-    nub_process_t pid, nub_addr_t image_list_address, nub_addr_t image_count) {
-  MachProcessSP procSP;
-  if (GetProcessSP(pid, procSP)) {
-    return procSP->GetLoadedDynamicLibrariesInfos(pid, image_list_address,
-                                                  image_count);
-  }
-  return JSONGenerator::ObjectSP();
-}
-
-JSONGenerator::ObjectSP DNBGetAllLoadedLibrariesInfos(nub_process_t pid) {
+JSONGenerator::ObjectSP
+DNBGetAllLoadedLibrariesInfos(nub_process_t pid, bool report_load_commands) {
   MachProcessSP procSP;
   if (GetProcessSP(pid, procSP)) {
-    return procSP->GetAllLoadedLibrariesInfos(pid);
+    return procSP->GetAllLoadedLibrariesInfos(pid, report_load_commands);
   }
   return JSONGenerator::ObjectSP();
 }
 
 JSONGenerator::ObjectSP
 DNBGetLibrariesInfoForAddresses(nub_process_t pid,
                                 std::vector<uint64_t> &macho_addresses) {
   MachProcessSP procSP;

lldb/tools/debugserver/source/MacOSX/MachProcess.h

#endif // WITH_SPRINGBOARD
   /// Use the dyld SPI present in macOS 10.12, iOS 10, tvOS 10,
   /// watchOS 3 and newer to get the load address, uuid, and filenames
   /// of all the libraries.  This only fills in those three fields in
   /// the 'struct binary_image_information' - call
   /// GetMachOInformationFromMemory to fill in the mach-o header/load
   /// command details.
   void GetAllLoadedBinariesViaDYLDSPI(
       std::vector<struct binary_image_information> &image_infos);
-  JSONGenerator::ObjectSP GetLoadedDynamicLibrariesInfos(
-      nub_process_t pid, nub_addr_t image_list_address, nub_addr_t image_count);
   JSONGenerator::ObjectSP
   GetLibrariesInfoForAddresses(nub_process_t pid,
                                std::vector<uint64_t> &macho_addresses);
-  JSONGenerator::ObjectSP GetAllLoadedLibrariesInfos(nub_process_t pid);
+  JSONGenerator::ObjectSP
+  GetAllLoadedLibrariesInfos(nub_process_t pid,
+                             bool fetch_report_load_commands);
   JSONGenerator::ObjectSP GetSharedCacheInfo(nub_process_t pid);
 
   nub_size_t GetNumThreads() const;
   nub_thread_t GetThreadAtIndex(nub_size_t thread_idx) const;
   nub_thread_t GetCurrentThread();
   nub_thread_t GetCurrentThreadMachPort();
   nub_thread_t SetCurrentThread(nub_thread_t tid);
   MachThreadList &GetThreadList() { return m_thread_list; }

lldb/tools/debugserver/source/MacOSX/MachProcess.mm

 JSONGenerator::ObjectSP MachProcess::FormatDynamicLibrariesIntoJSON(
     const std::vector<struct binary_image_information> &image_infos) {
 
   JSONGenerator::ArraySP image_infos_array_sp(new JSONGenerator::Array());
 
   const size_t image_count = image_infos.size();
 
   for (size_t i = 0; i < image_count; i++) {
-    if (!image_infos[i].is_valid_mach_header)
-      continue;
     JSONGenerator::DictionarySP image_info_dict_sp(
         new JSONGenerator::Dictionary());
     image_info_dict_sp->AddIntegerItem("load_address",
                                        image_infos[i].load_address);
     image_info_dict_sp->AddIntegerItem("mod_date", image_infos[i].mod_date);
     image_info_dict_sp->AddStringItem("pathname", image_infos[i].filename);
 
+    if (!image_infos[i].is_valid_mach_header) {
+      image_infos_array_sp->AddItem(image_info_dict_sp);
+      continue;
+    }
+
     uuid_string_t uuidstr;
     uuid_unparse_upper(image_infos[i].macho_info.uuid, uuidstr);
     image_info_dict_sp->AddStringItem("uuid", uuidstr);
 
     if (!image_infos[i].macho_info.min_version_os_name.empty() &&
         !image_infos[i].macho_info.min_version_os_version.empty()) {
       image_info_dict_sp->AddStringItem(
           "min_version_os_name", image_infos[i].macho_info.min_version_os_name);

   }
 
   JSONGenerator::DictionarySP reply_sp(new JSONGenerator::Dictionary());
   reply_sp->AddItem("images", image_infos_array_sp);
 
   return reply_sp;
 }
 
-// Get the shared library information using the old (pre-macOS 10.12, pre-iOS
-// 10, pre-tvOS 10, pre-watchOS 3)
-// code path.  We'll be given the address of an array of structures in the form
-// {void* load_addr, void* mod_date, void* pathname}
-//
-// In macOS 10.12 etc and newer, we'll use SPI calls into dyld to gather this
-// information.
-JSONGenerator::ObjectSP MachProcess::GetLoadedDynamicLibrariesInfos(
-    nub_process_t pid, nub_addr_t image_list_address, nub_addr_t image_count) {
-
-  JSONGenerator::ObjectSP empty_reply_sp(new JSONGenerator::Dictionary());
-  int pointer_size = GetInferiorAddrSize(pid);
-
-  std::vector<struct binary_image_information> image_infos;
-  size_t image_infos_size = image_count * 3 * pointer_size;
-
-  uint8_t *image_info_buf = (uint8_t *)malloc(image_infos_size);
-  if (image_info_buf == NULL) {
-    return empty_reply_sp;
-  }
-  if (ReadMemory(image_list_address, image_infos_size, image_info_buf) !=
-      image_infos_size) {
-    return empty_reply_sp;
-  }
-
-  /// First the image_infos array with (load addr, pathname, mod date)
-  /// tuples
-
-  for (size_t i = 0; i < image_count; i++) {
-    struct binary_image_information info;
-    nub_addr_t pathname_address;
-    if (pointer_size == 4) {
-      uint32_t load_address_32;
-      uint32_t pathname_address_32;
-      uint32_t mod_date_32;
-      ::memcpy(&load_address_32, image_info_buf + (i * 3 * pointer_size), 4);
-      ::memcpy(&pathname_address_32,
-               image_info_buf + (i * 3 * pointer_size) + pointer_size, 4);
-      ::memcpy(&mod_date_32,
-               image_info_buf + (i * 3 * pointer_size) + pointer_size +
-                   pointer_size,
-               4);
-      info.load_address = load_address_32;
-      info.mod_date = mod_date_32;
-      pathname_address = pathname_address_32;
-    } else {
-      uint64_t load_address_64;
-      uint64_t pathname_address_64;
-      uint64_t mod_date_64;
-      ::memcpy(&load_address_64, image_info_buf + (i * 3 * pointer_size), 8);
-      ::memcpy(&pathname_address_64,
-               image_info_buf + (i * 3 * pointer_size) + pointer_size, 8);
-      ::memcpy(&mod_date_64,
-               image_info_buf + (i * 3 * pointer_size) + pointer_size +
-                   pointer_size,
-               8);
-      info.load_address = load_address_64;
-      info.mod_date = mod_date_64;
-      pathname_address = pathname_address_64;
-    }
-    char strbuf[17];
-    info.filename = "";
-    uint64_t pathname_ptr = pathname_address;
-    bool still_reading = true;
-    while (still_reading && ReadMemory(pathname_ptr, sizeof(strbuf) - 1,
-                                       strbuf) == sizeof(strbuf) - 1) {
-      strbuf[sizeof(strbuf) - 1] = '\0';
-      info.filename += strbuf;
-      pathname_ptr += sizeof(strbuf) - 1;
-      // Stop if we found nul byte indicating the end of the string
-      for (size_t i = 0; i < sizeof(strbuf) - 1; i++) {
-        if (strbuf[i] == '\0') {
-          still_reading = false;
-          break;
-        }
-      }
-    }
-    uuid_clear(info.macho_info.uuid);
-    image_infos.push_back(info);
-  }
-  if (image_infos.size() == 0) {
-    return empty_reply_sp;
-  }
-
-  free(image_info_buf);
-
-  ///  Second, read the mach header / load commands for all the dylibs
-
-  for (size_t i = 0; i < image_count; i++) {
-    // The SPI to provide platform is not available on older systems.
-    uint32_t platform = 0;
-    if (GetMachOInformationFromMemory(platform, image_infos[i].load_address,
-                                      pointer_size,
-                                      image_infos[i].macho_info)) {
-      image_infos[i].is_valid_mach_header = true;
-    }
-  }
-
-  ///  Third, format all of the above in the JSONGenerator object.
-
-  return FormatDynamicLibrariesIntoJSON(image_infos);
-}
-
 /// From dyld SPI header dyld_process_info.h
 typedef void *dyld_process_info;
 struct dyld_process_cache_info {
   /// UUID of cache used by process.
   uuid_t cacheUUID;
   /// Load address of dyld shared cache.
   uint64_t cacheBaseAddress;
   /// Process is running without a dyld cache.

     }
   }
 }
 
 // Fetch information about all shared libraries using the dyld SPIs that exist
 // in
 // macOS 10.12, iOS 10, tvOS 10, watchOS 3 and newer.
 JSONGenerator::ObjectSP
-MachProcess::GetAllLoadedLibrariesInfos(nub_process_t pid) {
+MachProcess::GetAllLoadedLibrariesInfos(nub_process_t pid,
+                                        bool report_load_commands) {
 
   int pointer_size = GetInferiorAddrSize(pid);
   std::vector<struct binary_image_information> image_infos;
   GetAllLoadedBinariesViaDYLDSPI(image_infos);
-  uint32_t platform = GetPlatform();
-  const size_t image_count = image_infos.size();
-  for (size_t i = 0; i < image_count; i++) {
-    if (GetMachOInformationFromMemory(platform, image_infos[i].load_address,
-                                      pointer_size,
-                                      image_infos[i].macho_info)) {
-      image_infos[i].is_valid_mach_header = true;
+  if (report_load_commands) {
+    uint32_t platform = GetPlatform();
+    const size_t image_count = image_infos.size();
+    for (size_t i = 0; i < image_count; i++) {
+      if (GetMachOInformationFromMemory(platform, image_infos[i].load_address,
+                                        pointer_size,
+                                        image_infos[i].macho_info)) {
+        image_infos[i].is_valid_mach_header = true;
+      }
     }
   }
-    return FormatDynamicLibrariesIntoJSON(image_infos);
 }
 
 // Fetch information about the shared libraries at the given load addresses
 // using the
 // dyld SPIs that exist in macOS 10.12, iOS 10, tvOS 10, watchOS 3 and newer.
 JSONGenerator::ObjectSP MachProcess::GetLibrariesInfoForAddresses(
     nub_process_t pid, std::vector<uint64_t> &macho_addresses) {
 

 }
 
 // From dyld's internal podyld_process_info.h:
 
 JSONGenerator::ObjectSP MachProcess::GetSharedCacheInfo(nub_process_t pid) {
   JSONGenerator::DictionarySP reply_sp(new JSONGenerator::Dictionary());
 
   kern_return_t kern_ret;
   if (m_dyld_process_info_create && m_dyld_process_info_get_cache) {
     dyld_process_info info =
         m_dyld_process_info_create(m_task.TaskPort(), 0, &kern_ret);
     if (info) {
       struct dyld_process_cache_info shared_cache_info;
       m_dyld_process_info_get_cache(info, &shared_cache_info);
 
       reply_sp->AddIntegerItem("shared_cache_base_address",
                                shared_cache_info.cacheBaseAddress);

lldb/tools/debugserver/source/RNBRemote.cpp

       json << "}";
       std::string json_quoted = binary_encode_string(json.str());
       return SendPacket(json_quoted);
     }
   }
   return SendPacket("OK");
 }
 
-//  This packet may be called in one of three ways:
-//
-//  jGetLoadedDynamicLibrariesInfos:{"image_count":40,"image_list_address":4295244704}
-//      Look for an array of the old dyld_all_image_infos style of binary infos
-//      at the image_list_address.
-//      This an array of {void* load_addr, void* mod_date, void* pathname}
+//  This packet may be called in one of two ways:
 //
 //  jGetLoadedDynamicLibrariesInfos:{"fetch_all_solibs":true}
-//      Use the new style (macOS 10.12, tvOS 10, iOS 10, watchOS 3) dyld SPI to
-//      get a list of all the
-//      libraries loaded
+//      Use the new dyld SPI to get a list of all the libraries loaded
 //
 //  jGetLoadedDynamicLibrariesInfos:{"solib_addresses":[8382824135,3258302053,830202858503]}
 //      Use the new style (macOS 10.12, tvOS 10, iOS 10, watchOS 3) dyld SPI to
 //      get the information
 //      about the libraries loaded at these addresses.
 //
 rnb_err_t
 RNBRemote::HandlePacket_jGetLoadedDynamicLibrariesInfos(const char *p) {

   if (strncmp(p, get_loaded_dynamic_libraries_infos_str,
               sizeof(get_loaded_dynamic_libraries_infos_str) - 1) == 0) {
     p += strlen(get_loaded_dynamic_libraries_infos_str);
 
     JSONGenerator::ObjectSP json_sp;
 
     std::vector<uint64_t> macho_addresses;
     bool fetch_all_solibs = false;
+    bool report_load_commands = true;
+    get_boolean_value_for_key_name_from_json("report_load_commands", p,
+                                             report_load_commands);
+
     if (get_boolean_value_for_key_name_from_json("fetch_all_solibs", p,
                                                  fetch_all_solibs) &&
         fetch_all_solibs) {
-      json_sp = DNBGetAllLoadedLibrariesInfos(pid);
+      json_sp = DNBGetAllLoadedLibrariesInfos(pid, report_load_commands);
     } else if (get_array_of_ints_value_for_key_name_from_json(
                    "solib_addresses", p, macho_addresses)) {
       json_sp = DNBGetLibrariesInfoForAddresses(pid, macho_addresses);
-    } else {
-      nub_addr_t image_list_address =
-          get_integer_value_for_key_name_from_json("image_list_address", p);
-      nub_addr_t image_count =
-          get_integer_value_for_key_name_from_json("image_count", p);
-
-      if (image_list_address != INVALID_NUB_ADDRESS &&
-          image_count != INVALID_NUB_ADDRESS) {
-        json_sp = DNBGetLoadedDynamicLibrariesInfos(pid, image_list_address,
-                                                    image_count);
-      }
     }
 
     if (json_sp.get()) {
       std::ostringstream json_str;
       json_sp->DumpBinaryEscaped(json_str);
       json_sp->Clear();
       if (json_str.str().size() > 0) {
         return SendPacket(json_str.str());