This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by jasonmolenda on May 8 2023, 4:58 PM.

Details

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.

Diff Detail

Event Timeline

jasonmolenda created this revision.May 8 2023, 4:58 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 8 2023, 4:58 PM
jasonmolenda requested review of this revision.May 8 2023, 4:58 PM

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.

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.

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

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 accepted this revision.May 8 2023, 6:45 PM

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);
This revision is now accepted and ready to land.May 8 2023, 6:45 PM

Updating to reflect what I committed.