[Lldb-commits] [lldb] [lldb] Add 'modify' type watchpoints, make it default (PR #66308)
via lldb-commits
lldb-commits at lists.llvm.org
Wed Sep 13 18:19:34 PDT 2023
llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-lldb
<details>
<summary>Changes</summary>
Watchpoints in lldb can be either 'read', 'write', or 'read/write'. This is exposing the actual behavior of hardware watchpoints. gdb has a different behavior: a "write" type watchpoint only stops when the watched memory region *changes*.
A user is using a watchpoint for one of three reasons:
1. Want to find what is changing/corrupting this memory.
3. Want to find what is reading from this memory.
2. Want to find what is writing to this memory.
I believe (1) is the most common use case for watchpoints, and it currently can't be done in lldb -- the user needs to continue every time the same value is written to the watched-memory manually. I think gdb's behavior is the correct one. There are some use cases where a developer wants to find every function that writes/reads to/from a memory region, regardless of value, I want to still allow that functionality.
This is also a bit of groundwork for my large watchpoint support proposal
https://discourse.llvm.org/t/rfc-large-watchpoint-support-in-lldb/72116 where I will be adding support for AArch64 MASK watchpoints which watch power-of-2 memory regions. A user might ask to watch 24 bytes, and a MASK watchpoint stub can do this with a 32-byte MASK watchpoint if it is properly aligned. And we need to ignore writes to the final 8 bytes of that watched region, and not show those hits to the user.
This patch adds a new 'modify' watchpoint type and it is the default.
The part of this patch I think is the most questionable is the
SBTarget::CreateWatchpoint(addr, size, bool read, bool write, error) which I currently change the meaning of write==True to be Modify. This is part of making 'modify' the default watchpoint type - a driver is likely setting write==True for its watchpoints.
I chatted with Alex and Jim about this a little and I'm not sure how I should solve this for real. Add a new SBTarget::CreateWatchpoint API with a third `bool modify` argument in addition to the old one? Add a new SBWatchpointOptions class in case we want to add more types of watchpoints? I'm not sure what other types of watchpoints we'd want to hardcode in our support. We have conditional expressions or python callbacks that can do more unique operations.
There's more work here - the SB API needs to give the driver a way to specify all three possible types of watchpoint ('write' and 'modify' being exclusive of the other) - but I'd love to hear what other people think we should do for this API especially.
--
Full diff: https://github.com/llvm/llvm-project/pull/66308.diff
14 Files Affected:
- (modified) lldb/include/lldb/Breakpoint/Watchpoint.h (+4-1)
- (modified) lldb/include/lldb/Interpreter/OptionGroupWatchpoint.h (+4-2)
- (modified) lldb/include/lldb/lldb-defines.h (+3-1)
- (modified) lldb/source/API/SBTarget.cpp (+1-1)
- (modified) lldb/source/API/SBValue.cpp (+1-1)
- (modified) lldb/source/Breakpoint/Watchpoint.cpp (+39-4)
- (modified) lldb/source/Commands/CommandObjectWatchpoint.cpp (+23-7)
- (modified) lldb/source/Interpreter/OptionGroupWatchpoint.cpp (+7-2)
- (modified) lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp (+5-4)
- (modified) lldb/source/Target/StopInfo.cpp (+7)
- (modified) lldb/source/Target/Target.cpp (+2-1)
- (added) lldb/test/API/functionalities/watchpoint/modify-watchpoints/Makefile (+3)
- (added) lldb/test/API/functionalities/watchpoint/modify-watchpoints/TestModifyWatchpoint.py (+27)
- (added) lldb/test/API/functionalities/watchpoint/modify-watchpoints/main.c (+17)
<pre>
diff --git a/lldb/include/lldb/Breakpoint/Watchpoint.h b/lldb/include/lldb/Breakpoint/Watchpoint.h
index 8fde3b563a3f064..c86d66663c7f8c0 100644
--- a/lldb/include/lldb/Breakpoint/Watchpoint.h
+++ b/lldb/include/lldb/Breakpoint/Watchpoint.h
@@ -76,12 +76,14 @@ class Watchpoint : public std::enable_shared_from_this<Watchpoint>,
bool WatchpointRead() const;
bool WatchpointWrite() const;
+ bool WatchpointModify() const;
uint32_t GetIgnoreCount() const;
void SetIgnoreCount(uint32_t n);
void SetWatchpointType(uint32_t type, bool notify = true);
void SetDeclInfo(const std::string &str);
std::string GetWatchSpec();
void SetWatchSpec(const std::string &str);
+ bool WatchedValueReportable(const ExecutionContext &exe_ctx);
// Snapshot management interface.
bool IsWatchVariable() const;
@@ -212,7 +214,8 @@ class Watchpoint : public std::enable_shared_from_this<Watchpoint>,
// again, we check the count, if it is more than 1, it means the user-
// supplied actions actually want the watchpoint to be disabled!
uint32_t m_watch_read : 1, // 1 if we stop when the watched data is read from
- m_watch_write : 1; // 1 if we stop when the watched data is written to
+ m_watch_write : 1, // 1 if we stop when the watched data is written to
+ m_watch_modify : 1; // 1 if we stop when the watched data is changed
uint32_t m_ignore_count; // Number of times to ignore this watchpoint
std::string m_decl_str; // Declaration information, if any.
std::string m_watch_spec_str; // Spec for the watchpoint.
diff --git a/lldb/include/lldb/Interpreter/OptionGroupWatchpoint.h b/lldb/include/lldb/Interpreter/OptionGroupWatchpoint.h
index 201ab1d134f2554..66c87a6287319d8 100644
--- a/lldb/include/lldb/Interpreter/OptionGroupWatchpoint.h
+++ b/lldb/include/lldb/Interpreter/OptionGroupWatchpoint.h
@@ -31,13 +31,15 @@ class OptionGroupWatchpoint : public OptionGroup {
void OptionParsingStarting(ExecutionContext *execution_context) override;
// Note:
- // eWatchRead == LLDB_WATCH_TYPE_READ; and
+ // eWatchRead == LLDB_WATCH_TYPE_READ
// eWatchWrite == LLDB_WATCH_TYPE_WRITE
+ // eWatchModify == LLDB_WATCH_TYPE_MODIFY
enum WatchType {
eWatchInvalid = 0,
eWatchRead,
eWatchWrite,
- eWatchReadWrite
+ eWatchModify,
+ eWatchReadModify
};
WatchType watch_type;
diff --git a/lldb/include/lldb/lldb-defines.h b/lldb/include/lldb/lldb-defines.h
index 5e43f55224f5202..ce7fd6f3754516e 100644
--- a/lldb/include/lldb/lldb-defines.h
+++ b/lldb/include/lldb/lldb-defines.h
@@ -44,8 +44,10 @@
#define LLDB_WATCH_ID_IS_VALID(uid) ((uid) != (LLDB_INVALID_WATCH_ID))
#define LLDB_WATCH_TYPE_READ (1u << 0)
#define LLDB_WATCH_TYPE_WRITE (1u << 1)
+#define LLDB_WATCH_TYPE_MODIFY (1u << 2)
#define LLDB_WATCH_TYPE_IS_VALID(type) \
- ((type & LLDB_WATCH_TYPE_READ) || (type & LLDB_WATCH_TYPE_WRITE))
+ ((type & LLDB_WATCH_TYPE_READ) || (type & LLDB_WATCH_TYPE_WRITE) || \
+ (type & LLDB_WATCH_TYPE_MODIFY))
// Generic Register Numbers
#define LLDB_REGNUM_GENERIC_PC 0 // Program Counter
diff --git a/lldb/source/API/SBTarget.cpp b/lldb/source/API/SBTarget.cpp
index e38edf8e44652e6..46873001a85b4c8 100644
--- a/lldb/source/API/SBTarget.cpp
+++ b/lldb/source/API/SBTarget.cpp
@@ -1336,7 +1336,7 @@ lldb::SBWatchpoint SBTarget::WatchAddress(lldb::addr_t addr, size_t size,
if (read)
watch_type |= LLDB_WATCH_TYPE_READ;
if (write)
- watch_type |= LLDB_WATCH_TYPE_WRITE;
+ watch_type |= LLDB_WATCH_TYPE_MODIFY;
if (watch_type == 0) {
error.SetErrorString(
"Can't create a watchpoint that is neither read nor write.");
diff --git a/lldb/source/API/SBValue.cpp b/lldb/source/API/SBValue.cpp
index 738773c93c49b03..3dbd2d13616ebb4 100644
--- a/lldb/source/API/SBValue.cpp
+++ b/lldb/source/API/SBValue.cpp
@@ -1436,7 +1436,7 @@ lldb::SBWatchpoint SBValue::Watch(bool resolve_location, bool read, bool write,
if (read)
watch_type |= LLDB_WATCH_TYPE_READ;
if (write)
- watch_type |= LLDB_WATCH_TYPE_WRITE;
+ watch_type |= LLDB_WATCH_TYPE_MODIFY;
Status rc;
CompilerType type(value_sp->GetCompilerType());
diff --git a/lldb/source/Breakpoint/Watchpoint.cpp b/lldb/source/Breakpoint/Watchpoint.cpp
index f88e899eb711fed..d76f3638eff97ed 100644
--- a/lldb/source/Breakpoint/Watchpoint.cpp
+++ b/lldb/source/Breakpoint/Watchpoint.cpp
@@ -29,7 +29,8 @@ Watchpoint::Watchpoint(Target &target, lldb::addr_t addr, uint32_t size,
: StoppointSite(0, addr, size, hardware), m_target(target),
m_enabled(false), m_is_hardware(hardware), m_is_watch_variable(false),
m_is_ephemeral(false), m_disabled_count(0), m_watch_read(0),
- m_watch_write(0), m_ignore_count(0), m_being_created(true) {
+ m_watch_write(0), m_watch_modify(0), m_ignore_count(0),
+ m_being_created(true) {
if (type && type->IsValid())
m_type = *type;
@@ -211,6 +212,35 @@ bool Watchpoint::CaptureWatchedValue(const ExecutionContext &exe_ctx) {
return (m_new_value_sp && m_new_value_sp->GetError().Success());
}
+bool Watchpoint::WatchedValueReportable(const ExecutionContext &exe_ctx) {
+ if (!m_watch_modify)
+ return true;
+ if (!m_type.IsValid())
+ return true;
+
+ ConstString watch_name("$__lldb__watch_value");
+ Address watch_address(GetLoadAddress());
+ ValueObjectSP newest_valueobj_sp = ValueObjectMemory::Create(
+ exe_ctx.GetBestExecutionContextScope(), watch_name.GetStringRef(),
+ watch_address, m_type);
+ newest_valueobj_sp = newest_valueobj_sp->CreateConstantValue(watch_name);
+ DataExtractor new_data;
+ DataExtractor old_data;
+ Status error;
+ newest_valueobj_sp->GetData(new_data, error);
+ m_new_value_sp->GetData(old_data, error);
+
+ if (new_data.GetByteSize() != old_data.GetByteSize() ||
+ new_data.GetByteSize() == 0)
+ return true;
+
+ if (memcmp(new_data.GetDataStart(), old_data.GetDataStart(),
+ old_data.GetByteSize()) == 0)
+ return false; // Value has not changed, user requested modify watchpoint
+ else
+ return true;
+}
+
// RETURNS - true if we should stop at this breakpoint, false if we
// should continue.
@@ -268,10 +298,10 @@ void Watchpoint::DumpWithLevel(Stream *s,
description_level <= lldb::eDescriptionLevelVerbose);
s->Printf("Watchpoint %u: addr = 0x%8.8" PRIx64
- " size = %u state = %s type = %s%s",
+ " size = %u state = %s type = %s%s%s",
GetID(), GetLoadAddress(), m_byte_size,
IsEnabled() ? "enabled" : "disabled", m_watch_read ? "r" : "",
- m_watch_write ? "w" : "");
+ m_watch_write ? "w" : "", m_watch_modify ? "m" : "");
if (description_level >= lldb::eDescriptionLevelFull) {
if (!m_decl_str.empty())
@@ -333,10 +363,13 @@ void Watchpoint::SetEnabled(bool enabled, bool notify) {
void Watchpoint::SetWatchpointType(uint32_t type, bool notify) {
int old_watch_read = m_watch_read;
int old_watch_write = m_watch_write;
+ int old_watch_modify = m_watch_modify;
m_watch_read = (type & LLDB_WATCH_TYPE_READ) != 0;
m_watch_write = (type & LLDB_WATCH_TYPE_WRITE) != 0;
+ m_watch_modify = (type & LLDB_WATCH_TYPE_MODIFY) != 0;
if (notify &&
- (old_watch_read != m_watch_read || old_watch_write != m_watch_write))
+ (old_watch_read != m_watch_read || old_watch_write != m_watch_write ||
+ old_watch_modify != m_watch_modify))
SendWatchpointChangedEvent(eWatchpointEventTypeTypeChanged);
}
@@ -344,6 +377,8 @@ bool Watchpoint::WatchpointRead() const { return m_watch_read != 0; }
bool Watchpoint::WatchpointWrite() const { return m_watch_write != 0; }
+bool Watchpoint::WatchpointModify() const { return m_watch_modify != 0; }
+
uint32_t Watchpoint::GetIgnoreCount() const { return m_ignore_count; }
void Watchpoint::SetIgnoreCount(uint32_t n) {
diff --git a/lldb/source/Commands/CommandObjectWatchpoint.cpp b/lldb/source/Commands/CommandObjectWatchpoint.cpp
index a4929ea0d5159ac..69ca1ef42b6bd2e 100644
--- a/lldb/source/Commands/CommandObjectWatchpoint.cpp
+++ b/lldb/source/Commands/CommandObjectWatchpoint.cpp
@@ -803,7 +803,7 @@ class CommandObjectWatchpointSetVariable : public CommandObjectParsed {
"Set a watchpoint on a variable. "
"Use the '-w' option to specify the type of watchpoint and "
"the '-s' option to specify the byte size to watch for. "
- "If no '-w' option is specified, it defaults to write. "
+ "If no '-w' option is specified, it defaults to modify. "
"If no '-s' option is specified, it defaults to the variable's "
"byte size. "
"Note that there are limited hardware resources for watchpoints. "
@@ -878,9 +878,9 @@ corresponding to the byte size of the data type.");
return false;
}
- // If no '-w' is specified, default to '-w write'.
+ // If no '-w' is specified, default to '-w modify'.
if (!m_option_watchpoint.watch_type_specified) {
- m_option_watchpoint.watch_type = OptionGroupWatchpoint::eWatchWrite;
+ m_option_watchpoint.watch_type = OptionGroupWatchpoint::eWatchModify;
}
// We passed the sanity check for the command. Proceed to set the
@@ -947,7 +947,23 @@ corresponding to the byte size of the data type.");
}
// Now it's time to create the watchpoint.
- uint32_t watch_type = m_option_watchpoint.watch_type;
+ uint32_t watch_type = 0;
+ switch (m_option_watchpoint.watch_type) {
+ case OptionGroupWatchpoint::eWatchModify:
+ watch_type |= LLDB_WATCH_TYPE_MODIFY;
+ break;
+ case OptionGroupWatchpoint::eWatchRead:
+ watch_type |= LLDB_WATCH_TYPE_READ;
+ break;
+ case OptionGroupWatchpoint::eWatchReadModify:
+ watch_type |= LLDB_WATCH_TYPE_READ | LLDB_WATCH_TYPE_MODIFY;
+ break;
+ case OptionGroupWatchpoint::eWatchWrite:
+ watch_type |= LLDB_WATCH_TYPE_WRITE;
+ break;
+ case OptionGroupWatchpoint::eWatchInvalid:
+ break;
+ };
error.Clear();
WatchpointSP watch_sp =
@@ -999,7 +1015,7 @@ class CommandObjectWatchpointSetExpression : public CommandObjectRaw {
"Use the '-l' option to specify the language of the expression. "
"Use the '-w' option to specify the type of watchpoint and "
"the '-s' option to specify the byte size to watch for. "
- "If no '-w' option is specified, it defaults to write. "
+ "If no '-w' option is specified, it defaults to modify. "
"If no '-s' option is specified, it defaults to the target's "
"pointer byte size. "
"Note that there are limited hardware resources for watchpoints. "
@@ -1013,7 +1029,7 @@ class CommandObjectWatchpointSetExpression : public CommandObjectRaw {
R"(
Examples:
-(lldb) watchpoint set expression -w write -s 1 -- foo + 32
+(lldb) watchpoint set expression -w modify -s 1 -- foo + 32
Watches write access for the 1-byte region pointed to by the address 'foo + 32')");
@@ -1073,7 +1089,7 @@ class CommandObjectWatchpointSetExpression : public CommandObjectRaw {
// If no '-w' is specified, default to '-w write'.
if (!m_option_watchpoint.watch_type_specified) {
- m_option_watchpoint.watch_type = OptionGroupWatchpoint::eWatchWrite;
+ m_option_watchpoint.watch_type = OptionGroupWatchpoint::eWatchModify;
}
// We passed the sanity check for the command. Proceed to set the
diff --git a/lldb/source/Interpreter/OptionGroupWatchpoint.cpp b/lldb/source/Interpreter/OptionGroupWatchpoint.cpp
index c2f78d8c2dd14a1..4c3715a685c3b51 100644
--- a/lldb/source/Interpreter/OptionGroupWatchpoint.cpp
+++ b/lldb/source/Interpreter/OptionGroupWatchpoint.cpp
@@ -28,9 +28,14 @@ static constexpr OptionEnumValueElement g_watch_type[] = {
"Watch for write",
},
{
- OptionGroupWatchpoint::eWatchReadWrite,
+ OptionGroupWatchpoint::eWatchModify,
+ "modify",
+ "Watch for modifications",
+ },
+ {
+ OptionGroupWatchpoint::eWatchReadModify,
"read_write",
- "Watch for read/write",
+ "Watch for read/modify",
},
};
diff --git a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
index 8ca2072b987dc72..fe46e64ee77077b 100644
--- a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
+++ b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
@@ -3109,14 +3109,15 @@ static GDBStoppointType GetGDBStoppointType(Watchpoint *wp) {
assert(wp);
bool watch_read = wp->WatchpointRead();
bool watch_write = wp->WatchpointWrite();
+ bool watch_modify = wp->WatchpointModify();
- // watch_read and watch_write cannot both be false.
- assert(watch_read || watch_write);
- if (watch_read && watch_write)
+ // watch_read, watch_write, watch_modify cannot all be false.
+ assert(watch_read || watch_write || watch_modify);
+ if (watch_read && (watch_write || watch_modify))
return eWatchpointReadWrite;
else if (watch_read)
return eWatchpointRead;
- else // Must be watch_write, then.
+ else // Must be watch_write or watch_modify, then.
return eWatchpointWrite;
}
diff --git a/lldb/source/Target/StopInfo.cpp b/lldb/source/Target/StopInfo.cpp
index ff3d8f68833c827..01dc9f6770fbf2a 100644
--- a/lldb/source/Target/StopInfo.cpp
+++ b/lldb/source/Target/StopInfo.cpp
@@ -982,6 +982,13 @@ class StopInfoWatchpoint : public StopInfo {
m_should_stop = false;
}
}
+
+ // Don't stop if the watched region value is unmodified, and
+ // this is a Modify-type watchpoint.
+ if (m_should_stop && !wp_sp->WatchedValueReportable(exe_ctx)) {
+ m_should_stop = false;
+ }
+
// Finally, if we are going to stop, print out the new & old values:
if (m_should_stop) {
wp_sp->CaptureWatchedValue(exe_ctx);
diff --git a/lldb/source/Target/Target.cpp b/lldb/source/Target/Target.cpp
index 7429b9e80f26acc..220f0e3177838af 100644
--- a/lldb/source/Target/Target.cpp
+++ b/lldb/source/Target/Target.cpp
@@ -860,7 +860,8 @@ WatchpointSP Target::CreateWatchpoint(lldb::addr_t addr, size_t size,
size_t old_size = matched_sp->GetByteSize();
uint32_t old_type =
(matched_sp->WatchpointRead() ? LLDB_WATCH_TYPE_READ : 0) |
- (matched_sp->WatchpointWrite() ? LLDB_WATCH_TYPE_WRITE : 0);
+ (matched_sp->WatchpointWrite() ? LLDB_WATCH_TYPE_WRITE : 0) |
+ (matched_sp->WatchpointModify() ? LLDB_WATCH_TYPE_MODIFY : 0);
// Return the existing watchpoint if both size and type match.
if (size == old_size && kind == old_type) {
wp_sp = matched_sp;
diff --git a/lldb/test/API/functionalities/watchpoint/modify-watchpoints/Makefile b/lldb/test/API/functionalities/watchpoint/modify-watchpoints/Makefile
new file mode 100644
index 000000000000000..10495940055b63d
--- /dev/null
+++ b/lldb/test/API/functionalities/watchpoint/modify-watchpoints/Makefile
@@ -0,0 +1,3 @@
+C_SOURCES := main.c
+
+include Makefile.rules
diff --git a/lldb/test/API/functionalities/watchpoint/modify-watchpoints/TestModifyWatchpoint.py b/lldb/test/API/functionalities/watchpoint/modify-watchpoints/TestModifyWatchpoint.py
new file mode 100644
index 000000000000000..1a479c9f25e46c3
--- /dev/null
+++ b/lldb/test/API/functionalities/watchpoint/modify-watchpoints/TestModifyWatchpoint.py
@@ -0,0 +1,27 @@
+"""
+Confirm that lldb modify watchpoints only stop
+when the value being watched changes.
+"""
+
+
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+
+class ModifyWatchpointTestCase(TestBase):
+ NO_DEBUG_INFO_TESTCASE = True
+
+ def test_modify_watchpoint(self):
+ """Test that a modify watchpoint only stops when the value changes."""
+ self.build()
+ self.main_source_file = lldb.SBFileSpec("main.c")
+ (target, process, thread, bkpt) = lldbutil.run_to_source_breakpoint(
+ self, "break here", self.main_source_file
+ )
+
+ self.runCmd("watch set variable value")
+ process.Continue()
+ frame = process.GetSelectedThread().GetFrameAtIndex(0)
+ self.assertEqual(frame.locals["value"][0].GetValueAsUnsigned(), 10)
diff --git a/lldb/test/API/functionalities/watchpoint/modify-watchpoints/main.c b/lldb/test/API/functionalities/watchpoint/modify-watchpoints/main.c
new file mode 100644
index 000000000000000..3f137b36c40af8a
--- /dev/null
+++ b/lldb/test/API/functionalities/watchpoint/modify-watchpoints/main.c
@@ -0,0 +1,17 @@
+#include <stdint.h>
+int main() {
+ int value = 5;
+
+ value = 5; // break here
+ value = 5;
+ value = 5;
+ value = 5;
+ value = 5;
+ value = 5;
+ value = 10;
+ value = 10;
+ value = 10;
+ value = 10;
+
+ return value;
+}
</pre>
</details>
https://github.com/llvm/llvm-project/pull/66308
More information about the lldb-commits
mailing list