[lldb] Show module name in progress update for downloading symbols #85342
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Currently, we always show the argument passed to dsymForUUID in the corresponding progress update. Most of the time this is a UUID, but it can also be an absolute path. The former is pretty uninformative and the latter needlessly noisy. This changes the progress update to print the UUID and the module name, if both are available. Otherwise, we print the UUID or the module name depending on which one is available. We now also unconditionally pass the module file spec and architecture to DownloadObjectAndSymbolFile, while previously this was conditional on the file existing on-disk. This should be harmless: - We already check that the file exists in DownloadObjectAndSymbolFile. - It doesn't make sense to check the filesystem for the architecutre. rdar://124643548
|
@llvm/pr-subscribers-lldb Author: Jonas Devlieghere (JDevlieghere) ChangesCurrently, we always show the argument passed to dsymForUUID in the corresponding progress update. Most of the time this is a UUID, but it can also be an absolute path. The former is pretty uninformative and the latter needlessly noisy. This changes the progress update to print the UUID and the module name, if both are available. Otherwise, we print the UUID or the module name depending on which one is available. We now also unconditionally pass the module file spec and architecture to DownloadObjectAndSymbolFile, while previously this was conditional on the file existing on-disk. This should be harmless:
rdar://124643548 Full diff: https://github.com/llvm/llvm-project/pull/85342.diff 2 Files Affected:
diff --git a/lldb/source/Commands/CommandObjectTarget.cpp b/lldb/source/Commands/CommandObjectTarget.cpp
index b2346c2402a81d..ae6c6d5479a198 100644
--- a/lldb/source/Commands/CommandObjectTarget.cpp
+++ b/lldb/source/Commands/CommandObjectTarget.cpp
@@ -3377,7 +3377,7 @@ class CommandObjectTargetModulesList : public CommandObjectParsed {
case 'r': {
size_t ref_count = 0;
char in_shared_cache = 'Y';
-
+
ModuleSP module_sp(module->shared_from_this());
if (!ModuleList::ModuleIsInCache(module))
in_shared_cache = 'N';
@@ -4508,11 +4508,8 @@ class CommandObjectTargetSymbolsAdd : public CommandObjectParsed {
ModuleSpec module_spec;
module_spec.GetUUID() = frame_module_sp->GetUUID();
-
- if (FileSystem::Instance().Exists(frame_module_sp->GetPlatformFileSpec())) {
- module_spec.GetArchitecture() = frame_module_sp->GetArchitecture();
- module_spec.GetFileSpec() = frame_module_sp->GetPlatformFileSpec();
- }
+ module_spec.GetArchitecture() = frame_module_sp->GetArchitecture();
+ module_spec.GetFileSpec() = frame_module_sp->GetPlatformFileSpec();
if (!DownloadObjectAndSymbolFile(module_spec, result, flush)) {
result.AppendError("unable to find debug symbols for the current frame");
@@ -4557,12 +4554,8 @@ class CommandObjectTargetSymbolsAdd : public CommandObjectParsed {
ModuleSpec module_spec;
module_spec.GetUUID() = frame_module_sp->GetUUID();
-
- if (FileSystem::Instance().Exists(
- frame_module_sp->GetPlatformFileSpec())) {
- module_spec.GetArchitecture() = frame_module_sp->GetArchitecture();
- module_spec.GetFileSpec() = frame_module_sp->GetPlatformFileSpec();
- }
+ module_spec.GetFileSpec() = frame_module_sp->GetPlatformFileSpec();
+ module_spec.GetArchitecture() = frame_module_sp->GetArchitecture();
bool current_frame_flush = false;
if (DownloadObjectAndSymbolFile(module_spec, result, current_frame_flush))
diff --git a/lldb/source/Plugins/SymbolLocator/DebugSymbols/SymbolLocatorDebugSymbols.cpp b/lldb/source/Plugins/SymbolLocator/DebugSymbols/SymbolLocatorDebugSymbols.cpp
index f7df4650941a80..acdf962a447266 100644
--- a/lldb/source/Plugins/SymbolLocator/DebugSymbols/SymbolLocatorDebugSymbols.cpp
+++ b/lldb/source/Plugins/SymbolLocator/DebugSymbols/SymbolLocatorDebugSymbols.cpp
@@ -1066,11 +1066,21 @@ bool SymbolLocatorDebugSymbols::DownloadObjectAndSymbolFile(
command << lookup_arg;
// Log and report progress.
+ std::string lookup_desc;
+ if (uuid_ptr && file_spec_ptr)
+ lookup_desc =
+ llvm::formatv("{0} ({1})", file_spec_ptr->GetFilename().GetString(),
+ uuid_ptr->GetAsString());
+ else if (uuid_ptr)
+ lookup_desc = uuid_ptr->GetAsString();
+ else if (file_spec_ptr)
+ lookup_desc = file_spec_ptr->GetFilename().GetString();
+
Log *log = GetLog(LLDBLog::Host);
LLDB_LOG(log, "Calling {0} with {1} to find dSYM: {2}", dsymForUUID_exe_path,
- lookup_arg, command.GetString());
+ lookup_desc, command.GetString());
- Progress progress("Downloading symbol file", lookup_arg);
+ Progress progress("Downloading symbol file", lookup_desc);
// Invoke dsymForUUID.
int exit_status = -1;
|
|
LGTM! |
adrian-prantl
requested changes
Mar 15, 2024
|
|
||
| Progress progress("Downloading symbol file", lookup_arg); | ||
| Progress progress("Downloading symbol file", lookup_desc); |
There was a problem hiding this comment.
I think this needs to say Downloading symbol file for now, because otherwise it sounds like we're downloading file_spec?
There was a problem hiding this comment.
The 3 possible status messages are:
Downloading symbol file: libfoo.dylib (00000000-0000-0000-0000-000000000000)
Downloading symbol file: 00000000-0000-0000-0000-000000000000
Downloading symbol file: libfoo.dylib
For when we have both the file and UUID, only the UUID or only the file.
There was a problem hiding this comment.
Right, this makes it sounds as if we are downloading libfoo.dylib. But we're downloading the symbol file for libfoo.dylib, which is, e.g., libfoo.dylib.dSYM. Hence the "for"
There was a problem hiding this comment.
I like Adrian's proposal. We could also do "Downloading symbols for" or "Downloading debug info for" as options.
bulbazord
approved these changes
Mar 15, 2024
adrian-prantl
approved these changes
Mar 15, 2024
| Log *log = GetLog(LLDBLog::Host); | ||
| LLDB_LOG(log, "Calling {0} with {1} to find dSYM: {2}", dsymForUUID_exe_path, | ||
| lookup_arg, command.GetString()); | ||
| lookup_desc, command.GetString()); |
There was a problem hiding this comment.
Technically this log message also needs updating...
JDevlieghere
added a commit
to swiftlang/llvm-project
that referenced
this pull request
Mar 15, 2024
…lvm#85342) Currently, we always show the argument passed to dsymForUUID in the corresponding progress update. Most of the time this is a UUID, but it can also be an absolute path. The former is pretty uninformative and the latter needlessly noisy. This changes the progress update to print the UUID and the module name, if both are available. Otherwise, we print the UUID or the module name depending on which one is available. We now also unconditionally pass the module file spec and architecture to DownloadObjectAndSymbolFile, while previously this was conditional on the file existing on-disk. This should be harmless: - We already check that the file exists in DownloadObjectAndSymbolFile. - It doesn't make sense to check the filesystem for the architecutre. rdar://124643548 (cherry picked from commit b7dd601)
JDevlieghere
added a commit
to swiftlang/llvm-project
that referenced
this pull request
Mar 16, 2024
[lldb] Show module name in progress update for downloading symbols (llvm#85342)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently, we always show the argument passed to dsymForUUID in the corresponding progress update. Most of the time this is a UUID, but it can also be an absolute path. The former is pretty uninformative and the latter needlessly noisy.
This changes the progress update to print the UUID and the module name, if both are available. Otherwise, we print the UUID or the module name depending on which one is available.
We now also unconditionally pass the module file spec and architecture to DownloadObjectAndSymbolFile, while previously this was conditional on the file existing on-disk. This should be harmless:
rdar://124643548