Fix AArch64 watchpoint handlers in NativeRegisterContextLinux_arm64

[omjavaid]

Summary

There were some bugs that needed to be fixed in watchpoint handling code on arm64.

Watchpoints were being written to all watchpoint registers and cache was not being maintained correctly.

This patch fixes up all these issues. Watchpoints now work on arm64 except that there is race condition which is not allowing lldb to step out of a watchpoint hit before we can continue again so inferior gets stuck at that point. Manually disabling the watchpoint then issuing a step and then enabling the watchpoint again fixes shows that watchpoints are being installed and removed correctly.

[tberghammer]

Generally looks good to me. I am happy to push the 2 cleanup change to a separate CL but please check that the read/write flag calculation is correct.

source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp

514–519

You use this pattern several times in this class. It would be nice if you can move it out to a separate function to avoid code duplication.

537

I think this expression is incorrect. Based on http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0500d/CEGDIEBB.html I don't see why you need this at all and if watch_flags==0x3 then it will set watch_flags to 0x7 what looks wrong for me.

753

The 0/1 for the type isn't too descriptive. Please use an enum here (or the NT_ARM_HW_WATCH/NT_ARM_HW_BREAK constants)

[clayborg]

I will defer to Tamas Berghammer since I have no expertise is arm64 watchpoints.

[omjavaid]

I have updated this patch after incorporating suggestions.

Is it good for commit now?

[tberghammer]

Looks good. Thank you for fixing it.

source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp

755

Please use eDREGTypeWATCH instead of '0'

source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.h

private:
 
         bool
         IsGPR(unsigned reg) const;
 
         bool
         IsFPR(unsigned reg) const;
 
         Error
-        ReadHardwareDebugInfo(unsigned int &watch_count , unsigned int &break_count);
+        ReadHardwareDebugInfo();
 
         Error
-        WriteHardwareDebugRegs(lldb::addr_t *addr_buf, uint32_t *cntrl_buf, int type, int count);
+        WriteHardwareDebugRegs(int type);
     };
 
 } // namespace process_linux
 } // namespace lldb_private
 
 #endif // #ifndef lldb_NativeRegisterContextLinux_arm64_h
 
 #endif // defined (__arm64__) || defined (__aarch64__)

