This is an archive of the discontinued LLVM Phabricator instance.

A handful of fixes to lldb's "scan the local filesystem for kexts & kernels" feature
ClosedPublic

Authored by jasonmolenda on Sep 30 2020, 8:46 PM.

Details

Summary

This started out with one small idea I had and then got a bit of PatchCreep while I was digging into a subsystem I haven't touched in a year or two. The patch is all isolated to darwin kernel debugging, it doesn't modify anything outside of that. The things I changed here:

  1. When looking on the local filesystem for a kernel binary + dSYM pair, if that fails, look through any dSYMs that we saw alone without a binary, and load those as a binary+dSYM pair all by itself. It's not ideal, but it will mostly work.
  1. Remove the 'platform.plugin.darwin-kernel.search-locally-for-kexts' setting. I originally added this setting when I first added the local filesystem scan for kexts/kernels, fearing that people would ask for a way to disable it. No one has ever asked. I thought of making the setting a no-op and documenting it as obsoleted, but eventually it'll have to be removed and someone will get an error in their .lldbinit file if they're still using it. Might as well make that now.
  1. Change plugin.dynamic-loader.darwin-kernel.load-kexts so that it doesn't disable local kernel binary scanning. It was a nonobvious interaction, I see kernel developers trip over this occasionally.
  1. Split up PlatformDarwinKernel::GetSharedModule into GetSharedModuleKext and GetSharedModuleKernel. This method was unnecessarily large and the blocks of code had no interaction with each other; splitting it out makes it easier to read and modify.
  1. I keep track of possible kernel .dSYM.yaa (a file archive format) files that I come across during the scan, but I'm only reporting it in the statistics for now, I don't think I want to pay the cost of expanding these to see if they're a match.

Diff Detail

Event Timeline

jasonmolenda created this revision.Sep 30 2020, 8:46 PM
jasonmolenda requested review of this revision.Sep 30 2020, 8:46 PM

I don't know much about this code so I've left some inline nits.

lldb/source/Plugins/Platform/MacOSX/PlatformDarwinKernel.cpp
505

Nit: You could assign the StringRef to a temporary so you don't have to repeat file_spec.GetFilename().GetStringRef() thrice.

679

FileSpec has a GetFileNameExtension but this works too.

683–684

FileSpec::GetFileNameStrippingExtension

715

FileSpec::GetFileNameStrippingExtension

865

Huh, how did we end up with NULL again, someone ran a clang tidy check over lldb that replaces it with nullptr. Do we only build this on Darwin maybe? If the rest of the file uses NULL we can fix that inconsistency in a separate commit.

Incorporate Jonas' suggestions, including changing the NULLs to nullptr's in the section of code I was changing right now. Also found an unintentional shadowing of module_sp in GetSharedModuleKernel (that was present before my patchset) and fixed that; it works either way, but it's not correct.

This revision was not accepted when it landed; it landed in state Needs Review.Oct 1 2020, 6:55 PM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.