[lldb] Split out NetBSD/x86 watchpoint impl for unification

[mgorny]

Summary

Split the current NetBSD watchpoint implementation for x86 into Utility,
and revamp it to improve readability. This code is meant to be used
as a common class for all x86 watchpoint implementation, particularly
these on FreeBSD and Linux.

The code uses global watchpoint enable bits, as required by the NetBSD
kernel. If it ever becomes necessary for any platform to use local
enable bits instead, this can be trivially absracted out.

The code also postpones clearing DR6 until a new different watchpoint
is being set in place of the old one. This is necessary since LLDB
repeatedly reenables watchpoints on all threads, by clearing
and restoring them. When DR6 is cleared as a part of that, then pending
events on other threads can no longer be associated with watchpoints
correctly.

[mgorny]

lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_x86_64.cpp

296 ↗

(On Diff #299649)

I'm wondering if we can eliminate the init inside NativeRegisterContextRegisterInfo

lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_x86_64.h

110 ↗

(On Diff #299649)

I have changed this to facilitate WriteRegister(). Alternatively, I suppose we could reverse the loop to match ReadRegister().

lldb/source/Plugins/Process/Utility/WatchpointMixin_x86.cpp

146–155 ↗

(On Diff #299649)

This is the DR6 cleanup I was talking about.

183 ↗

(On Diff #299649)

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.

[labath]

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

lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux.h

18 ↗

(On Diff #299649)

I believe the more common spelling is public virtual

lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_x86_64.cpp

296 ↗

(On Diff #299649)

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.

lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_x86_64.h

110 ↗

(On Diff #299649)

I think this is fine.

lldb/source/Plugins/Process/Utility/WatchpointMixin_x86.cpp

9 ↗

(On Diff #299649)

This class won't be using any os- or arch-specific features, so we can just drop these.

146–155 ↗

(On Diff #299649)

This seems fine. Maybe worth mentioning that this is here to avoid confusing the NetBSD kernel.

183 ↗

(On Diff #299649)

No clue. If it's not done currently, then I guess it's not needed.

[mgorny]

In D89874#2344561, @labath wrote:

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

I don't have a strong opinion here. I went for the mixin approach since that was your suggest wrt register caching. I suppose in this case it doesn't matter much but it could make things easier (or harder) as we have more potentially disjoint functions and partially transitioned source tree.

[mgorny]

lldb/source/Plugins/Process/Utility/WatchpointMixin_x86.cpp

9 ↗

(On Diff #299649)

Does it make sense to compile code that isn't going to be used on other platforms?

146–155 ↗

(On Diff #299649)

I'm going to go through all the code and see what can be improved and/or documented better.

183 ↗

(On Diff #299649)

I think it might have been done as part of the implicit disable/reenable.

[mgorny]

Deleted unnecessary constructor and switched public/virtual order. More changes coming.

[mgorny]

For the record, I'm planning to clang-format the code at the very end.

[krytarowski]

lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_x86_64.cpp

580 ↗

(On Diff #299704)

Can we have IsGPR(reg_index) || IsDR(reg_index)?

[mgorny]

Revamped the whole file to hopefully make it a bit less magical.

• Moved all the constants into inline functions and one constexpr var.
• Added more 'visual' comments what bits are touched.
• Added comments about NetBSD-specific hacks.
• Inlined RegisterInfo magic into GetDR().
• Reduced the number of local variables, and renamed reg_value variables to clearly indicate which reg it is.
• Removed duplicate cleaning of DR6 when clearing all hw watchpoints — the cleanup is done on adding new watchpoints anyway.

lldb/include/lldb/lldb-defines.h

57–58 ↗

(On Diff #299731)

I'm wondering if we could change these two values to avoid having separate LLDB_GDB.... I suppose it could break if API clients relied on them.

lldb/source/Plugins/Process/Utility/WatchpointMixin_x86.cpp

59–60 ↗

(On Diff #299704)

Technically this could be replaced by break, and return error below but I suppose it's more readable as-is.

[mgorny]

Adding more people in case they wanted to chime in, as this code will be reused on FreeBSD and NetBSD too.

[mgorny]

Migrate NetBSD plugin as well. This implied adding ClearWatchpointHit() as a virtual method to the base class.

[labath]

This looks very nice.

In D89874#2344759, @mgorny wrote:

In D89874#2344561, @labath wrote:

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

I don't have a strong opinion here. I went for the mixin approach since that was your suggest wrt register caching. I suppose in this case it doesn't matter much but it could make things easier (or harder) as we have more potentially disjoint functions and partially transitioned source tree.

Ok, I see what you mean. We can keep this class watchpoint-specific, and re-evaluate later. I'd still like to have "NativeRegisterContext" in the name, though...

In D89874#2345649, @mgorny wrote:

Migrate NetBSD plugin as well.

It might be reasonable to land these as separate changes -- to reduce churn if anything breaks. Maybe start with NetBSD, since on linux you'll also have to update all NativeRegisterContextLinux_ARCH classes ?

lldb/include/lldb/lldb-defines.h

62–65 ↗

(On Diff #299800)

I don't want to add any more defines than what we already have. The modern way to handle this would be via an enum. But that's best left for a separate patch. Let's just revert this here.

lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_x86_64.cpp

580 ↗

(On Diff #299704)

Yeah, that does sound better.

lldb/source/Plugins/Process/Utility/WatchpointMixin_x86.cpp

9 ↗

(On Diff #299649)

It helps with maintenance because people can check if it at least compiles. For example, this change (making NativeRegisterContextLinux inherit virtually) will require changes in all of the NativeRegisterContextLinux_ARCH constructors, but those will have to be done blindly. We could even think of adding unit tests for this stuff, if we wanted to be super fancy...

At the same time, I don't think this hurts much because even the simplest form of dead code removal should be able to strip a file thats completely unreferenced.

[mgorny]

Updated per comments, made it NetBSD-only for now.

[labath]

Looks good. Just don't forget to reformat the patch.

[mgorny]

Reformatted version, uploaded for completeness.

lldb/include/lldb/Host/common/NativeRegisterContext.h

public:
 
   virtual uint32_t NumSupportedHardwareWatchpoints();
 
   virtual uint32_t SetHardwareWatchpoint(lldb::addr_t addr, size_t size,
                                          uint32_t watch_flags);
 
   virtual bool ClearHardwareWatchpoint(uint32_t hw_index);
 
+  virtual Status ClearWatchpointHit(uint32_t hw_index);
+
   virtual Status ClearAllHardwareWatchpoints();
 
   virtual Status IsWatchpointHit(uint32_t wp_index, bool &is_hit);
 
   virtual Status GetWatchpointHitIndex(uint32_t &wp_index,
                                        lldb::addr_t trap_addr);
 
   virtual Status IsWatchpointVacant(uint32_t wp_index, bool &is_vacant);

lldb/source/Host/common/NativeRegisterContext.cpp

uint32_t NativeRegisterContext::SetHardwareWatchpoint(lldb::addr_t addr,
                                                       uint32_t watch_flags) {
   return LLDB_INVALID_INDEX32;
 }
 
 bool NativeRegisterContext::ClearHardwareWatchpoint(uint32_t hw_index) {
   return false;
 }
 
+Status NativeRegisterContext::ClearWatchpointHit(uint32_t hw_index) {
+  return Status("not implemented");
+}
+
 Status NativeRegisterContext::ClearAllHardwareWatchpoints() {
   return Status("not implemented");
 }
 
 Status NativeRegisterContext::IsWatchpointHit(uint32_t wp_index, bool &is_hit) {
   is_hit = false;
   return Status("not implemented");
 }

lldb/source/Plugins/Process/NetBSD/NativeRegisterContextNetBSD.h

 
 #include "Plugins/Process/Utility/NativeRegisterContextRegisterInfo.h"
 
 namespace lldb_private {
 namespace process_netbsd {
 
 class NativeProcessNetBSD;
 
-class NativeRegisterContextNetBSD : public NativeRegisterContextRegisterInfo {
+class NativeRegisterContextNetBSD
+    : public virtual NativeRegisterContextRegisterInfo {
 public:
-  NativeRegisterContextNetBSD(NativeThreadProtocol &native_thread,
-                              RegisterInfoInterface *reg_info_interface_p);
-
   // This function is implemented in the NativeRegisterContextNetBSD_*
   // subclasses to create a new instance of the host specific
   // NativeRegisterContextNetBSD. The implementations can't collide as only one
   // NativeRegisterContextNetBSD_* variant should be compiled into the final
   // executable.
   static NativeRegisterContextNetBSD *
   CreateHostNativeRegisterContextNetBSD(const ArchSpec &target_arch,
                                         NativeThreadProtocol &native_thread);
   virtual Status
   CopyHardwareWatchpointsFrom(NativeRegisterContextNetBSD &source) = 0;
 
-  virtual Status ClearWatchpointHit(uint32_t wp_index) = 0;
-
 protected:
   Status DoRegisterSet(int req, void *buf);
   virtual NativeProcessNetBSD &GetProcess();
   virtual ::pid_t GetProcessPid();
 };
 
 } // namespace process_netbsd
 } // namespace lldb_private
 
 #endif // #ifndef lldb_NativeRegisterContextNetBSD_h

lldb/source/Plugins/Process/NetBSD/NativeRegisterContextNetBSD.cpp

 using namespace lldb_private;
 using namespace lldb_private::process_netbsd;
 
 // clang-format off
 #include <sys/types.h>
 #include <sys/ptrace.h>
 // clang-format on
 
-NativeRegisterContextNetBSD::NativeRegisterContextNetBSD(
-    NativeThreadProtocol &native_thread,
-    RegisterInfoInterface *reg_info_interface_p)
-    : NativeRegisterContextRegisterInfo(native_thread,
-                                        reg_info_interface_p) {}
-
 Status NativeRegisterContextNetBSD::DoRegisterSet(int ptrace_req, void *buf) {
   return NativeProcessNetBSD::PtraceWrapper(ptrace_req, GetProcessPid(), buf,
                                             m_thread.GetID());
 }
 
 NativeProcessNetBSD &NativeRegisterContextNetBSD::GetProcess() {
   return static_cast<NativeProcessNetBSD &>(m_thread.GetProcess());
 }
 
 ::pid_t NativeRegisterContextNetBSD::GetProcessPid() {
   return GetProcess().GetID();
 }

lldb/source/Plugins/Process/NetBSD/NativeRegisterContextNetBSD_x86_64.h

 #include <sys/param.h>
 #include <sys/types.h>
 #include <sys/ptrace.h>
 #include <machine/reg.h>
 // clang-format on
 
 #include "Plugins/Process/NetBSD/NativeRegisterContextNetBSD.h"
 #include "Plugins/Process/Utility/RegisterContext_x86.h"
+#include "Plugins/Process/Utility/NativeRegisterContextWatchpoint_x86.h"
 #include "Plugins/Process/Utility/lldb-x86-register-enums.h"
 
 #if defined(PT_GETXSTATE) && defined(PT_SETXSTATE)
 #define HAVE_XSTATE
 #endif
 
 namespace lldb_private {
 namespace process_netbsd {
 
 class NativeProcessNetBSD;
 
-class NativeRegisterContextNetBSD_x86_64 : public NativeRegisterContextNetBSD {
+class NativeRegisterContextNetBSD_x86_64
+    : public NativeRegisterContextNetBSD,
+      public NativeRegisterContextWatchpoint_x86 {
 public:
   NativeRegisterContextNetBSD_x86_64(const ArchSpec &target_arch,
                                      NativeThreadProtocol &native_thread);
   uint32_t GetRegisterSetCount() const override;
 
   const RegisterSet *GetRegisterSet(uint32_t set_index) const override;
 
   Status ReadRegister(const RegisterInfo *reg_info,
                       RegisterValue &reg_value) override;
 
   Status WriteRegister(const RegisterInfo *reg_info,
                        const RegisterValue &reg_value) override;
 
   Status ReadAllRegisterValues(lldb::DataBufferSP &data_sp) override;
 
   Status WriteAllRegisterValues(const lldb::DataBufferSP &data_sp) override;
 
-  Status IsWatchpointHit(uint32_t wp_index, bool &is_hit) override;
-
-  Status GetWatchpointHitIndex(uint32_t &wp_index,
-                               lldb::addr_t trap_addr) override;
-
-  Status IsWatchpointVacant(uint32_t wp_index, bool &is_vacant) override;
-
-  bool ClearHardwareWatchpoint(uint32_t wp_index) override;
-
-  Status ClearWatchpointHit(uint32_t wp_index) override;
-
-  Status ClearAllHardwareWatchpoints() override;
-
-  Status SetHardwareWatchpointWithIndex(lldb::addr_t addr, size_t size,
-                                        uint32_t watch_flags,
-                                        uint32_t wp_index);
-
-  uint32_t SetHardwareWatchpoint(lldb::addr_t addr, size_t size,
-                                 uint32_t watch_flags) override;
-
-  lldb::addr_t GetWatchpointAddress(uint32_t wp_index) override;
-
-  uint32_t NumSupportedHardwareWatchpoints() override;
-
   Status
   CopyHardwareWatchpointsFrom(NativeRegisterContextNetBSD &source) override;
 
 private:
   // Private member types.
   enum { GPRegSet, FPRegSet, XStateRegSet, DBRegSet };
 
   // Private member variables.

lldb/source/Plugins/Process/NetBSD/NativeRegisterContextNetBSD_x86_64.cpp

if (HostInfo::GetArchitecture().GetAddressByteSize() == 4) {
     // X86_64 hosts know how to work with 64-bit and 32-bit EXEs using the x86_64
     // register context.
     return new RegisterContextNetBSD_x86_64(target_arch);
   }
 }
 
 NativeRegisterContextNetBSD_x86_64::NativeRegisterContextNetBSD_x86_64(
     const ArchSpec &target_arch, NativeThreadProtocol &native_thread)
-    : NativeRegisterContextNetBSD(native_thread,
-                                  CreateRegisterInfoInterface(target_arch)),
+    : NativeRegisterContextRegisterInfo(
+          native_thread, CreateRegisterInfoInterface(target_arch)),
       m_gpr(), m_fpr(), m_dbr() {}
 
 // CONSIDER after local and llgs debugging are merged, register set support can
 // be moved into a base x86-64 class with IsRegisterSetAvailable made virtual.
 uint32_t NativeRegisterContextNetBSD_x86_64::GetRegisterSetCount() const {
   uint32_t sets = 0;
   for (uint32_t set_index = 0; set_index < k_num_register_sets; ++set_index) {
     if (GetSetForNativeRegNum(set_index) != -1)
case llvm::Triple::x86:
     return lldb_dr0_i386 + num;
   case llvm::Triple::x86_64:
     return lldb_dr0_x86_64 + num;
   default:
     llvm_unreachable("Unhandled target architecture.");
   }
 }
 
-Status NativeRegisterContextNetBSD_x86_64::IsWatchpointHit(uint32_t wp_index,
-                                                           bool &is_hit) {
-  if (wp_index >= NumSupportedHardwareWatchpoints())
-    return Status("Watchpoint index out of range");
-
-  RegisterValue reg_value;
-  const RegisterInfo *const reg_info = GetRegisterInfoAtIndex(GetDR(6));
-  Status error = ReadRegister(reg_info, reg_value);
-  if (error.Fail()) {
-    is_hit = false;
-    return error;
-  }
-
-  uint64_t status_bits = reg_value.GetAsUInt64();
-
-  is_hit = status_bits & (1 << wp_index);
-
-  return error;
-}
-
-Status NativeRegisterContextNetBSD_x86_64::GetWatchpointHitIndex(
-    uint32_t &wp_index, lldb::addr_t trap_addr) {
-  uint32_t num_hw_wps = NumSupportedHardwareWatchpoints();
-  for (wp_index = 0; wp_index < num_hw_wps; ++wp_index) {
-    bool is_hit;
-    Status error = IsWatchpointHit(wp_index, is_hit);
-    if (error.Fail()) {
-      wp_index = LLDB_INVALID_INDEX32;
-      return error;
-    } else if (is_hit) {
-      return error;
-    }
-  }
-  wp_index = LLDB_INVALID_INDEX32;
-  return Status();
-}
-
-Status NativeRegisterContextNetBSD_x86_64::IsWatchpointVacant(uint32_t wp_index,
-                                                              bool &is_vacant) {
-  if (wp_index >= NumSupportedHardwareWatchpoints())
-    return Status("Watchpoint index out of range");
-
-  RegisterValue reg_value;
-  const RegisterInfo *const reg_info = GetRegisterInfoAtIndex(GetDR(7));
-  Status error = ReadRegister(reg_info, reg_value);
-  if (error.Fail()) {
-    is_vacant = false;
-    return error;
-  }
-
-  uint64_t control_bits = reg_value.GetAsUInt64();
-
-  is_vacant = !(control_bits & (1 << (2 * wp_index + 1)));
-
-  return error;
-}
-
-Status NativeRegisterContextNetBSD_x86_64::SetHardwareWatchpointWithIndex(
-    lldb::addr_t addr, size_t size, uint32_t watch_flags, uint32_t wp_index) {
-
-  if (wp_index >= NumSupportedHardwareWatchpoints())
-    return Status("Watchpoint index out of range");
-
-  // Read only watchpoints aren't supported on x86_64. Fall back to read/write
-  // waitchpoints instead.
-  // TODO: Add logic to detect when a write happens and ignore that watchpoint
-  // hit.
-  if (watch_flags == 0x2)
-    watch_flags = 0x3;
-
-  if (watch_flags != 0x1 && watch_flags != 0x3)
-    return Status("Invalid read/write bits for watchpoint");
-
-  if (size != 1 && size != 2 && size != 4 && size != 8)
-    return Status("Invalid size for watchpoint");
-
-  bool is_vacant;
-  Status error = IsWatchpointVacant(wp_index, is_vacant);
-  if (error.Fail())
-    return error;
-  if (!is_vacant)
-    return Status("Watchpoint index not vacant");
-
-  const RegisterInfo *const reg_info_dr7 = GetRegisterInfoAtIndex(GetDR(7));
-  RegisterValue dr7_value;
-  error = ReadRegister(reg_info_dr7, dr7_value);
-  if (error.Fail())
-    return error;
-
-  // for watchpoints 0, 1, 2, or 3, respectively, set bits 1, 3, 5, or 7
-  uint64_t enable_bit = 1 << (2 * wp_index + 1);
-
-  // set bits 16-17, 20-21, 24-25, or 28-29
-  // with 0b01 for write, and 0b11 for read/write
-  uint64_t rw_bits = watch_flags << (16 + 4 * wp_index);
-
-  // set bits 18-19, 22-23, 26-27, or 30-31
-  // with 0b00, 0b01, 0b10, or 0b11
-  // for 1, 2, 8 (if supported), or 4 bytes, respectively
-  uint64_t size_bits = (size == 8 ? 0x2 : size - 1) << (18 + 4 * wp_index);
-
-  uint64_t bit_mask = (0x3 << (2 * wp_index)) | (0xF << (16 + 4 * wp_index));
-
-  uint64_t control_bits = dr7_value.GetAsUInt64() & ~bit_mask;
-
-  control_bits |= enable_bit | rw_bits | size_bits;
-
-  const RegisterInfo *const reg_info_drN =
-      GetRegisterInfoAtIndex(GetDR(wp_index));
-  RegisterValue drN_value;
-  error = ReadRegister(reg_info_drN, drN_value);
-  if (error.Fail())
-    return error;
-
-  // 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));
-    if (error.Fail())
-      return error;
-  }
-
-  error = WriteRegister(reg_info_dr7, RegisterValue(control_bits));
-  if (error.Fail())
-    return error;
-
-  error.Clear();
-  return error;
-}
-
-bool NativeRegisterContextNetBSD_x86_64::ClearHardwareWatchpoint(
-    uint32_t wp_index) {
-  if (wp_index >= NumSupportedHardwareWatchpoints())
-    return false;
-
-  // for watchpoints 0, 1, 2, or 3, respectively, clear bits 0-1, 2-3, 4-5
-  // or 6-7 of the debug control register (DR7)
-  const RegisterInfo *const reg_info_dr7 = GetRegisterInfoAtIndex(GetDR(7));
-  RegisterValue reg_value;
-  Status error = ReadRegister(reg_info_dr7, reg_value);
-  if (error.Fail())
-    return false;
-  uint64_t bit_mask = 0x3 << (2 * wp_index);
-  uint64_t control_bits = reg_value.GetAsUInt64() & ~bit_mask;
-
-  return WriteRegister(reg_info_dr7, RegisterValue(control_bits)).Success();
-}
-
-Status NativeRegisterContextNetBSD_x86_64::ClearWatchpointHit(uint32_t wp_index) {
-  if (wp_index >= NumSupportedHardwareWatchpoints())
-    return Status("Watchpoint index out of range");
-
-  // for watchpoints 0, 1, 2, or 3, respectively, check bits 0, 1, 2, or 3 of
-  // the debug status register (DR6)
-  const RegisterInfo *const reg_info_dr6 = GetRegisterInfoAtIndex(GetDR(6));
-  RegisterValue reg_value;
-  Status error = ReadRegister(reg_info_dr6, reg_value);
-  if (error.Fail())
-    return error;
-
-  uint64_t bit_mask = 1 << wp_index;
-  uint64_t status_bits = reg_value.GetAsUInt64() & ~bit_mask;
-  return WriteRegister(reg_info_dr6, RegisterValue(status_bits));
-}
-
-Status NativeRegisterContextNetBSD_x86_64::ClearAllHardwareWatchpoints() {
-  RegisterValue reg_value;
-
-  // clear bits {0-4} of the debug status register (DR6)
-  const RegisterInfo *const reg_info_dr6 = GetRegisterInfoAtIndex(GetDR(6));
-  Status error = ReadRegister(reg_info_dr6, reg_value);
-  if (error.Fail())
-    return error;
-  uint64_t bit_mask = 0xF;
-  uint64_t status_bits = reg_value.GetAsUInt64() & ~bit_mask;
-  error = WriteRegister(reg_info_dr6, RegisterValue(status_bits));
-  if (error.Fail())
-    return error;
-
-  // clear bits {0-7,16-31} of the debug control register (DR7)
-  const RegisterInfo *const reg_info_dr7 = GetRegisterInfoAtIndex(GetDR(7));
-  error = ReadRegister(reg_info_dr7, reg_value);
-  if (error.Fail())
-    return error;
-  bit_mask = 0xFF | (0xFFFF << 16);
-  uint64_t control_bits = reg_value.GetAsUInt64() & ~bit_mask;
-  return WriteRegister(reg_info_dr7, RegisterValue(control_bits));
-}
-
-uint32_t NativeRegisterContextNetBSD_x86_64::SetHardwareWatchpoint(
-    lldb::addr_t addr, size_t size, uint32_t watch_flags) {
-  Log *log(GetLogIfAllCategoriesSet(LIBLLDB_LOG_WATCHPOINTS));
-  const uint32_t num_hw_watchpoints = NumSupportedHardwareWatchpoints();
-  for (uint32_t wp_index = 0; wp_index < num_hw_watchpoints; ++wp_index) {
-    bool is_vacant;
-    Status error = IsWatchpointVacant(wp_index, is_vacant);
-    if (is_vacant) {
-      error = SetHardwareWatchpointWithIndex(addr, size, watch_flags, wp_index);
-      if (error.Success())
-        return wp_index;
-    }
-    if (error.Fail() && log) {
-      LLDB_LOGF(log, "NativeRegisterContextNetBSD_x86_64::%s Error: %s",
-                __FUNCTION__, error.AsCString());
-    }
-  }
-  return LLDB_INVALID_INDEX32;
-}
-
-lldb::addr_t
-NativeRegisterContextNetBSD_x86_64::GetWatchpointAddress(uint32_t wp_index) {
-  if (wp_index >= NumSupportedHardwareWatchpoints())
-    return LLDB_INVALID_ADDRESS;
-  RegisterValue reg_value;
-  const RegisterInfo *const reg_info_drN =
-      GetRegisterInfoAtIndex(GetDR(wp_index));
-  if (ReadRegister(reg_info_drN, reg_value).Fail())
-    return LLDB_INVALID_ADDRESS;
-  return reg_value.GetAsUInt64();
-}
-
-uint32_t NativeRegisterContextNetBSD_x86_64::NumSupportedHardwareWatchpoints() {
-  // Available debug address registers: dr0, dr1, dr2, dr3
-  return 4;
-}
-
 Status NativeRegisterContextNetBSD_x86_64::CopyHardwareWatchpointsFrom(
     NativeRegisterContextNetBSD &source) {
   auto &r_source = static_cast<NativeRegisterContextNetBSD_x86_64&>(source);
   Status res = r_source.ReadRegisterSet(DBRegSet);
   if (!res.Fail()) {
     // copy dbregs only if any watchpoints were set
     if ((r_source.m_dbr.dr[7] & 0xFF) == 0)
       return res;
 
     m_dbr = r_source.m_dbr;
     res = WriteRegisterSet(DBRegSet);
   }
   return res;
 }
 
 #endif // defined(__x86_64__)

lldb/source/Plugins/Process/Utility/CMakeLists.txt

 add_lldb_library(lldbPluginProcessUtility
   AuxVector.cpp
   DynamicRegisterInfo.cpp
   FreeBSDSignals.cpp
   GDBRemoteSignals.cpp
   HistoryThread.cpp
   HistoryUnwind.cpp
   InferiorCallPOSIX.cpp
   LinuxProcMaps.cpp
   LinuxSignals.cpp
   MipsLinuxSignals.cpp
   NativeRegisterContextRegisterInfo.cpp
+  NativeRegisterContextWatchpoint_x86.cpp
   NetBSDSignals.cpp
   RegisterContextDarwin_arm.cpp
   RegisterContextDarwin_arm64.cpp
   RegisterContextDarwin_i386.cpp
   RegisterContextDarwin_x86_64.cpp
   RegisterContextDummy.cpp
   RegisterContextFreeBSD_i386.cpp
   RegisterContextFreeBSD_mips64.cpp

lldb/source/Plugins/Process/Utility/NativeRegisterContextWatchpoint_x86.h

+//===-- NativeRegisterContextWatchpoint_x86.h -------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef lldb_NativeRegisterContextWatchpoint_x86_h
+#define lldb_NativeRegisterContextWatchpoint_x86_h
+
+#include "Plugins/Process/Utility/NativeRegisterContextRegisterInfo.h"
+
+namespace lldb_private {
+
+class NativeRegisterContextWatchpoint_x86
+    : public virtual NativeRegisterContextRegisterInfo {
+public:
+  Status IsWatchpointHit(uint32_t wp_index, bool &is_hit) override;
+
+  Status GetWatchpointHitIndex(uint32_t &wp_index,
+                               lldb::addr_t trap_addr) override;
+
+  Status IsWatchpointVacant(uint32_t wp_index, bool &is_vacant) override;
+
+  bool ClearHardwareWatchpoint(uint32_t wp_index) override;
+
+  Status ClearWatchpointHit(uint32_t wp_index) override;
+
+  Status ClearAllHardwareWatchpoints() override;
+
+  Status SetHardwareWatchpointWithIndex(lldb::addr_t addr, size_t size,
+                                        uint32_t watch_flags,
+                                        uint32_t wp_index);
+
+  uint32_t SetHardwareWatchpoint(lldb::addr_t addr, size_t size,
+                                 uint32_t watch_flags) override;
+
+  lldb::addr_t GetWatchpointAddress(uint32_t wp_index) override;
+
+  uint32_t NumSupportedHardwareWatchpoints() override;
+
+private:
+  const RegisterInfo *GetDR(int num) const;
+};
+
+} // namespace lldb_private
+
+#endif // #ifndef lldb_NativeRegisterContextWatchpoint_x86_h

lldb/source/Plugins/Process/Utility/NativeRegisterContextWatchpoint_x86.cpp

+//===-- NativeRegisterContextWatchpoint_x86.cpp ---------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "NativeRegisterContextWatchpoint_x86.h"
+
+#include "lldb/Utility/Log.h"
+#include "lldb/Utility/RegisterValue.h"
+
+#include "Plugins/Process/Utility/lldb-x86-register-enums.h"
+
+using namespace lldb_private;
+
+// Returns mask/value for status bit of wp_index in DR6
+static inline uint64_t GetStatusBit(uint32_t wp_index) {
+  // DR6: ...BBBB
+  //         3210 <- status bits for bp./wp. i; 1 if hit
+  return 1 << wp_index;
+}
+
+// Returns mask/value for global enable bit of wp_index in DR7
+static inline uint64_t GetEnableBit(uint32_t wp_index) {
+  // DR7: ...GLGLGLGL
+  //         33221100 <- global/local enable for bp./wp.; 1 if enabled
+  // we use global bits because NetBSD kernel does not preserve local
+  // bits reliably; Linux seems fine with either
+  return 1 << (2 * wp_index + 1);
+}
+
+// Returns mask for both enable bits of wp_index in DR7
+static inline uint64_t GetBothEnableBitMask(uint32_t wp_index) {
+  // DR7: ...GLGLGLGL
+  //         33221100 <- global/local enable for bp./wp.; 1 if enabled
+  return 3 << (2 * wp_index + 1);
+}
+
+// Returns value for type bits of wp_index in DR7
+static inline uint64_t GetWatchTypeBits(uint32_t watch_flags,
+                                        uint32_t wp_index) {
+  // DR7:
+  // bit: 3322222222221111...
+  //      1098765432109876...
+  // val: SSTTSSTTSSTTSSTT...
+  // wp.: 3333222211110000...
+  //
+  // where T - type is 01 for write, 11 for r/w
+  return watch_flags << (16 + 4 * wp_index);
+}
+
+// Returns value for size bits of wp_index in DR7
+static inline uint64_t GetWatchSizeBits(uint32_t size, uint32_t wp_index) {
+  // DR7:
+  // bit: 3322222222221111...
+  //      1098765432109876...
+  // val: SSTTSSTTSSTTSSTT...
+  // wp.: 3333222211110000...
+  //
+  // where S - size is:
+  // 00 for 1 byte
+  // 01 for 2 bytes
+  // 10 for 8 bytes
+  // 11 for 4 bytes
+  return (size == 8 ? 0x2 : size - 1) << (18 + 4 * wp_index);
+}
+
+// Returns bitmask for all bits controlling wp_index in DR7
+static inline uint64_t GetWatchControlBitmask(uint32_t wp_index) {
+  // DR7:
+  // bit: 33222222222211111111110000000000
+  //      10987654321098765432109876543210
+  // val: SSTTSSTTSSTTSSTTxxxxxxGLGLGLGLGL
+  // wp.: 3333222211110000xxxxxxEE33221100
+  return GetBothEnableBitMask(wp_index) | (0xF << (16 + 4 * wp_index));
+}
+
+// Bit mask for control bits regarding all watchpoints.
+static constexpr uint64_t watchpoint_all_control_bit_mask = 0xFFFF00FF;
+
+const RegisterInfo *NativeRegisterContextWatchpoint_x86::GetDR(int num) const {
+  assert(num >= 0 && num <= 7);
+  switch (GetRegisterInfoInterface().GetTargetArchitecture().GetMachine()) {
+  case llvm::Triple::x86:
+    return GetRegisterInfoAtIndex(lldb_dr0_i386 + num);
+  case llvm::Triple::x86_64:
+    return GetRegisterInfoAtIndex(lldb_dr0_x86_64 + num);
+  default:
+    llvm_unreachable("Unhandled target architecture.");
+  }
+}
+
+Status NativeRegisterContextWatchpoint_x86::IsWatchpointHit(uint32_t wp_index,
+                                                            bool &is_hit) {
+  if (wp_index >= NumSupportedHardwareWatchpoints())
+    return Status("Watchpoint index out of range");
+
+  RegisterValue dr6;
+  Status error = ReadRegister(GetDR(6), dr6);
+  if (error.Fail())
+    is_hit = false;
+  else
+    is_hit = dr6.GetAsUInt64() & GetStatusBit(wp_index);
+
+  return error;
+}
+
+Status NativeRegisterContextWatchpoint_x86::GetWatchpointHitIndex(
+    uint32_t &wp_index, lldb::addr_t trap_addr) {
+  uint32_t num_hw_wps = NumSupportedHardwareWatchpoints();
+  for (wp_index = 0; wp_index < num_hw_wps; ++wp_index) {
+    bool is_hit;
+    Status error = IsWatchpointHit(wp_index, is_hit);
+    if (error.Fail()) {
+      wp_index = LLDB_INVALID_INDEX32;
+      return error;
+    } else if (is_hit) {
+      return error;
+    }
+  }
+  wp_index = LLDB_INVALID_INDEX32;
+  return Status();
+}
+
+Status
+NativeRegisterContextWatchpoint_x86::IsWatchpointVacant(uint32_t wp_index,
+                                                        bool &is_vacant) {
+  if (wp_index >= NumSupportedHardwareWatchpoints())
+    return Status("Watchpoint index out of range");
+
+  RegisterValue dr7;
+  Status error = ReadRegister(GetDR(7), dr7);
+  if (error.Fail())
+    is_vacant = false;
+  else
+    is_vacant = !(dr7.GetAsUInt64() & GetEnableBit(wp_index));
+
+  return error;
+}
+
+Status NativeRegisterContextWatchpoint_x86::SetHardwareWatchpointWithIndex(
+    lldb::addr_t addr, size_t size, uint32_t watch_flags, uint32_t wp_index) {
+
+  if (wp_index >= NumSupportedHardwareWatchpoints())
+    return Status("Watchpoint index out of range");
+
+  // Read only watchpoints aren't supported on x86_64. Fall back to read/write
+  // waitchpoints instead.
+  // TODO: Add logic to detect when a write happens and ignore that watchpoint
+  // hit.
+  if (watch_flags == 2)
+    watch_flags = 3;
+
+  if (watch_flags != 1 && watch_flags != 3)
+    return Status("Invalid read/write bits for watchpoint");
+  if (size != 1 && size != 2 && size != 4 && size != 8)
+    return Status("Invalid size for watchpoint");
+
+  bool is_vacant;
+  Status error = IsWatchpointVacant(wp_index, is_vacant);
+  if (error.Fail())
+    return error;
+  if (!is_vacant)
+    return Status("Watchpoint index not vacant");
+
+  RegisterValue dr7, drN;
+  error = ReadRegister(GetDR(7), dr7);
+  if (error.Fail())
+    return error;
+  error = ReadRegister(GetDR(wp_index), drN);
+  if (error.Fail())
+    return error;
+
+  uint64_t control_bits = dr7.GetAsUInt64() & ~GetWatchControlBitmask(wp_index);
+  control_bits |= GetEnableBit(wp_index) |
+                  GetWatchTypeBits(watch_flags, wp_index) |
+                  GetWatchSizeBits(size, wp_index);
+
+  // Clear dr6 if address or bits changed (i.e. we're not reenabling the same
+  // watchpoint).  This can not be done when clearing watchpoints since
+  // the gdb-remote protocol repeatedly clears and readds watchpoints on all
+  // program threads, effectively clearing pending events on NetBSD.
+  // NB: enable bits in dr7 are always 0 here since we're (re)adding it
+  if (drN.GetAsUInt64() != addr ||
+      (dr7.GetAsUInt64() & GetWatchControlBitmask(wp_index)) !=
+          (GetWatchTypeBits(watch_flags, wp_index) |
+           GetWatchSizeBits(size, wp_index))) {
+    ClearWatchpointHit(wp_index);
+
+    // We skip update to drN if neither address nor mode changed.
+    error = WriteRegister(GetDR(wp_index), RegisterValue(addr));
+    if (error.Fail())
+      return error;
+  }
+
+  error = WriteRegister(GetDR(7), RegisterValue(control_bits));
+  if (error.Fail())
+    return error;
+
+  return error;
+}
+
+bool NativeRegisterContextWatchpoint_x86::ClearHardwareWatchpoint(
+    uint32_t wp_index) {
+  if (wp_index >= NumSupportedHardwareWatchpoints())
+    return false;
+
+  RegisterValue dr7;
+  Status error = ReadRegister(GetDR(7), dr7);
+  if (error.Fail())
+    return false;
+
+  return WriteRegister(GetDR(7), RegisterValue(dr7.GetAsUInt64() &
+                                               ~GetBothEnableBitMask(wp_index)))
+      .Success();
+}
+
+Status
+NativeRegisterContextWatchpoint_x86::ClearWatchpointHit(uint32_t wp_index) {
+  if (wp_index >= NumSupportedHardwareWatchpoints())
+    return Status("Watchpoint index out of range");
+
+  RegisterValue dr6;
+  Status error = ReadRegister(GetDR(6), dr6);
+  if (error.Fail())
+    return error;
+
+  return WriteRegister(
+      GetDR(6), RegisterValue(dr6.GetAsUInt64() & ~GetStatusBit(wp_index)));
+}
+
+Status NativeRegisterContextWatchpoint_x86::ClearAllHardwareWatchpoints() {
+  RegisterValue dr7;
+  Status error = ReadRegister(GetDR(7), dr7);
+  if (error.Fail())
+    return error;
+  return WriteRegister(
+      GetDR(7),
+      RegisterValue(dr7.GetAsUInt64() & ~watchpoint_all_control_bit_mask));
+}
+
+uint32_t NativeRegisterContextWatchpoint_x86::SetHardwareWatchpoint(
+    lldb::addr_t addr, size_t size, uint32_t watch_flags) {
+  Log *log(GetLogIfAllCategoriesSet(LIBLLDB_LOG_WATCHPOINTS));
+  const uint32_t num_hw_watchpoints = NumSupportedHardwareWatchpoints();
+  for (uint32_t wp_index = 0; wp_index < num_hw_watchpoints; ++wp_index) {
+    bool is_vacant;
+    Status error = IsWatchpointVacant(wp_index, is_vacant);
+    if (is_vacant) {
+      error = SetHardwareWatchpointWithIndex(addr, size, watch_flags, wp_index);
+      if (error.Success())
+        return wp_index;
+    }
+    if (error.Fail() && log) {
+      LLDB_LOGF(log, "NativeRegisterContextWatchpoint_x86::%s Error: %s",
+                __FUNCTION__, error.AsCString());
+    }
+  }
+  return LLDB_INVALID_INDEX32;
+}
+
+lldb::addr_t
+NativeRegisterContextWatchpoint_x86::GetWatchpointAddress(uint32_t wp_index) {
+  if (wp_index >= NumSupportedHardwareWatchpoints())
+    return LLDB_INVALID_ADDRESS;
+  RegisterValue drN;
+  if (ReadRegister(GetDR(wp_index), drN).Fail())
+    return LLDB_INVALID_ADDRESS;
+  return drN.GetAsUInt64();
+}
+
+uint32_t
+NativeRegisterContextWatchpoint_x86::NumSupportedHardwareWatchpoints() {
+  // Available debug address registers: dr0, dr1, dr2, dr3
+  return 4;
+}