Skip to content

[lldb][NFCI] Remove CommandObjectProcessHandle::VerifyCommandOptionValue #79901

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Feb 14, 2024
Merged

[lldb][NFCI] Remove CommandObjectProcessHandle::VerifyCommandOptionValue #79901

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3cd6afa
[lldb][NFCI] Minor refactor to CommandObjectProcessHandle::VerifyComm…
bulbazord Jan 29, 2024
a93faa9
Formatting
bulbazord Jan 29, 2024
8e78455
[lldb] Remove VerifyCommandOptionValue entirely
bulbazord Jan 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
126 changes: 59 additions & 67 deletions lldb/source/Commands/CommandObjectProcess.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1591,26 +1591,6 @@ class CommandObjectProcessHandle : public CommandObjectParsed {

Options *GetOptions() override { return &m_options; }

bool VerifyCommandOptionValue(const std::string &option, int &real_value) {
bool okay = true;
bool success = false;
bool tmp_value = OptionArgParser::ToBoolean(option, false, &success);

if (success && tmp_value)
real_value = 1;
else if (success && !tmp_value)
real_value = 0;
else {
// If the value isn't 'true' or 'false', it had better be 0 or 1.
if (!llvm::to_integer(option, real_value))
real_value = 3;
if (real_value != 0 && real_value != 1)
okay = false;
}

return okay;
}

void PrintSignalHeader(Stream &str) {
str.Printf("NAME PASS STOP NOTIFY\n");
str.Printf("=========== ===== ===== ======\n");
Expand Down Expand Up @@ -1666,33 +1646,52 @@ class CommandObjectProcessHandle : public CommandObjectParsed {
// the user's options.
ProcessSP process_sp = target.GetProcessSP();

int stop_action = -1; // -1 means leave the current setting alone
int pass_action = -1; // -1 means leave the current setting alone
int notify_action = -1; // -1 means leave the current setting alone
std::optional<bool> stop_action = {};
std::optional<bool> pass_action = {};
std::optional<bool> notify_action = {};

if (!m_options.stop.empty() &&
!VerifyCommandOptionValue(m_options.stop, stop_action)) {
result.AppendError("Invalid argument for command option --stop; must be "
"true or false.\n");
return;
if (!m_options.stop.empty()) {
bool success = false;
bool value = OptionArgParser::ToBoolean(m_options.stop, false, &success);
Comment on lines +1653 to +1655
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why don't we do this error checking in CommandObjectProcessHandle::CommandOptions::SetOptionValue(...)? We can return an error from there instead of making it successfully though option parsing only to look closer at the option values and return an error later in this function?

Also, I see many places use this version of OptionArgParser::ToBoolean(...), and there is a ton of duplicated code like:

  • call OptionArgParser::ToBoolean(...)
  • check value of success
  • make error string and everyone picks their own "hey this isn't a valid boolean value (like Jim already mentioned below).

Since there are already so many uses of the current and only version of OptionArgParser::ToBoolean(...), maybe we make an overload of this:

llvm::Expected<bool> OptionArgParser::ToBoolean(llvm::StringRef ref);

Then in the `CommandObjectProcessHandle::CommandOptions::SetOptionValue(...) function we can do:

case 's':
  if (auto ExpectedBool = OptionArgParser::ToBoolean(option_arg)) {
    stop = *ExpectedBool;
  else
    error = Status(ExpectedBool.takeError());

And the new OptionArgParser::ToBoolean() overload would return an error as Jim wants below:

"Invalid <boolean> value "%s"

Where %s gets substitued with "option_arg.str().c_str()". And then the help would help people figure this out.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My plan (that I wrote up 2 weeks ago) was to remove VerifyCommandOptionValue entirely in this commit and then refactor ToBoolean the way you suggested as a follow-up to this change. Does that sound good to you?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is fine.

if (!success) {
result.AppendError(
Copy link
Collaborator

@jimingham jimingham Jan 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is up to you, since the old code also lied. But this error is not true, the values can also be yes, no, 1, 0. Really should be: should be a <boolean> value, and then all we have to do is teach help <boolean> not to lie:
(lldb) help <boolean> <boolean> -- A Boolean value: 'true' or 'false'

It might be nice if these OptionArgParser::ToWhatever functions could help produce the error string, it's awkward that everybody has to make up their own version of the "what is right" string.

"Invalid argument for command option --stop; must be "
"true or false.\n");
return;
}

stop_action = value;
}

if (!m_options.notify.empty() &&
!VerifyCommandOptionValue(m_options.notify, notify_action)) {
result.AppendError("Invalid argument for command option --notify; must "
"be true or false.\n");
return;
if (!m_options.pass.empty()) {
bool success = false;
bool value = OptionArgParser::ToBoolean(m_options.pass, false, &success);
if (!success) {
result.AppendError(
"Invalid argument for command option --pass; must be "
"true or false.\n");
return;
}
pass_action = value;
}

if (!m_options.pass.empty() &&
!VerifyCommandOptionValue(m_options.pass, pass_action)) {
result.AppendError("Invalid argument for command option --pass; must be "
"true or false.\n");
return;
if (!m_options.notify.empty()) {
bool success = false;
bool value =
OptionArgParser::ToBoolean(m_options.notify, false, &success);
if (!success) {
result.AppendError("Invalid argument for command option --notify; must "
"be true or false.\n");
return;
}
notify_action = value;
}

if (!m_options.notify.empty() && !notify_action.has_value()) {
}

bool no_actions = (stop_action == -1 && pass_action == -1
&& notify_action == -1);
bool no_actions = (!stop_action.has_value() && !pass_action.has_value() &&
!notify_action.has_value());
if (m_options.only_target_values && !no_actions) {
result.AppendError("-t is for reporting, not setting, target values.");
return;
Expand Down Expand Up @@ -1730,16 +1729,14 @@ class CommandObjectProcessHandle : public CommandObjectParsed {
if (signals_sp) {
int32_t signo = signals_sp->GetSignalNumberFromName(arg.c_str());
if (signo != LLDB_INVALID_SIGNAL_NUMBER) {
// Casting the actions as bools here should be okay, because
// VerifyCommandOptionValue guarantees the value is either 0 or 1.
if (stop_action != -1)
signals_sp->SetShouldStop(signo, stop_action);
if (pass_action != -1) {
bool suppress = !pass_action;
if (stop_action.has_value())
signals_sp->SetShouldStop(signo, *stop_action);
if (pass_action.has_value()) {
bool suppress = !*pass_action;
signals_sp->SetShouldSuppress(signo, suppress);
}
if (notify_action != -1)
signals_sp->SetShouldNotify(signo, notify_action);
if (notify_action.has_value())
signals_sp->SetShouldNotify(signo, *notify_action);
++num_signals_set;
} else {
result.AppendErrorWithFormat("Invalid signal name '%s'\n",
Expand All @@ -1759,40 +1756,35 @@ class CommandObjectProcessHandle : public CommandObjectParsed {
}
num_signals_set = num_args;
}
auto set_lazy_bool = [] (int action) -> LazyBool {
LazyBool lazy;
if (action == -1)
lazy = eLazyBoolCalculate;
else if (action)
lazy = eLazyBoolYes;
else
lazy = eLazyBoolNo;
return lazy;
auto set_lazy_bool = [](std::optional<bool> action) -> LazyBool {
if (!action.has_value())
return eLazyBoolCalculate;
return (*action) ? eLazyBoolYes : eLazyBoolNo;
};

// If there were no actions, we're just listing, don't add the dummy:
if (!no_actions)
target.AddDummySignal(arg.ref(),
set_lazy_bool(pass_action),
target.AddDummySignal(arg.ref(), set_lazy_bool(pass_action),
set_lazy_bool(notify_action),
set_lazy_bool(stop_action));
}
} else {
// No signal specified, if any command options were specified, update ALL
// signals. But we can't do this without a process since we don't know
// all the possible signals that might be valid for this target.
if (((notify_action != -1) || (stop_action != -1) || (pass_action != -1))
&& process_sp) {
if ((notify_action.has_value() || stop_action.has_value() ||
pass_action.has_value()) &&
process_sp) {
if (m_interpreter.Confirm(
"Do you really want to update all the signals?", false)) {
int32_t signo = signals_sp->GetFirstSignalNumber();
while (signo != LLDB_INVALID_SIGNAL_NUMBER) {
if (notify_action != -1)
signals_sp->SetShouldNotify(signo, notify_action);
if (stop_action != -1)
signals_sp->SetShouldStop(signo, stop_action);
if (pass_action != -1) {
bool suppress = !pass_action;
if (notify_action.has_value())
signals_sp->SetShouldNotify(signo, *notify_action);
if (stop_action.has_value())
signals_sp->SetShouldStop(signo, *stop_action);
if (pass_action.has_value()) {
bool suppress = !*pass_action;
signals_sp->SetShouldSuppress(signo, suppress);
}
signo = signals_sp->GetNextSignalNumber(signo);
Expand Down