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

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

I like this, and I think this is in a pretty good shape as it stands. The thing I'm not sure of is the naming of the class. I'd probably name it something like NativeRegisterContext_x86 (and hope that it can include non-watchpoint functionality in the future). As it stands now, this is not really a mix-in but a full register context class and in theory one can imagine that some target which only supports a single architecture will not bother with the NativeRegisterContextLinux stuff, but inherit straight from `NativeRegisterContext_x86`. (It's comment can allude to the fact that it is indented to be mixed with subclasses implementing os-specific functionality, which will also help explaining the virtual inheritance.)



================
Comment at: lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux.h:18
 
-class NativeRegisterContextLinux : public NativeRegisterContextRegisterInfo {
+class NativeRegisterContextLinux : virtual public NativeRegisterContextRegisterInfo {
 public:
----------------
I believe the more common spelling is `public virtual`


================
Comment at: lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_x86_64.cpp:296
     const ArchSpec &target_arch, NativeThreadProtocol &native_thread)
-    : NativeRegisterContextLinux(native_thread,
+    : NativeRegisterContextRegisterInfo(native_thread, CreateRegisterInfoInterface(target_arch)),
+      NativeRegisterContextLinux(native_thread,
----------------
mgorny wrote:
> I'm wondering if we can eliminate the init inside `NativeRegisterContextRegisterInfo`
Actually I'd delete the constructor of `NativeRegisterContextLinux`. 

C++ standard says:
```
An abstract base class is never a most derived class, this its constructors never initialize virtual base classes, therefore the corresponding mem-initializers may be omitted.
```
Since the only thing that constructor does (used to do) is call NativeRegisterContextRegisterInfo's ctor, we can just delete it.


================
Comment at: lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_x86_64.h:110
 
-  bool IsGPR(uint32_t reg_index) const;
+  bool IsGPROrDR(uint32_t reg_index) const;
 
----------------
mgorny wrote:
> I have changed this to facilitate `WriteRegister()`. Alternatively, I suppose we could reverse the loop to match `ReadRegister()`.
I think this is fine.


================
Comment at: lldb/source/Plugins/Process/Utility/WatchpointMixin_x86.cpp:9
+
+#if defined(__i386__) || defined(__x86_64__)
+
----------------
This class won't be using any os- or arch-specific features, so we can just drop these.


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


================
Comment at: lldb/source/Plugins/Process/Utility/WatchpointMixin_x86.cpp:183
+
+Status WatchpointMixin_x86::ClearWatchpointHit(uint32_t wp_index) {
+  if (wp_index >= NumSupportedHardwareWatchpoints())
----------------
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.


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

https://reviews.llvm.org/D89874