[Lldb-commits] [PATCH] D89874: [lldb] Unify x86 watchpoint implementation on Linux and NetBSD (and future FreeBSD plugin) [WIP]

[Michał Górny via Phabricator via lldb-commits]

mgorny marked 4 inline comments as done.
mgorny added inline comments.


================
Comment at: lldb/source/Plugins/Process/Utility/WatchpointMixin_x86.cpp:9
+
+#if defined(__i386__) || defined(__x86_64__)
+
----------------
labath wrote:
> This class won't be using any os- or arch-specific features, so we can just drop these.
Does it make sense to compile code that isn't going to be used on other platforms?


================
Comment at: lldb/source/Plugins/Process/Utility/WatchpointMixin_x86.cpp:146-155
+  // clear dr6 if address or bits changed (i.e. we're not reenabling the same
+  // watchpoint)
+  if (drN_value.GetAsUInt64() != addr ||
+      (dr7_value.GetAsUInt64() & bit_mask) != (rw_bits | size_bits)) {
+    ClearWatchpointHit(wp_index);
+
+    error = WriteRegister(reg_info_drN, RegisterValue(addr));
----------------
labath wrote:
> mgorny wrote:
> > This is the DR6 cleanup I was talking about.
> This seems fine. Maybe worth mentioning that this is here to avoid confusing the NetBSD kernel.
I'm going to go through all the code and see what can be improved and/or documented better.


================
Comment at: lldb/source/Plugins/Process/Utility/WatchpointMixin_x86.cpp:183
+
+Status WatchpointMixin_x86::ClearWatchpointHit(uint32_t wp_index) {
+  if (wp_index >= NumSupportedHardwareWatchpoints())
----------------
labath wrote:
> mgorny wrote:
> > On NetBSD, we have to call this explicitly when processing watchpoint hit (in SIGTRAP handling). Not sure if Linux would need that too (in which case we'd have to add a virtual method to the base class, I guess), or if the kernel takes care of it.
> No clue. If it's not done currently, then I guess it's not needed.
I think it might have been done as part of the implicit disable/reenable.


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

https://reviews.llvm.org/D89874