[lldb][SymbolFileDWARF] Don't search for DWP files on macOS #139554
Conversation
A contrived performance measurement on my local machine: 1. Attach LLDB to LLDB 2. Measure how much time it takes to run `b operator[]` Without this patch, we would spend a total of 400ms (which was ~4-5% of total execution) looking for DWP files (and failing to find them because they are not a concept on macOS). We call into `GetDwpSymbolFile` via `DebugNamesDWARFIndex::GetFunctions -> ManualDWARFIndex::GetFunctions -> ManualDWARFIndex::Index`. rdar://148212738
|
@llvm/pr-subscribers-lldb Author: Michael Buch (Michael137) ChangesA contrived performance measurement on my local machine:
Without this patch, we would spend a total of 400ms (which was ~4-5% of total execution) looking for DWP files (and failing to find them because they are not a concept on macOS). We call into rdar://148212738 Full diff: https://github.com/llvm/llvm-project/pull/139554.diff 1 Files Affected:
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
index 907d63eb51afe..3f53e562ba65b 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
@@ -4211,6 +4211,9 @@ SymbolFileDWARFDebugMap *SymbolFileDWARF::GetDebugMapSymfile() {
const std::shared_ptr<SymbolFileDWARFDwo> &SymbolFileDWARF::GetDwpSymbolFile() {
llvm::call_once(m_dwp_symfile_once_flag, [this]() {
+ if (m_objfile_sp->GetArchitecture().GetTriple().isAppleMachO())
+ return;
+
// Create a list of files to try and append .dwp to.
FileSpecList symfiles;
// Append the module's object file path.
|
| @@ -4211,6 +4211,9 @@ SymbolFileDWARFDebugMap *SymbolFileDWARF::GetDebugMapSymfile() { | |||
|
|
|||
| const std::shared_ptr<SymbolFileDWARFDwo> &SymbolFileDWARF::GetDwpSymbolFile() { | |||
| llvm::call_once(m_dwp_symfile_once_flag, [this]() { | |||
| if (m_objfile_sp->GetArchitecture().GetTriple().isAppleMachO()) | |||
There was a problem hiding this comment.
Should this be a check for the ObjectFile subclass instead?
There was a problem hiding this comment.
@JDevlieghere Maybe you have a better layering suggestion?
There was a problem hiding this comment.
That was my first inclination. But then I was reading the discussion at #139170 (comment) and was going to see where that goes before adding one of these kinds of APIs to ObjectFile
There was a problem hiding this comment.
Yeah, this sounds like very much a dual problem to that, so it would be nice if they used the same approach. The thing that makes this fuzzier is that in this case you can't really say that SymbolFileDWARF requires ObjectFileMachO (or any other) because one can imagine a setup where someone knows they will only ever need to debug DWARF+ELF and so they want to strip out all of the other object file plugins. OTOH, I can also imagine us saying we don't want to support that. :P
There was a problem hiding this comment.
Not sure what the resolution was re. the plugin dependencies discussion. Are we find with the current approach?
you can't really say that SymbolFileDWARF requires ObjectFileMachO (or any other) because one can imagine a setup where someone knows they will only ever need to debug DWARF+ELF and so they want to strip out all of the other object file plugins.
Good question. If we don't want to restrict a use-case such as the one you describe, I'm happy to add a SupportsDWP API to ObjectFile. Any thoughts @JDevlieghere ?
There was a problem hiding this comment.
I think this is a good solution until we've documented the dependency policy from #139170
| @@ -4211,6 +4211,9 @@ SymbolFileDWARFDebugMap *SymbolFileDWARF::GetDebugMapSymfile() { | |||
|
|
|||
| const std::shared_ptr<SymbolFileDWARFDwo> &SymbolFileDWARF::GetDwpSymbolFile() { | |||
| llvm::call_once(m_dwp_symfile_once_flag, [this]() { | |||
| if (m_objfile_sp->GetArchitecture().GetTriple().isAppleMachO()) | |||
There was a problem hiding this comment.
I think this is a good solution until we've documented the dependency policy from #139170
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/195/builds/9716 Here is the relevant piece of the build log for the reference |
) (cherry picked from commit f5c6b7b)
A contrived performance measurement on my local machine:
b operator[]Without this patch, we would spend a total of 400ms (which was ~4-5% of total execution) looking for DWP files (and failing to find them because they are not a concept on macOS).
We call into
GetDwpSymbolFileviaDebugNamesDWARFIndex::GetFunctions -> ManualDWARFIndex::GetFunctions -> ManualDWARFIndex::Index.rdar://148212738