[lldb] WatchAddress ignores modify option #124847

puremourning · 2025-01-28T21:53:30Z

The WatchAddress API includes a flag to indicate if watchpoint should be for read, modify or both. This API uses 2 booleans, but the 'modify' flag was ignored and WatchAddress unconditionally watched write (actually modify).

We now only watch for modify when the modify flag is true.

The included test fails prior to this patch and succeeds after. That is previously specifying False for modify would still stop on write, but after the patch correctly only stops on read

llvmbot · 2025-01-28T21:54:03Z

@llvm/pr-subscribers-lldb

Author: Ben Jackson (puremourning)

Changes

The WatchAddress API includes a flag to indicate if watchpoint should be for read, modify or both. This API uses 2 booleans, but the 'modify' flag was ignored and WatchAddress unconditionally watched write (actually modify).

We now only watch for modify when the modify flag is true.

The included test fails prior to this patch and succeeds after. That is previously specifying False for modify would still stop on write, but after the patch correctly only stops on read

Full diff: https://github.com/llvm/llvm-project/pull/124847.diff

3 Files Affected:

(modified) lldb/source/API/SBTarget.cpp (+3-1)
(added) lldb/test/API/python_api/watchpoint/TestWatchpointRead.py (+130)
(modified) lldb/test/API/python_api/watchpoint/watchlocation/TestTargetWatchAddress.py (+70-1)

diff --git a/lldb/source/API/SBTarget.cpp b/lldb/source/API/SBTarget.cpp
index 2a33161bc21edc..9192f0be1c85b9 100644
--- a/lldb/source/API/SBTarget.cpp
+++ b/lldb/source/API/SBTarget.cpp
@@ -1342,7 +1342,9 @@ lldb::SBWatchpoint SBTarget::WatchAddress(lldb::addr_t addr, size_t size,
 
   SBWatchpointOptions options;
   options.SetWatchpointTypeRead(read);
-  options.SetWatchpointTypeWrite(eWatchpointWriteTypeOnModify);
+  if (modify) {
+    options.SetWatchpointTypeWrite(eWatchpointWriteTypeOnModify);
+  }
   return WatchpointCreateByAddress(addr, size, options, error);
 }
 