source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp

 uint32_t
 NativeRegisterContextLinux_arm64::SetHardwareBreakpoint (lldb::addr_t addr, size_t size)
 {
     Log *log(lldb_private::GetLogIfAllCategoriesSet (LIBLLDB_LOG_WATCHPOINTS));
 
     if (log)
         log->Printf ("NativeRegisterContextLinux_arm64::%s()", __FUNCTION__);
 
-    NativeProcessProtocolSP process_sp (m_thread.GetProcess ());
-    if (!process_sp)
-        return false;
-
     // Check if our hardware breakpoint and watchpoint information is updated.
     if (m_refresh_hwdebug_info)
     {
-        ReadHardwareDebugInfo (m_max_hwp_supported, m_max_hbp_supported);
+        ReadHardwareDebugInfo ();
         m_refresh_hwdebug_info = false;
     }
 
     uint32_t control_value, bp_index;
 
     // Check if size has a valid hardware breakpoint length.
     if (size != 4)
         return LLDB_INVALID_INDEX32;  // Invalid size for a AArch64 hardware breakpoint
NativeRegisterContextLinux_arm64::SetHardwareBreakpoint (lldb::addr_t addr, size_t size)
 
     // Add new or update existing watchpoint
     if ((m_hbr_regs[bp_index].control & 1) == 0)
     {
         m_hbr_regs[bp_index].address = addr;
         m_hbr_regs[bp_index].control = control_value;
         m_hbr_regs[bp_index].refcount = 1;
 
-        //TODO: PTRACE CALL HERE for an UPDATE
+        // PTRACE call to set corresponding hardware breakpoint register.
+        WriteHardwareDebugRegs(1);
     }
     else
         m_hbr_regs[bp_index].refcount++;
 
     return bp_index;
 }
 
 bool
 NativeRegisterContextLinux_arm64::ClearHardwareBreakpoint (uint32_t hw_idx)
 {
     Log *log(lldb_private::GetLogIfAllCategoriesSet (LIBLLDB_LOG_WATCHPOINTS));
 
     if (log)
         log->Printf ("NativeRegisterContextLinux_arm64::%s()", __FUNCTION__);
 
+    // Check if our hardware breakpoint and watchpoint information is updated.
+    if (m_refresh_hwdebug_info)
+    {
+        ReadHardwareDebugInfo ();
+        m_refresh_hwdebug_info = false;
+    }
+
     if (hw_idx >= m_max_hbp_supported)
         return false;
 
     // Update reference count if multiple references.
     if (m_hbr_regs[hw_idx].refcount > 1)
     {
         m_hbr_regs[hw_idx].refcount--;
         return true;
     }
     else if (m_hbr_regs[hw_idx].refcount == 1)
     {
         m_hbr_regs[hw_idx].control &= ~1;
         m_hbr_regs[hw_idx].address = 0;
         m_hbr_regs[hw_idx].refcount = 0;
 
-        //TODO: PTRACE CALL HERE for an UPDATE
-        return true;
+        // PTRACE call to clear corresponding hardware breakpoint register.
+        WriteHardwareDebugRegs(1);
     }
 
     return false;
 }
 
 uint32_t
 NativeRegisterContextLinux_arm64::NumSupportedHardwareWatchpoints ()
 {
     Log *log(lldb_private::GetLogIfAllCategoriesSet (LIBLLDB_LOG_WATCHPOINTS));
 
     if (log)
         log->Printf ("NativeRegisterContextLinux_arm64::%s()", __FUNCTION__);
 
+    // Check if our hardware breakpoint and watchpoint information is updated.
+    if (m_refresh_hwdebug_info)
+    {
+        ReadHardwareDebugInfo ();
+        m_refresh_hwdebug_info = false;
+    }
+
     return m_max_hwp_supported;
 }
 
 uint32_t
 NativeRegisterContextLinux_arm64::SetHardwareWatchpoint (lldb::addr_t addr, size_t size, uint32_t watch_flags)
 {
     Log *log(lldb_private::GetLogIfAllCategoriesSet (LIBLLDB_LOG_WATCHPOINTS));
 
     if (log)
         log->Printf ("NativeRegisterContextLinux_arm64::%s()", __FUNCTION__);
     
-    NativeProcessProtocolSP process_sp (m_thread.GetProcess ());
-    if (!process_sp)
-        return false;
-
-    
     // Check if our hardware breakpoint and watchpoint information is updated.
     if (m_refresh_hwdebug_info)
     {
-        ReadHardwareDebugInfo (m_max_hwp_supported, m_max_hbp_supported);
+        ReadHardwareDebugInfo ();
         m_refresh_hwdebug_info = false;
     }
     
     uint32_t control_value, wp_index;
 
-
+    // Check if we are setting watchpoint other than read/write/access
     if (watch_flags != 0x1 && watch_flags != 0x2 && watch_flags != 0x3)
-        return 0;//Error ("Invalid read/write bits for watchpoint");
+        return LLDB_INVALID_INDEX32;
 
     // Check if size has a valid hardware watchpoint length.
     if (size != 1 && size != 2 && size != 4 && size != 8)
-        return 0;//Error ("Invalid size for watchpoint");
+        return LLDB_INVALID_INDEX32;
 
     // Check 8-byte alignment for hardware watchpoint target address.
     // TODO: Add support for watching un-aligned addresses
     if (addr & 0x07)
-        return 0;//Error ("LLDB for AArch64 currently supports 8-byte alignment for hardware watchpoint target address.");
+        return LLDB_INVALID_INDEX32;
+
+    // Flip watchpoint flag bits to match AArch64 write-read bit configuration.
+    watch_flags = ((watch_flags >> 1) | (watch_flags << 1));
 
     // Setup control value
     control_value = watch_flags << 3;
     control_value |= ((1 << size) - 1) << 5;
     control_value |= (2 << 1) | 1;
 
     // Iterate over stored watchpoints
     // Find a free wp_index or update reference count if duplicate.
NativeRegisterContextLinux_arm64::SetHardwareWatchpoint (lldb::addr_t addr, size_t size, uint32_t watch_flags)
     }
 
      if (wp_index == LLDB_INVALID_INDEX32)
         return LLDB_INVALID_INDEX32;
 
     // Add new or update existing watchpoint
     if ((m_hwp_regs[wp_index].control & 1) == 0)
     {
+        // Update watchpoint in local cache
         m_hwp_regs[wp_index].address = addr;
         m_hwp_regs[wp_index].control = control_value;
         m_hwp_regs[wp_index].refcount = 1;
 
         // PTRACE call to set corresponding watchpoint register.
-        WriteHardwareDebugRegs(&addr, &control_value, 0, wp_index);
+        WriteHardwareDebugRegs(0);
     }
     else
         m_hwp_regs[wp_index].refcount++;
 
     return wp_index;
 }
 
 bool
 NativeRegisterContextLinux_arm64::ClearHardwareWatchpoint (uint32_t wp_index)
 {
     Log *log(lldb_private::GetLogIfAllCategoriesSet (LIBLLDB_LOG_WATCHPOINTS));
 
     if (log)
         log->Printf ("NativeRegisterContextLinux_arm64::%s()", __FUNCTION__);
 
-    NativeProcessProtocolSP process_sp (m_thread.GetProcess ());
-    if (!process_sp)
-        return false;
+    // Check if our hardware breakpoint and watchpoint information is updated.
+    if (m_refresh_hwdebug_info)
+    {
+        ReadHardwareDebugInfo ();
+        m_refresh_hwdebug_info = false;
+    }
 
     if (wp_index >= m_max_hwp_supported)
         return false;
 
     // Update reference count if multiple references.
     if (m_hwp_regs[wp_index].refcount > 1)
     {
         m_hwp_regs[wp_index].refcount--;
         return true;
     }
     else if (m_hwp_regs[wp_index].refcount == 1)
     {
+        // Update watchpoint in local cache
         m_hwp_regs[wp_index].control &= ~1;
         m_hwp_regs[wp_index].address = 0;
         m_hwp_regs[wp_index].refcount = 0;
 
-        //TODO: PTRACE CALL HERE for an UPDATE
-        WriteHardwareDebugRegs(&m_hwp_regs[wp_index].address, &m_hwp_regs[wp_index].control, 0, wp_index);
+        // Ptrace call to update hardware debug registers
+        WriteHardwareDebugRegs(0);
         return true;
     }
 
     return false;
 }
 
 Error
 NativeRegisterContextLinux_arm64::ClearAllHardwareWatchpoints ()
 {
     Log *log(lldb_private::GetLogIfAllCategoriesSet (LIBLLDB_LOG_WATCHPOINTS));
 
     if (log)
         log->Printf ("NativeRegisterContextLinux_arm64::%s()", __FUNCTION__);
 
-    NativeProcessProtocolSP process_sp (m_thread.GetProcess ());
-
-    Error ml_error;
-    ml_error.SetErrorToErrno();
-    if (!process_sp)
-        return ml_error;
+    // Check if our hardware breakpoint and watchpoint information is updated.
+    if (m_refresh_hwdebug_info)
+    {
+        ReadHardwareDebugInfo ();
+        m_refresh_hwdebug_info = false;
+    }
 
     for (uint32_t i = 0; i < m_max_hwp_supported; i++)
     {
         if (m_hwp_regs[i].control & 0x01)
         {
+            // Clear watchpoints in local cache
             m_hwp_regs[i].control &= ~1;
             m_hwp_regs[i].address = 0;
             m_hwp_regs[i].refcount = 0;
 
-            WriteHardwareDebugRegs(&m_hwp_regs[i].address, &m_hwp_regs[i].control, 0, i);
+            // Ptrace call to update hardware debug registers
+            WriteHardwareDebugRegs(0);
         }
     }
 
     return Error();
 }
 
 uint32_t
 NativeRegisterContextLinux_arm64::GetWatchpointSize(uint32_t wp_index)
