[Lldb-commits] [PATCH] D150158: Add optional to debugserver's "tell me about all binaries in the process" packet, to limit it to just load addresses and names

[Alex Langford via Phabricator via lldb-commits]

bulbazord added a comment.

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?



================
Comment at: lldb/tools/debugserver/source/MacOSX/MachProcess.mm:930-931
+    if (!image_infos[i].is_valid_mach_header) {
+      image_infos_array_sp->AddItem(image_info_dict_sp);
+      continue;
+    }
----------------
Why do we still want to add information about this dylib if the mach header isn't valid? 


================
Comment at: lldb/tools/debugserver/source/RNBRemote.cpp:5932-5933
 //
 //  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
 //
----------------
Should probably update this comment with `"report_load_commands":false` as well.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D150158/new/

https://reviews.llvm.org/D150158