diff --git a/lldb/test/API/python_api/watchpoint/TestWatchpointRead.py b/lldb/test/API/python_api/watchpoint/TestWatchpointRead.py
new file mode 100644
index 00000000000000..67b04464748108
--- /dev/null
+++ b/lldb/test/API/python_api/watchpoint/TestWatchpointRead.py
@@ -0,0 +1,130 @@
+"""
+Use lldb Python SBTarget API to set read watchpoints
+"""
+
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+
+class SetWatchpointAPITestCase(TestBase):
+    NO_DEBUG_INFO_TESTCASE = True
+
+    def setUp(self):
+        # Call super's setUp().
+        TestBase.setUp(self)
+        # Our simple source filename.
+        self.source = "main.c"
+        # Find the line number to break inside main().
+        self.line = line_number(self.source, "// Set break point at this line.")
+        self.build()
+
+
+    def test_read_watchpoint_watch_address(self):
+        exe = self.getBuildArtifact("a.out")
+
+        target = self.dbg.CreateTarget(exe)
+        self.assertTrue(target, VALID_TARGET)
+
+        # Now create a breakpoint on main.c.
+        breakpoint = target.BreakpointCreateByLocation(self.source, self.line)
+        self.assertTrue(
+            breakpoint and breakpoint.GetNumLocations() == 1, VALID_BREAKPOINT
+        )
+
+        # Now launch the process, and do not stop at the entry point.
+        process = target.LaunchSimple(None, None, self.get_process_working_directory())
+
+        # We should be stopped due to the breakpoint.  Get frame #0.
+        process = target.GetProcess()
+        self.assertState(process.GetState(), lldb.eStateStopped, PROCESS_STOPPED)
+        thread = lldbutil.get_stopped_thread(process, lldb.eStopReasonBreakpoint)
+        frame0 = thread.GetFrameAtIndex(0)
+
+
+        value = frame0.FindValue("global", lldb.eValueTypeVariableGlobal)
+        local = frame0.FindValue("local", lldb.eValueTypeVariableLocal)
+        error = lldb.SBError()
+
+        watchpoint = target.WatchAddress(
+            value.GetLoadAddress(), 1, True, False, error
+        )
+        self.assertTrue(
+            value and local and watchpoint, "Successfully found the values and set a watchpoint"
+        )
+        self.DebugSBValue(value)
+        self.DebugSBValue(local)
+
+        # Hide stdout if not running with '-t' option.
+        if not self.TraceOn():
+            self.HideStdout()
+
+        print(watchpoint)
+
+        # Continue.  Expect the program to stop due to the variable being
+        # read, but *not* written to.
+        process.Continue()
+
+        if self.TraceOn():
+            lldbutil.print_stacktraces(process)
+
+        self.assertTrue(
+            local.GetValueAsSigned() > 0,
+            "The local variable has been incremented"
+        )
+
+    def test_read_watchpoint_watch_create_by_address(self):
+        exe = self.getBuildArtifact("a.out")
+
+        target = self.dbg.CreateTarget(exe)
+        self.assertTrue(target, VALID_TARGET)
+
+        # Now create a breakpoint on main.c.
+        breakpoint = target.BreakpointCreateByLocation(self.source, self.line)
+        self.assertTrue(
+            breakpoint and breakpoint.GetNumLocations() == 1, VALID_BREAKPOINT
+        )
+
+        # Now launch the process, and do not stop at the entry point.
+        process = target.LaunchSimple(None, None, self.get_process_working_directory())
+
+        # We should be stopped due to the breakpoint.  Get frame #0.
+        process = target.GetProcess()
+        self.assertState(process.GetState(), lldb.eStateStopped, PROCESS_STOPPED)
+        thread = lldbutil.get_stopped_thread(process, lldb.eStopReasonBreakpoint)
+        frame0 = thread.GetFrameAtIndex(0)
+
+
+        value = frame0.FindValue("global", lldb.eValueTypeVariableGlobal)
+        local = frame0.FindValue("local", lldb.eValueTypeVariableLocal)
+        error = lldb.SBError()
+
+        wp_opts = lldb.SBWatchpointOptions()
+        wp_opts.SetWatchpointTypeRead(True)
+        watchpoint = target.WatchpointCreateByAddress(
+            value.GetLoadAddress(), 1, wp_opts, error
+        )
+        self.assertTrue(
+            value and local and watchpoint, "Successfully found the values and set a watchpoint"
+        )
+        self.DebugSBValue(value)
+        self.DebugSBValue(local)
+
+        # Hide stdout if not running with '-t' option.
+        if not self.TraceOn():
+            self.HideStdout()
+
+        print(watchpoint)
+
+        # Continue.  Expect the program to stop due to the variable being
+        # read, but *not* written to.
+        process.Continue()
+
+        if self.TraceOn():
+            lldbutil.print_stacktraces(process)
+
+        self.assertTrue(
+            local.GetValueAsSigned() > 0,
+            "The local variable has been incremented"
+        )
diff --git a/lldb/test/API/python_api/watchpoint/watchlocation/TestTargetWatchAddress.py b/lldb/test/API/python_api/watchpoint/watchlocation/TestTargetWatchAddress.py
index cbab3c6382e431..7a0e42a4fc2781 100644
--- a/lldb/test/API/python_api/watchpoint/watchlocation/TestTargetWatchAddress.py
+++ b/lldb/test/API/python_api/watchpoint/watchlocation/TestTargetWatchAddress.py
@@ -21,7 +21,7 @@ def setUp(self):
         # This is for verifying that watch location works.
         self.violating_func = "do_bad_thing_with_location"
 
-    def test_watch_address(self):
+    def test_watch_create_by_address(self):
         """Exercise SBTarget.WatchpointCreateByAddress() API to set a watchpoint."""
         self.build()
         exe = self.getBuildArtifact("a.out")
@@ -88,6 +88,75 @@ def test_watch_address(self):
 
         # This finishes our test.
 
