[lldb] add plugin names to process save-core error output. #143126
Conversation
|
@llvm/pr-subscribers-lldb Author: Ebuka Ezike (da-viper) Changescontinuation of #142684 to show plugin names. From issue #14258 Full diff: https://github.com/llvm/llvm-project/pull/143126.diff 3 Files Affected:
diff --git a/lldb/include/lldb/Core/PluginManager.h b/lldb/include/lldb/Core/PluginManager.h
index e2f709ecd2ff7..7996dc3fdf3f3 100644
--- a/lldb/include/lldb/Core/PluginManager.h
+++ b/lldb/include/lldb/Core/PluginManager.h
@@ -212,6 +212,8 @@ class PluginManager {
static Status SaveCore(const lldb::ProcessSP &process_sp,
lldb_private::SaveCoreOptions &core_options);
+ static llvm::SmallVector<llvm::StringRef> GetSaveCorePluginNames();
+
// ObjectContainer
static bool RegisterPlugin(
llvm::StringRef name, llvm::StringRef description,
diff --git a/lldb/source/Core/PluginManager.cpp b/lldb/source/Core/PluginManager.cpp
index de815e6308838..d02e3030081dc 100644
--- a/lldb/source/Core/PluginManager.cpp
+++ b/lldb/source/Core/PluginManager.cpp
@@ -800,6 +800,16 @@ Status PluginManager::SaveCore(const lldb::ProcessSP &process_sp,
return error;
}
+llvm::SmallVector<llvm::StringRef> PluginManager::GetSaveCorePluginNames() {
+ llvm::SmallVector<llvm::StringRef> plugin_names;
+ auto instances = GetObjectFileInstances().GetSnapshot();
+ for (auto &instance : instances) {
+ if (instance.save_core)
+ plugin_names.emplace_back(instance.name);
+ }
+ return plugin_names;
+}
+
#pragma mark ObjectContainer
struct ObjectContainerInstance
diff --git a/lldb/source/Symbol/SaveCoreOptions.cpp b/lldb/source/Symbol/SaveCoreOptions.cpp
index d884b00a47b00..c1c85571c8a78 100644
--- a/lldb/source/Symbol/SaveCoreOptions.cpp
+++ b/lldb/source/Symbol/SaveCoreOptions.cpp
@@ -22,8 +22,26 @@ Status SaveCoreOptions::SetPluginName(const char *name) {
}
if (!PluginManager::IsRegisteredObjectFilePluginName(name)) {
- return Status::FromErrorStringWithFormat(
- "plugin name '%s' is not a valid ObjectFile plugin name", name);
+ StreamString stream;
+ stream.Printf("plugin name '%s' is not a valid ObjectFile plugin name.",
+ name);
+
+ llvm::SmallVector<llvm::StringRef> plugin_names =
+ PluginManager::GetSaveCorePluginNames();
+ if (!plugin_names.empty()) {
+ stream.PutCString(" valid values are: ");
+ bool is_first = true;
+ for (llvm::StringRef plugin_name : plugin_names) {
+ llvm::StringRef delimiter = ", ";
+ if (is_first) {
+ delimiter = "";
+ is_first = false;
+ }
+ stream.Printf("%s\"%s\"", delimiter.data(), plugin_name.data());
+ }
+ stream.PutChar('.');
+ }
+ return Status(stream.GetString().str());
}
m_plugin_name = name;
|
|
Requested from the comments, I added the available plugins that can save a core file. |
To repeat a comment from the issue, this is what ninja does for example: So their help text is static but Now the cheapest way to do this is to say in the help: And this will be an error because there's no "list" plugin, but you get the list of valid plugins so it's the same result for the user. Or you can special case "list" such that it's not an error, the only difference is the return status and the first line of the output. Looking at this code, the point where you check the name is quite buried. If it were up in the process save-core command then it would be easier. I assume it's done this way because you can reach the code via the API as well, so I understand the decision. You could do a special check earlier in the command but it's a lot of effort for the same user experience. I'm happy to overlook that it's technically an error to list the valid values. So:
|
|
We can generate help text at runtime (e.g. ) but I'm not sure you'd have access to the pluginmanager from there.There's some stuff for tab completing plugin names, we could hook that up, but that feels like a nice to have. I'm not sure as a user I'd even think to try it anyway, something in the help/error message is much more discoverable. |
|
I overrode the |
|
It seems that the completion for optional arguments is currently not working, I tested this with other commands such as
llvm-project/lldb/source/Interpreter/Options.cpp Lines 1234 to 1240 in a08bf50 It first assumes that the, option argument will have a different index from the option. but it can only be --plugin-name=minidump # valid optional argument
--plugin-name minidump # minidump is treated as a new argument in the commands entered.
# same for short option
-pminidump # valid
-p minidump # not validThe other issue is that completions uses |
|
I did a little more digging, optional completion is not really supported anywhere.
command = "process save core --plugin-name=m"
parsed_args = ["process", "save-core", "--plugin-name=m"]
last_argument_prefix = "--plugin-name=m"
# we have to add the option prefix to the completion.
completion_returned = ["--plugin-name=minidump", "--plugin-name=mach-o"]
command = "process save-core --plugin-name=m"
parsed_args = ["process", "save-core", "--plugin-name", "m"] # this may collide with the normal args.
last_argument_prefix= "m"
competion_returned = ["minidump", "mach-o"] |
How about you remove the completion parts from this PR and tackle that topic in another. That way you can get something landed sooner. |
show the plugin names in when running `help plugin save-core`
da-viper
force-pushed
the
process-save-core-help
branch
from
14d4ff5 to
fc849bd
Compare
Dropped all completion related changes. |
lldb/test/API/functionalities/process_save_core/TestProcessSaveCore.py
Outdated
Show resolved
Hide resolved
|
Once my comments are addressed this will be good to land. Only concern was changing the default behaviour, and this PR does not go anywhere near that now. |
The |
I'm not sure anything other than minidump actually works, but technically that's not this patch's problem. Is it possible these plugins don't override some default member function that needs to return false? Or they can save cores but I'd be surprised if so. |
This is fine then. |
|
Just add the new tests and this will be good to go. Figure out whether the other plugins are lying later :) |
|
The mach-o plugin works but the pecoff is an alias to minidump llvm-project/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp Lines 6642 to 6670 in 4ec6d12 llvm-project/lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp Lines 358 to 365 in 4ec6d12 |
|
Ok so they aren't lying then. I suppose pecoff -> Windows and on Windows they use minidump among other things. |
| // Report only for the plugin that was specified. | ||
| if (!plugin_name.empty()) | ||
| return Status::FromErrorStringWithFormatv( | ||
| "The \"{}\" plugin is not able to save a core for this process.", | ||
| plugin_name); | ||
|
|
||
| return Status::FromErrorString( | ||
| "no ObjectFile plugins were able to save a core for this process"); | ||
| } |
There was a problem hiding this comment.
These two errors don't have test coverage.
"The "{}" plugin is not able to save a core for this process."
To hit this one we'd need a plugin that can save a core, but not for this process.
"no ObjectFile plugins were able to save a core for this process"
Same thing but there needs to be > 1 plugin that tried.
And the original error doesn't have coverage either, presumably because no one has the appetite to inject a problem into a process. Which is fair enough.
So yes these don't have coverage but it's not the end of the world if someone gets the wrong one so I'm not going to demand tests for them.
continuation of [llvm#142684](llvm#142684) to show plugin names. From issue [llvm#14258](llvm#142581)
continuation of [llvm#142684](llvm#142684) to show plugin names. From issue [llvm#14258](llvm#142581)
continuation of #142684 to show plugin names.
From issue #14258