jasonmolenda created this revision.
jasonmolenda added a reviewer: bulbazord.
jasonmolenda added a project: LLDB.
Herald added a subscriber: JDevlieghere.
Herald added a project: All.
jasonmolenda requested review of this revision.
Herald added a subscriber: lldb-commits.

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.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D150158

Files:
  lldb/docs/lldb-gdb-remote.txt
  lldb/tools/debugserver/source/DNB.cpp
  lldb/tools/debugserver/source/DNB.h
  lldb/tools/debugserver/source/MacOSX/MachProcess.h
  lldb/tools/debugserver/source/MacOSX/MachProcess.mm
  lldb/tools/debugserver/source/RNBRemote.cpp

Index: lldb/tools/debugserver/source/RNBRemote.cpp
===================================================================
--- lldb/tools/debugserver/source/RNBRemote.cpp
+++ lldb/tools/debugserver/source/RNBRemote.cpp
@@ -5927,17 +5927,10 @@
   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
@@ -5964,24 +5957,17 @@
 
     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()) {
Index: lldb/tools/debugserver/source/MacOSX/MachProcess.mm
===================================================================
--- lldb/tools/debugserver/source/MacOSX/MachProcess.mm
+++ lldb/tools/debugserver/source/MacOSX/MachProcess.mm
@@ -919,8 +919,6 @@
   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",
@@ -928,6 +926,11 @@
     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);
@@ -1000,109 +1003,6 @@
   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 {
@@ -1162,21 +1062,24 @@
 // 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);
+  return FormatDynamicLibrariesIntoJSON(image_infos);
 }
 
 // Fetch information about the shared libraries at the given load addresses
Index: lldb/tools/debugserver/source/MacOSX/MachProcess.h
===================================================================
--- lldb/tools/debugserver/source/MacOSX/MachProcess.h
+++ lldb/tools/debugserver/source/MacOSX/MachProcess.h
@@ -271,12 +271,12 @@
   /// 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;
Index: lldb/tools/debugserver/source/DNB.h
===================================================================
--- lldb/tools/debugserver/source/DNB.h
+++ lldb/tools/debugserver/source/DNB.h
@@ -210,9 +210,8 @@
                           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);
Index: lldb/tools/debugserver/source/DNB.cpp
===================================================================
--- lldb/tools/debugserver/source/DNB.cpp
+++ lldb/tools/debugserver/source/DNB.cpp
@@ -1023,20 +1023,11 @@
   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();
 }
Index: lldb/docs/lldb-gdb-remote.txt
===================================================================
--- lldb/docs/lldb-gdb-remote.txt
+++ lldb/docs/lldb-gdb-remote.txt
@@ -2001,19 +2001,16 @@
 //  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]}
 //
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Reply via email to