jasonmolenda created this revision.
jasonmolenda added reviewers: labath, omjavaid, DavidSpickett.
jasonmolenda added a project: LLDB.
Herald added subscribers: JDevlieghere, atanasyan, kristof.beyls, arichardson,
sdardis.
Herald added a project: All.
jasonmolenda requested review of this revision.
Herald added a subscriber: lldb-commits.
Orig posted in https://reviews.llvm.org/D147674 as part of a debugserver patch,
creating two phabs to track those separate patches so it's easier to review.
This patch fixes a problem with watchpoints on AArch targets using lldb-server
(and soon debugserver), where a large write that begins before a watched
region, and extends into the watched region, is reported as a trap address
before the watched memory range. I can show this on an AArch64 Ubuntu install:
{
uint8_t buf[8];
uint64_t *u64_p = (uint64_t *) buf;
*u64_p = 5; // break here
(*u64_p)++;
At the `break here` line, if and do `watch set variable buf[2]` and `c`, the
packets from lldb server look like
< 21> send packet: $Z2,fffffffff36a,1#75
< 6> read packet: $OK#9a
< 5> send packet: $c#63
<838> read packet: $T05thread:493;name:a.out; [...]
;reason:watchpoint;description:323831343734393736373037343334203320323831343734393736373037343332;#db
The `description` is `281474976707434 3 281474976707432` aka
`0x0000fffffffff36a 3 0x0000fffffffff368`. The first address is an address
contained within the hit watchpoint. The second number is the hardware
register index. The third number is the actual address that was accessed,
(lldb) v &buf u64_p
(uint8_t (*)[8]) &buf = 0x0000fffffffff368
(uint64_t *) u64_p = 0x0000fffffffff368
(lldb)
Right now, lldb has a rule that "for MIPS an ARM, if the third number is NOT
contained within any watchpoint region, silently continue past this
watchpoint". From the user's perspective, lldb misses the watchpoint
modification. We see this a lot on macOS with bzero()/memset() when you're
watching an element of an object and it is overwritten.
For MIPS this behavior is correct because any access to an 8B granule triggers
a watchpoint in that granule. If I watch 4 bytes at 0x1004, and someone
accesses 1 byte at 0x1002, a watchpoint will trigger. Jaydeep Patil in 2015 (
https://reviews.llvm.org/D11672 ) added this third argument and the "silently
step over it and don't tell the user" behavior; that patch decodes the
instruction that caused the trap to determine the actual address that was
accessed.
However, the intended use/meaning of this third field was very confusing (to
me), and in 2016 Omair ( https://reviews.llvm.org/D21516 ) enabled the same
behavior on ARM targets and reported the actual accessed address. I'm not
exactly sure what issue motivated this, it may have simply been a
misunderstanding of how StopInfo decodes and uses these three addresses. If
nothing is done on ARM targets and an access outside a watched region, lldb
stops execution and doesn't know how to disable the correct watchpoint &
advance execution, my guess is that he was addressing that issue.
As the reason:watchpoint's `description` value has grown from "wp addr + hw reg
index" to "wp addr + hw reg index + actual trap address", with implicit
behavior on MIPS that an actual trap address silently continues if it's outside
the range of a watchpoint, this is not the best. We should stop using
`description` and add 2-4 keys in the stop info packet, e.g. `wp-address:`,
`wp-reg-index:`, `wp-hit-address:`, `silently-continue:` that can be provided
as needed. We can remove the "on MIPS do this" behavior from generic lldb --
the stub is probably in the best position to know if this is a false watchpoint
tigger that should be ignored, and it can tell lldb. Looking at the latest ARM
ARM A-Profile docs, this seems like a situation that can exist in some modes as
well, we may need to flag this from AArch64 debugserver/lldb-server in the
future.
We don't currently have MIPS supported in lldb - it was removed c. 2021 in
anticipation of someone starting to actively maintain it again. But this
"silently continue" behavior may be needed for AArch64, and it may be needed
again for MIPS if that target is revived.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D147816
Files:
lldb/docs/lldb-gdb-remote.txt
lldb/include/lldb/Target/StopInfo.h
lldb/source/Plugins/Process/Utility/NativeRegisterContextDBReg_arm64.h
lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
lldb/source/Target/StopInfo.cpp
Index: lldb/source/Target/StopInfo.cpp
===================================================================
--- lldb/source/Target/StopInfo.cpp
+++ lldb/source/Target/StopInfo.cpp
@@ -666,9 +666,8 @@
WatchpointSP watchpoint_sp;
};
- StopInfoWatchpoint(Thread &thread, break_id_t watch_id,
- lldb::addr_t watch_hit_addr)
- : StopInfo(thread, watch_id), m_watch_hit_addr(watch_hit_addr) {}
+ StopInfoWatchpoint(Thread &thread, break_id_t watch_id, bool silently_skip)
+ : StopInfo(thread, watch_id), m_silently_skip(silently_skip) {}
~StopInfoWatchpoint() override = default;
@@ -893,27 +892,9 @@
WatchpointSentry sentry(process_sp, wp_sp);
- /*
- * MIPS: Last 3bits of the watchpoint address are masked by the kernel.
- * For example:
- * 'n' is at 0x120010d00 and 'm' is 0x120010d04. When a watchpoint is
- * set at 'm', then
- * watch exception is generated even when 'n' is read/written. To handle
- * this case,
- * server emulates the instruction at PC and finds the base address of
- * the load/store
- * instruction and appends it in the description of the stop-info
- * packet. If watchpoint
- * is not set on this address by user then this do not stop.
- */
- if (m_watch_hit_addr != LLDB_INVALID_ADDRESS) {
- WatchpointSP wp_hit_sp =
- thread_sp->CalculateTarget()->GetWatchpointList().FindByAddress(
- m_watch_hit_addr);
- if (!wp_hit_sp) {
- m_should_stop = false;
- wp_sp->IncrementFalseAlarmsAndReviseHitCount();
- }
+ if (m_silently_skip) {
+ m_should_stop = false;
+ wp_sp->IncrementFalseAlarmsAndReviseHitCount();
}
if (wp_sp->GetHitCount() <= wp_sp->GetIgnoreCount()) {
@@ -1035,7 +1016,7 @@
bool m_should_stop = false;
bool m_should_stop_is_valid = false;
- lldb::addr_t m_watch_hit_addr;
+ bool m_silently_skip = false;
bool m_step_over_plan_complete = false;
bool m_using_step_over_plan = false;
};
@@ -1372,10 +1353,11 @@
return StopInfoSP(new StopInfoBreakpoint(thread, break_id, should_stop));
}
-StopInfoSP
-StopInfo::CreateStopReasonWithWatchpointID(Thread &thread, break_id_t watch_id,
- lldb::addr_t watch_hit_addr) {
- return StopInfoSP(new StopInfoWatchpoint(thread, watch_id, watch_hit_addr));
+StopInfoSP StopInfo::CreateStopReasonWithWatchpointID(Thread &thread,
+ break_id_t watch_id,
+ bool silently_continue) {
+ return StopInfoSP(
+ new StopInfoWatchpoint(thread, watch_id, silently_continue));
}
StopInfoSP StopInfo::CreateStopReasonWithSignal(Thread &thread, int signo,
Index: lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
===================================================================
--- lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
+++ lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
@@ -1749,33 +1749,60 @@
} else if (reason == "trap") {
// Let the trap just use the standard signal stop reason below...
} else if (reason == "watchpoint") {
+ // We will have between 1 and 3 fields in the description.
+ //
+ // \a wp_addr which is the original start address that
+ // lldb requested be watched, or an address that the
+ // hardware reported. This address should be within the
+ // range of a currently active watchpoint region - lldb
+ // should be able to find a watchpoint with this address.
+ //
+ // \a wp_index is the hardware watchpoint register number.
+ //
+ // \a wp_hit_addr is the actual address reported by the hardware,
+ // which may be outside the range of a region we are watching.
+ //
+ // On MIPS, we may get a false watchpoint exception where an
+ // access to the same 8 byte granule as a watchpoint will trigger,
+ // even if the access was not within the range of the watched
+ // region. When we get a \a wp_hit_addr outside the range of any
+ // set watchpoint, continue execution without making it visible to
+ // the user.
+ //
+ // On ARM, a related issue where a large access that starts
+ // before the watched region (and extends into the watched
+ // region) may report a hit address before the watched region.
+ // lldb will not find the "nearest" watchpoint to
+ // disable/step/re-enable it, so one of the valid watchpoint
+ // addresses should be provided as \a wp_addr.
StringExtractor desc_extractor(description.c_str());
addr_t wp_addr = desc_extractor.GetU64(LLDB_INVALID_ADDRESS);
uint32_t wp_index = desc_extractor.GetU32(LLDB_INVALID_INDEX32);
addr_t wp_hit_addr = desc_extractor.GetU64(LLDB_INVALID_ADDRESS);
watch_id_t watch_id = LLDB_INVALID_WATCH_ID;
- if (wp_addr != LLDB_INVALID_ADDRESS) {
- WatchpointSP wp_sp;
+ bool silently_continue = false;
+ WatchpointSP wp_sp;
+ if (wp_hit_addr != LLDB_INVALID_ADDRESS) {
+ wp_sp = GetTarget().GetWatchpointList().FindByAddress(wp_hit_addr);
+ // On MIPS, \a wp_hit_addr outside the range of a watched
+ // region means we should silently continue, it is a false hit.
ArchSpec::Core core = GetTarget().GetArchitecture().GetCore();
- if ((core >= ArchSpec::kCore_mips_first &&
- core <= ArchSpec::kCore_mips_last) ||
- (core >= ArchSpec::eCore_arm_generic &&
- core <= ArchSpec::eCore_arm_aarch64))
- wp_sp =
- GetTarget().GetWatchpointList().FindByAddress(wp_hit_addr);
- if (!wp_sp)
- wp_sp = GetTarget().GetWatchpointList().FindByAddress(wp_addr);
- if (wp_sp) {
- wp_sp->SetHardwareIndex(wp_index);
- watch_id = wp_sp->GetID();
- }
+ if (!wp_sp && core >= ArchSpec::kCore_mips_first &&
+ core <= ArchSpec::kCore_mips_last)
+ silently_continue = true;
+ }
+ if (!wp_sp && wp_addr != LLDB_INVALID_ADDRESS)
+ wp_sp = GetTarget().GetWatchpointList().FindByAddress(wp_addr);
+ if (wp_sp) {
+ wp_sp->SetHardwareIndex(wp_index);
+ watch_id = wp_sp->GetID();
}
if (watch_id == LLDB_INVALID_WATCH_ID) {
Log *log(GetLog(GDBRLog::Watchpoints));
LLDB_LOGF(log, "failed to find watchpoint");
}
thread_sp->SetStopInfo(StopInfo::CreateStopReasonWithWatchpointID(
- *thread_sp, watch_id, wp_hit_addr));
+ *thread_sp, watch_id, silently_continue));
handled = true;
} else if (reason == "exception") {
thread_sp->SetStopInfo(StopInfo::CreateStopReasonWithException(
@@ -2202,6 +2229,10 @@
if (wp_sp)
wp_index = wp_sp->GetHardwareIndex();
+ // Rewrite this to mimic the "reason:watchpoint" and
+ // "description:ADDR" entries in the
+ // stop-reply packet, handled in
+ // ProcessGDBRemote::SetThreadStopInfo
reason = "watchpoint";
StreamString ostr;
ostr.Printf("%" PRIu64 " %" PRIu32, wp_addr, wp_index);
Index: lldb/source/Plugins/Process/Utility/NativeRegisterContextDBReg_arm64.h
===================================================================
--- lldb/source/Plugins/Process/Utility/NativeRegisterContextDBReg_arm64.h
+++ lldb/source/Plugins/Process/Utility/NativeRegisterContextDBReg_arm64.h
@@ -56,6 +56,12 @@
protected:
// Debug register info for hardware breakpoints and watchpoints management.
+ // Watchpoints: For a user requested size 4 at addr 0x1004,
+ // \a real_addr is 0x1004
+ // \a address is 0x1000
+ // size is 8
+ // If a one-byte write to 0x1006 is the most recent watchpoint trap,
+ // \a hit_addr is 0x1006
struct DREG {
lldb::addr_t address; // Breakpoint/watchpoint address value.
lldb::addr_t hit_addr; // Address at which last watchpoint trigger exception
Index: lldb/include/lldb/Target/StopInfo.h
===================================================================
--- lldb/include/lldb/Target/StopInfo.h
+++ lldb/include/lldb/Target/StopInfo.h
@@ -109,9 +109,9 @@
static lldb::StopInfoSP CreateStopReasonWithBreakpointSiteID(
Thread &thread, lldb::break_id_t break_id, bool should_stop);
- static lldb::StopInfoSP CreateStopReasonWithWatchpointID(
- Thread &thread, lldb::break_id_t watch_id,
- lldb::addr_t watch_hit_addr = LLDB_INVALID_ADDRESS);
+ static lldb::StopInfoSP
+ CreateStopReasonWithWatchpointID(Thread &thread, lldb::break_id_t watch_id,
+ bool silently_continue = false);
static lldb::StopInfoSP
CreateStopReasonWithSignal(Thread &thread, int signo,
Index: lldb/docs/lldb-gdb-remote.txt
===================================================================
--- lldb/docs/lldb-gdb-remote.txt
+++ lldb/docs/lldb-gdb-remote.txt
@@ -1559,17 +1559,57 @@
// "signal" stopped due to an actual unix signal, not
// just the debugger using a unix signal to keep
// the GDB remote client happy.
-// "watchpoint". Should be used in conjunction with
-// the "watch"/"rwatch"/"awatch" key value pairs.
+// "watchpoint". Can be used with of the
+// "watch"/"rwatch"/"awatch" key value pairs.
+// Or can be used *instead* of those keys,
+// with the specially formatted "description" field.
// "exception" an exception stop reason. Use with
// the "description" key/value pair to describe the
// exceptional event the user should see as the stop
// reason.
// "description" ascii-hex An ASCII hex string that contains a more descriptive
-// reason that the thread stopped. This is only needed
-// if none of the key/value pairs are enough to
-// describe why something stopped.
-//
+// reason that the thread stopped. This is only needed
+// if none of the key/value pairs are enough to
+// describe why something stopped.
+//
+// For "reason:watchpoint", "description" is an ascii-hex
+// encoded string with between one and three base10 numbers,
+// space separated. The three numbers are
+// 1. watchpoint address. This address should always be within
+// a memory region lldb has a watchpoint on.
+// On architectures where the actual reported hit address may
+// be outside the watchpoint that was triggered, the remote
+// stub should determine which watchpoint was triggered and
+// report an address from within its range.
+// 2. watchpoint hardware register index number.
+// 3. actual watchpoint trap address, which may be outside
+// the range of any watched region of memory. On MIPS, an addr
+// outside a watched range means lldb should disable the wp,
+// step, re-enable the wp and continue silently.
+//
+// On MIPS, the low 3 bits are masked so if a watchpoint is on
+// 0x1004, a 2-byte write to 0x1000 will trigger the watchpoint
+// (a false positive hit), and lldb needs to disable the
+// watchpoint at 0x1004, inst-step, then re-enable the watchpoint
+// and not make this a user visible event. The description here
+// would be "0x1004 0 0x1000". lldb needs a known watchpoint address
+// in the first field, so it can disable it & step.
+//
+// On AArch64 we have a related issue, where you watch 4 bytes at
+// 0x1004, an instruction does an 8-byte write starting at
+// 0x1000 (a true watchpoint hit) and the hardware may report the
+// trap address as 0x1000 - before the watched memory region -
+// with the write extending into the watched region. This can
+// be reported as "0x1004 0 0x1000". lldb will use 0x1004 to
+// identify which Watchpoint was triggered, and can report 0x1000
+// to the user. The behavior of silently stepping over the
+// watchpoint, with an 3rd field addr outside the range, is
+// restricted to MIPS.
+// There may be false-positive watchpoint hits on AArch64 as well,
+// in the SVE Streaming Mode, but that is less common (v. ESR
+// register flag "WPF", "Watchpoint might be False-Positive") and
+// not currently handled by lldb.
+//
// "threads" comma-sep-base16 A list of thread ids for all threads (including
// the thread that we're reporting as stopped) that
// are live in the process right now. lldb may
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
