[lldb] [Process/NetBSD] Fix handling concurrent watchpoint events

[mgorny]

Summary

Fix handling concurrent watchpoint events so that they are reported
correctly in LLDB.

If multiple watchpoints are hit concurrently, the NetBSD kernel reports
them as series of SIGTRAPs with a thread specified, and the debugger
investigates DR6 in order to establish which watchpoint was hit. This
is normally fine.

However, LLDB disables and reenables the watchpoint on all threads after
each hit, which results in the hit status from DR6 being wiped.
As a result, it can't establish which watchpoint was hit in successive
SIGTRAP processing.

In order to workaround this problem, clear DR6 only if the breakpoint
is overwritten with a new one. More specifically, move cleaning DR6
from ClearHardwareWatchpoint() to SetHardwareWatchpointWithIndex(),
and do that only if the newly requested watchpoint is different
from the one being set previously. This ensures that the disable-enable
logic of LLDB does not clear watchpoint hit status for the remaining
threads.

This also involves refactoring of watchpoint logic. With the old logic,
clearing watchpoint involved wiping dr6 & dr7, and setting it setting
dr{0..3} & dr7. With the new logic, only enable bit is cleared
from dr7, and the remaining bits are cleared/overwritten while setting
new watchpoint.

[krytarowski]

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

324

I presume that in this code path we land into a scenario that:

1. Trap on a different LWP
2. User sets new watchpoints
3. We land here with a SIGTRAP on old watchpoint that was wiped out.

If so, we shall ignore this report, bail out and resume execution with PT_CONTINUE.

I think that this path could be some remnant from Linux shared trap reasons.

[mgorny]

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

324

I was wondering whether it's better to silently ignore unknown watchpoints or stop the process. Decided the latter is better for the user, in case it carried some useful information still. Not to mention it has lower risk of crashing lldb-server if I get some logic wrong.

[mgorny]

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

892

Just occurred to me I can probably reuse the new function here.

[mgorny]

Updated as noted in comments, particularly reuse ClearWatchpointHit() when setting new wp.

[mgorny]

Included fixed tests.

[labath]

LGTM, since this seems to be the best we can do given the current netbsd behavior.

However, I'd like to repeat what I said on the IRC, that I consider this behavior of netbsd to be unreasonable. Serializing signals delivery seems nice at a first glance, but when those signals reflect events which are "trapped" (== caught only after the relevant thing has happened), as is the case with watchpoints on x86, things just start getting weird. This means that if we get a "concurrent" watchpoint hits, we will only report the one of the watchpoint hits, but the entire state of the program will be already in the "post-hit" state for both watchpoints (the memory will contain the new value, PC will point after the trapping instruction, etc.). I think this state can be very confusing to the user, particularly if he decides to run an expression in this state (to figure out what happened?) and then this expression is interrupted by the delayed delivery of the second watchpoint signal.

[krytarowski]

In D70025#1758800, @labath wrote:

LGTM, since this seems to be the best we can do given the current netbsd behavior.

However, I'd like to repeat what I said on the IRC, that I consider this behavior of netbsd to be unreasonable.

Thanks for the feedback. Evaluating the options, we will keep using our current model. There are no plans for any modifications in this domain.

The end user of a debugger is just expected to see no difference.

lldb/packages/Python/lldbsuite/test/functionalities/thread/concurrent_events/TestConcurrentManyWatchpoints.py

 from __future__ import print_function
 
 import unittest2
 
 from lldbsuite.test.decorators import *
 from lldbsuite.test.concurrent_base import ConcurrentEventsBase
 from lldbsuite.test.lldbtest import TestBase
 
 
 @skipIfWindows
 class ConcurrentManyWatchpoints(ConcurrentEventsBase):
 
     mydir = ConcurrentEventsBase.compute_mydir(__file__)
 
     # Atomic sequences are not supported yet for MIPS in LLDB.
     @skipIf(triple='^mips')
-    @expectedFailureNetBSD
     @add_test_categories(["watchpoint"])
     @skipIfOutOfTreeDebugserver
     def test(self):
         """Test 100 watchpoints from 100 threads."""
         self.build(dictionary=self.getBuildFlags())
         self.do_thread_actions(num_watchpoint_threads=100)