NativeRegisterContextLinux_arm64::GetWatchpointAddress (uint32_t wp_index)
 
     if (WatchpointIsEnabled(wp_index))
         return m_hwp_regs[wp_index].address;
     else
         return LLDB_INVALID_ADDRESS;
 }
 
 Error
-NativeRegisterContextLinux_arm64::ReadHardwareDebugInfo(unsigned int &watch_count,
-                                                        unsigned int &break_count)
+NativeRegisterContextLinux_arm64::ReadHardwareDebugInfo()
 {
     ::pid_t tid = m_thread.GetID();
 
     int regset = NT_ARM_HW_WATCH;
     struct iovec ioVec;
     struct user_hwdebug_state dreg_state;
     Error error;
 
     ioVec.iov_base = &dreg_state;
     ioVec.iov_len = sizeof (dreg_state);
     error = NativeProcessLinux::PtraceWrapper(PTRACE_GETREGSET, tid, &regset, &ioVec, ioVec.iov_len);
-    watch_count = dreg_state.dbg_info & 0xff;
+    m_max_hwp_supported = dreg_state.dbg_info & 0xff;
 
     regset = NT_ARM_HW_BREAK;
     error = NativeProcessLinux::PtraceWrapper(PTRACE_GETREGSET, tid, &regset, &ioVec, ioVec.iov_len);
-    break_count = dreg_state.dbg_info & 0xff;
+    m_max_hbp_supported = dreg_state.dbg_info & 0xff;
 
     return error;
 }
 
 Error