+    def test_watch_address(self):
+        """Exercise SBTarget.WatchAddress() API to set a watchpoint.
+        Same as test_watch_create_by_address, but uses the simpler API.
+        """
+        self.build()
+        exe = self.getBuildArtifact("a.out")
+
+        # Create a target by the debugger.
+        target = self.dbg.CreateTarget(exe)
+        self.assertTrue(target, VALID_TARGET)
+
+        # Now create a breakpoint on main.c.
+        breakpoint = target.BreakpointCreateByLocation(self.source, self.line)
+        self.assertTrue(
+            breakpoint and breakpoint.GetNumLocations() == 1, VALID_BREAKPOINT
+        )
+
+        # Now launch the process, and do not stop at the entry point.
+        process = target.LaunchSimple(None, None, self.get_process_working_directory())
+
+        # We should be stopped due to the breakpoint.  Get frame #0.
+        process = target.GetProcess()
+        self.assertState(process.GetState(), lldb.eStateStopped, PROCESS_STOPPED)
+        thread = lldbutil.get_stopped_thread(process, lldb.eStopReasonBreakpoint)
+        frame0 = thread.GetFrameAtIndex(0)
+
+        value = frame0.FindValue("g_char_ptr", lldb.eValueTypeVariableGlobal)
+        pointee = value.CreateValueFromAddress(
+            "pointee", value.GetValueAsUnsigned(0), value.GetType().GetPointeeType()
+        )
+        # Watch for write to *g_char_ptr.
+        error = lldb.SBError()
+        watch_read = False
+        watch_write = True
+        watchpoint = target.WatchAddress(
+            value.GetValueAsUnsigned(), 1, watch_read, watch_write, error
+        )
+        self.assertTrue(
+            value and watchpoint, "Successfully found the pointer and set a watchpoint"
+        )
+        self.DebugSBValue(value)
+        self.DebugSBValue(pointee)
+
+        # Hide stdout if not running with '-t' option.
+        if not self.TraceOn():
+            self.HideStdout()
+
+        print(watchpoint)
+
+        # Continue.  Expect the program to stop due to the variable being
+        # written to.
+        process.Continue()
+
+        if self.TraceOn():
+            lldbutil.print_stacktraces(process)
+
+        thread = lldbutil.get_stopped_thread(process, lldb.eStopReasonWatchpoint)
+        self.assertTrue(thread, "The thread stopped due to watchpoint")
+        self.DebugSBValue(value)
+        self.DebugSBValue(pointee)
+
+        self.expect(
+            lldbutil.print_stacktrace(thread, string_buffer=True),
+            exe=False,
+            substrs=[self.violating_func],
+        )
+
+        # This finishes our test.
+
     # No size constraint on MIPS for watches
     @skipIf(archs=["mips", "mipsel", "mips64", "mips64el"])
     @skipIf(archs=["s390x"])  # Likewise on SystemZ

github-actions · 2025-01-28T21:59:20Z

✅ With the latest revision this PR passed the Python code formatter.

lldb/source/API/SBTarget.cpp

jasonmolenda

This looks good to me, thanks for fixing this. This method used to take two bools, read, and write, I redefined the second to modified when I changed the default watchpoints to be modify-style. The method previously had a block doing

-    uint32_t watch_type = 0;
-    if (read)
-      watch_type |= LLDB_WATCH_TYPE_READ;
-    if (write)
-      watch_type |= LLDB_WATCH_TYPE_WRITE;

and in rewriting this to set an SBWatchpointOptions object, I didn't update it correctly to allow someone to request a read-only watchpoint. The test cases look good, thanks for taking the time to write those. I'd address the nit Jonas pointed out.

puremourning · 2025-01-30T13:33:05Z

This looks good to me, thanks for fixing this. This method used to take two bools, read, and write, I redefined the second to modified when I changed the default watchpoints to be modify-style. The method previously had a block doing
-    uint32_t watch_type = 0;
-    if (read)
-      watch_type |= LLDB_WATCH_TYPE_READ;
-    if (write)
-      watch_type |= LLDB_WATCH_TYPE_WRITE;
and in rewriting this to set an SBWatchpointOptions object, I didn't update it correctly to allow someone to request a read-only watchpoint. The test cases look good, thanks for taking the time to write those. I'd address the nit Jonas pointed out.

