[lldb] [Process/FreeBSDRemote] Explicitly copy dbregs to new threads

[mgorny]

Summary

Explicitly copy dbregs to new threads to ensure that watchpoints
are propagated properly. Fixes the test failure due to apparent kernel
race between reporting a new thread and resuming main thread execution
that makes implicit inheritance of dbregs unreliable. By copying them
explicitly, we ensure that the new thread correctly respects watchpoints
that were set after the thread was created but before it was reported.

The code is copied from the NetBSD plugin and modernized to use
llvm::Error.

[mgorny]

Alternatively, we could relist threads on every stop.

[labath]

So, is this like a kernel bug that we're working around here? Would it make sense to file a bug on the freebsd tracker and reference it from here?

[mgorny]

https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=250954

I'm still trying to figure out whether it's a real bug or just we're relying on too many variables.

[labath]

Interesting. It would still probably be useful to mention the uncertain state of this approach in the code somewhere...

[mgorny]

Yeah, I'm not going to proceed until I get more feedback from FreeBSD end.

[mgorny]

Added a verbose comment why we're doing that.

[mgorny]

clang-format

[labath]

lldb/source/Plugins/Process/FreeBSDRemote/NativeProcessFreeBSD.cpp

214

maybe this cast could be inside GetCurrentThread? Linux has it that way...

[mgorny]

lldb/source/Plugins/Process/FreeBSDRemote/NativeProcessFreeBSD.cpp

214

I suppose that makes sense. I'll try it in a followup commit.

[mgorny]

lldb/source/Plugins/Process/FreeBSDRemote/NativeProcessFreeBSD.cpp

214

Actually, we're using this method exactly once, so I don't think it makes sense at this point. However, I'll keep it in mind in case it becomes used more frequently.

lldb/source/Plugins/Process/FreeBSDRemote/NativeProcessFreeBSD.cpp

void NativeProcessFreeBSD::MonitorSIGTRAP(lldb::pid_t pid) {
 
   LLDB_LOG(log, "got SIGTRAP, pid = {0}, lwpid = {1}, flags = {2:x}", pid,
            info.pl_lwpid, info.pl_flags);
   NativeThreadFreeBSD *thread = nullptr;
 
   if (info.pl_flags & (PL_FLAG_BORN | PL_FLAG_EXITED)) {
     if (info.pl_flags & PL_FLAG_BORN) {
       LLDB_LOG(log, "monitoring new thread, tid = {0}", info.pl_lwpid);
-      AddThread(info.pl_lwpid);
+      NativeThreadFreeBSD &t = AddThread(info.pl_lwpid);
+
+      // Technically, the FreeBSD kernel copies the debug registers to new
+      // threads.  However, there is a non-negligible delay between acquiring
+      // the DR values and reporting the new thread during which the user may
+      // establish a new watchpoint.  In order to ensure that watchpoints
+      // established during this period are propagated to new threads,
+      // explicitly copy the DR value at the time the new thread is reported.
+      //
+      // See also: https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=250954
+
+      llvm::Error error = t.CopyWatchpointsFrom(
+          static_cast<NativeThreadFreeBSD &>(*GetCurrentThread()));
+      if (error) {
+        LLDB_LOG(log, "failed to copy watchpoints to new thread {0}: {1}",
+                 info.pl_lwpid, llvm::toString(std::move(error)));
+        SetState(StateType::eStateInvalid);
+        return;
+      }
     } else /*if (info.pl_flags & PL_FLAG_EXITED)*/ {
       LLDB_LOG(log, "thread exited, tid = {0}", info.pl_lwpid);
       RemoveThread(info.pl_lwpid);
     }
 
     Status error =
         PtraceWrapper(PT_CONTINUE, pid, reinterpret_cast<void *>(1), 0);
     if (error.Fail())

lldb/source/Plugins/Process/FreeBSDRemote/NativeRegisterContextFreeBSD.h

public:
   // This function is implemented in the NativeRegisterContextFreeBSD_*
   // subclasses to create a new instance of the host specific
   // NativeRegisterContextFreeBSD. The implementations can't collide as only one
   // NativeRegisterContextFreeBSD_* variant should be compiled into the final
   // executable.
   static NativeRegisterContextFreeBSD *
   CreateHostNativeRegisterContextFreeBSD(const ArchSpec &target_arch,
                                          NativeThreadProtocol &native_thread);
+  virtual llvm::Error
+  CopyHardwareWatchpointsFrom(NativeRegisterContextFreeBSD &source) = 0;
 
 protected:
   virtual NativeProcessFreeBSD &GetProcess();
   virtual ::pid_t GetProcessPid();
 };
 
 } // namespace process_freebsd
 } // namespace lldb_private
 
 #endif // #ifndef lldb_NativeRegisterContextFreeBSD_h

lldb/source/Plugins/Process/FreeBSDRemote/NativeRegisterContextFreeBSD_x86_64.h

public:
 
   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;
 
