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

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda added inline comments.


================
Comment at: lldb/docs/lldb-gdb-remote.txt:1609
+//           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
----------------
DavidSpickett wrote:
> What does this "v." mean? Is it short for "see here " or some kind of citation?
Yeah, maybe not widely known.  "v." short for "vide" latin, see. cf https://latinlexicon.org/LNS_abbreviations.php ;)


================
Comment at: lldb/source/Plugins/Process/Utility/NativeRegisterContextDBReg_arm64.h:62
+  //   \a address is 0x1000
+  //   size is 8
+  //   If a one-byte write to 0x1006 is the most recent watchpoint trap,
----------------
DavidSpickett wrote:
> Worth noting why here? (minimum watch size and alignment I guess)
I was taking notes as I read the source to understand how arm linux behaved, and left those notes in the header, maybe I should just remove them. The DREG struct has three addresses in it, and it wasn't really clear to me what was stored in them.


================
Comment at: lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp:2232-2235
+        // Rewrite this to mimic the "reason:watchpoint" and
+        // "description:ADDR" entries in the
+        // stop-reply packet, handled in
+        // ProcessGDBRemote::SetThreadStopInfo
----------------
DavidSpickett wrote:
> Is this a note to self or is this code now actually doing that?
Sorry that wasn't clear.  We can receive watchpoint notifications one of three ways:  `watch`/`awatch`/`rwatch` (standard gdb RSP), `reason:watchpoint` with a `description` asciihex string of 1-to-3 numbers, and on Darwin systems, a mach exception.  ProcessGDBRemote rewrites watch/awatch/rwatch to `reason:watchpoint` + `description` with only the one address, and that's the only thing decoded into a StopInfo object.


================
Comment at: lldb/source/Target/StopInfo.cpp:1019
   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;
----------------
DavidSpickett wrote:
> Please document this here as well.
> 
> I am not 100% whether silently skip true means always silently skip this watchpoint, or silently skip out of range hits attributed to this watchpoint.
Good point.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D147816/new/

https://reviews.llvm.org/D147816