-NativeRegisterContextLinux_arm64::WriteHardwareDebugRegs(lldb::addr_t *addr_buf,
-                                                         uint32_t *cntrl_buf,
-                                                         int type,
-                                                         int count)
+NativeRegisterContextLinux_arm64::WriteHardwareDebugRegs(int type)
 {
     struct iovec ioVec;
     struct user_hwdebug_state dreg_state;
     Error error;
 
     memset (&dreg_state, 0, sizeof (dreg_state));
     ioVec.iov_base = &dreg_state;
     ioVec.iov_len = sizeof (dreg_state);
 
     if (type == 0)
+    {
         type = NT_ARM_HW_WATCH;
+
+        for (uint32_t i = 0; i < m_max_hwp_supported; i++)
+        {
+            dreg_state.dbg_regs[i].addr = m_hwp_regs[i].address;
+            dreg_state.dbg_regs[i].ctrl = m_hwp_regs[i].control;
+        }
+    }
     else
+    {
         type = NT_ARM_HW_BREAK;
 
-    for (int i = 0; i < count; i++)
+        for (uint32_t i = 0; i < m_max_hbp_supported; i++)
         {
-        dreg_state.dbg_regs[i].addr = addr_buf[i];
-        dreg_state.dbg_regs[i].ctrl = cntrl_buf[i];
+            dreg_state.dbg_regs[i].addr = m_hbr_regs[i].address;
+            dreg_state.dbg_regs[i].ctrl = m_hbr_regs[i].control;
+        }
     }
 
     return NativeProcessLinux::PtraceWrapper(PTRACE_SETREGSET, m_thread.GetID(), &type, &ioVec, ioVec.iov_len);
 }
 
 Error
 NativeRegisterContextLinux_arm64::DoReadRegisterValue(uint32_t offset,
                                                       const char* reg_name,