This is an archive of the discontinued LLVM Phabricator instance.

Add support for a version 2 of "main bin spec" Mach-O LC_NOTE in corefiles, handle specifications with a slide value
ClosedPublic

Authored by jasonmolenda on Dec 21 2021, 1:32 AM.

Details

Summary

The Mach-O corefiles have a "main bin spec" LC_NOTE which allows the corefile creator to specify a binary's UUID, load address, and type (userland, kernel, frimware). This patch extends that structure with two new fields - an optional slide, and an optional platform (currently unused).

We have a corefile creator that will only know the offset that a binary was loaded at, as well as its UUID, so lldb will need to find the original binary and add the offset to its file address to get the correct load address.

I changed the ObjectFile::GetCorefileMainBinaryInfo method from taking a load address, to taking a value and a bool of whether the value is an address or an offset. This is based on Module::SetLoadAddress, and it seemed good to stick with that argument style.

The majority of the patch is changing TestFirmwareCorefiles.py. I changed create-empty-corefile.cpp to work on Intel or Apple Silicon macs, have it generate version 2 of the 'main bin spec' LC_NOTE, both with an address and with a slide.

Diff Detail

Event Timeline

jasonmolenda created this revision.Dec 21 2021, 1:32 AM
jasonmolenda requested review of this revision.Dec 21 2021, 1:32 AM
JDevlieghere accepted this revision.Dec 21 2021, 10:25 AM

A few small nits but the change itself looks sound to me. LGTM.

lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
7052

No need to change this, but if you ever have situation where the buffer is variable length, LLVM's formatv makes this very easy:

std::string s = llvm::formatv("mem-image+{0:x}", image.load_address)
lldb/source/Plugins/Process/mach-core/ProcessMachCore.cpp
224

You can hoist this out of the if-check and reuse it in all the blocks instead of having to duplicate the variable each time.

230

When I was reading this code, I found this a little confusing, given that value_is_offset is an input parameter and this is meant as a constant. Not sure how to make this better other than possibly using /*value_is_offset=*/ true. Anyway small nit, don't feel like you need to change anything here.

lldb/test/API/macosx/lc-note/firmware-corefile/TestFirmwareCorefiles.py
41

Nice

72–88

Given that you already build the corefiles only once, would it make sense to make these separate tests so that *if* they fail, you can immediately tell from the logs? Or maybe these tests are sharing something that I missed?

177

I believe you need to pass exist_ok to avoid this failing when the dir already exists.

184

os.path.join

191

We shouldn't be hardcoding this, we already have llvm-dwarfdump as a test dependency. That said, unlike dsymutil there's no good way to get the dwarfdump binary. There's other tests doing the same, so we (I?) should address this in another patch.

200–227

I feel like we have a handful of tests creating this dsymforuuid wrapper. At some point we should consider moving this in the lldbutil so we don't have to copy-paste this everywhere.

This revision is now accepted and ready to land.Dec 21 2021, 10:25 AM

Update patch to address Jonas' feedback. Most of the changes in this update are to TestFirmwareCorefiles.py, cleaning its structure up given how the test has grown over time.