[Lldb-commits] [PATCH] D147816: Clarify how watchpoint description in stop packets work, fix AArch64 unintended behavior

Fri Apr 7 15:17:49 PDT 2023

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D147816.511801.patch
Type: text/x-patch
Size: 13612 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20230407/37fe7619/attachment.bin>