[lldb] Add 'modify' type watchpoints, make it default #66308
Conversation
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.
|
@llvm/pr-subscribers-lldb ChangesWatchpoints 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:
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 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 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 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:
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;
+}
|
|
Just to be clear, I need to find a solution for SBTarget::CreateWatchpoint before this PR is ready to land. I'm inclined towards adding a third bool parameter 'modify', but I'm not sure that's the best choice. |
| if (!m_type.IsValid()) | ||
| return true; | ||
|
|
||
| ConstString watch_name("$__lldb__watch_value"); |
There was a problem hiding this comment.
Seems like you can pass this directly to ValueObjectMemory::Create as a string literal and make @bulbazord happy?
There was a problem hiding this comment.
I originally did it that way with them in mind! But I call ValueObject::CreateConstantValue with it a little bit later, and that takes a ConstString. I didn't chase it further to see if the ValueObject really couldn't allocate storage for a copy of the name.
There was a problem hiding this comment.
I've been reading through ValueObject and I'm not sure yet what the implications of removing ConstString from ValueObject are. If and when we can remove it from ValueObject, we'll be able to clean up this one too, so I would say don't worry about it right now.
As a suggestion, if you anticipate WatchedValueReportable being called many times, it would be less work to create that ConstString once as a static rather than creating one on each invocation.
| // Note: | ||
| // eWatchRead == LLDB_WATCH_TYPE_READ; and | ||
| // eWatchRead == LLDB_WATCH_TYPE_READ | ||
| // eWatchWrite == LLDB_WATCH_TYPE_WRITE | ||
| // eWatchModify == LLDB_WATCH_TYPE_MODIFY |
There was a problem hiding this comment.
These should be Doxygen (///) comments.
| DataExtractor new_data; | ||
| DataExtractor old_data; | ||
| Status error; | ||
| newest_valueobj_sp->GetData(new_data, error); | ||
| m_new_value_sp->GetData(old_data, error); |
There was a problem hiding this comment.
Maybe this is just personal preference but I like to define the local as close to its use as possible. This feels a bit "C99".
| newest_valueobj_sp = newest_valueobj_sp->CreateConstantValue(watch_name); | ||
| DataExtractor new_data; | ||
| DataExtractor old_data; | ||
| Status error; |
There was a problem hiding this comment.
Should we do anything with the error here? At least log it if something went wrong?
There was a problem hiding this comment.
+1, don't want to drop an error on the floor if we have one.
| else | ||
| return true; |
lldb/source/Target/StopInfo.cpp
Outdated
| if (m_should_stop && !wp_sp->WatchedValueReportable(exe_ctx)) { | ||
| m_should_stop = false; | ||
| } |
There was a problem hiding this comment.
LLVM says don’t use braces on simple single-statement bodies.
There was a problem hiding this comment.
Since users probably want a behavior like GDB by default, one idea is to just add a setting:
(llldb) settings show target.watchpoint-stop-on-write [always|changed]
When watchpoints are set, they just read the current value for this. Most of your internal code in this patch can stay the same. This allows LLDB to do what most people want by default and would require no changes to the command line command options or any API changes.
| Status error; | ||
| newest_valueobj_sp->GetData(new_data, error); | ||
| m_new_value_sp->GetData(old_data, error); | ||
|
|
There was a problem hiding this comment.
Do we need to update m_new_value_sp with the value in newst_valueobj_sp somewhere so we detect when things change again?
There was a problem hiding this comment.
In StopInfoWatchpoint::PerformAction() we decide if we're m_should_stop and if so, we call Watchpoint::CaptureWatchedValue (one of the things we do before that call is compare the value and decide if we're going to stop or not). CaptureWatchedValue copies the "new" value into the m_old_value_sp and then creates a new constant value object for m_new_value_sp. I copied that same way of creating the const ValueObject in this method; it's the method before this one in Watchpoint.cpp. So the copying of the new value to the previous value is already handled by that code in CaptureWatchedValue.
There was a problem hiding this comment.
Should add, because I'm sure someone will wonder about this: Yes, I'm creating a constant ValueObject with the watched memory here, and then throwing that ValueObject away. And if we decide to stop, we'll create the constant ValueObject again with the same memory, to show it to the user. I'm relying on the memory read cache to only read that memory from the inferior process once at a single stop, otherwise this double-creation of a ValueObject would be a real perf hit to watchpoints.
| @@ -0,0 +1,17 @@ | |||
| #include <stdint.h> | |||
| int main() { | |||
| int value = 5; | |||
There was a problem hiding this comment.
If you take the address of "value" then it will always be in memory. Here we might be just getting lucky that the compiler choses to put this on the stack at -O0.
There was a problem hiding this comment.
Yeah, if this was compiled with any optimization at all, the body of the function becomes mv x0, #0xa; ret so I didn't try to defeat compiler cleverness.
| value = 5; | ||
| value = 5; | ||
| value = 10; | ||
| value = 10; |
There was a problem hiding this comment.
Can we check that it stops when the value changes from 10 to something else to ensure we detect all watchpoint changes and we can ensure that m_new_value_sp and newest_valueobj_sp are doing their jobs correctly? We need to make sure we stop on all changes, here we are testing on change
There was a problem hiding this comment.
We could change it back to 5 to ensure that the watchpoint detects when we change from 5 to 10 (and stops) and then back to 5 (it should stop again because it changed from 10 to 5).
There was a problem hiding this comment.
Good point. I figured existing watchpoint tests would fail if that bug occurred, but it would be easy to test it here explicitly.
Anytime we keep adding more options to an API call we have two options:
If you think you will add more watchpoint options at some point in the future, go the SBWatchpointOptions route. |
Yeah exactly, I wasn't restating the options as clearly in that last comment, that's exactly what I meant. I can't think of other options we would want to build in to a Watchpoint at creation time - but the argument for SBWatchpointOptions is that ten-years-in-the-future-Jason may want another flag. Probably the best thing is to look at the SBTarget BreakpointCreate* methods; there are a dozen different methods for the different types of breakpoints you might want to create (address breakpoint, file & line breakpoint, symbol name breakpoint etc). An interesting aside is that none of the SB API methods take a flag for whether breakpoint should be set using a software or hardware breakpoint. Jonas added that feature to debugserver a few years ago for x86_64 and aarch64, and I think he added the For a watchpoint, we only have |
|
Maybe I should just use a flag param, and adopt the BreakpointCreate style naming convention, even if there's only this one API right now (we should provide all three so driver authors don't need to duplicate the work, especially for a variable path). e.g. with |
| 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); |
There was a problem hiding this comment.
It might be worth putting that comment into the assertion so it's a little easier to understand at first glance?
assert((watch_read || watch_write || watch_modify) && "watch_read, watch_write, watch_modify cannot all be false.");
The SBTarget::Launch(SBLaunchInfo) and SBTarget::Attach(SBAttachInfo) are some of the best example of using an options class. Breakpoints never tried to use an options class, we just kept adding new APIs which makes our API a bit messier, but we must keep APIs after they are added. So I would vote for the SBWatchpointOptions method. In the future we might have move complex ways to track watchpoints or want to track multiple address ranges with a single watchpoint?
Yeah, we probably didn't want to add yet another API call to just add access to the flag.
Yeah, it isn't too hard and it allows us great flexibility in the future. I have a patch coming for improving the saving of core files that allows users to specify a custom memory region list. I am creating a new options class named SBProcessSaveCoreOptions for the SBProcess::SaveCore(SBProcessSaveCoreOptions options) function as we can always add more flags to this class. |
@bulbazord what do you think about this suggestion? Would you still prefer an Options class? |
If we are going to add an overload I would suggest keeping with just adding a "bool modify" as it is more clear an usable. The options does seem like overkill for just one bool I admit, it just depends on what kind of options we might want to watchpoints in the future. If this is the last change to watchpoints, then add a new API. If we think we will add more options at some point (try to think if the new fancy watchpoints Jason is about to add support for might need more options?) then do the Options class route. |
You and Alex both preferred adding an Options class to pass in to this (and future WatchpointCreate API) so I'll write that up for my next revision of this PR, I didn't see your earlier message talking about your preference for that when I ping'ed Alex on their opinion. You were talking about how you think the proliferation of BreakpointCreate API was not ideal. Do you think there should be a It's probably best to just go with |
I'm alright with an Options class. What I would like to see is flexibility instead of adding new overloads every time we may want to add new levers and knobs to change Watchpoint behavior. |
|
I'm not sure I see the benefit of the SBWatchpointSpec including the way the watched region is specified. That's just moving the proliferation of API's from SBTarget::WatchpointCreate*() to the SBWatchpointSpec constructor. We'd have to have:
SBWatchpointSpec::SBWatchpointSpec(SBAddress addr, int flags);
SBWatchpointSpec::SBWatchpointSpec(SBValue value, int flags);
SBWatchpointSpec::SBWatchpointSpec(const char *expr, int flags);
And then users would do:
wp_spec = lldb.SBWatchpointSpec("foo + 5", lldb.eWatchpointTypeModify)
wp = target.WatchpointCreate(wp_spec)
So that's really one more API plus some boilerplate to construct the options. If an SBWatchpointSpec were the sort of thing you'd reuse, there might be some sense in making that a separate entity, but I don't see that for watchpoints. Otherwise, I don't think it adds much.
Jim
… On Sep 14, 2023, at 3:12 PM, Jason Molenda ***@***.***> wrote:
SBWatchpoint SBTarget::WatchpointCreateByAddress(addr_t address, size_t size, uint32_t access_flags, SBError &error);
with eWatchpointAccess{Read,Write,Modify} flags defined.
@bulbazord <https://github.com/bulbazord> what do you think about this suggestion? Would you still prefer an Options class?
If we are going to add an overload I would suggest keeping with just adding a "bool modify" as it is more clear an usable. The options does seem like overkill for just one bool I admit, it just depends on what kind of options we might want to watchpoints in the future. If this is the last change to watchpoints, then add a new API. If we think we will add more options at some point (try to think if the new fancy watchpoints Jason is about to add support for might need more options?) then do the Options class route.
You and Alex both preferred adding an Options class to pass in to this (and future WatchpointCreate API) so I'll write that up for my next revision of this PR, I didn't see your earlier message talking about your preference for that when I ping'ed Alex on their opinion.
You were talking about how you think the proliferation of BreakpointCreate API was not ideal. Do you think there should be a SBTarget::WatchpointCreate(SBWatchpointSpec, SBError &error) and you would specify (1) addr + size, (2) variable, (3) expression + size, as well as the access flags, for the Watchpoint? I don't mind the separate SBTarget methods approach for Breakpoints, it seems like six of one, half a dozen of the other to me.
It's probably best to just go with SBTarget::WatchpointCreateByAddress and add a new SBWatchpointOptions with the access flags only.
—
Reply to this email directly, view it on GitHub <#66308 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADUPVW4XRSYKJBWO3X4RAY3X2N6NVANCNFSM6AAAAAA4XKAUOQ>.
You are receiving this because your review was requested.
|
I do like the SBWatchpointSpec idea as then we don't need any overloads for the different kinds of watchpoints (address, value, etc). One thing I really like about the options/spec approach is if you are writing a new command in lldb that sets a watchpoint, and if that command has options in the python, then it is easy to populate the options/spec gradually and then set a watchpoint at the end. So the flow can be:
That being said I understand that it makes using python to quickly set watchpoints a bit more of a pain, so I am not voting 100% in any direction, just throwing stuff out there. I already have to check the APIs everytime I use python since we have many overloads on functions already, not a big deal if we add more. One thing to think about though is to make sure there aren't and default python arguments that would make it hard to call the existing API or the new API. There is some build warning when I build right now about a method being covered up due to default args when swig parses the header files + .i files that might be having this issue. So if we go the route of overloading we just need to make sure we avoid that issue in python. The other way we can configure this is to add an accessor to the SBWatchpoint object itself for non common options like "stop on all writes even when not modified". So we could leave the create API alone, and then add a method like: So we do the right thing by only stopping on a write the modifes by default and then we can further configure the breakpoint after it has been created. Looking at the 6 overloads for setting breakpoints by file and line, 7 for breakpoint by name(s), 6 for regex, I still vote either the options route or adding an accessor on the SBWatchpoint object after it is created in lieu of adding overloads. |
Add a SBWatchpointOptions class for specifying the read/write/modify type of a watchpoint. Mark SBTarget::WatchAddress as deprecated, add SBTarget::WatchpointCreateByAddress. When a user specifies a "read-write" watchpoint, don't override the "write" half of that to modify. In this case, the behavior the user probably wants is to see every access to this memory range, not just modifications of the value. Address first round of feedback from Jonas, Greg, Alex.
|
|
||
| void SetWatchpointTypeRead(bool read); | ||
| bool GetWatchpointTypeRead() const; | ||
|
|
There was a problem hiding this comment.
Add header doc here to explain that write mode will stop on any write even if the value doesn't change
| ~SBWatchpointOptions(); | ||
|
|
||
| const SBWatchpointOptions &operator=(const lldb::SBWatchpointOptions &rhs); | ||
|
|
There was a problem hiding this comment.
add a header doc explaining this will stop any time memory is read from. Might want to mention if this works along with "modify" at all to only stop when reading a value and that value has changed?
There was a problem hiding this comment.
I don't think we can do read+modify, actually. I fixed a subtle bug in this latest commit in Watchpoint::WatchedValueReportable() where we know that a watched memory region may have been accessed. If it's read+modify, we don't know if it's been read from, or written to (value mutating or not). Unless we emulate the instruction that was executed, it could be a read, or a write with the same value, or it could be an access near the watched region which triggered the stop. (this latter happening with large watchpoint support where watching a 24 byte region may actually need to watch 32 bytes, or on an AArch64 processor in streaming SVE mode when a write is near the watched region)
Effectively, if a watchpoint is read+modify, it will behave the same as read+write -- we will have to stop on every access to that memory region (or near that memory region, for the above reasons).
I only came to understand this behavior this afternoon while debugging a testsuite failure, I need to do a fuller once-over of my changes to see if there are other updates I need to make.
| void SetWatchpointTypeWrite(bool write); | ||
| bool GetWatchpointTypeWrite() const; | ||
|
|
||
| void SetWatchpointTypeModify(bool modify); |
There was a problem hiding this comment.
add a header doc explaining this does and if it can be used along with "read" or "write" or only "write"?
lldb/source/API/SBTarget.cpp
Outdated
| @@ -1326,23 +1326,35 @@ lldb::SBWatchpoint SBTarget::WatchAddress(lldb::addr_t addr, size_t size, | |||
| SBError &error) { | |||
| LLDB_INSTRUMENT_VA(this, addr, size, read, write, error); | |||
|
|
|||
| SBWatchpointOptions options; | |||
| options.SetWatchpointTypeRead(read); | |||
| options.SetWatchpointTypeModify(write); | |||
There was a problem hiding this comment.
do we want to rename this argument from "write" to "modify" to clarify what will happen? Variable names are not included in the mangled name, so we can rename as long as we don't change the type and not affect the API backwards compat. If we do rename, then update the header file as well
| class WatchpointOptionsImpl { | ||
| public: | ||
| bool m_read = false; | ||
| bool m_write = false; | ||
| bool m_modify = false; | ||
| }; |
There was a problem hiding this comment.
This could be moved to Watchpoint.h and then used in our internal API as well if you like that idea.
There was a problem hiding this comment.
Yeah I originally wrote that little class in Watchpoint.h and then I moved it into the SBWatchpointOptions because I wasn't using it anywhere else. tbh I thought the class for the three bools was overkill right now but I agree that there's long-term value in making the SB API cleanly extensible. But for the lldb_private API, I'm less worried about using three bools for now. If it does become more than that, an Options class would be a thing to adopt.
There was a problem hiding this comment.
Yeah, we can do anything internally, so it doesn't really matter what we do inside as we can always change it at any point as long as the public API is solid.
| wp = target.WatchAddress(a_bytebuf_6_addr, 4, False, True, err) | ||
| wp_opts = lldb.SBWatchpointOptions() | ||
| wp_opts.SetWatchpointTypeModify(True) | ||
| wp = target.WatchpointCreateByAddress(a_bytebuf_6_addr, 4, wp_opts, err) | ||
| self.assertTrue(err.Success()) |
There was a problem hiding this comment.
No need to change this test if it still uses the old API. Revert?
There was a problem hiding this comment.
I was marking SBTarget::WatchAddress as deprecated, so I thought updating the API tests to use the current API was the best choice. It might be good to keep one using the old API though, to catch a break in it.
| wp_opts = lldb.SBWatchpointOptions() | ||
| wp_opts.SetWatchpointTypeRead(True) | ||
| wp_opts.SetWatchpointTypeModify(True) | ||
| obj.WatchpointCreateByAddress(123, 8, wp_opts, error) |
There was a problem hiding this comment.
Revert and keep old API usage?
There was a problem hiding this comment.
Overall looks pretty solid. Just need to nail down the headerdoc for the "modify" accesors and clarify that it only works in combination with "write" and probably change the old API from "bool read, bool write" to be "bool read, bool modify" to clearly indicate what will happen. Or checkout the suggestion for the lldb::WatchpointWriteType enum and let me know what you think
| void SetWatchpointTypeRead(bool read); | ||
| bool GetWatchpointTypeRead() const; | ||
|
|
||
| void SetWatchpointTypeWrite(bool write); | ||
| bool GetWatchpointTypeWrite() const; | ||
|
|
||
| void SetWatchpointTypeModify(bool modify); | ||
| bool GetWatchpointTypeModify() const; |
There was a problem hiding this comment.
Since we are changing the API, would it be more clear to just have an enum and accessor to make it clear when we still stop will happen during writes? We can keep the SetWatchpointTypeRead(bool), but for the write maybe something like:
// Put this into lldb-enumerations.h
namespace lldb {
enum class WatchpointWriteType {
Disabled, // Don't stop when the data being watched is written to
Always, // Stop on any write access to the the value being watched even if value doesn't change
OnModify // Stop only on write access that modifies the value being watched
};
} // namespace lldb
The we have just one accessor for the write control like:
lldb::WatchpointWriteType GetWatchpointWriteType();
void SetWathpointWriteType(lldb::WatchpointWriteType watch_type);
The current API in this class might be confusing to the user as to what needs to be set:
- If I want to stop when the value is modfied, do I just set "modify" or also need to set "write"?
- Can I set "read" and "modify"? (we said no on this, but it might still be confusing to users as they might think they can)
- what does setting "read", "write" and "modify" do?
There was a problem hiding this comment.
This is a good idea. I was a unhappy with "read", "write", and "modify" as if they were all independently selectable values or something, but hadn't thought of a better way of expressing write/modify as a type of write watchpoint.
| class WatchpointOptionsImpl { | ||
| public: | ||
| bool m_read = false; | ||
| bool m_write = false; | ||
| bool m_modify = false; | ||
| }; |
There was a problem hiding this comment.
Yeah, we can do anything internally, so it doesn't really matter what we do inside as we can always change it at any point as long as the public API is solid.
Add a new `WatchpointWriteType` to lldb-enumerations, change SBWatchpointOptions::SetWatchpointTypeWrite() to take that enum as its argument, eWatchpointWriteTypeOnModify, eWatchpointWriteTypeAlways, or eWatchpointWriteTypeDisabled. This clarifies that a watchpoint is either a "read" watchpoint, a "write" watchpoint, or both. And if it's a "write" watchpoint, it is either 'always' or 'modify'.
| @@ -6,11 +6,7 @@ | |||
| ) lldb::SBWatchpointOptions::SetWatchpointTypeRead; | |||
| %feature("docstring", "Gets whether the watchpoint should stop on read accesses." | |||
| ) lldb::SBWatchpointOptions::GetWatchpointTypeRead; | |||
| %feature("docstring", "Sets whether the watchpoint should stop on write accesses." | |||
| %feature("docstring", "Sets whether the watchpoint should stop on write accesses. eWatchpointWriteTypeOnModify is the most commonly useful mode, where lldb will stop when the watched value has changed. eWatchpointWriteTypeAlways will stop on any write to the watched region, and on some targets there can false watchpoint stops where memory near the watched region was written, and lldb cannot detect that it is a spurious stop." | |||
There was a problem hiding this comment.
Do we need to state "and on some targets there can false watchpoint stops where memory near the watched region was written, and lldb cannot detect that it is a spurious stop."? Can we remove that? Seems like information that most users wouldn't need to know?
There was a problem hiding this comment.
Yeah I'll remove that text. It's going to be part of my large watchpoints enhancement -- on stubs that support it on AArch64, we'll use a masking watchpoint, so you can watch a 48 byte object but we'll do it by watching 64 bytes of memory. for a straight-up "write" watchpoint, we can't tell whether the 48 bytes were written to with the same value, or the 16 bytes we aren't intending to watch were written to. Also with AArch64 in SVE Streaming Mode, the address matching is at lower accuracy, so a write near a watchpoint can match the watchpoint, even if it didn't actually access the memory range.
There was a problem hiding this comment.
(and the "modify" watchpoint type solves both of these, because we can only report watchpoint hits where the watched memory region changes)
|
That watchpoints can be inaccurate is probably news to a lot of debugger users, and knowing that might save somebody a fruitless hour wondering "how that variable could be being modified here..." But I don't think most folks are likely to see the API doc string. Might be more useful to mention this in the "watch set" help.
Jikm
… On Sep 18, 2023, at 6:38 PM, Greg Clayton ***@***.***> wrote:
@clayborg approved this pull request.
Thanks for the changes! Looks great
In lldb/bindings/interface/SBWatchpointOptionsDocstrings.i <#66308 (comment)>:
> @@ -6,11 +6,7 @@
) lldb::SBWatchpointOptions::SetWatchpointTypeRead;
%feature("docstring", "Gets whether the watchpoint should stop on read accesses."
) lldb::SBWatchpointOptions::GetWatchpointTypeRead;
-%feature("docstring", "Sets whether the watchpoint should stop on write accesses."
+%feature("docstring", "Sets whether the watchpoint should stop on write accesses. eWatchpointWriteTypeOnModify is the most commonly useful mode, where lldb will stop when the watched value has changed. eWatchpointWriteTypeAlways will stop on any write to the watched region, and on some targets there can false watchpoint stops where memory near the watched region was written, and lldb cannot detect that it is a spurious stop."
Do we need to state "and on some targets there can false watchpoint stops where memory near the watched region was written, and lldb cannot detect that it is a spurious stop."? Can we remove that? Seems like information that most users wouldn't need to know?
—
Reply to this email directly, view it on GitHub <#66308 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADUPVWYQDHG3WAIHK77KF2DX3DZRNANCNFSM6AAAAAA4XKAUOQ>.
You are receiving this because your review was requested.
|
in the SBWatchpointOptions docstring.
TestStepOverWatchpoint.py and TestUnalignedWatchpoint.py are failing on the ubuntu and debian bots https://lab.llvm.org/buildbot/#/builders/68/builds/60204 https://lab.llvm.org/buildbot/#/builders/96/builds/45623 and the newly added test TestModifyWatchpoint.py does not work on windows bot https://lab.llvm.org/buildbot/#/builders/219/builds/5708 I will debug tomorrow morning and reland. This reverts commit 3692267.
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. 2. Want to find what is writing to this memory. 3. Want to find what is reading from 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. rdar://108234227
…6308)" TestStepOverWatchpoint.py and TestUnalignedWatchpoint.py are failing on the ubuntu and debian bots https://lab.llvm.org/buildbot/#/builders/68/builds/60204 https://lab.llvm.org/buildbot/#/builders/96/builds/45623 and the newly added test TestModifyWatchpoint.py does not work on windows bot https://lab.llvm.org/buildbot/#/builders/219/builds/5708 I will debug tomorrow morning and reland. This reverts commit 3692267.
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. 2. Want to find what is writing to this memory. 3. Want to find what is reading from 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. Re-landing this patch after addressing testsuite failures found in CI on Linux, Intel machines, and windows. rdar://108234227
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. 2. Want to find what is writing to this memory. 3. Want to find what is reading from 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. Re-landing this patch after addressing testsuite failures found in CI on Linux, Intel machines, and windows. rdar://108234227 (cherry picked from commit 933ad5c)
[lldb] Add 'modify' type watchpoints, make it default (llvm#66308)
This reverts commit 933ad5c. This caused 1 test failure and an unexpected pass on AArch64 Linux: https://lab.llvm.org/buildbot/#/builders/96/builds/45765 Wasn't reported because the bot was already red at the time.
|
I've reverted this due to failures on Arm/AArch64 Linux, well, one test failing, one passing unexpectedly. Likely they are expecting the watchpoint to go off even when not modified. If I can fix them I'll do that and reland this. |
This reverts commit a7b78ca. With updates to the tests. TestWatchTaggedAddress.py: Updated the expected watchpoint types, though I'm not sure there should be a differnt default for the two ways of setting them, that needs to be confirmed. TestStepOverWatchpoint.py: Skipped this everywhere because I think what used to happen is you couldn't put 2 watchpoints on the same address (after alignment). I guess that this is now allowed because modify watchpoints aren't accounted for, but likely should be. Needs investigating.
|
I've updated the tests and relanded but there is still some stuff to check. The tagged address test is working fine but the default watchpoint type is different between the 2 commands, which seems wrong at first glance. For the other one see #26405 for some context. I guess somewhere we need to check for a read/write or modify break on the location before allowing the user to set another one, since that's where the test used to fail when it tried to place the second watchpoint. Since the globals are all chars and their addresses when aligned down were the same, I think. See if you can figure that one out. |
|
Where the "commands" are: They default to different types. |
|
Thanks @DavidSpickett for looking into these. Let me looked into the tagged ptr watchpoint issue right now, that sounds like a bug. One of the changes I made in the second landing of my patch was to the TestStepOverWatchpoint C file, which had three 1-byte globals, one which it would put a I don't know if lldb-server would handle this correctly, but debugserver wouldn't. And it's not what the test case intends to be testing. When I re-landed my patch, I changed the variables to |
|
FWIW I've found that |
|
Ah, I see the bug where |
|
@DavidSpickett pushed the fix for the |
|
re. TestStepOverWatchpoint.py this test case annoys me greatly. It wants to test that we get a watchpoint trap when stepping over a function that accesses the memory, and that we can stop on the watchpoint access when instruction stepping. It wants to test this for read and write watchpoints. It's operating on 1-byte globals, which are likely in the same dword as mentioned above. MIPS and s390x both skip it expressedly because we're trying to watch the same word/dword of memory with both read & write access -- which isn't what the test is meant to be testing. It starts by putting a read watchpoint on the read variable, and then for the case where it's doing write watchpoint testing, it adds a second watchpoint on the watch variable. The read watchpoint is completely irrelevant in the watch-testing case. None of this "read and write watch the same word of memory" is necessary! The variables can be larger so they don't share memory granule, and we don't need to set a read watchpoint in the "test write-watchpoints" test method at all. The test won't work on Darwin systems because hardware watchpoints/breakpoints are currently disabled while instruction stepping by the kernel. So the bit of the test that instruction steps until a watchpoint is accessed will not succeed. I'm going to leave this as expected fail for everyone for a tiny bit longer until I can test a bit of a rewrite. |
…6308)" This reverts commit 933ad5c. This caused 1 test failure and an unexpected pass on AArch64 Linux: https://lab.llvm.org/buildbot/#/builders/96/builds/45765 Wasn't reported because the bot was already red at the time. (cherry picked from commit a7b78ca)
…6308)" This reverts commit a7b78ca. With updates to the tests. TestWatchTaggedAddress.py: Updated the expected watchpoint types, though I'm not sure there should be a differnt default for the two ways of setting them, that needs to be confirmed. TestStepOverWatchpoint.py: Skipped this everywhere because I think what used to happen is you couldn't put 2 watchpoints on the same address (after alignment). I guess that this is now allowed because modify watchpoints aren't accounted for, but likely should be. Needs investigating. (cherry picked from commit 75e8620)
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:
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 modifyargument 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.