jasonmolenda wrote:
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
jasonmolenda wrote:
@DavidSpickett pushed the fix for the `watch set expression` bug in
```
commit 35e3939cb06d19942a30fd39f1416f49a7b982a4
Author: Jason Molenda
Date: Thu Sep 21 14:48:19 2023 -0700
watch set expression's default type was wrong with new modify type
```
jasonmolenda wrote:
Ah, I see the bug where `watch set expression` ends up marked as `rw`.
CommandObjectWatchpointSetExpression::DoExecute passes
`OptionGroupWatchpoint::eWatchModify` as the type to Target::CreateWatchpoint
(which takes that as a uint32_t). It passes that `uint32_t kind` to
jasonmolenda wrote:
FWIW I've found that `watch set expression -s ` will ignore the size
specified if there is a clang type of the expression, it seems. :(
https://github.com/llvm/llvm-project/pull/66308
___
lldb-commits mailing list
jasonmolenda wrote:
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
DavidSpickett wrote:
Where the "commands" are:
```
watchpoint set expression -s 4 -- tagged_ptr
watchpoint set variable global_var
```
They default to different types.
https://github.com/llvm/llvm-project/pull/66308
___
lldb-commits mailing list
DavidSpickett wrote:
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
DavidSpickett wrote:
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.
https://github.com/jasonmolenda closed
https://github.com/llvm/llvm-project/pull/66308
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
https://github.com/jasonmolenda updated
https://github.com/llvm/llvm-project/pull/66308
>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
@@ -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
@@ -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
jimingham wrote:
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
https://github.com/clayborg approved this pull request.
Thanks for the changes! Looks great
https://github.com/llvm/llvm-project/pull/66308
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
@@ -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
https://github.com/clayborg edited
https://github.com/llvm/llvm-project/pull/66308
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
https://github.com/jasonmolenda updated
https://github.com/llvm/llvm-project/pull/66308
>From 1efbce9f3b754f45372f0098163e70c2118b57d9 Mon Sep 17 00:00:00 2001
From: Jason Molenda
Date: Wed, 13 Sep 2023 17:38:31 -0700
Subject: [PATCH 1/3] [lldb] Add 'modify' type watchpoints, make it default
@@ -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:
@@ -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:
@@ -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:
https://github.com/clayborg commented:
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
https://github.com/clayborg edited
https://github.com/llvm/llvm-project/pull/66308
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
https://github.com/jasonmolenda edited
https://github.com/llvm/llvm-project/pull/66308
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
@@ -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 =
https://github.com/jasonmolenda edited
https://github.com/llvm/llvm-project/pull/66308
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
@@ -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:
https://github.com/jasonmolenda edited
https://github.com/llvm/llvm-project/pull/66308
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
https://github.com/jasonmolenda edited
https://github.com/llvm/llvm-project/pull/66308
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
@@ -1326,23 +1326,35 @@ lldb::SBWatchpoint SBTarget::WatchAddress(lldb::addr_t
addr, size_t size,
SBError ) {
LLDB_INSTRUMENT_VA(this, addr, size, read, write, error);
+ SBWatchpointOptions options;
+
@@ -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:
@@ -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:
@@ -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:
@@ -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:
@@ -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)
+
@@ -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 =
@@ -1326,23 +1326,35 @@ lldb::SBWatchpoint SBTarget::WatchAddress(lldb::addr_t
addr, size_t size,
SBError ) {
LLDB_INSTRUMENT_VA(this, addr, size, read, write, error);
+ SBWatchpointOptions options;
+
@@ -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:
https://github.com/jasonmolenda updated
https://github.com/llvm/llvm-project/pull/66308
>From 1efbce9f3b754f45372f0098163e70c2118b57d9 Mon Sep 17 00:00:00 2001
From: Jason Molenda
Date: Wed, 13 Sep 2023 17:38:31 -0700
Subject: [PATCH 1/2] [lldb] Add 'modify' type watchpoints, make it default
clayborg wrote:
> 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
jimingham wrote:
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:
bulbazord wrote:
> > ```
> >SBWatchpoint SBTarget::WatchpointCreateByAddress(addr_t address, size_t
> > size, uint32_t access_flags, SBError );
> > ```
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> > with `eWatchpointAccess{Read,Write,Modify}` flags
jasonmolenda wrote:
> > > ```
> > >SBWatchpoint SBTarget::WatchpointCreateByAddress(addr_t address,
> > > size_t size, uint32_t access_flags, SBError );
> > > ```
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > > with
clayborg wrote:
> > ```
> >SBWatchpoint SBTarget::WatchpointCreateByAddress(addr_t address, size_t
> > size, uint32_t access_flags, SBError );
> > ```
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> > with `eWatchpointAccess{Read,Write,Modify}` flags
jasonmolenda wrote:
> ```
>SBWatchpoint SBTarget::WatchpointCreateByAddress(addr_t address, size_t
> size, uint32_t access_flags, SBError );
> ```
>
> with `eWatchpointAccess{Read,Write,Modify}` flags defined.
@bulbazord what do you think about this suggestion? Would you still prefer an
clayborg wrote:
> > > 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.
> >
> >
> > Anytime we keep adding more
@@ -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
@@ -211,6 +212,35 @@ bool Watchpoint::CaptureWatchedValue(const
ExecutionContext _ctx) {
return (m_new_value_sp && m_new_value_sp->GetError().Success());
}
+bool Watchpoint::WatchedValueReportable(const ExecutionContext _ctx) {
+ if (!m_watch_modify)
+return true;
+
jasonmolenda wrote:
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.
```
jasonmolenda wrote:
> > 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.
>
> Anytime we keep adding more options to an API
@@ -211,6 +212,35 @@ bool Watchpoint::CaptureWatchedValue(const
ExecutionContext _ctx) {
return (m_new_value_sp && m_new_value_sp->GetError().Success());
}
+bool Watchpoint::WatchedValueReportable(const ExecutionContext _ctx) {
+ if (!m_watch_modify)
+return true;
+
@@ -0,0 +1,17 @@
+#include
+int main() {
+ int value = 5;
jasonmolenda wrote:
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.
@@ -211,6 +212,35 @@ bool Watchpoint::CaptureWatchedValue(const
ExecutionContext _ctx) {
return (m_new_value_sp && m_new_value_sp->GetError().Success());
}
+bool Watchpoint::WatchedValueReportable(const ExecutionContext _ctx) {
+ if (!m_watch_modify)
+return true;
+
@@ -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;
jasonmolenda wrote:
Good point. I figured existing watchpoint tests
@@ -211,6 +212,35 @@ bool Watchpoint::CaptureWatchedValue(const
ExecutionContext _ctx) {
return (m_new_value_sp && m_new_value_sp->GetError().Success());
}
+bool Watchpoint::WatchedValueReportable(const ExecutionContext _ctx) {
+ if (!m_watch_modify)
+return true;
+
@@ -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;
clayborg wrote:
We could change it back to 5 to ensure that the
@@ -0,0 +1,17 @@
+#include
+int main() {
+ int value = 5;
clayborg wrote:
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.
@@ -211,6 +212,35 @@ bool Watchpoint::CaptureWatchedValue(const
ExecutionContext _ctx) {
return (m_new_value_sp && m_new_value_sp->GetError().Success());
}
+bool Watchpoint::WatchedValueReportable(const ExecutionContext _ctx) {
+ if (!m_watch_modify)
+return true;
+
https://github.com/clayborg edited
https://github.com/llvm/llvm-project/pull/66308
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
https://github.com/clayborg commented:
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
@@ -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;
clayborg wrote:
Can we check that it stops when the value changes from
https://github.com/JDevlieghere review_requested
https://github.com/llvm/llvm-project/pull/66308
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
https://github.com/JDevlieghere review_requested
https://github.com/llvm/llvm-project/pull/66308
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
@@ -211,6 +212,35 @@ bool Watchpoint::CaptureWatchedValue(const
ExecutionContext _ctx) {
return (m_new_value_sp && m_new_value_sp->GetError().Success());
}
+bool Watchpoint::WatchedValueReportable(const ExecutionContext _ctx) {
+ if (!m_watch_modify)
+return true;
+
@@ -211,6 +212,35 @@ bool Watchpoint::CaptureWatchedValue(const
ExecutionContext _ctx) {
return (m_new_value_sp && m_new_value_sp->GetError().Success());
}
+bool Watchpoint::WatchedValueReportable(const ExecutionContext _ctx) {
+ if (!m_watch_modify)
+return true;
+
@@ -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 &&
@@ -211,6 +212,35 @@ bool Watchpoint::CaptureWatchedValue(const
ExecutionContext _ctx) {
return (m_new_value_sp && m_new_value_sp->GetError().Success());
}
+bool Watchpoint::WatchedValueReportable(const ExecutionContext _ctx) {
+ if (!m_watch_modify)
+return true;
+
https://github.com/JDevlieghere edited
https://github.com/llvm/llvm-project/pull/66308
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
@@ -211,6 +212,35 @@ bool Watchpoint::CaptureWatchedValue(const
ExecutionContext _ctx) {
return (m_new_value_sp && m_new_value_sp->GetError().Success());
}
+bool Watchpoint::WatchedValueReportable(const ExecutionContext _ctx) {
+ if (!m_watch_modify)
+return true;
+
@@ -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 ==
jasonmolenda wrote:
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.
https://github.com/llvm/llvm-project/pull/66308
llvmbot wrote:
@llvm/pr-subscribers-lldb
Changes
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
https://github.com/llvmbot labeled
https://github.com/llvm/llvm-project/pull/66308
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
https://github.com/jasonmolenda review_requested
https://github.com/llvm/llvm-project/pull/66308
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
https://github.com/jasonmolenda created
https://github.com/llvm/llvm-project/pull/66308:
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
74 matches
Mail list logo
