From 1efbce9f3b754f45372f0098163e70c2118b57d9 Mon Sep 17 00:00:00 2001 From: Jason Molenda Date: Wed, 13 Sep 2023 17:38:31 -0700 Subject: [PATCH 1/4] [lldb] Add 'modify' type watchpoints, make it default 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. --- lldb/include/lldb/Breakpoint/Watchpoint.h | 5 ++- .../lldb/Interpreter/OptionGroupWatchpoint.h | 6 ++- lldb/include/lldb/lldb-defines.h | 4 +- lldb/source/API/SBTarget.cpp | 2 +- lldb/source/API/SBValue.cpp | 2 +- lldb/source/Breakpoint/Watchpoint.cpp | 43 +++++++++++++++++-- .../Commands/CommandObjectWatchpoint.cpp | 30 ++++++++++--- .../Interpreter/OptionGroupWatchpoint.cpp | 9 +++- .../Process/gdb-remote/ProcessGDBRemote.cpp | 9 ++-- lldb/source/Target/StopInfo.cpp | 7 +++ lldb/source/Target/Target.cpp | 3 +- .../watchpoint/modify-watchpoints/Makefile | 3 ++ .../TestModifyWatchpoint.py | 27 ++++++++++++ .../watchpoint/modify-watchpoints/main.c | 17 ++++++++ 14 files changed, 143 insertions(+), 24 deletions(-) create mode 100644 lldb/test/API/functionalities/watchpoint/modify-watchpoints/Makefile create mode 100644 lldb/test/API/functionalities/watchpoint/modify-watchpoints/TestModifyWatchpoint.py create mode 100644 lldb/test/API/functionalities/watchpoint/modify-watchpoints/main.c diff --git a/lldb/include/lldb/Breakpoint/Watchpoint.h b/lldb/include/lldb/Breakpoint/Watchpoint.h index 8fde3b563a3f0..c86d66663c7f8 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, 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, // 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 201ab1d134f25..66c87a6287319 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 5e43f55224f52..ce7fd6f375451 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 e38edf8e44652..46873001a85b4 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 738773c93c49b..3dbd2d13616eb 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 f88e899eb711f..d76f3638eff97 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 a4929ea0d5159..69ca1ef42b6bd 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 c2f78d8c2dd14..4c3715a685c3b 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 8ca2072b987dc..fe46e64ee7707 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 ff3d8f68833c8..01dc9f6770fbf 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 7429b9e80f26a..220f0e3177838 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 0000000000000..10495940055b6 --- /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 0000000000000..1a479c9f25e46 --- /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 0000000000000..3f137b36c40af --- /dev/null +++ b/lldb/test/API/functionalities/watchpoint/modify-watchpoints/main.c @@ -0,0 +1,17 @@ +#include +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; +} From 6648995d5c9a68adb678807d8cb1e808d9138863 Mon Sep 17 00:00:00 2001 From: Jason Molenda Date: Fri, 15 Sep 2023 14:14:44 -0700 Subject: [PATCH 2/4] Update changes for first round of feedback 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. --- lldb/bindings/headers.swig | 1 + .../interface/SBWatchpointOptionsDocstrings.i | 16 +++++ lldb/bindings/interfaces.swig | 2 + lldb/include/lldb/API/SBTarget.h | 6 ++ lldb/include/lldb/API/SBWatchpointOptions.h | 44 ++++++++++++ .../lldb/Interpreter/OptionGroupWatchpoint.h | 10 +-- lldb/include/lldb/lldb-forward.h | 1 + lldb/source/API/CMakeLists.txt | 1 + lldb/source/API/SBTarget.cpp | 38 +++++++---- lldb/source/API/SBValue.cpp | 13 +++- lldb/source/API/SBWatchpoint.cpp | 3 +- lldb/source/API/SBWatchpointOptions.cpp | 67 +++++++++++++++++++ lldb/source/Breakpoint/Watchpoint.cpp | 26 ++++--- .../Commands/CommandObjectWatchpoint.cpp | 4 +- .../Interpreter/OptionGroupWatchpoint.cpp | 4 +- .../Process/gdb-remote/ProcessGDBRemote.cpp | 3 +- lldb/source/Target/StopInfo.cpp | 3 +- .../watchpoints/multiple_hits/main.cpp | 4 +- .../watchpoints/unaligned-watchpoint/main.c | 2 +- .../TestNoWatchpointSupportInfo.py | 4 +- .../large-watchpoint/TestLargeWatchpoint.py | 4 +- .../TestModifyWatchpoint.py | 12 ++++ .../watchpoint/modify-watchpoints/main.c | 3 + .../TestUnalignedSpanningDwords.py | 4 +- .../default-constructor/sb_target.py | 5 +- .../watchlocation/TestTargetWatchAddress.py | 20 +++--- 26 files changed, 247 insertions(+), 53 deletions(-) create mode 100644 lldb/bindings/interface/SBWatchpointOptionsDocstrings.i create mode 100644 lldb/include/lldb/API/SBWatchpointOptions.h create mode 100644 lldb/source/API/SBWatchpointOptions.cpp diff --git a/lldb/bindings/headers.swig b/lldb/bindings/headers.swig index f7871765624dd..d392ed43d8c0c 100644 --- a/lldb/bindings/headers.swig +++ b/lldb/bindings/headers.swig @@ -77,4 +77,5 @@ #include "lldb/API/SBValueList.h" #include "lldb/API/SBVariablesOptions.h" #include "lldb/API/SBWatchpoint.h" +#include "lldb/API/SBWatchpointOptions.h" %} diff --git a/lldb/bindings/interface/SBWatchpointOptionsDocstrings.i b/lldb/bindings/interface/SBWatchpointOptionsDocstrings.i new file mode 100644 index 0000000000000..5076065d285d5 --- /dev/null +++ b/lldb/bindings/interface/SBWatchpointOptionsDocstrings.i @@ -0,0 +1,16 @@ +%feature("docstring", +"A container for options to use when creating watchpoints." +) lldb::SBWatchpointOptions; + +%feature("docstring", "Sets whether the watchpoint should stop on read accesses." +) 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." +) lldb::SBWatchpointOptions::SetWatchpointTypeWrite; +%feature("docstring", "Gets whether the watchpoint should stop on write accesses." +) lldb::SBWatchpointOptions::GetWatchpointTypeWrite; +%feature("docstring", "Sets whether the watchpoint should stop on modify accesses." +) lldb::SBWatchpointOptions::SetWatchpointTypeModify; +%feature("docstring", "Gets whether the watchpoint should stop on modify accesses." +) lldb::SBWatchpointOptions::GetWatchpointTypeModify; diff --git a/lldb/bindings/interfaces.swig b/lldb/bindings/interfaces.swig index 53f8fcc8145ac..306cfe6838932 100644 --- a/lldb/bindings/interfaces.swig +++ b/lldb/bindings/interfaces.swig @@ -80,6 +80,7 @@ %include "./interface/SBValueListDocstrings.i" %include "./interface/SBVariablesOptionsDocstrings.i" %include "./interface/SBWatchpointDocstrings.i" +%include "./interface/SBWatchpointOptionsDocstrings.i" /* API headers */ %include "lldb/API/SBAddress.h" @@ -152,6 +153,7 @@ %include "lldb/API/SBValueList.h" %include "lldb/API/SBVariablesOptions.h" %include "lldb/API/SBWatchpoint.h" +%include "lldb/API/SBWatchpointOptions.h" /* Extensions for SB classes */ %include "./interface/SBAddressExtensions.i" diff --git a/lldb/include/lldb/API/SBTarget.h b/lldb/include/lldb/API/SBTarget.h index 07bd6a8b76d2f..06026e7d0b863 100644 --- a/lldb/include/lldb/API/SBTarget.h +++ b/lldb/include/lldb/API/SBTarget.h @@ -21,6 +21,7 @@ #include "lldb/API/SBType.h" #include "lldb/API/SBValue.h" #include "lldb/API/SBWatchpoint.h" +#include "lldb/API/SBWatchpointOptions.h" namespace lldb_private { namespace python { @@ -828,9 +829,14 @@ class LLDB_API SBTarget { lldb::SBWatchpoint FindWatchpointByID(lldb::watch_id_t watch_id); + LLDB_DEPRECATED("WatchAddress deprecated, use WatchpointCreateByAddress") lldb::SBWatchpoint WatchAddress(lldb::addr_t addr, size_t size, bool read, bool write, SBError &error); + lldb::SBWatchpoint + WatchpointCreateByAddress(lldb::addr_t addr, size_t size, + lldb::SBWatchpointOptions options, SBError &error); + bool EnableAllWatchpoints(); bool DisableAllWatchpoints(); diff --git a/lldb/include/lldb/API/SBWatchpointOptions.h b/lldb/include/lldb/API/SBWatchpointOptions.h new file mode 100644 index 0000000000000..6f561bca1cb23 --- /dev/null +++ b/lldb/include/lldb/API/SBWatchpointOptions.h @@ -0,0 +1,44 @@ +//===-- SBWatchpointOptions.h -----------------------------------*- C++ -*-===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +#ifndef LLDB_API_SBWATCHPOINTOPTIONS_H +#define LLDB_API_SBWATCHPOINTOPTIONS_H + +#include "lldb/API/SBDefines.h" + +class WatchpointOptionsImpl; + +namespace lldb { + +class LLDB_API SBWatchpointOptions { +public: + SBWatchpointOptions(); + + SBWatchpointOptions(const lldb::SBWatchpointOptions &rhs); + + ~SBWatchpointOptions(); + + const SBWatchpointOptions &operator=(const lldb::SBWatchpointOptions &rhs); + + void SetWatchpointTypeRead(bool read); + bool GetWatchpointTypeRead() const; + + void SetWatchpointTypeWrite(bool write); + bool GetWatchpointTypeWrite() const; + + void SetWatchpointTypeModify(bool modify); + bool GetWatchpointTypeModify() const; + +private: + // This auto_pointer is made in the constructor and is always valid. + mutable std::unique_ptr m_opaque_up; +}; + +} // namespace lldb + +#endif // LLDB_API_SBWATCHPOINTOPTIONS_H diff --git a/lldb/include/lldb/Interpreter/OptionGroupWatchpoint.h b/lldb/include/lldb/Interpreter/OptionGroupWatchpoint.h index 66c87a6287319..f009fa145f303 100644 --- a/lldb/include/lldb/Interpreter/OptionGroupWatchpoint.h +++ b/lldb/include/lldb/Interpreter/OptionGroupWatchpoint.h @@ -30,16 +30,16 @@ class OptionGroupWatchpoint : public OptionGroup { void OptionParsingStarting(ExecutionContext *execution_context) override; - // Note: - // eWatchRead == LLDB_WATCH_TYPE_READ - // eWatchWrite == LLDB_WATCH_TYPE_WRITE - // eWatchModify == LLDB_WATCH_TYPE_MODIFY + /// eWatchRead == LLDB_WATCH_TYPE_READ + /// eWatchWrite == LLDB_WATCH_TYPE_WRITE + /// eWatchModify == LLDB_WATCH_TYPE_MODIFY + /// eWatchReadWrite == LLDB_WATCH_TYPE_READ | LLDB_WATCH_TYPE_WRITE enum WatchType { eWatchInvalid = 0, eWatchRead, eWatchWrite, eWatchModify, - eWatchReadModify + eWatchReadWrite }; WatchType watch_type; diff --git a/lldb/include/lldb/lldb-forward.h b/lldb/include/lldb/lldb-forward.h index c159d82911a04..3cd71c8a4ba3c 100644 --- a/lldb/include/lldb/lldb-forward.h +++ b/lldb/include/lldb/lldb-forward.h @@ -284,6 +284,7 @@ class VariableList; class Watchpoint; class WatchpointList; class WatchpointOptions; +class WatchpointSetOptions; struct CompilerContext; struct LineEntry; struct PropertyDefinition; diff --git a/lldb/source/API/CMakeLists.txt b/lldb/source/API/CMakeLists.txt index 39ac451c471c5..910cb667e16b7 100644 --- a/lldb/source/API/CMakeLists.txt +++ b/lldb/source/API/CMakeLists.txt @@ -91,6 +91,7 @@ add_lldb_library(liblldb SHARED ${option_framework} SBValueList.cpp SBVariablesOptions.cpp SBWatchpoint.cpp + SBWatchpointOptions.cpp SBUnixSignals.cpp SystemInitializerFull.cpp ${lldb_python_wrapper} diff --git a/lldb/source/API/SBTarget.cpp b/lldb/source/API/SBTarget.cpp index 46873001a85b4..5b4eea7a6ccef 100644 --- a/lldb/source/API/SBTarget.cpp +++ b/lldb/source/API/SBTarget.cpp @@ -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); + return WatchpointCreateByAddress(addr, size, options, error); +} + +lldb::SBWatchpoint +SBTarget::WatchpointCreateByAddress(lldb::addr_t addr, size_t size, + SBWatchpointOptions options, + SBError &error) { + LLDB_INSTRUMENT_VA(this, addr, size, options, error); + SBWatchpoint sb_watchpoint; lldb::WatchpointSP watchpoint_sp; TargetSP target_sp(GetSP()); - if (target_sp && (read || write) && addr != LLDB_INVALID_ADDRESS && - size > 0) { + uint32_t watch_type = 0; + if (options.GetWatchpointTypeRead()) + watch_type |= LLDB_WATCH_TYPE_READ; + if (options.GetWatchpointTypeWrite()) + watch_type |= LLDB_WATCH_TYPE_WRITE; + if (options.GetWatchpointTypeModify()) + watch_type |= LLDB_WATCH_TYPE_MODIFY; + if (watch_type == 0) { + error.SetErrorString("Can't create a watchpoint that is neither read nor " + "write nor modify."); + return sb_watchpoint; + } + if (target_sp && addr != LLDB_INVALID_ADDRESS && size > 0) { std::lock_guard guard(target_sp->GetAPIMutex()); - uint32_t watch_type = 0; - if (read) - watch_type |= LLDB_WATCH_TYPE_READ; - if (write) - watch_type |= LLDB_WATCH_TYPE_MODIFY; - if (watch_type == 0) { - error.SetErrorString( - "Can't create a watchpoint that is neither read nor write."); - return sb_watchpoint; - } - // Target::CreateWatchpoint() is thread safe. Status cw_error; // This API doesn't take in a type, so we can't figure out what it is. diff --git a/lldb/source/API/SBValue.cpp b/lldb/source/API/SBValue.cpp index 3dbd2d13616eb..e14f1196c6316 100644 --- a/lldb/source/API/SBValue.cpp +++ b/lldb/source/API/SBValue.cpp @@ -1433,10 +1433,17 @@ lldb::SBWatchpoint SBValue::Watch(bool resolve_location, bool read, bool write, return sb_watchpoint; uint32_t watch_type = 0; - if (read) + if (read) { watch_type |= LLDB_WATCH_TYPE_READ; - if (write) - watch_type |= LLDB_WATCH_TYPE_MODIFY; + // read + write, the most likely intention + // is to catch all writes to this, not just + // value modifications. + if (write) + watch_type |= LLDB_WATCH_TYPE_WRITE; + } else { + if (write) + watch_type |= LLDB_WATCH_TYPE_MODIFY; + } Status rc; CompilerType type(value_sp->GetCompilerType()); diff --git a/lldb/source/API/SBWatchpoint.cpp b/lldb/source/API/SBWatchpoint.cpp index 6a63e65956bc4..8b4e0ad3178b1 100644 --- a/lldb/source/API/SBWatchpoint.cpp +++ b/lldb/source/API/SBWatchpoint.cpp @@ -354,7 +354,8 @@ bool SBWatchpoint::IsWatchingWrites() { std::lock_guard guard( watchpoint_sp->GetTarget().GetAPIMutex()); - return watchpoint_sp->WatchpointWrite(); + return watchpoint_sp->WatchpointWrite() || + watchpoint_sp->WatchpointModify(); } return false; diff --git a/lldb/source/API/SBWatchpointOptions.cpp b/lldb/source/API/SBWatchpointOptions.cpp new file mode 100644 index 0000000000000..619ad411b6c06 --- /dev/null +++ b/lldb/source/API/SBWatchpointOptions.cpp @@ -0,0 +1,67 @@ +//===-- SBWatchpointOptions.cpp -------------------------------------------===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +#include "lldb/API/SBWatchpointOptions.h" +#include "lldb/Breakpoint/Watchpoint.h" +#include "lldb/Utility/Instrumentation.h" + +#include "Utils.h" + +using namespace lldb; +using namespace lldb_private; + +class WatchpointOptionsImpl { +public: + bool m_read = false; + bool m_write = false; + bool m_modify = false; +}; + + +SBWatchpointOptions::SBWatchpointOptions() + : m_opaque_up(new WatchpointOptionsImpl()) { + LLDB_INSTRUMENT_VA(this); +} + +SBWatchpointOptions::SBWatchpointOptions(const SBWatchpointOptions &rhs) { + LLDB_INSTRUMENT_VA(this, rhs); + + m_opaque_up = clone(rhs.m_opaque_up); +} + +const SBWatchpointOptions & +SBWatchpointOptions::operator=(const SBWatchpointOptions &rhs) { + LLDB_INSTRUMENT_VA(this, rhs); + + if (this != &rhs) + m_opaque_up = clone(rhs.m_opaque_up); + return *this; +} + +SBWatchpointOptions::~SBWatchpointOptions() = default; + +void SBWatchpointOptions::SetWatchpointTypeRead(bool read) { + m_opaque_up->m_read = read; +} +bool SBWatchpointOptions::GetWatchpointTypeRead() const { + return m_opaque_up->m_read; +} + +void SBWatchpointOptions::SetWatchpointTypeWrite(bool write) { + m_opaque_up->m_write = write; +} +bool SBWatchpointOptions::GetWatchpointTypeWrite() const { + return m_opaque_up->m_write; +} + +void SBWatchpointOptions::SetWatchpointTypeModify(bool modify) { + m_opaque_up->m_modify = modify; +} +bool SBWatchpointOptions::GetWatchpointTypeModify() const { + return m_opaque_up->m_modify; +} diff --git a/lldb/source/Breakpoint/Watchpoint.cpp b/lldb/source/Breakpoint/Watchpoint.cpp index d76f3638eff97..14144214faea7 100644 --- a/lldb/source/Breakpoint/Watchpoint.cpp +++ b/lldb/source/Breakpoint/Watchpoint.cpp @@ -194,7 +194,7 @@ bool Watchpoint::IsWatchVariable() const { return m_is_watch_variable; } void Watchpoint::SetWatchVariable(bool val) { m_is_watch_variable = val; } bool Watchpoint::CaptureWatchedValue(const ExecutionContext &exe_ctx) { - ConstString watch_name("$__lldb__watch_value"); + ConstString g_watch_name("$__lldb__watch_value"); m_old_value_sp = m_new_value_sp; Address watch_address(GetLoadAddress()); if (!m_type.IsValid()) { @@ -206,29 +206,35 @@ bool Watchpoint::CaptureWatchedValue(const ExecutionContext &exe_ctx) { return false; } m_new_value_sp = ValueObjectMemory::Create( - exe_ctx.GetBestExecutionContextScope(), watch_name.GetStringRef(), + exe_ctx.GetBestExecutionContextScope(), g_watch_name.GetStringRef(), watch_address, m_type); - m_new_value_sp = m_new_value_sp->CreateConstantValue(watch_name); + m_new_value_sp = m_new_value_sp->CreateConstantValue(g_watch_name); return (m_new_value_sp && m_new_value_sp->GetError().Success()); } bool Watchpoint::WatchedValueReportable(const ExecutionContext &exe_ctx) { - if (!m_watch_modify) + if (!m_watch_modify || m_watch_read) return true; if (!m_type.IsValid()) return true; - ConstString watch_name("$__lldb__watch_value"); + ConstString g_watch_name("$__lldb__watch_value"); Address watch_address(GetLoadAddress()); ValueObjectSP newest_valueobj_sp = ValueObjectMemory::Create( - exe_ctx.GetBestExecutionContextScope(), watch_name.GetStringRef(), + exe_ctx.GetBestExecutionContextScope(), g_watch_name.GetStringRef(), watch_address, m_type); - newest_valueobj_sp = newest_valueobj_sp->CreateConstantValue(watch_name); + newest_valueobj_sp = newest_valueobj_sp->CreateConstantValue(g_watch_name); + Status error; + DataExtractor new_data; DataExtractor old_data; - Status error; + newest_valueobj_sp->GetData(new_data, error); + if (error.Fail()) + return true; m_new_value_sp->GetData(old_data, error); + if (error.Fail()) + return true; if (new_data.GetByteSize() != old_data.GetByteSize() || new_data.GetByteSize() == 0) @@ -237,8 +243,8 @@ bool Watchpoint::WatchedValueReportable(const ExecutionContext &exe_ctx) { 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; + + return true; } // RETURNS - true if we should stop at this breakpoint, false if we diff --git a/lldb/source/Commands/CommandObjectWatchpoint.cpp b/lldb/source/Commands/CommandObjectWatchpoint.cpp index 69ca1ef42b6bd..83c2fb824bb9d 100644 --- a/lldb/source/Commands/CommandObjectWatchpoint.cpp +++ b/lldb/source/Commands/CommandObjectWatchpoint.cpp @@ -955,8 +955,8 @@ corresponding to the byte size of the data type."); case OptionGroupWatchpoint::eWatchRead: watch_type |= LLDB_WATCH_TYPE_READ; break; - case OptionGroupWatchpoint::eWatchReadModify: - watch_type |= LLDB_WATCH_TYPE_READ | LLDB_WATCH_TYPE_MODIFY; + case OptionGroupWatchpoint::eWatchReadWrite: + watch_type |= LLDB_WATCH_TYPE_READ | LLDB_WATCH_TYPE_WRITE; break; case OptionGroupWatchpoint::eWatchWrite: watch_type |= LLDB_WATCH_TYPE_WRITE; diff --git a/lldb/source/Interpreter/OptionGroupWatchpoint.cpp b/lldb/source/Interpreter/OptionGroupWatchpoint.cpp index 4c3715a685c3b..c3708e7a1e80f 100644 --- a/lldb/source/Interpreter/OptionGroupWatchpoint.cpp +++ b/lldb/source/Interpreter/OptionGroupWatchpoint.cpp @@ -33,9 +33,9 @@ static constexpr OptionEnumValueElement g_watch_type[] = { "Watch for modifications", }, { - OptionGroupWatchpoint::eWatchReadModify, + OptionGroupWatchpoint::eWatchReadWrite, "read_write", - "Watch for read/modify", + "Watch for read/write", }, }; diff --git a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp index fe46e64ee7707..77e8da5c05030 100644 --- a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp +++ b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp @@ -3112,7 +3112,8 @@ static GDBStoppointType GetGDBStoppointType(Watchpoint *wp) { bool watch_modify = wp->WatchpointModify(); // watch_read, watch_write, watch_modify cannot all be false. - assert(watch_read || watch_write || watch_modify); + assert((watch_read || watch_write || watch_modify) && + "watch_read, watch_write, watch_modify cannot all be false."); if (watch_read && (watch_write || watch_modify)) return eWatchpointReadWrite; else if (watch_read) diff --git a/lldb/source/Target/StopInfo.cpp b/lldb/source/Target/StopInfo.cpp index 01dc9f6770fbf..a189f4be1926a 100644 --- a/lldb/source/Target/StopInfo.cpp +++ b/lldb/source/Target/StopInfo.cpp @@ -985,9 +985,8 @@ class StopInfoWatchpoint : public StopInfo { // 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)) { + 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) { diff --git a/lldb/test/API/commands/watchpoints/multiple_hits/main.cpp b/lldb/test/API/commands/watchpoints/multiple_hits/main.cpp index c8ce8a0188691..f3596f21fa213 100644 --- a/lldb/test/API/commands/watchpoints/multiple_hits/main.cpp +++ b/lldb/test/API/commands/watchpoints/multiple_hits/main.cpp @@ -1,5 +1,6 @@ -#include #include +#include +#include alignas(16) uint8_t buf[32]; // This uses inline assembly to generate an instruction that writes to a large // block of memory. If it fails on your compiler/architecture, please add @@ -8,6 +9,7 @@ alignas(16) uint8_t buf[32]; // this test. int main() { + memset(buf, UINT32_MAX, 8); #if defined(__i386__) || defined(__x86_64__) asm volatile ("movdqa %%xmm0, %0" : : "m"(buf)); #elif defined(__arm__) diff --git a/lldb/test/API/commands/watchpoints/unaligned-watchpoint/main.c b/lldb/test/API/commands/watchpoints/unaligned-watchpoint/main.c index b071346e933a5..9d8de7a190b21 100644 --- a/lldb/test/API/commands/watchpoints/unaligned-watchpoint/main.c +++ b/lldb/test/API/commands/watchpoints/unaligned-watchpoint/main.c @@ -5,7 +5,7 @@ int main() { uint8_t buf[8]; uint64_t val; } a; - a.val = 0; // break here + a.val = UINT64_MAX; // break here for (int i = 0; i < 5; i++) { a.val = i; printf("a.val is %lu\n", a.val); diff --git a/lldb/test/API/functionalities/gdb_remote_client/TestNoWatchpointSupportInfo.py b/lldb/test/API/functionalities/gdb_remote_client/TestNoWatchpointSupportInfo.py index aaf4ef3e52c70..3f4132b921bab 100644 --- a/lldb/test/API/functionalities/gdb_remote_client/TestNoWatchpointSupportInfo.py +++ b/lldb/test/API/functionalities/gdb_remote_client/TestNoWatchpointSupportInfo.py @@ -58,7 +58,9 @@ def qXferRead(self, obj, annex, offset, length): print(result.GetOutput()) err = lldb.SBError() - wp = target.WatchAddress(0x100, 8, False, True, err) + wp_opts = lldb.SBWatchpointOptions() + wp_opts.SetWatchpointTypeModify(True) + wp = target.WatchpointCreateByAddress(0x100, 8, wp_opts, err) if self.TraceOn() and (err.Fail() or wp.IsValid == False): strm = lldb.SBStream() err.GetDescription(strm) diff --git a/lldb/test/API/functionalities/watchpoint/large-watchpoint/TestLargeWatchpoint.py b/lldb/test/API/functionalities/watchpoint/large-watchpoint/TestLargeWatchpoint.py index 07895c192ec1b..5e3d16c6042b0 100644 --- a/lldb/test/API/functionalities/watchpoint/large-watchpoint/TestLargeWatchpoint.py +++ b/lldb/test/API/functionalities/watchpoint/large-watchpoint/TestLargeWatchpoint.py @@ -45,7 +45,9 @@ def test_large_watchpoint(self): # to a 1024 byte boundary to begin with, force alignment. wa_256_addr = (array_addr + 1024) & ~(1024 - 1) err = lldb.SBError() - wp = target.WatchAddress(wa_256_addr, 1024, False, True, err) + wp_opts = lldb.SBWatchpointOptions() + wp_opts.SetWatchpointTypeModify(True) + wp = target.WatchpointCreateByAddress(wa_256_addr, 1024, wp_opts, err) self.assertTrue(wp.IsValid()) self.assertSuccess(err) diff --git a/lldb/test/API/functionalities/watchpoint/modify-watchpoints/TestModifyWatchpoint.py b/lldb/test/API/functionalities/watchpoint/modify-watchpoints/TestModifyWatchpoint.py index 1a479c9f25e46..2cbc8d43ce426 100644 --- a/lldb/test/API/functionalities/watchpoint/modify-watchpoints/TestModifyWatchpoint.py +++ b/lldb/test/API/functionalities/watchpoint/modify-watchpoints/TestModifyWatchpoint.py @@ -25,3 +25,15 @@ def test_modify_watchpoint(self): process.Continue() frame = process.GetSelectedThread().GetFrameAtIndex(0) self.assertEqual(frame.locals["value"][0].GetValueAsUnsigned(), 10) + + process.Continue() + frame = process.GetSelectedThread().GetFrameAtIndex(0) + self.assertEqual(frame.locals["value"][0].GetValueAsUnsigned(), 5) + + process.Continue() + frame = process.GetSelectedThread().GetFrameAtIndex(0) + self.assertEqual(frame.locals["value"][0].GetValueAsUnsigned(), 7) + + process.Continue() + frame = process.GetSelectedThread().GetFrameAtIndex(0) + self.assertEqual(frame.locals["value"][0].GetValueAsUnsigned(), 9) diff --git a/lldb/test/API/functionalities/watchpoint/modify-watchpoints/main.c b/lldb/test/API/functionalities/watchpoint/modify-watchpoints/main.c index 3f137b36c40af..819c96c15fb08 100644 --- a/lldb/test/API/functionalities/watchpoint/modify-watchpoints/main.c +++ b/lldb/test/API/functionalities/watchpoint/modify-watchpoints/main.c @@ -12,6 +12,9 @@ int main() { value = 10; value = 10; value = 10; + value = 5; + value = 7; + value = 9; return value; } diff --git a/lldb/test/API/functionalities/watchpoint/unaligned-spanning-two-dwords/TestUnalignedSpanningDwords.py b/lldb/test/API/functionalities/watchpoint/unaligned-spanning-two-dwords/TestUnalignedSpanningDwords.py index 602532c26b079..6d041cb5709d1 100644 --- a/lldb/test/API/functionalities/watchpoint/unaligned-spanning-two-dwords/TestUnalignedSpanningDwords.py +++ b/lldb/test/API/functionalities/watchpoint/unaligned-spanning-two-dwords/TestUnalignedSpanningDwords.py @@ -46,7 +46,9 @@ def test_unaligned_watchpoint(self): a_bytebuf_6 = frame.GetValueForVariablePath("a.bytebuf[6]") a_bytebuf_6_addr = a_bytebuf_6.GetAddress().GetLoadAddress(target) err = lldb.SBError() - 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()) self.assertTrue(wp.IsEnabled()) self.assertEqual(wp.GetWatchSize(), 4) diff --git a/lldb/test/API/python_api/default-constructor/sb_target.py b/lldb/test/API/python_api/default-constructor/sb_target.py index 2ba72d39f9101..baf7851263ea2 100644 --- a/lldb/test/API/python_api/default-constructor/sb_target.py +++ b/lldb/test/API/python_api/default-constructor/sb_target.py @@ -53,7 +53,10 @@ def fuzz_obj(obj): obj.GetByteOrder() obj.GetTriple() error = lldb.SBError() - obj.WatchAddress(123, 8, True, True, error) + wp_opts = lldb.SBWatchpointOptions() + wp_opts.SetWatchpointTypeRead(True) + wp_opts.SetWatchpointTypeModify(True) + obj.WatchpointCreateByAddress(123, 8, wp_opts, error) obj.GetBroadcaster() obj.GetDescription(lldb.SBStream(), lldb.eDescriptionLevelBrief) obj.Clear() diff --git a/lldb/test/API/python_api/watchpoint/watchlocation/TestTargetWatchAddress.py b/lldb/test/API/python_api/watchpoint/watchlocation/TestTargetWatchAddress.py index 79124b3991e7d..e09423be2e9e5 100644 --- a/lldb/test/API/python_api/watchpoint/watchlocation/TestTargetWatchAddress.py +++ b/lldb/test/API/python_api/watchpoint/watchlocation/TestTargetWatchAddress.py @@ -1,5 +1,5 @@ """ -Use lldb Python SBtarget.WatchAddress() API to create a watchpoint for write of '*g_char_ptr'. +Use lldb Python SBtarget.WatchpointCreateByAddress() API to create a watchpoint for write of '*g_char_ptr'. """ import lldb @@ -8,7 +8,7 @@ from lldbsuite.test import lldbutil -class TargetWatchAddressAPITestCase(TestBase): +class TargetWatchpointCreateByAddressPITestCase(TestBase): NO_DEBUG_INFO_TESTCASE = True def setUp(self): @@ -22,7 +22,7 @@ def setUp(self): self.violating_func = "do_bad_thing_with_location" def test_watch_address(self): - """Exercise SBTarget.WatchAddress() API to set a watchpoint.""" + """Exercise SBTarget.WatchpointCreateByAddress() API to set a watchpoint.""" self.build() exe = self.getBuildArtifact("a.out") @@ -51,8 +51,10 @@ def test_watch_address(self): ) # Watch for write to *g_char_ptr. error = lldb.SBError() - watchpoint = target.WatchAddress( - value.GetValueAsUnsigned(), 1, False, True, error + wp_opts = lldb.SBWatchpointOptions() + wp_opts.SetWatchpointTypeModify(True) + watchpoint = target.WatchpointCreateByAddress( + value.GetValueAsUnsigned(), 1, wp_opts, error ) self.assertTrue( value and watchpoint, "Successfully found the pointer and set a watchpoint" @@ -90,7 +92,7 @@ def test_watch_address(self): @skipIf(archs=["mips", "mipsel", "mips64", "mips64el"]) @skipIf(archs=["s390x"]) # Likewise on SystemZ def test_watch_address_with_invalid_watch_size(self): - """Exercise SBTarget.WatchAddress() API but pass an invalid watch_size.""" + """Exercise SBTarget.WatchpointCreateByAddress() API but pass an invalid watch_size.""" self.build() exe = self.getBuildArtifact("a.out") @@ -124,8 +126,10 @@ def test_watch_address_with_invalid_watch_size(self): if self.getArchitecture() not in ["arm64", "arm64e", "arm64_32"]: # Watch for write to *g_char_ptr. error = lldb.SBError() - watchpoint = target.WatchAddress( - value.GetValueAsUnsigned(), 365, False, True, error + wp_opts = lldb.SBWatchpointOptions() + wp_opts.SetWatchpointTypeModify(True) + watchpoint = target.WatchpointCreateByAddress( + value.GetValueAsUnsigned(), 365, wp_opts, error ) self.assertFalse(watchpoint) self.expect( From f8da42f5e1a00292d63b52735b51ca3505c14723 Mon Sep 17 00:00:00 2001 From: Jason Molenda Date: Mon, 18 Sep 2023 15:52:58 -0700 Subject: [PATCH 3/4] Address Greg's feedback, most important, enum for type of write 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'. --- .../interface/SBWatchpointOptionsDocstrings.i | 8 ++---- lldb/include/lldb/API/SBTarget.h | 2 +- lldb/include/lldb/API/SBWatchpointOptions.h | 9 +++---- lldb/include/lldb/lldb-enumerations.h | 15 +++++++++++ lldb/source/API/SBTarget.cpp | 8 +++--- lldb/source/API/SBWatchpointOptions.cpp | 26 ++++++++++++------- .../TestNoWatchpointSupportInfo.py | 2 +- .../large-watchpoint/TestLargeWatchpoint.py | 2 +- .../TestUnalignedSpanningDwords.py | 2 +- .../default-constructor/sb_target.py | 2 +- .../watchlocation/TestTargetWatchAddress.py | 4 +-- 11 files changed, 48 insertions(+), 32 deletions(-) diff --git a/lldb/bindings/interface/SBWatchpointOptionsDocstrings.i b/lldb/bindings/interface/SBWatchpointOptionsDocstrings.i index 5076065d285d5..3b696f5228ed9 100644 --- a/lldb/bindings/interface/SBWatchpointOptionsDocstrings.i +++ b/lldb/bindings/interface/SBWatchpointOptionsDocstrings.i @@ -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." ) lldb::SBWatchpointOptions::SetWatchpointTypeWrite; -%feature("docstring", "Gets whether the watchpoint should stop on write accesses." +%feature("docstring", "Gets whether the watchpoint should stop on write accesses, returning WatchpointWriteType to indicate the type of write watching that is enabled, or eWatchpointWriteTypeDisabled." ) lldb::SBWatchpointOptions::GetWatchpointTypeWrite; -%feature("docstring", "Sets whether the watchpoint should stop on modify accesses." -) lldb::SBWatchpointOptions::SetWatchpointTypeModify; -%feature("docstring", "Gets whether the watchpoint should stop on modify accesses." -) lldb::SBWatchpointOptions::GetWatchpointTypeModify; diff --git a/lldb/include/lldb/API/SBTarget.h b/lldb/include/lldb/API/SBTarget.h index 06026e7d0b863..83087623088c5 100644 --- a/lldb/include/lldb/API/SBTarget.h +++ b/lldb/include/lldb/API/SBTarget.h @@ -831,7 +831,7 @@ class LLDB_API SBTarget { LLDB_DEPRECATED("WatchAddress deprecated, use WatchpointCreateByAddress") lldb::SBWatchpoint WatchAddress(lldb::addr_t addr, size_t size, bool read, - bool write, SBError &error); + bool modify, SBError &error); lldb::SBWatchpoint WatchpointCreateByAddress(lldb::addr_t addr, size_t size, diff --git a/lldb/include/lldb/API/SBWatchpointOptions.h b/lldb/include/lldb/API/SBWatchpointOptions.h index 6f561bca1cb23..5d1d6ab9e09ff 100644 --- a/lldb/include/lldb/API/SBWatchpointOptions.h +++ b/lldb/include/lldb/API/SBWatchpointOptions.h @@ -25,14 +25,13 @@ class LLDB_API SBWatchpointOptions { const SBWatchpointOptions &operator=(const lldb::SBWatchpointOptions &rhs); + /// Stop when the watched memory region is read. void SetWatchpointTypeRead(bool read); bool GetWatchpointTypeRead() const; - void SetWatchpointTypeWrite(bool write); - bool GetWatchpointTypeWrite() const; - - void SetWatchpointTypeModify(bool modify); - bool GetWatchpointTypeModify() const; + /// Stop when the watched memory region is written to/modified + void SetWatchpointTypeWrite(lldb::WatchpointWriteType write_type); + lldb::WatchpointWriteType GetWatchpointTypeWrite() const; private: // This auto_pointer is made in the constructor and is always valid. diff --git a/lldb/include/lldb/lldb-enumerations.h b/lldb/include/lldb/lldb-enumerations.h index 21e098481ce80..36f3030c5226d 100644 --- a/lldb/include/lldb/lldb-enumerations.h +++ b/lldb/include/lldb/lldb-enumerations.h @@ -431,6 +431,21 @@ FLAGS_ENUM(WatchpointEventType){ eWatchpointEventTypeThreadChanged = (1u << 11), eWatchpointEventTypeTypeChanged = (1u << 12)}; +enum WatchpointWriteType { + /// Don't stop when the watched memory region is written to. + eWatchpointWriteTypeDisabled, + /// Stop on any write access to the memory region, even if + /// the value doesn't change. On some architectures, a write + /// near the memory region may be falsely reported as a match, + /// and notify this spurious stop as a watchpoint trap. + eWatchpointWriteTypeAlways, + /// Stop on a write to the memory region that changes its value. + /// This is most likely the behavior a user expects, and is the + /// behavior in gdb. lldb can silently ignore writes near the + /// watched memory region that are reported as accesses to lldb. + eWatchpointWriteTypeOnModify +}; + /// Programming language type. /// /// These enumerations use the same language enumerations as the DWARF diff --git a/lldb/source/API/SBTarget.cpp b/lldb/source/API/SBTarget.cpp index 5b4eea7a6ccef..a4a7ac338c207 100644 --- a/lldb/source/API/SBTarget.cpp +++ b/lldb/source/API/SBTarget.cpp @@ -1322,13 +1322,13 @@ SBWatchpoint SBTarget::FindWatchpointByID(lldb::watch_id_t wp_id) { } lldb::SBWatchpoint SBTarget::WatchAddress(lldb::addr_t addr, size_t size, - bool read, bool write, + bool read, bool modify, SBError &error) { LLDB_INSTRUMENT_VA(this, addr, size, read, write, error); SBWatchpointOptions options; options.SetWatchpointTypeRead(read); - options.SetWatchpointTypeModify(write); + options.SetWatchpointTypeWrite(eWatchpointWriteTypeOnModify); return WatchpointCreateByAddress(addr, size, options, error); } @@ -1344,9 +1344,9 @@ SBTarget::WatchpointCreateByAddress(lldb::addr_t addr, size_t size, uint32_t watch_type = 0; if (options.GetWatchpointTypeRead()) watch_type |= LLDB_WATCH_TYPE_READ; - if (options.GetWatchpointTypeWrite()) + if (options.GetWatchpointTypeWrite() == eWatchpointWriteTypeAlways) watch_type |= LLDB_WATCH_TYPE_WRITE; - if (options.GetWatchpointTypeModify()) + if (options.GetWatchpointTypeWrite() == eWatchpointWriteTypeOnModify) watch_type |= LLDB_WATCH_TYPE_MODIFY; if (watch_type == 0) { error.SetErrorString("Can't create a watchpoint that is neither read nor " diff --git a/lldb/source/API/SBWatchpointOptions.cpp b/lldb/source/API/SBWatchpointOptions.cpp index 619ad411b6c06..62e9c21a85795 100644 --- a/lldb/source/API/SBWatchpointOptions.cpp +++ b/lldb/source/API/SBWatchpointOptions.cpp @@ -52,16 +52,22 @@ bool SBWatchpointOptions::GetWatchpointTypeRead() const { return m_opaque_up->m_read; } -void SBWatchpointOptions::SetWatchpointTypeWrite(bool write) { - m_opaque_up->m_write = write; -} -bool SBWatchpointOptions::GetWatchpointTypeWrite() const { - return m_opaque_up->m_write; +void SBWatchpointOptions::SetWatchpointTypeWrite( + WatchpointWriteType write_type) { + if (write_type == eWatchpointWriteTypeOnModify) { + m_opaque_up->m_write = false; + m_opaque_up->m_modify = true; + } else if (write_type == eWatchpointWriteTypeAlways) { + m_opaque_up->m_write = true; + m_opaque_up->m_modify = false; + } else + m_opaque_up->m_write = m_opaque_up->m_modify = false; } -void SBWatchpointOptions::SetWatchpointTypeModify(bool modify) { - m_opaque_up->m_modify = modify; -} -bool SBWatchpointOptions::GetWatchpointTypeModify() const { - return m_opaque_up->m_modify; +WatchpointWriteType SBWatchpointOptions::GetWatchpointTypeWrite() const { + if (m_opaque_up->m_modify) + return eWatchpointWriteTypeOnModify; + if (m_opaque_up->m_write) + return eWatchpointWriteTypeAlways; + return eWatchpointWriteTypeDisabled; } diff --git a/lldb/test/API/functionalities/gdb_remote_client/TestNoWatchpointSupportInfo.py b/lldb/test/API/functionalities/gdb_remote_client/TestNoWatchpointSupportInfo.py index 3f4132b921bab..a51b228e917cc 100644 --- a/lldb/test/API/functionalities/gdb_remote_client/TestNoWatchpointSupportInfo.py +++ b/lldb/test/API/functionalities/gdb_remote_client/TestNoWatchpointSupportInfo.py @@ -59,7 +59,7 @@ def qXferRead(self, obj, annex, offset, length): err = lldb.SBError() wp_opts = lldb.SBWatchpointOptions() - wp_opts.SetWatchpointTypeModify(True) + wp_opts.SetWatchpointTypeWrite(lldb.eWatchpointWriteTypeOnModify) wp = target.WatchpointCreateByAddress(0x100, 8, wp_opts, err) if self.TraceOn() and (err.Fail() or wp.IsValid == False): strm = lldb.SBStream() diff --git a/lldb/test/API/functionalities/watchpoint/large-watchpoint/TestLargeWatchpoint.py b/lldb/test/API/functionalities/watchpoint/large-watchpoint/TestLargeWatchpoint.py index 5e3d16c6042b0..c5e161497e628 100644 --- a/lldb/test/API/functionalities/watchpoint/large-watchpoint/TestLargeWatchpoint.py +++ b/lldb/test/API/functionalities/watchpoint/large-watchpoint/TestLargeWatchpoint.py @@ -46,7 +46,7 @@ def test_large_watchpoint(self): wa_256_addr = (array_addr + 1024) & ~(1024 - 1) err = lldb.SBError() wp_opts = lldb.SBWatchpointOptions() - wp_opts.SetWatchpointTypeModify(True) + wp_opts.SetWatchpointTypeWrite(lldb.eWatchpointWriteTypeOnModify) wp = target.WatchpointCreateByAddress(wa_256_addr, 1024, wp_opts, err) self.assertTrue(wp.IsValid()) self.assertSuccess(err) diff --git a/lldb/test/API/functionalities/watchpoint/unaligned-spanning-two-dwords/TestUnalignedSpanningDwords.py b/lldb/test/API/functionalities/watchpoint/unaligned-spanning-two-dwords/TestUnalignedSpanningDwords.py index 6d041cb5709d1..e37a74f1f5aac 100644 --- a/lldb/test/API/functionalities/watchpoint/unaligned-spanning-two-dwords/TestUnalignedSpanningDwords.py +++ b/lldb/test/API/functionalities/watchpoint/unaligned-spanning-two-dwords/TestUnalignedSpanningDwords.py @@ -47,7 +47,7 @@ def test_unaligned_watchpoint(self): a_bytebuf_6_addr = a_bytebuf_6.GetAddress().GetLoadAddress(target) err = lldb.SBError() wp_opts = lldb.SBWatchpointOptions() - wp_opts.SetWatchpointTypeModify(True) + wp_opts.SetWatchpointTypeWrite(lldb.eWatchpointWriteTypeOnModify) wp = target.WatchpointCreateByAddress(a_bytebuf_6_addr, 4, wp_opts, err) self.assertTrue(err.Success()) self.assertTrue(wp.IsEnabled()) diff --git a/lldb/test/API/python_api/default-constructor/sb_target.py b/lldb/test/API/python_api/default-constructor/sb_target.py index baf7851263ea2..1acf1024a0d91 100644 --- a/lldb/test/API/python_api/default-constructor/sb_target.py +++ b/lldb/test/API/python_api/default-constructor/sb_target.py @@ -55,7 +55,7 @@ def fuzz_obj(obj): error = lldb.SBError() wp_opts = lldb.SBWatchpointOptions() wp_opts.SetWatchpointTypeRead(True) - wp_opts.SetWatchpointTypeModify(True) + wp_opts.SetWatchpointTypeWrite(lldb.eWatchpointWriteTypeOnModify) obj.WatchpointCreateByAddress(123, 8, wp_opts, error) obj.GetBroadcaster() obj.GetDescription(lldb.SBStream(), lldb.eDescriptionLevelBrief) diff --git a/lldb/test/API/python_api/watchpoint/watchlocation/TestTargetWatchAddress.py b/lldb/test/API/python_api/watchpoint/watchlocation/TestTargetWatchAddress.py index e09423be2e9e5..bf31819d4070d 100644 --- a/lldb/test/API/python_api/watchpoint/watchlocation/TestTargetWatchAddress.py +++ b/lldb/test/API/python_api/watchpoint/watchlocation/TestTargetWatchAddress.py @@ -52,7 +52,7 @@ def test_watch_address(self): # Watch for write to *g_char_ptr. error = lldb.SBError() wp_opts = lldb.SBWatchpointOptions() - wp_opts.SetWatchpointTypeModify(True) + wp_opts.SetWatchpointTypeWrite(lldb.eWatchpointWriteTypeOnModify) watchpoint = target.WatchpointCreateByAddress( value.GetValueAsUnsigned(), 1, wp_opts, error ) @@ -127,7 +127,7 @@ def test_watch_address_with_invalid_watch_size(self): # Watch for write to *g_char_ptr. error = lldb.SBError() wp_opts = lldb.SBWatchpointOptions() - wp_opts.SetWatchpointTypeModify(True) + wp_opts.SetWatchpointTypeWrite(lldb.eWatchpointWriteTypeOnModify) watchpoint = target.WatchpointCreateByAddress( value.GetValueAsUnsigned(), 365, wp_opts, error ) From 19d2f0614756686da5ba78dcb945d1880fac9ef1 Mon Sep 17 00:00:00 2001 From: Jason Molenda Date: Mon, 18 Sep 2023 19:12:14 -0700 Subject: [PATCH 4/4] Don't detail the corner case of false watchpoint stops in the SBWatchpointOptions docstring. --- lldb/bindings/interface/SBWatchpointOptionsDocstrings.i | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lldb/bindings/interface/SBWatchpointOptionsDocstrings.i b/lldb/bindings/interface/SBWatchpointOptionsDocstrings.i index 3b696f5228ed9..370bf95401dcb 100644 --- a/lldb/bindings/interface/SBWatchpointOptionsDocstrings.i +++ b/lldb/bindings/interface/SBWatchpointOptionsDocstrings.i @@ -6,7 +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. 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." +%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, even if it's the value is the same." ) lldb::SBWatchpointOptions::SetWatchpointTypeWrite; %feature("docstring", "Gets whether the watchpoint should stop on write accesses, returning WatchpointWriteType to indicate the type of write watching that is enabled, or eWatchpointWriteTypeDisabled." ) lldb::SBWatchpointOptions::GetWatchpointTypeWrite;