Page MenuHomePhabricator

Look for both .debug and dsym debugging symbol information for stripped executable.
ClosedPublic

Authored by flackr on Apr 21 2015, 2:49 PM.

Details

Summary

Currently Symbols::LocateExecutableSymbolFile on MacOSX only looks for external dsym debugging information, however if running on a stripped dwarf executable it should also check for a .debug file as well.

Diff Detail

Repository
rL LLVM

Event Timeline

flackr updated this revision to Diff 24173.Apr 21 2015, 2:49 PM
flackr retitled this revision from to Look for both .debug and dsym debugging symbol information for stripped executable..
flackr updated this object.
flackr edited the test plan for this revision. (Show Details)
flackr added reviewers: clayborg, tberghammer.
flackr set the repository for this revision to rL LLVM.
flackr added a subscriber: Unknown Object (MLST).

I wanted to float this patch by you to get an early impression. Previously we only looked for dsym debugging info on mac and only .debug on linux. This patch should allow us to locate either. What I've tried to do is move as much of the symbol location into common/Symbols.cpp that I can. What do you think of this direction?

Interestingly the Test TestSharedLibStrippedSymbols.py should have caught this on mac except that test/make/Makefile.rules only seems to strip dsym on darwin. I can try to fix this too and verify with this patch.

source/Host/common/Symbols.cpp
146 ↗(On Diff #24173)

I didn't notice this when I was moving the code over but I'll remove this commented out code.

flackr added inline comments.Apr 22 2015, 5:40 AM
source/Host/common/Symbols.cpp
219 ↗(On Diff #24173)

Also, is there a better place to put these forward declarations? My impression was that include/Host/Symbols.h was for things we wanted to publicly expose whereas this is sort of an implementation detail, but if it can go there I will move it.

tberghammer edited edge metadata.Apr 22 2015, 5:56 AM

I don't like having these forward declarations but I think the problem have a different root cause.

I would do the following changes in order to get rid of them and clean up the code further:

  • Remove (almost) all conditional compilation because when we compile these files into LLDB we need all implementation
  • Move all OSX and MachO specific code into macosx/Symbols.cpp
  • Make the common class the base class of the OSX specific class and replace the static functions with virtual ones for greater flexibility
clayborg requested changes to this revision.Apr 22 2015, 10:26 AM
clayborg edited edge metadata.

So there is some code "source/Host/macosx/Symbols.cpp":

SkinnyMachOFileContainsArchAndUUID(...)
UniversalMachOFileContainsArchAndUUID(...)
FileAtPathContainsArchAndUUID(...)

That was created long before I eventually created the static class function:

static size_t
ObjectFile::GetModuleSpecifications (const FileSpec &file,
                         lldb::offset_t file_offset,
                         lldb::offset_t file_size,
                         ModuleSpecList &specs);

We should switch over to using ObjectFile::GetModuleSpecifications() in both "source/Host/macosx/Symbols.cpp" and if needed in "source/Host/common/Symbols.cpp" and update this patch. This should reduce the size of the patch considerably.

source/Host/common/Symbols.cpp
33–43 ↗(On Diff #24173)

This function is not needed, please use the static class function:

size_t ObjectFile::GetModuleSpecifications (const FileSpec &file,
                             lldb::offset_t file_offset,
                             lldb::offset_t file_size,
                             ModuleSpecList &specs);
114–123 ↗(On Diff #24173)

This function isn't needed please use ObjectFile::GetModuleSpecifications()

177–183 ↗(On Diff #24173)

This function isn't needed, please use ObjectFile::GetModuleSpecifications().

This revision now requires changes to proceed.Apr 22 2015, 10:26 AM
flackr updated this revision to Diff 24323.Apr 23 2015, 12:53 PM
flackr edited edge metadata.

Use ObjectFile::GetModuleSpecifications to check for matching module spec.

Greg, please check if I'm checking the module spec correctly. I'm not sure if I should only be looking for matching UUID and arch whereas the ModuleSpecList::FindMatchingModuleSpec seems like it will check the FileSpec and PlatformFileSpec as well. It passes the stripped symbols test though which I presume is testing it.

I don't like having these forward declarations but I think the problem have a different root cause.

I would do the following changes in order to get rid of them and clean up the code further:

  • Remove (almost) all conditional compilation because when we compile these files into LLDB we need all implementation
  • Move all OSX and MachO specific code into macosx/Symbols.cpp
  • Make the common class the base class of the OSX specific class and replace the static functions with virtual ones for greater flexibility

At this point I think the only ugly bit is LocateMacOSXFilesUsingDebugSymbols. I could add this to the Symbols.h header to avoid the need for conditional compilation.

clayborg accepted this revision.Apr 23 2015, 1:03 PM
clayborg edited edge metadata.

Looks good.

This revision is now accepted and ready to land.Apr 23 2015, 1:03 PM
flackr updated this revision to Diff 24395.Apr 24 2015, 10:17 AM
flackr edited edge metadata.

The lovely test suite caught a couple errors which I've addressed in this patch

  • NULL is acceptable input for arch or uuid to Symbols::FindSymbolFileInBundle to ignore that attribute.
  • Properly check for dsym symbols even if there's no symbol filename in the module spec.

Looks good as long as the test suite is happy.

This revision was automatically updated to reflect the committed changes.