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.
Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
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. |
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. |
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
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(). |
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.
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.
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.