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

[David Spickett via Phabricator via lldb-commits]

DavidSpickett added a comment.

Thanks for looking into all this, it makes my head hurt thinking about it :)

It will likely take us (Linaro) a while to do the lldb-server changes, but I do want to see this fixed eventually. We have some work upcoming for memcopy operations that will need special care for watchpoints, so that is a good excuse to do it.



================
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
----------------
What does this "v." mean? Is it short for "see here " or some kind of citation?


================
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,
----------------
Worth noting why here? (minimum watch size and alignment I guess)


================
Comment at: lldb/source/Plugins/Process/Utility/NativeRegisterContextDBReg_arm64.h:64
+  //   If a one-byte write to 0x1006 is the most recent watchpoint trap,
+  //   \a hit_addr is 0x1006
   struct DREG {
----------------
Also does this need to use `///` for the doxygen `\a` thing to work?

(I've no idea, Jonas is forever telling me to use `///` though :) )


================
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
----------------
Is this a note to self or is this code now actually doing that?


================
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;
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147816