This is an archive of the discontinued LLVM Phabricator instance.

[lldb] [mach-o corefiles] If we have LC_NOTE metadata and can't find a binary, don't fall back to an exhaustive scan

Authored by jasonmolenda on Aug 4 2023, 5:41 PM.



We have some corefiles which are intended to debug a standalone binary, and have an LC_NOTE mach-o load command with the UUID and/or address of that binary. If the search for that binary fails for any reason, we don't load the intended binary. ProcessMachCore then falls back to doing an exhaustive scan of the corefile memory pages looking for a darwin kernel or a userland dyld binary. And the case where this comes up most often, there is a darwin kernel. lldb starts doing a darwin kernel debug session and that's not at all what the user is intending to do.

This patch changes ProcessMachCore::LoadBinariesViaMetadata to return if any metadata record was found, even if we didn't find the binary, and ProcessMachCore::LoadBinariesAndSetDYLD will only start an exhaustive search if we had no metadata.

Diff Detail

Event Timeline

jasonmolenda created this revision.Aug 4 2023, 5:41 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 4 2023, 5:41 PM
jasonmolenda requested review of this revision.Aug 4 2023, 5:41 PM

Patch looks good to me, one small suggestion about naming.


Since LoadBinariesViaMetadata can fail and we now care about the return value, what do you think about the name TryLoadBinariesViaMetadata? I think a name that indicates that it may fail to actually load would make it easier to read at a glance.

JDevlieghere added inline comments.Aug 7 2023, 1:24 PM

The same is true for the other methods though, so I don't know if that is really all that meaningful. IIUC, before this patch, all these methods conveyed whether they were successful by setting m_dyld_plugin_name. Now, LoadBinariesViaMetadata does something extra, which is conveyed by the boolean. I dislike the inconsistency, but I can't really think of a better way of doing this, I considered suggesting an out-parameter but that doesn't really improve anything.

This revision is now accepted and ready to land.Aug 8 2023, 12:05 PM

Rebase on current TOT sources, and add a comment to the ProcessMachCore::LoadBinariesViaMetadata prototype in the header documenting the meaning of the return value, I agree with Alex I should make this a little clearer.