Page MenuHomePhabricator

Add support to lldb for reading a "load binary" LC_NOTE with Mach-O corefiles
ClosedPublic

Authored by jasonmolenda on Dec 10 2021, 12:20 AM.

Details

Summary

For some of our non-userland / non-kernel environments, there can be multiple binary images involved in the environment, and each environment has a different way of discovering these binaries and would require a specialized DynamicLoader plugin in lldb to handle. This patch adds a new "load binary" LC_NOTE for corefiles which allows the corefile creator to list the binaries that lldb should attempt to load, and where to load them, for viewing the corefile.

The actual patch to read this new LC_NOTE is a dozen lines thanks to the "all image infos" support I added earlier this year; the mechanisms are similar. "all image infos" always gave us the segment load addresses for each binary, but "load binaries" allows for load address or a slide to be specified. So I added support to handle those cases in ObjectFileMachO, and added logging to help debug any problems.

I updated my TestCorefileDefaultPtrauth.py test from https://reviews.llvm.org/D115431 to add this new LC_NOTE to the corefile that it creates. Previously, TestCorefileDefaultPtrauth.py was manually loading the a.out binary and setting its load address; now the API test load the executable into a Target and deletes the Target, to get it registered in lldb's global module cache. Then we load the corefile and the binary is discovered by its UUID. No additional tests were needed to confirm the binary was loaded; the test looking at a global will fail if it is not.

Diff Detail

Event Timeline

jasonmolenda created this revision.Dec 10 2021, 12:20 AM
jasonmolenda requested review of this revision.Dec 10 2021, 12:20 AM
DavidSpickett added inline comments.
lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
6984

Seems like the cast wouldn't be needed:

const char *GetCStr(lldb::offset_t *offset_ptr) const;
7005

Maybe not for this change but if you made this early return you could dedent all the following by one level.

if (!image_infos.IsValid())
  return added_images;
7035

You can use the LLDB_LOGF to do the if (log) for you I think. Then do image.uuid.GetAsString().c_str() directly as one of the parameters.

7065

Is the slide address the amount you slide the load address *by* or is it an address all to itself? My assumption was the former.
(which makes the log message a bit odd since it's a distance not an amount)

7077

unslid sounds a bit like you removed a slide from it, I'd go with "without a slide" but that's just me.

jasonmolenda marked 5 inline comments as done.Dec 10 2021, 6:34 PM
jasonmolenda added inline comments.
lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
7005

Oh yes, nice.

7035

Oh that's clever, but I prefer the Printf method so I'll do it this way.

7065

Yeah, I was banging out these log messages past bedtime trying to wrap it up, and wasn't as clear as I meant to be. Cleaned these up, thanks.

jasonmolenda marked 3 inline comments as done.

Update patch to address the points David made. Thanks!

Clean up the way the newly loaded Module is added to the Target, and we notify for breakpoint etc evaluation in ObjectFileMachO::LoadCoreFileImages.

DavidSpickett accepted this revision.Dec 13 2021, 6:53 AM

LGTM assuming the change in TestFunctionStarts.py wasn't meant to be included.

lldb/test/API/macosx/function-starts/TestFunctionStarts.py
68 ↗(On Diff #393668)

Stray change?

This revision is now accepted and ready to land.Dec 13 2021, 6:53 AM
jasonmolenda added inline comments.Dec 13 2021, 9:23 AM
lldb/test/API/macosx/function-starts/TestFunctionStarts.py
68 ↗(On Diff #393668)

*cough* lol I was looking into a testsuite failure and forgot ;)