Page MenuHomePhabricator

[lldb/Command] Add --force option for `watchpoint delete` command
ClosedPublic

Authored by mib on Jan 2 2020, 10:48 AM.

Details

Summary

[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 <medismail.bennani@gmail.com>

Diff Detail

Event Timeline

mib created this revision.Jan 2 2020, 10:48 AM

LGTM with the inline comment(s) addressed.

lldb/source/Commands/CommandObjectWatchpoint.cpp
463

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.

529

I know it's not code you touched but moving this into the first if-clause would allow for an early return.

This revision is now accepted and ready to land.Jan 2 2020, 10:58 AM
friss added a subscriber: friss.Jan 2 2020, 11:08 AM
friss added inline comments.
lldb/packages/Python/lldbsuite/test/commands/watchpoints/watchpoint_commands/TestWatchpointCommands.py
162–179

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

194–196

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?

199

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.

203–206

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.

mib updated this revision to Diff 236029.Jan 3 2020, 5:26 AM
mib marked an inline comment as done.

Merged --force flag and auto-confirm setting test.
Refactored implementation.

mib marked 4 inline comments as done.Jan 3 2020, 5:29 AM
mib marked an inline comment as done.Jan 3 2020, 5:38 AM
mib added inline comments.
lldb/packages/Python/lldbsuite/test/commands/watchpoints/watchpoint_commands/TestWatchpointCommands.py
194–196

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.

friss accepted this revision.Jan 3 2020, 9:27 AM
JDevlieghere accepted this revision.Jan 3 2020, 9:29 AM
This revision was automatically updated to reflect the committed changes.
mib marked an inline comment as done.