From 7644d8ba4dc4e06b2db2bfdb7f4d76b9356cb288 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20G=C3=B3rny?= Date: Fri, 8 Nov 2019 18:42:37 +0100 Subject: [PATCH] [lldb] [Process/NetBSD] Fix handling concurrent watchpoint events 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. Differential Revision: https://reviews.llvm.org/D70025 --- .../TestConcurrentManyWatchpoints.py | 1 - .../TestConcurrentNWatchNBreak.py | 1 - .../TestConcurrentTwoWatchpointThreads.py | 1 - ...stConcurrentTwoWatchpointsOneBreakpoint.py | 1 - ...currentTwoWatchpointsOneDelayBreakpoint.py | 1 - .../watchlocation/TestSetWatchlocation.py | 1 - .../Process/NetBSD/NativeProcessNetBSD.cpp | 9 ++- .../NetBSD/NativeRegisterContextNetBSD.h | 2 + .../NativeRegisterContextNetBSD_x86_64.cpp | 58 ++++++++++++------- .../NativeRegisterContextNetBSD_x86_64.h | 2 + 10 files changed, 47 insertions(+), 30 deletions(-) diff --git a/lldb/packages/Python/lldbsuite/test/functionalities/thread/concurrent_events/TestConcurrentManyWatchpoints.py b/lldb/packages/Python/lldbsuite/test/functionalities/thread/concurrent_events/TestConcurrentManyWatchpoints.py index cce537ac77ae..55c7d3f7521b 100644 --- a/lldb/packages/Python/lldbsuite/test/functionalities/thread/concurrent_events/TestConcurrentManyWatchpoints.py +++ b/lldb/packages/Python/lldbsuite/test/functionalities/thread/concurrent_events/TestConcurrentManyWatchpoints.py @@ -14,7 +14,6 @@ class ConcurrentManyWatchpoints(ConcurrentEventsBase): # Atomic sequences are not supported yet for MIPS in LLDB. @skipIf(triple='^mips') - @expectedFailureNetBSD @add_test_categories(["watchpoint"]) @skipIfOutOfTreeDebugserver def test(self): diff --git a/lldb/packages/Python/lldbsuite/test/functionalities/thread/concurrent_events/TestConcurrentNWatchNBreak.py b/lldb/packages/Python/lldbsuite/test/functionalities/thread/concurrent_events/TestConcurrentNWatchNBreak.py index 84a4ac6f7a34..b921ac04ccc5 100644 --- a/lldb/packages/Python/lldbsuite/test/functionalities/thread/concurrent_events/TestConcurrentNWatchNBreak.py +++ b/lldb/packages/Python/lldbsuite/test/functionalities/thread/concurrent_events/TestConcurrentNWatchNBreak.py @@ -15,7 +15,6 @@ class ConcurrentNWatchNBreak(ConcurrentEventsBase): @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.""" diff --git a/lldb/packages/Python/lldbsuite/test/functionalities/thread/concurrent_events/TestConcurrentTwoWatchpointThreads.py b/lldb/packages/Python/lldbsuite/test/functionalities/thread/concurrent_events/TestConcurrentTwoWatchpointThreads.py index 62eac557cfcc..025d91169451 100644 --- a/lldb/packages/Python/lldbsuite/test/functionalities/thread/concurrent_events/TestConcurrentTwoWatchpointThreads.py +++ b/lldb/packages/Python/lldbsuite/test/functionalities/thread/concurrent_events/TestConcurrentTwoWatchpointThreads.py @@ -15,7 +15,6 @@ class ConcurrentTwoWatchpointThreads(ConcurrentEventsBase): @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. """ diff --git a/lldb/packages/Python/lldbsuite/test/functionalities/thread/concurrent_events/TestConcurrentTwoWatchpointsOneBreakpoint.py b/lldb/packages/Python/lldbsuite/test/functionalities/thread/concurrent_events/TestConcurrentTwoWatchpointsOneBreakpoint.py index 31d6e425a627..5e95531ae09a 100644 --- a/lldb/packages/Python/lldbsuite/test/functionalities/thread/concurrent_events/TestConcurrentTwoWatchpointsOneBreakpoint.py +++ b/lldb/packages/Python/lldbsuite/test/functionalities/thread/concurrent_events/TestConcurrentTwoWatchpointsOneBreakpoint.py @@ -15,7 +15,6 @@ class ConcurrentTwoWatchpointsOneBreakpoint(ConcurrentEventsBase): @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. """ diff --git a/lldb/packages/Python/lldbsuite/test/functionalities/thread/concurrent_events/TestConcurrentTwoWatchpointsOneDelayBreakpoint.py b/lldb/packages/Python/lldbsuite/test/functionalities/thread/concurrent_events/TestConcurrentTwoWatchpointsOneDelayBreakpoint.py index 16584472c975..aa57e816bb58 100644 --- a/lldb/packages/Python/lldbsuite/test/functionalities/thread/concurrent_events/TestConcurrentTwoWatchpointsOneDelayBreakpoint.py +++ b/lldb/packages/Python/lldbsuite/test/functionalities/thread/concurrent_events/TestConcurrentTwoWatchpointsOneDelayBreakpoint.py @@ -15,7 +15,6 @@ class ConcurrentTwoWatchpointsOneDelayBreakpoint(ConcurrentEventsBase): @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. """ diff --git a/lldb/packages/Python/lldbsuite/test/python_api/watchpoint/watchlocation/TestSetWatchlocation.py b/lldb/packages/Python/lldbsuite/test/python_api/watchpoint/watchlocation/TestSetWatchlocation.py index 28e18620ab11..97559212b17d 100644 --- a/lldb/packages/Python/lldbsuite/test/python_api/watchpoint/watchlocation/TestSetWatchlocation.py +++ b/lldb/packages/Python/lldbsuite/test/python_api/watchpoint/watchlocation/TestSetWatchlocation.py @@ -28,7 +28,6 @@ class SetWatchlocationAPITestCase(TestBase): 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() diff --git a/lldb/source/Plugins/Process/NetBSD/NativeProcessNetBSD.cpp b/lldb/source/Plugins/Process/NetBSD/NativeProcessNetBSD.cpp index 372ac059cb5d..4cfcdab6aab3 100644 --- a/lldb/source/Plugins/Process/NetBSD/NativeProcessNetBSD.cpp +++ b/lldb/source/Plugins/Process/NetBSD/NativeProcessNetBSD.cpp @@ -8,7 +8,7 @@ #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" @@ -305,15 +305,18 @@ void NativeProcessNetBSD::MonitorSIGTRAP(lldb::pid_t pid) { if (!thread) break; + auto ®ctx = static_cast( + 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; } diff --git a/lldb/source/Plugins/Process/NetBSD/NativeRegisterContextNetBSD.h b/lldb/source/Plugins/Process/NetBSD/NativeRegisterContextNetBSD.h index 2947454af2d8..13e023d856d2 100644 --- a/lldb/source/Plugins/Process/NetBSD/NativeRegisterContextNetBSD.h +++ b/lldb/source/Plugins/Process/NetBSD/NativeRegisterContextNetBSD.h @@ -34,6 +34,8 @@ public: virtual Status CopyHardwareWatchpointsFrom(NativeRegisterContextNetBSD &source) = 0; + virtual Status ClearWatchpointHit(uint32_t wp_index) = 0; + protected: Status DoRegisterSet(int req, void *buf); virtual NativeProcessNetBSD &GetProcess(); diff --git a/lldb/source/Plugins/Process/NetBSD/NativeRegisterContextNetBSD_x86_64.cpp b/lldb/source/Plugins/Process/NetBSD/NativeRegisterContextNetBSD_x86_64.cpp index afb62b8170ef..57ef39d4a07d 100644 --- a/lldb/source/Plugins/Process/NetBSD/NativeRegisterContextNetBSD_x86_64.cpp +++ b/lldb/source/Plugins/Process/NetBSD/NativeRegisterContextNetBSD_x86_64.cpp @@ -853,10 +853,10 @@ Status NativeRegisterContextNetBSD_x86_64::SetHardwareWatchpointWithIndex( 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; @@ -874,16 +874,28 @@ Status NativeRegisterContextNetBSD_x86_64::SetHardwareWatchpointWithIndex( 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); - error = WriteRegister(reg_info_drN, RegisterValue(addr)); + 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; @@ -897,32 +909,36 @@ bool NativeRegisterContextNetBSD_x86_64::ClearHardwareWatchpoint( 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; - // for watchpoints 0, 1, 2, or 3, respectively, clear bits 0, 1, 2, or 3 of + 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(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() { diff --git a/lldb/source/Plugins/Process/NetBSD/NativeRegisterContextNetBSD_x86_64.h b/lldb/source/Plugins/Process/NetBSD/NativeRegisterContextNetBSD_x86_64.h index 0da0351a7b0c..54b8a806267f 100644 --- a/lldb/source/Plugins/Process/NetBSD/NativeRegisterContextNetBSD_x86_64.h +++ b/lldb/source/Plugins/Process/NetBSD/NativeRegisterContextNetBSD_x86_64.h @@ -58,6 +58,8 @@ public: 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,