+  llvm::Error
+  CopyHardwareWatchpointsFrom(NativeRegisterContextFreeBSD &source) override;
+
 private:
   // Private member types.
   enum { GPRegSet, FPRegSet, XSaveRegSet, DBRegSet };
   enum {
     YMMXSaveSet,
     MaxXSaveSet = YMMXSaveSet,
   };
 

lldb/source/Plugins/Process/FreeBSDRemote/NativeRegisterContextFreeBSD_x86_64.cpp

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.");
   }
 }
 
+llvm::Error NativeRegisterContextFreeBSD_x86_64::CopyHardwareWatchpointsFrom(
+    NativeRegisterContextFreeBSD &source) {
+  auto &r_source = static_cast<NativeRegisterContextFreeBSD_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 llvm::Error::success();
+
+    m_dbr = r_source.m_dbr;
+    res = WriteRegisterSet(DBRegSet);
+  }
+  return res.ToError();
+}
+
 #endif // defined(__x86_64__)

lldb/source/Plugins/Process/FreeBSDRemote/NativeThreadFreeBSD.h

private:
   void SetStoppedByTrace();
   void SetStoppedByExec();
   void SetStoppedByWatchpoint(uint32_t wp_index);
   void SetStoppedWithNoReason();
   void SetStopped();
   void SetRunning();
   void SetStepping();
 
-  Status CopyWatchpointsFrom(NativeThreadFreeBSD &source);
+  llvm::Error CopyWatchpointsFrom(NativeThreadFreeBSD &source);
 
   // Member Variables
   lldb::StateType m_state;
   ThreadStopInfo m_stop_info;
   std::unique_ptr<NativeRegisterContextFreeBSD> m_reg_context_up;
   std::string m_stop_description;
   using WatchpointIndexMap = std::map<lldb::addr_t, uint32_t>;
   WatchpointIndexMap m_watchpoint_index_map;
   WatchpointIndexMap m_hw_break_index_map;
 };
 
 typedef std::shared_ptr<NativeThreadFreeBSD> NativeThreadFreeBSDSP;
 } // namespace process_freebsd
 } // namespace lldb_private
 
 #endif // #ifndef liblldb_NativeThreadFreeBSD_H_

lldb/source/Plugins/Process/FreeBSDRemote/NativeThreadFreeBSD.cpp

while (1) {
     void *ptr = len == 0 ? nullptr : kp.data();
     int error = ::sysctl(mib, 4, ptr, &len, nullptr, 0);
     if (ptr == nullptr || (error != 0 && errno == ENOMEM)) {
       kp.resize(len / sizeof(struct kinfo_proc));
       continue;
     }
     if (error != 0) {
       len = 0;
-      LLDB_LOG(log, "tid = {0} in state {1} failed to get thread name: {2}", GetID(),
-               m_state, strerror(errno));
+      LLDB_LOG(log, "tid = {0} in state {1} failed to get thread name: {2}",
+               GetID(), m_state, strerror(errno));
     }
     kp.resize(len / sizeof(struct kinfo_proc));
     break;
   }
 
-  for (auto& procinfo : kp) {
+  for (auto &procinfo : kp) {
     if (procinfo.ki_tid == static_cast<lwpid_t>(GetID()))
       return procinfo.ki_tdname;
   }
 
   return "";
 }
 
 lldb::StateType NativeThreadFreeBSD::GetState() { return m_state; }
Status NativeThreadFreeBSD::RemoveHardwareBreakpoint(lldb::addr_t addr) {
   uint32_t bp_index = bp->second;
   if (GetRegisterContext().ClearHardwareBreakpoint(bp_index)) {
     m_hw_break_index_map.erase(bp);
     return Status();
   }
 
   return Status("Clearing hardware breakpoint failed.");
 }
+
+llvm::Error
+NativeThreadFreeBSD::CopyWatchpointsFrom(NativeThreadFreeBSD &source) {
+  llvm::Error s = GetRegisterContext().CopyHardwareWatchpointsFrom(
+      source.GetRegisterContext());
+  if (!s) {
+    m_watchpoint_index_map = source.m_watchpoint_index_map;
+    m_hw_break_index_map = source.m_hw_break_index_map;
+  }
+  return s;
+}

lldb/test/API/commands/watchpoints/multiple_threads/TestWatchpointMultipleThreads.py

class WatchpointForMultipleThreadsTestCase(TestBase):
     mydir = TestBase.compute_mydir(__file__)
     NO_DEBUG_INFO_TESTCASE = True
     main_spec = lldb.SBFileSpec("main.cpp", False)
 
     def test_watchpoint_before_thread_start(self):
         """Test that we can hit a watchpoint we set before starting another thread"""
         self.do_watchpoint_test("Before running the thread")
 
-    @expectedFailureAll(oslist=["freebsd"])
     def test_watchpoint_after_thread_launch(self):
         """Test that we can hit a watchpoint we set after launching another thread"""
         self.do_watchpoint_test("After launching the thread")
 
     def test_watchpoint_after_thread_start(self):
         """Test that we can hit a watchpoint we set after another thread starts"""
         self.do_watchpoint_test("After running the thread")