[Lldb-commits] [PATCH] D150928: Two bug fixes for loading process save-core created core files, plus perf improvements

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda created this revision.
jasonmolenda added reviewers: jingham, 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.

I had two bugs with lldb's mach-o corefile loader using the "all image infos" LC_NOTE, and while working on those, I was looking closely at the code and found several perf mistakes where I would re-do work multiple times.  This patch unfortunately ends up doing several things at once.

1. If we load binaries in ObjectFileMachO via the "all image infos" LC_NOTE, `ProcessMachCore::LoadBinariesViaMetadata()` should detect that binaries were loaded by it, and not then scan for a `dyld` in the corefile, throw away the Target image list already constructed, and try to recreate it (and possibly not be successful).  Also fix a mistake for the "main bin spec" LC_NOTE with userland process type in `ProcessMachCore::LoadBinariesViaMetadata`, noting that this is a userland dyld situation.  It ProcessMachCore currently falls back to a scan, and finds dyld by exhaustive search, but that's unnecessary work.

2. Binaries would first be loaded into the Target at their mach-header vm address, then ObjectFileMachO would load each binary's segments at the correct vmaddrs.  With binaries in the Darwin shared cache, the actual layout in memory has TEXT and DATA in discontiguous vm ranges.  But when we load all the segments in the Target at the mach header address contiguously, we get "address 0x... maps to more than one section" warnings.  The addresses for the file would then be set correctly on a segment-by-segment basis in ObjectFileMachO, but the warnings were displayed to the user.  For this, I add a `set_address_in_target` bool argument to `DynamicLoader::LoadBinaryWithUUIDAndAddress` to control whether it sets the address of the binary it finds in the Target, or if it leaves this to the caller.  In `ObjectFileMachO::LoadCoreFileImages` when a binary has per-segment vmaddrs, it sets a local `set_load_address` and we pass that in to our calls.  Update other callers to all pass `set_load_address=true` (e.g. `ProcessGDBRemote::LoadStubBinaries`) to `DynamicLoader::LoadBinaryWithUUIDAndAddress` to preserve existing behavior for them.

3. Perf fix in `ObjectFileMachO::LoadCoreFileImages` where it would try to find the binary for a UUID multiple times as different approaches were unsuccessful.  These attempts can be slow, and doing them multiple times can be more slow.  Also a lot of code duplication if we have `image.load_address` or `image.slide`, but most lldb internal API take a uint64_t value and a "value is offset/slide" bool specifically to avoid this kind of duplication..

4. A previously committed perf fix https://reviews.llvm.org/D150621 was also driven by this investigation, but that patch was easily chiseled off into a separate patch, so I did that one already.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D150928

Files:
  lldb/include/lldb/Target/DynamicLoader.h
  lldb/source/Core/DynamicLoader.cpp
  lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
  lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
  lldb/source/Plugins/Process/mach-core/ProcessMachCore.cpp

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D150928.523631.patch
Type: text/x-patch
Size: 14297 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20230519/223fa51b/attachment.bin>