lldb/packages/Python/lldbsuite/test/functionalities/thread/concurrent_events/TestConcurrentNWatchNBreak.py

 @skipIfWindows
 class ConcurrentNWatchNBreak(ConcurrentEventsBase):
 
     mydir = ConcurrentEventsBase.compute_mydir(__file__)
 
     @skipIfFreeBSD  # timing out on buildbot
     # Atomic sequences are not supported yet for MIPS in LLDB.
     @skipIf(triple='^mips')
-    @expectedFailureNetBSD
     @add_test_categories(["watchpoint"])
     def test(self):
         """Test with 5 watchpoint and breakpoint threads."""
         self.build(dictionary=self.getBuildFlags())
         self.do_thread_actions(num_watchpoint_threads=5,
                                num_breakpoint_threads=5)

lldb/packages/Python/lldbsuite/test/functionalities/thread/concurrent_events/TestConcurrentTwoWatchpointThreads.py

 @skipIfWindows
 class ConcurrentTwoWatchpointThreads(ConcurrentEventsBase):
 
     mydir = ConcurrentEventsBase.compute_mydir(__file__)
 
     @skipIfFreeBSD  # timing out on buildbot
     # Atomic sequences are not supported yet for MIPS in LLDB.
     @skipIf(triple='^mips')
-    @expectedFailureNetBSD
     @add_test_categories(["watchpoint"])
     def test(self):
         """Test two threads that trigger a watchpoint. """
         self.build(dictionary=self.getBuildFlags())
         self.do_thread_actions(num_watchpoint_threads=2)

lldb/packages/Python/lldbsuite/test/functionalities/thread/concurrent_events/TestConcurrentTwoWatchpointsOneBreakpoint.py

 @skipIfWindows
 class ConcurrentTwoWatchpointsOneBreakpoint(ConcurrentEventsBase):
 
     mydir = ConcurrentEventsBase.compute_mydir(__file__)
 
     @skipIfFreeBSD  # timing out on buildbot
     # Atomic sequences are not supported yet for MIPS in LLDB.
     @skipIf(triple='^mips')
-    @expectedFailureNetBSD
     @add_test_categories(["watchpoint"])
     def test(self):
         """Test two threads that trigger a watchpoint and one breakpoint thread. """
         self.build(dictionary=self.getBuildFlags())
         self.do_thread_actions(
             num_watchpoint_threads=2,
             num_breakpoint_threads=1)

lldb/packages/Python/lldbsuite/test/functionalities/thread/concurrent_events/TestConcurrentTwoWatchpointsOneDelayBreakpoint.py

 @skipIfWindows
 class ConcurrentTwoWatchpointsOneDelayBreakpoint(ConcurrentEventsBase):
 
     mydir = ConcurrentEventsBase.compute_mydir(__file__)
 
     @skipIfFreeBSD  # timing out on buildbot
     # Atomic sequences are not supported yet for MIPS in LLDB.
     @skipIf(triple='^mips')
-    @expectedFailureNetBSD
     @add_test_categories(["watchpoint"])
     def test(self):
         """Test two threads that trigger a watchpoint and one (1 second delay) breakpoint thread. """
         self.build(dictionary=self.getBuildFlags())
         self.do_thread_actions(
             num_watchpoint_threads=2,
             num_delay_breakpoint_threads=1)

lldb/packages/Python/lldbsuite/test/python_api/watchpoint/watchlocation/TestSetWatchlocation.py

def setUp(self):
         self.source = 'main.cpp'
         # Find the line number to break inside main().
         self.line = line_number(
             self.source, '// Set break point at this line.')
         # This is for verifying that watch location works.
         self.violating_func = "do_bad_thing_with_location"
 
     @add_test_categories(['pyapi'])
-    @expectedFailureNetBSD
     def test_watch_location(self):
         """Exercise SBValue.WatchPointee() API to set a watchpoint."""
         self.build()
         exe = self.getBuildArtifact("a.out")
 
         # Create a target by the debugger.
         target = self.dbg.CreateTarget(exe)
         self.assertTrue(target, VALID_TARGET)

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

 //===-- NativeProcessNetBSD.cpp ------------------------------- -*- 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
 //
 //===----------------------------------------------------------------------===//
 
 #include "NativeProcessNetBSD.h"
 
