[lldb] Standardize command option parsing error messages #82273
Conversation
bulbazord
requested review from
adrian-prantl,
clayborg,
felipepiovezan,
medismailben and
jimingham
|
@llvm/pr-subscribers-lldb Author: Alex Langford (bulbazord) ChangesI have been looking to simplify parsing logic and improve the interfaces so that they are both easier to use and harder to abuse. To be specific, I am referring to functions such as Through working on that, I encountered 2 inconveniences:
To address these frustrations, I think it would be easiest to first standardize the error reporting mechanism when parsing options in commands. I do so by introducing Concretely, it would look something like this: After this, updating the interfaces for parsing the values themselves should become simpler. Because this can be adopted incrementally, this should be able to done over the course of time instead of all at once as a giant difficult-to-review change. I've changed exactly one function where this function would be used as an illustration of what I am proposing. Full diff: https://github.com/llvm/llvm-project/pull/82273.diff 5 Files Affected:
diff --git a/lldb/include/lldb/Interpreter/Options.h b/lldb/include/lldb/Interpreter/Options.h
index bf74927cf99db8..3351fb517d4df3 100644
--- a/lldb/include/lldb/Interpreter/Options.h
+++ b/lldb/include/lldb/Interpreter/Options.h
@@ -336,6 +336,16 @@ class OptionGroupOptions : public Options {
bool m_did_finalize = false;
};
+llvm::Error CreateOptionParsingError(llvm::StringRef option_arg,
+ const char short_option,
+ llvm::StringRef long_option = {},
+ llvm::StringRef additional_context = {});
+
+static constexpr llvm::StringLiteral g_bool_parsing_error_message =
+ "Failed to parse as boolean";
+static constexpr llvm::StringLiteral g_int_parsing_error_message =
+ "Failed to parse as integer";
+
} // namespace lldb_private
#endif // LLDB_INTERPRETER_OPTIONS_H
diff --git a/lldb/source/Commands/CommandObjectBreakpoint.cpp b/lldb/source/Commands/CommandObjectBreakpoint.cpp
index 3fdf5cd3cd43d2..fc2217608a0bb9 100644
--- a/lldb/source/Commands/CommandObjectBreakpoint.cpp
+++ b/lldb/source/Commands/CommandObjectBreakpoint.cpp
@@ -64,6 +64,8 @@ class lldb_private::BreakpointOptionGroup : public OptionGroup {
Status error;
const int short_option =
g_breakpoint_modify_options[option_idx].short_option;
+ const char *long_option =
+ g_breakpoint_modify_options[option_idx].long_option;
switch (short_option) {
case 'c':
@@ -84,18 +86,17 @@ class lldb_private::BreakpointOptionGroup : public OptionGroup {
case 'G': {
bool value, success;
value = OptionArgParser::ToBoolean(option_arg, false, &success);
- if (success) {
+ if (success)
m_bp_opts.SetAutoContinue(value);
- } else
- error.SetErrorStringWithFormat(
- "invalid boolean value '%s' passed for -G option",
- option_arg.str().c_str());
+ else
+ error = CreateOptionParsingError(option_arg, short_option, long_option,
+ g_bool_parsing_error_message);
} break;
case 'i': {
uint32_t ignore_count;
if (option_arg.getAsInteger(0, ignore_count))
- error.SetErrorStringWithFormat("invalid ignore count '%s'",
- option_arg.str().c_str());
+ error = CreateOptionParsingError(option_arg, short_option, long_option,
+ g_int_parsing_error_message);
else
m_bp_opts.SetIgnoreCount(ignore_count);
} break;
@@ -105,27 +106,29 @@ class lldb_private::BreakpointOptionGroup : public OptionGroup {
if (success) {
m_bp_opts.SetOneShot(value);
} else
- error.SetErrorStringWithFormat(
- "invalid boolean value '%s' passed for -o option",
- option_arg.str().c_str());
+ error = CreateOptionParsingError(option_arg, short_option, long_option,
+ g_bool_parsing_error_message);
} break;
case 't': {
lldb::tid_t thread_id = LLDB_INVALID_THREAD_ID;
if (option_arg == "current") {
if (!execution_context) {
- error.SetErrorStringWithFormat("No context to determine current "
- "thread");
+ error = CreateOptionParsingError(
+ option_arg, short_option, long_option,
+ "No context to determine current thread");
} else {
ThreadSP ctx_thread_sp = execution_context->GetThreadSP();
if (!ctx_thread_sp || !ctx_thread_sp->IsValid()) {
- error.SetErrorStringWithFormat("No currently selected thread");
+ error =
+ CreateOptionParsingError(option_arg, short_option, long_option,
+ "No currently selected thread");
} else {
thread_id = ctx_thread_sp->GetID();
}
}
} else if (option_arg.getAsInteger(0, thread_id)) {
- error.SetErrorStringWithFormat("invalid thread id string '%s'",
- option_arg.str().c_str());
+ error = CreateOptionParsingError(option_arg, short_option, long_option,
+ g_int_parsing_error_message);
}
if (thread_id != LLDB_INVALID_THREAD_ID)
m_bp_opts.SetThreadID(thread_id);
@@ -139,8 +142,8 @@ class lldb_private::BreakpointOptionGroup : public OptionGroup {
case 'x': {
uint32_t thread_index = UINT32_MAX;
if (option_arg.getAsInteger(0, thread_index)) {
- error.SetErrorStringWithFormat("invalid thread index string '%s'",
- option_arg.str().c_str());
+ error = CreateOptionParsingError(option_arg, short_option, long_option,
+ g_int_parsing_error_message);
} else {
m_bp_opts.GetThreadSpec()->SetIndex(thread_index);
}
diff --git a/lldb/source/Interpreter/Options.cpp b/lldb/source/Interpreter/Options.cpp
index 89fe69009d9036..51b7e6b26b6efa 100644
--- a/lldb/source/Interpreter/Options.cpp
+++ b/lldb/source/Interpreter/Options.cpp
@@ -1365,3 +1365,16 @@ llvm::Expected<Args> Options::Parse(const Args &args,
argv.erase(argv.begin(), argv.begin() + OptionParser::GetOptionIndex());
return ReconstituteArgsAfterParsing(argv, args);
}
+
+llvm::Error lldb_private::CreateOptionParsingError(
+ llvm::StringRef option_arg, const char short_option,
+ llvm::StringRef long_option, llvm::StringRef additional_context) {
+ std::string buffer;
+ llvm::raw_string_ostream stream(buffer);
+ stream << "Invalid value ('" << option_arg << "') for -" << short_option;
+ if (!long_option.empty())
+ stream << " (" << long_option << ")";
+ if (!additional_context.empty())
+ stream << ": " << additional_context;
+ return llvm::createStringError(llvm::inconvertibleErrorCode(), buffer);
+}
diff --git a/lldb/unittests/Interpreter/CMakeLists.txt b/lldb/unittests/Interpreter/CMakeLists.txt
index 5b5268ffe97321..54cea995084d3d 100644
--- a/lldb/unittests/Interpreter/CMakeLists.txt
+++ b/lldb/unittests/Interpreter/CMakeLists.txt
@@ -2,6 +2,7 @@ add_lldb_unittest(InterpreterTests
TestCommandPaths.cpp
TestCompletion.cpp
TestOptionArgParser.cpp
+ TestOptions.cpp
TestOptionValue.cpp
TestOptionValueFileColonLine.cpp
TestRegexCommand.cpp
diff --git a/lldb/unittests/Interpreter/TestOptions.cpp b/lldb/unittests/Interpreter/TestOptions.cpp
new file mode 100644
index 00000000000000..93474e3c5713c3
--- /dev/null
+++ b/lldb/unittests/Interpreter/TestOptions.cpp
@@ -0,0 +1,29 @@
+//===-- TestOptions.cpp ---------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "lldb/Interpreter/Options.h"
+#include "gtest/gtest.h"
+
+#include "llvm/Testing/Support/Error.h"
+
+using namespace lldb_private;
+
+TEST(OptionsTest, CreateOptionParsingError) {
+ ASSERT_THAT_ERROR(
+ CreateOptionParsingError("yippee", 'f', "fun",
+ "unable to convert 'yippee' to boolean"),
+ llvm::FailedWithMessage("Invalid value ('yippee') for -f (fun): unable "
+ "to convert 'yippee' to boolean"));
+
+ ASSERT_THAT_ERROR(
+ CreateOptionParsingError("52", 'b', "bean-count"),
+ llvm::FailedWithMessage("Invalid value ('52') for -b (bean-count)"));
+
+ ASSERT_THAT_ERROR(CreateOptionParsingError("c", 'm'),
+ llvm::FailedWithMessage("Invalid value ('c') for -m"));
+}
|
|
This is a good direction. The API for creating the error message seems fine, a known part and a "site-specific error" is a workable division w/o being over designed. The one thing that bugs me about this whole setup is that we know the argument type for each option, so even more of the stuff we're doing by hand here should be automated. We should know the That's a bigger problem than you are addressing now. The reason it comes up at all with this patch is that you also shouldn't need to add the g_bool_parsing_error_message context by hand, we should know that from the option type. I thought about that when I saw the code:
I was first thinking "we should put an assert there, it shouldn't be possible to have an option parse fail and just say:
In the end I think this is fine. As you work more on this it might be nice to add some kind of trap to catch people who don't say why a value is wrong. And far down the road, the Option error reporting could provide a useful fallback for common option kinds like bool, int, etc. But that can all be added. This is a fine way to get started. |
|
Actually, I'm going to leave this as approved, but please add a header doc comment on your new API when you commit. Preferably with a stirring exhortation for people to use this in new code. |
There was a problem hiding this comment.
This LGTM!
I don't think I can see far enough ahead on what you are planning here, but I was just wondering if the ultimate goal is to have the option_arg.getAsT return an Expected<T>. In this case, wouldn't all these arguments (short option, long option, additional context) have to be part of the interface of getAsT? I suspect this is not your goal, but I can't see a way around that
The ultimate goal is to have option parsing be much more streamlined and automated. The majority of option parsing is just taking some primitive value and trying to grab its value from a string. There are places where we need additional context (e.g. in this PR, there's a place where we want to select a thread ID so there's more validation needed). I want to be able to write those pretty naturally too. The first step in my plan is to centralize how we create error messages. The majority of parsing errors will be "I couldn't turn this into a boolean/integer/address/other", so I'm going to create something that is better than what we have today. The step after that will be to revamp the parsing step. Ideally instead of writing |
Ohhh I think I see it now!
And the |
I would like it to be that way. Let's see if it works out! 😄 |
I have been looking to simplify parsing logic and improve the interfaces
so that they are both easier to use and harder to abuse. To be specific,
I am referring to functions such as `OptionArgParser::ToBoolean`: I
would like to go from its current interface to something more like
`llvm::Error<bool> ToBoolean(llvm::StringRef option_arg)`.
Through working on that, I encountered 2 inconveniences:
1. Option parsing code is not uniform. Every function writes a slightly
different error message, so incorporating an error message from the
`ToBoolean` implementation is going to be laborious as I figure out
what exactly needs to change or stay the same.
2. Changing the interface of `ToBoolean` would require a global atomic
change across all of the Command code. This would be quite
frustrating to do because of the non-uniformity of our existing code.
To address these frustrations, I think it would be easiest to first
standardize the error reporting mechanism when parsing options in
commands. I do so by introducing `CreateOptionParsingError` which will
create an error message of the shape:
Invalid valud ('${option_arg}') for -${short_value} ('${long_value}'): ${additional_context}
Concretely, it would look something like this:
(lldb) breakpoint set -n main -G yay
error: Invalid value ('yay') for -G (auto-continue): Failed to parse as boolean
After this, updating the interfaces for parsing the values themselves
should become simpler. Because this can be adopted incrementally, this
should be able to done over the course of time instead of all at once as
a giant difficult-to-review change. I've changed exactly one function
where this function would be used as an illustration of what I am
proposing.
bulbazord
force-pushed
the
create-option-parsing-error
branch
from
062ec05 to
c8d8d96
Compare
|
I've added a doxygen comment to the new function I introduced. I plan on landing this later today if there are no objections. |
I have been looking to simplify parsing logic and improve the interfaces so that they are both easier to use and harder to abuse. To be specific, I am referring to functions such as
OptionArgParser::ToBoolean: I would like to go from its current interface to something more likellvm::Error<bool> ToBoolean(llvm::StringRef option_arg).Through working on that, I encountered 2 inconveniences:
ToBooleanimplementation is going to be laborious as I figure out what exactly needs to change or stay the same.ToBooleanwould require a global atomic change across all of the Command code. This would be quite frustrating to do because of the non-uniformity of our existing code.To address these frustrations, I think it would be easiest to first standardize the error reporting mechanism when parsing options in commands. I do so by introducing
CreateOptionParsingErrorwhich will create an error message of the shape:Invalid valud ('${option_arg}') for -${short_value} ('${long_value}'): ${additional_context}
Concretely, it would look something like this:
(lldb) breakpoint set -n main -G yay
error: Invalid value ('yay') for -G (auto-continue): Failed to parse as boolean
After this, updating the interfaces for parsing the values themselves should become simpler. Because this can be adopted incrementally, this should be able to done over the course of time instead of all at once as a giant difficult-to-review change. I've changed exactly one function where this function would be used as an illustration of what I am proposing.