[Lldb-commits] [PATCH] D72096: [lldb/Command] Add --force option for `watchpoint delete` command
This revision was automatically updated to reflect the committed changes.
mib marked an inline comment as done.
Closed by commit rGdf71f92fbb7c: [lldb/Command] Add --force option for
`watchpoint delete` command (authored by mib).
Changed prior to commit:
https://reviews.llvm.org/D72096?vs=236029=236159#toc
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D72096/new/
https://reviews.llvm.org/D72096
Files:
lldb/packages/Python/lldbsuite/test/commands/watchpoints/watchpoint_commands/TestWatchpointCommands.py
lldb/source/Commands/CommandObjectWatchpoint.cpp
lldb/source/Commands/Options.td
Index: lldb/source/Commands/Options.td
===
--- lldb/source/Commands/Options.td
+++ lldb/source/Commands/Options.td
@@ -1126,3 +1126,8 @@
"to run as command for this watchpoint. Be sure to give a module name if "
"appropriate.">;
}
+
+let Command = "watchpoint delete" in {
+ def watchpoint_delete_force : Option<"force", "f">, Group<1>,
+Desc<"Delete all watchpoints without querying for confirmation.">;
+}
Index: lldb/source/Commands/CommandObjectWatchpoint.cpp
===
--- lldb/source/Commands/CommandObjectWatchpoint.cpp
+++ lldb/source/Commands/CommandObjectWatchpoint.cpp
@@ -415,6 +415,10 @@
};
// CommandObjectWatchpointDelete
+#define LLDB_OPTIONS_watchpoint_delete
+#include "CommandOptions.inc"
+
+// CommandObjectWatchpointDelete
#pragma mark Delete
class CommandObjectWatchpointDelete : public CommandObjectParsed {
@@ -423,7 +427,8 @@
: CommandObjectParsed(interpreter, "watchpoint delete",
"Delete the specified watchpoint(s). If no "
"watchpoints are specified, delete them all.",
-nullptr, eCommandRequiresTarget) {
+nullptr, eCommandRequiresTarget),
+m_options() {
CommandArgumentEntry arg;
CommandObject::AddIDsArgumentData(arg, eArgTypeWatchpointID,
eArgTypeWatchpointIDRange);
@@ -434,6 +439,41 @@
~CommandObjectWatchpointDelete() override = default;
+ Options *GetOptions() override { return _options; }
+
+ class CommandOptions : public Options {
+ public:
+CommandOptions() : Options(), m_force(false) {}
+
+~CommandOptions() override = default;
+
+Status SetOptionValue(uint32_t option_idx, llvm::StringRef option_arg,
+ ExecutionContext *execution_context) override {
+ const int short_option = m_getopt_table[option_idx].val;
+
+ switch (short_option) {
+ case 'f':
+m_force = true;
+break;
+ default:
+llvm_unreachable("Unimplemented option");
+ }
+
+ return {};
+}
+
+void OptionParsingStarting(ExecutionContext *execution_context) override {
+ m_force = false;
+}
+
+llvm::ArrayRef GetDefinitions() override {
+ return llvm::makeArrayRef(g_watchpoint_delete_options);
+}
+
+// Instance variables to hold the values for command options.
+bool m_force;
+ };
+
protected:
bool DoExecute(Args , CommandReturnObject ) override {
Target *target = ();
@@ -453,8 +493,9 @@
return false;
}
-if (command.GetArgumentCount() == 0) {
- if (!m_interpreter.Confirm(
+if (command.empty()) {
+ if (!m_options.m_force &&
+ !m_interpreter.Confirm(
"About to delete all watchpoints, do you want to do that?",
true)) {
result.AppendMessage("Operation cancelled...");
@@ -465,27 +506,31 @@
(uint64_t)num_watchpoints);
}
result.SetStatus(eReturnStatusSuccessFinishNoResult);
-} else {
- // Particular watchpoints selected; delete them.
- std::vector wp_ids;
- if (!CommandObjectMultiwordWatchpoint::VerifyWatchpointIDs(
- target, command, wp_ids)) {
-result.AppendError("Invalid watchpoints specification.");
-result.SetStatus(eReturnStatusFailed);
-return false;
- }
+ return result.Succeeded();
+}
- int count = 0;
- const size_t size = wp_ids.size();
- for (size_t i = 0; i < size; ++i)
-if (target->RemoveWatchpointByID(wp_ids[i]))
- ++count;
- result.AppendMessageWithFormat("%d watchpoints deleted.\n", count);
- result.SetStatus(eReturnStatusSuccessFinishNoResult);
+// Particular watchpoints selected; delete them.
+std::vector wp_ids;
+if (!CommandObjectMultiwordWatchpoint::VerifyWatchpointIDs(target, command,
+ wp_ids)) {
+ result.AppendError("Invalid watchpoints specification.");
+ result.SetStatus(eReturnStatusFailed);
+ return false;
}
+int count = 0;
+const size_t size = wp_ids.size();
+for
[Lldb-commits] [PATCH] D72096: [lldb/Command] Add --force option for `watchpoint delete` command
mib marked an inline comment as done.
mib added inline comments.
Comment at:
lldb/packages/Python/lldbsuite/test/commands/watchpoints/watchpoint_commands/TestWatchpointCommands.py:194-196
+# Delete the watchpoint immediately using the force option.
+self.expect("watchpoint delete --force",
+substrs=['All watchpoints removed.'])
friss wrote:
> Can't we just add this to the end of another test without spinning up a new
> process?
>
> Did you verify that the test failed before your patch? I'm asking because I
> don't know what m_interpreter.Confirm() does when there's no PTY connected.
> Does it default to no or yes?
I merge both tests (`auto-confirm` enabled & `--force` flag) into one.
FWIU, m_interpreter.Confirm(message, default_answer) checks first the
auto-confirm setting, When enabled, it returns the default answer (true in this
case) otherwise, it triggers the IOHandler for the user input.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D72096/new/
https://reviews.llvm.org/D72096
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D72096: [lldb/Command] Add --force option for `watchpoint delete` command
mib updated this revision to Diff 236029.
mib marked an inline comment as done.
mib added a comment.
Merged `--force` flag and `auto-confirm` setting test.
Refactored implementation.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D72096/new/
https://reviews.llvm.org/D72096
Files:
lldb/packages/Python/lldbsuite/test/commands/watchpoints/watchpoint_commands/TestWatchpointCommands.py
lldb/source/Commands/CommandObjectWatchpoint.cpp
lldb/source/Commands/Options.td
Index: lldb/source/Commands/Options.td
===
--- lldb/source/Commands/Options.td
+++ lldb/source/Commands/Options.td
@@ -1126,3 +1126,8 @@
"to run as command for this watchpoint. Be sure to give a module name if "
"appropriate.">;
}
+
+let Command = "watchpoint delete" in {
+ def watchpoint_delete_force : Option<"force", "f">, Group<1>,
+Desc<"Delete all watchpoints without querying for confirmation.">;
+}
Index: lldb/source/Commands/CommandObjectWatchpoint.cpp
===
--- lldb/source/Commands/CommandObjectWatchpoint.cpp
+++ lldb/source/Commands/CommandObjectWatchpoint.cpp
@@ -414,6 +414,10 @@
}
};
+// CommandObjectWatchpointDelete
+#define LLDB_OPTIONS_watchpoint_delete
+#include "CommandOptions.inc"
+
// CommandObjectWatchpointDelete
#pragma mark Delete
@@ -423,7 +427,8 @@
: CommandObjectParsed(interpreter, "watchpoint delete",
"Delete the specified watchpoint(s). If no "
"watchpoints are specified, delete them all.",
-nullptr, eCommandRequiresTarget) {
+nullptr, eCommandRequiresTarget),
+m_options() {
CommandArgumentEntry arg;
CommandObject::AddIDsArgumentData(arg, eArgTypeWatchpointID,
eArgTypeWatchpointIDRange);
@@ -434,6 +439,41 @@
~CommandObjectWatchpointDelete() override = default;
+ Options *GetOptions() override { return _options; }
+
+ class CommandOptions : public Options {
+ public:
+CommandOptions() : Options(), m_force(false) {}
+
+~CommandOptions() override = default;
+
+Status SetOptionValue(uint32_t option_idx, llvm::StringRef option_arg,
+ ExecutionContext *execution_context) override {
+ const int short_option = m_getopt_table[option_idx].val;
+
+ switch (short_option) {
+ case 'f':
+m_force = true;
+break;
+ default:
+llvm_unreachable("Unimplemented option");
+ }
+
+ return {};
+}
+
+void OptionParsingStarting(ExecutionContext *execution_context) override {
+ m_force = false;
+}
+
+llvm::ArrayRef GetDefinitions() override {
+ return llvm::makeArrayRef(g_watchpoint_delete_options);
+}
+
+// Instance variables to hold the values for command options.
+bool m_force;
+ };
+
protected:
bool DoExecute(Args , CommandReturnObject ) override {
Target *target = ();
@@ -453,8 +493,9 @@
return false;
}
-if (command.GetArgumentCount() == 0) {
- if (!m_interpreter.Confirm(
+if (command.empty()) {
+ if (!m_options.m_force &&
+ !m_interpreter.Confirm(
"About to delete all watchpoints, do you want to do that?",
true)) {
result.AppendMessage("Operation cancelled...");
@@ -465,27 +506,31 @@
(uint64_t)num_watchpoints);
}
result.SetStatus(eReturnStatusSuccessFinishNoResult);
-} else {
- // Particular watchpoints selected; delete them.
- std::vector wp_ids;
- if (!CommandObjectMultiwordWatchpoint::VerifyWatchpointIDs(
- target, command, wp_ids)) {
-result.AppendError("Invalid watchpoints specification.");
-result.SetStatus(eReturnStatusFailed);
-return false;
- }
+ return result.Succeeded();
+}
- int count = 0;
- const size_t size = wp_ids.size();
- for (size_t i = 0; i < size; ++i)
-if (target->RemoveWatchpointByID(wp_ids[i]))
- ++count;
- result.AppendMessageWithFormat("%d watchpoints deleted.\n", count);
- result.SetStatus(eReturnStatusSuccessFinishNoResult);
+// Particular watchpoints selected; delete them.
+std::vector wp_ids;
+if (!CommandObjectMultiwordWatchpoint::VerifyWatchpointIDs(target, command,
+ wp_ids)) {
+ result.AppendError("Invalid watchpoints specification.");
+ result.SetStatus(eReturnStatusFailed);
+ return false;
}
+int count = 0;
+const size_t size = wp_ids.size();
+for (size_t i = 0; i < size; ++i)
+ if (target->RemoveWatchpointByID(wp_ids[i]))
+++count;
+result.AppendMessageWithFormat("%d watchpoints deleted.\n", count);
+
[Lldb-commits] [PATCH] D72096: [lldb/Command] Add --force option for `watchpoint delete` command
friss added inline comments.
Comment at:
lldb/packages/Python/lldbsuite/test/commands/watchpoints/watchpoint_commands/TestWatchpointCommands.py:162-179
+self.build(dictionary=self.d)
+self.setTearDownCleanup(dictionary=self.d)
+
+exe = self.getBuildArtifact(self.exe_name)
+self.runCmd("file " + exe, CURRENT_EXECUTABLE_SET)
+
+# Add a breakpoint to set a watchpoint when stopped on the breakpoint.
If you cannot reuse another test (see bellow), all the setup can be replaced by
`lldbutil.run_to_line_breakpoint` or `lldbutil.run_to_source_breakpoint`
Comment at:
lldb/packages/Python/lldbsuite/test/commands/watchpoints/watchpoint_commands/TestWatchpointCommands.py:194-196
+# Delete the watchpoint immediately using the force option.
+self.expect("watchpoint delete --force",
+substrs=['All watchpoints removed.'])
Can't we just add this to the end of another test without spinning up a new
process?
Did you verify that the test failed before your patch? I'm asking because I
don't know what m_interpreter.Confirm() does when there's no PTY connected.
Does it default to no or yes?
Comment at:
lldb/packages/Python/lldbsuite/test/commands/watchpoints/watchpoint_commands/TestWatchpointCommands.py:199
+# Use the '-v' option to do verbose listing of the watchpoint.
+self.runCmd("watchpoint list -v")
+
You don't test the result of this command, so you don't actually test that
deleting the breakpoints really happened. Is there an SB API to list
watchpoints? If there is, it would be a better fit for this test instead of
matching the output of a command.
Comment at:
lldb/packages/Python/lldbsuite/test/commands/watchpoints/watchpoint_commands/TestWatchpointCommands.py:203-206
+# There should be no more watchpoint hit and the process status should
+# be 'exited'.
+self.expect("process status",
+substrs=['exited'])
I see, this is the actual test that no watchpoints are present. I'm fine with
adding this new test, I think testing it this way makes sense, but please also
find a way to make sure the list of watchpoints is empty.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D72096/new/
https://reviews.llvm.org/D72096
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D72096: [lldb/Command] Add --force option for `watchpoint delete` command
JDevlieghere accepted this revision as: JDevlieghere.
JDevlieghere added a comment.
This revision is now accepted and ready to land.
LGTM with the inline comment(s) addressed.
Comment at: lldb/source/Commands/CommandObjectWatchpoint.cpp:463
+
+ return error;
+}
Given that error is never actually populated, it might be nice to use `return
{}` to make it clear that we're returning a default constructed instance.
Comment at: lldb/source/Commands/CommandObjectWatchpoint.cpp:529
return result.Succeeded();
}
I know it's not code you touched but moving this into the first if-clause would
allow for an early return.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D72096/new/
https://reviews.llvm.org/D72096
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D72096: [lldb/Command] Add --force option for `watchpoint delete` command
mib created this revision.
mib added reviewers: LLDB, teemperor.
mib added a project: LLDB.
Herald added a subscriber: lldb-commits.
[lldb/Command] Add --force option for `watchpoint delete` command
Currently, there is no option to delete all the watchpoint without LLDB
asking for a confirmation. Besides making the watchpoint delete command
homogeneous with the breakpoint delete command, this option could also
become handy to trigger automated watchpoint deletion i.e. using
breakpoint actions.
rdar://42560586
Signed-off-by: Med Ismail Bennani
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D72096
Files:
lldb/packages/Python/lldbsuite/test/commands/watchpoints/watchpoint_commands/TestWatchpointCommands.py
lldb/source/Commands/CommandObjectWatchpoint.cpp
lldb/source/Commands/Options.td
Index: lldb/source/Commands/Options.td
===
--- lldb/source/Commands/Options.td
+++ lldb/source/Commands/Options.td
@@ -1126,3 +1126,8 @@
"to run as command for this watchpoint. Be sure to give a module name if "
"appropriate.">;
}
+
+let Command = "watchpoint delete" in {
+ def watchpoint_delete_force : Option<"force", "f">, Group<1>,
+Desc<"Delete all watchpoints without querying for confirmation.">;
+}
Index: lldb/source/Commands/CommandObjectWatchpoint.cpp
===
--- lldb/source/Commands/CommandObjectWatchpoint.cpp
+++ lldb/source/Commands/CommandObjectWatchpoint.cpp
@@ -414,6 +414,10 @@
}
};
+// CommandObjectWatchpointDelete
+#define LLDB_OPTIONS_watchpoint_delete
+#include "CommandOptions.inc"
+
// CommandObjectWatchpointDelete
#pragma mark Delete
@@ -423,7 +427,8 @@
: CommandObjectParsed(interpreter, "watchpoint delete",
"Delete the specified watchpoint(s). If no "
"watchpoints are specified, delete them all.",
-nullptr, eCommandRequiresTarget) {
+nullptr, eCommandRequiresTarget),
+m_options() {
CommandArgumentEntry arg;
CommandObject::AddIDsArgumentData(arg, eArgTypeWatchpointID,
eArgTypeWatchpointIDRange);
@@ -434,6 +439,42 @@
~CommandObjectWatchpointDelete() override = default;
+ Options *GetOptions() override { return _options; }
+
+ class CommandOptions : public Options {
+ public:
+CommandOptions() : Options(), m_force(false) {}
+
+~CommandOptions() override = default;
+
+Status SetOptionValue(uint32_t option_idx, llvm::StringRef option_arg,
+ ExecutionContext *execution_context) override {
+ Status error;
+ const int short_option = m_getopt_table[option_idx].val;
+
+ switch (short_option) {
+ case 'f':
+m_force = true;
+break;
+ default:
+llvm_unreachable("Unimplemented option");
+ }
+
+ return error;
+}
+
+void OptionParsingStarting(ExecutionContext *execution_context) override {
+ m_force = false;
+}
+
+llvm::ArrayRef GetDefinitions() override {
+ return llvm::makeArrayRef(g_watchpoint_delete_options);
+}
+
+// Instance variables to hold the values for command options.
+bool m_force;
+ };
+
protected:
bool DoExecute(Args , CommandReturnObject ) override {
Target *target = ();
@@ -453,8 +494,9 @@
return false;
}
-if (command.GetArgumentCount() == 0) {
- if (!m_interpreter.Confirm(
+if (command.empty()) {
+ if (!m_options.m_force &&
+ !m_interpreter.Confirm(
"About to delete all watchpoints, do you want to do that?",
true)) {
result.AppendMessage("Operation cancelled...");
@@ -486,6 +528,9 @@
return result.Succeeded();
}
+
+private:
+ CommandOptions m_options;
};
// CommandObjectWatchpointIgnore
Index: lldb/packages/Python/lldbsuite/test/commands/watchpoints/watchpoint_commands/TestWatchpointCommands.py
===
--- lldb/packages/Python/lldbsuite/test/commands/watchpoints/watchpoint_commands/TestWatchpointCommands.py
+++ lldb/packages/Python/lldbsuite/test/commands/watchpoints/watchpoint_commands/TestWatchpointCommands.py
@@ -155,6 +155,56 @@
self.expect("process status",
substrs=['exited'])
+# Read-write watchpoints not supported on SystemZ
+@expectedFailureAll(archs=['s390x'])
+def test_rw_watchpoint_delete(self):
+"""Test delete watchpoint and expect not to stop for watchpoint."""
+self.build(dictionary=self.d)
+self.setTearDownCleanup(dictionary=self.d)
+
+exe = self.getBuildArtifact(self.exe_name)
+self.runCmd("file " + exe, CURRENT_EXECUTABLE_SET)
+
+# Add a breakpoint to set a watchpoint when stopped on the breakpoint.
+