-
+#include "Plugins/Process/NetBSD/NativeRegisterContextNetBSD.h"
 #include "Plugins/Process/POSIX/ProcessPOSIXLog.h"
 #include "lldb/Host/HostProcess.h"
 #include "lldb/Host/common/NativeRegisterContext.h"
 #include "lldb/Host/posix/ProcessLauncherPosixFork.h"
 #include "lldb/Target/Process.h"
 #include "lldb/Utility/State.h"
 #include "llvm/Support/Errno.h"
 
if (error.Fail()) {
       SetState(StateType::eStateInvalid);
       return;
     }
   } break;
   case TRAP_DBREG: {
     if (!thread)
       break;
 
+    auto &regctx = static_cast<NativeRegisterContextNetBSD &>(
+        thread->GetRegisterContext());
     uint32_t wp_index = LLDB_INVALID_INDEX32;
-    Status error = thread->GetRegisterContext().GetWatchpointHitIndex(
-        wp_index, (uintptr_t)info.psi_siginfo.si_addr);
+    Status error = regctx.GetWatchpointHitIndex(wp_index,
+        (uintptr_t)info.psi_siginfo.si_addr);
     if (error.Fail())
       LLDB_LOG(log,
                "received error while checking for watchpoint hits, pid = "
                "{0}, LWP = {1}, error = {2}", pid, info.psi_lwpid, error);
     if (wp_index != LLDB_INVALID_INDEX32) {
       thread->SetStoppedByWatchpoint(wp_index);
+      regctx.ClearWatchpointHit(wp_index);
       SetState(StateType::eStateStopped, true);
       break;
     }
 
     thread->SetStoppedByTrace();
     SetState(StateType::eStateStopped, true);
   } break;
   }
 }
 
 void NativeProcessNetBSD::MonitorSignal(lldb::pid_t pid, int signal) {
   ptrace_siginfo_t info;
   const auto siginfo_err =

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

public:
   // 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_x86_64.h

public:
 
   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/source/Plugins/Process/NetBSD/NativeRegisterContextNetBSD_x86_64.cpp

Status NativeRegisterContextNetBSD_x86_64::SetHardwareWatchpointWithIndex(
 
   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 reg_value;
   const RegisterInfo *const reg_info_dr7 =
       GetRegisterInfoAtIndex(lldb_dr7_x86_64);
-  error = ReadRegister(reg_info_dr7, reg_value);
+  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 = reg_value.GetAsUInt64() & ~bit_mask;
+  uint64_t control_bits = dr7_value.GetAsUInt64() & ~bit_mask;
 
   control_bits |= enable_bit | rw_bits | size_bits;
 
   const RegisterInfo *const reg_info_drN =
       GetRegisterInfoAtIndex(lldb_dr0_x86_64 + 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(lldb_dr7_x86_64);
   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, clear bits 0, 1, 2, or 3 of
+  // 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(lldb_dr6_x86_64);
+  RegisterValue reg_value;
   Status error = ReadRegister(reg_info_dr6, reg_value);
   if (error.Fail())
-    return false;
+    return error;
+
   uint64_t bit_mask = 1 << wp_index;
   uint64_t status_bits = reg_value.GetAsUInt64() & ~bit_mask;
-  error = WriteRegister(reg_info_dr6, RegisterValue(status_bits));
-  if (error.Fail())
-    return false;
-
-  // for watchpoints 0, 1, 2, or 3, respectively, clear bits {0-1,16-19},
-  // {2-3,20-23}, {4-5,24-27}, or {6-7,28-31} of the debug control register
-  // (DR7)
-  const RegisterInfo *const reg_info_dr7 =
-      GetRegisterInfoAtIndex(lldb_dr7_x86_64);
-  error = ReadRegister(reg_info_dr7, reg_value);
-  if (error.Fail())
-    return false;
-  bit_mask = (0x3 << (2 * wp_index)) | (0xF << (16 + 4 * wp_index));
-  uint64_t control_bits = reg_value.GetAsUInt64() & ~bit_mask;
-  return WriteRegister(reg_info_dr7, RegisterValue(control_bits)).Success();
+  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(lldb_dr6_x86_64);