Thanks, yeah I figured. Nit squashed.

puremourning · 2025-01-30T13:48:39Z

OK so the test is failing on CI, that's bad news.

will check

FAIL: LLDB (/var/lib/buildkite-agent/builds/linux-56-59b8f5d88-j6q2n-1/llvm-project/github-pull-requests/build/bin/clang-x86_64) :: test_read_watchpoint_watch_address (TestWatchpointRead.SetReadOnlyWatchpointTestCase)
FAIL: LLDB (/var/lib/buildkite-agent/builds/linux-56-59b8f5d88-j6q2n-1/llvm-project/github-pull-requests/build/bin/clang-x86_64) :: test_read_watchpoint_watch_create_by_address (TestWatchpointRead.SetReadOnlyWatchpointTestCase)
======================================================================
FAIL: test_read_watchpoint_watch_address (TestWatchpointRead.SetReadOnlyWatchpointTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/var/lib/buildkite-agent/builds/linux-56-59b8f5d88-j6q2n-1/llvm-project/github-pull-requests/lldb/test/API/python_api/watchpoint/TestWatchpointRead.py", line 69, in test_read_watchpoint_watch_address
    self.assertTrue(
AssertionError: False is not true : The local variable has been incremented
Config=x86_64-/var/lib/buildkite-agent/builds/linux-56-59b8f5d88-j6q2n-1/llvm-project/github-pull-requests/build/bin/clang
======================================================================
FAIL: test_read_watchpoint_watch_create_by_address (TestWatchpointRead.SetReadOnlyWatchpointTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/var/lib/buildkite-agent/builds/linux-56-59b8f5d88-j6q2n-1/llvm-project/github-pull-requests/lldb/test/API/python_api/watchpoint/TestWatchpointRead.py", line 123, in test_read_watchpoint_watch_create_by_address
    self.assertTrue(
AssertionError: False is not true : The local variable has been incremented
Config=x86_64-/var/lib/buildkite-agent/builds/linux-56-59b8f5d88-j6q2n-1/llvm-project/github-pull-requests/build/bin/clang
----------------------------------------------------------------------
Ran 2 tests in 0.908s

puremourning · 2025-01-30T14:16:48Z

Hmm seems the behaviour on intel differs to arm. will need to think about it.

puremourning · 2025-01-31T21:35:20Z

I think the issue is that... we just don't support read-only watchpoints on intel because the hardware doesn't support it.

I guess I'll mark this test as "skip on x86"

The WatchAddress API includes a flag to indicate if watchpoint should be for read, modify or both. This API uses 2 booleans, but the 'modify' flag was ignored and WatchAddress unconditionally watched write (actually modify). We now only watch for modify when the modify flag is true.

clayborg · 2025-01-31T22:04:16Z

Seeing as you are working on watchpoints here, I found that void WatchpointList::RemoveAll(bool notify) is sending the wrong event of Target::eBroadcastBitBreakpointChanged instead of sending Target::eBroadcastBitWatchpointChanged. Might be a good fix to get in to ensure watchpoints are solid.

puremourning · 2025-01-31T22:14:39Z

Seeing as you are working on watchpoints here, I found that void WatchpointList::RemoveAll(bool notify) is sending the wrong event of Target::eBroadcastBitBreakpointChanged instead of sending Target::eBroadcastBitWatchpointChanged. Might be a good fix to get in to ensure watchpoints are solid.

Seems simple enough to change it. Though I worry about bug-compatibility, in case someone has code expecting the "wrong" event.

Let me change it quickly and see what breaks in the tests.... If you're confident it is the "right" thing to do I can push another PR.

puremourning · 2025-01-31T22:30:02Z

Looking again, of course you're right and the behaviour of RemoveAll and Remove is inconsistent today, so I don't see a problem with fixing it.

puremourning · 2025-01-31T22:41:06Z

#125312

JDevlieghere · 2025-02-04T23:06:22Z

@puremourning Please confirm whether this is ready to be merged and I can press the ✅ button.

puremourning · 2025-02-04T23:57:24Z

Yep. Good to go.

The WatchAddress API includes a flag to indicate if watchpoint should be for read, modify or both. This API uses 2 booleans, but the 'modify' flag was ignored and WatchAddress unconditionally watched write (actually modify). We now only watch for modify when the modify flag is true. --- The included test fails prior to this patch and succeeds after. That is previously specifying `False` for `modify` would still stop on _write_, but after the patch correctly only stops on _read_

puremourning requested a review from JDevlieghere as a code owner January 28, 2025 21:53

llvmbot added the lldb label Jan 28, 2025

puremourning mentioned this pull request Jan 28, 2025

Add full support for data breakpoints vadimcn/codelldb#1161

Closed

puremourning force-pushed the lldb-fix-watch-address branch 2 times, most recently from afae250 to 3baafc2 Compare January 28, 2025 21:59

puremourning force-pushed the lldb-fix-watch-address branch from 3baafc2 to 1edd2a7 Compare January 28, 2025 22:05

JDevlieghere reviewed Jan 28, 2025

View reviewed changes

lldb/source/API/SBTarget.cpp Outdated Show resolved Hide resolved

lldb/source/API/SBTarget.cpp Show resolved Hide resolved

jasonmolenda self-requested a review January 28, 2025 23:20

jasonmolenda approved these changes Jan 28, 2025

View reviewed changes

puremourning force-pushed the lldb-fix-watch-address branch from 1edd2a7 to 169a8d8 Compare January 30, 2025 13:32

puremourning force-pushed the lldb-fix-watch-address branch from 169a8d8 to 6b3ba3f Compare January 31, 2025 21:41

puremourning changed the title ~~LLDB: WatchAddress ignores modify option~~ [lldb] WatchAddress ignores modify option Feb 1, 2025

JDevlieghere approved these changes Feb 4, 2025

View reviewed changes

JDevlieghere merged commit e8d437f into llvm:main Feb 5, 2025
7 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[lldb] WatchAddress ignores modify option #124847

[lldb] WatchAddress ignores modify option #124847

Uh oh!

puremourning commented Jan 28, 2025

Uh oh!

llvmbot commented Jan 28, 2025

Uh oh!

github-actions bot commented Jan 28, 2025 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

jasonmolenda left a comment

Uh oh!

puremourning commented Jan 30, 2025

Uh oh!

puremourning commented Jan 30, 2025

Uh oh!

puremourning commented Jan 30, 2025

Uh oh!

puremourning commented Jan 31, 2025

Uh oh!

clayborg commented Jan 31, 2025

Uh oh!

puremourning commented Jan 31, 2025

Uh oh!

puremourning commented Jan 31, 2025

Uh oh!

puremourning commented Jan 31, 2025

Uh oh!

JDevlieghere commented Feb 4, 2025

Uh oh!

puremourning commented Feb 4, 2025

Uh oh!

Uh oh!

Uh oh!

[lldb] WatchAddress ignores modify option #124847

[lldb] WatchAddress ignores modify option #124847

Uh oh!

Conversation

puremourning commented Jan 28, 2025

Uh oh!

llvmbot commented Jan 28, 2025

Uh oh!

github-actions bot commented Jan 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jasonmolenda left a comment

Choose a reason for hiding this comment

Uh oh!

puremourning commented Jan 30, 2025

Uh oh!

puremourning commented Jan 30, 2025

Uh oh!

puremourning commented Jan 30, 2025

Uh oh!

puremourning commented Jan 31, 2025

Uh oh!

clayborg commented Jan 31, 2025

Uh oh!

puremourning commented Jan 31, 2025

Uh oh!

puremourning commented Jan 31, 2025

Uh oh!

puremourning commented Jan 31, 2025

Uh oh!

JDevlieghere commented Feb 4, 2025

Uh oh!

puremourning commented Feb 4, 2025

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Jan 28, 2025 •

edited

Loading