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

         bool
         WatchpointIsEnabled(uint32_t wp_index);
 
+        // Debug register type select
+        enum DREGType
+        {
+            eDREGTypeWATCH = 0,
+            eDREGTypeBREAK
+        };
+
     protected:
         Error
         DoReadRegisterValue(uint32_t offset,

         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 hwbType);
     };
 
 } // namespace process_linux

source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp

     if (log)
         log->Printf ("NativeRegisterContextLinux_arm64::%s()", __FUNCTION__);
 
-    NativeProcessProtocolSP process_sp (m_thread.GetProcess ());
-    if (!process_sp)
-        return false;
+    // Read hardware breakpoint and watchpoint information.
+    ReadHardwareDebugInfo ();
 
-    // Check if our hardware breakpoint and watchpoint information is updated.
-    if (m_refresh_hwdebug_info)
-    {
-        ReadHardwareDebugInfo (m_max_hwp_supported, m_max_hbp_supported);
-        m_refresh_hwdebug_info = false;
-    }
-
     uint32_t control_value, bp_index;
 
     // Check if size has a valid hardware breakpoint length.

         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(eDREGTypeBREAK);
     }
     else
         m_hbr_regs[bp_index].refcount++;

     if (log)
         log->Printf ("NativeRegisterContextLinux_arm64::%s()", __FUNCTION__);
 
+    // Read hardware breakpoint and watchpoint information.
+    ReadHardwareDebugInfo ();
+
     if (hw_idx >= m_max_hbp_supported)
         return false;
 

         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(eDREGTypeBREAK);
     }
 
     return false;

     if (log)
         log->Printf ("NativeRegisterContextLinux_arm64::%s()", __FUNCTION__);
 
+    // Read hardware breakpoint and watchpoint information.
+    ReadHardwareDebugInfo ();
+
     return m_max_hwp_supported;
 }
 

 
     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);
-        m_refresh_hwdebug_info = false;
-    }
+    // Read hardware breakpoint and watchpoint information.
+    ReadHardwareDebugInfo ();
     
     uint32_t control_value, wp_index;
 
+    // Check if we are setting watchpoint other than read/write/access
+    // Also update watchpoint flag to match AArch64 write-read bit configuration.
+    switch (watch_flags)
+    {
+        case 1:
+            watch_flags = 2;
+            break;
+        case 2:
+            watch_flags = 1;
+            break;
+        case 3:
+            break;
+        default:
+            return LLDB_INVALID_INDEX32;
+    }
 
-    if (watch_flags != 0x1 && watch_flags != 0x2 && watch_flags != 0x3)
-        return 0;//Error ("Invalid read/write bits for watchpoint");
-
     // 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;
 
     // Setup control value
     control_value = watch_flags << 3;

     // 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(eDREGTypeWATCH);
     }
     else
         m_hwp_regs[wp_index].refcount++;

     if (log)
         log->Printf ("NativeRegisterContextLinux_arm64::%s()", __FUNCTION__);
 
-    NativeProcessProtocolSP process_sp (m_thread.GetProcess ());
-    if (!process_sp)
-        return false;
+    // Read hardware breakpoint and watchpoint information.
+    ReadHardwareDebugInfo ();
 
     if (wp_index >= m_max_hwp_supported)
         return false;

     }
     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(eDREGTypeWATCH);
         return true;
     }
 

     if (log)
         log->Printf ("NativeRegisterContextLinux_arm64::%s()", __FUNCTION__);
 
-    NativeProcessProtocolSP process_sp (m_thread.GetProcess ());
+    // Read hardware breakpoint and watchpoint information.
+    ReadHardwareDebugInfo ();
 
-    Error ml_error;
-    ml_error.SetErrorToErrno();
-    if (!process_sp)
-        return ml_error;
-
     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(eDREGTypeWATCH);
         }
     }
 

 }
 
 Error
-NativeRegisterContextLinux_arm64::ReadHardwareDebugInfo(unsigned int &watch_count,
-                                                        unsigned int &break_count)
+NativeRegisterContextLinux_arm64::ReadHardwareDebugInfo()
 {
+    if (!m_refresh_hwdebug_info)
+    {
+        return Error();
+    }
+
     ::pid_t tid = m_thread.GetID();
 
     int regset = NT_ARM_HW_WATCH;

     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;
 
+    m_refresh_hwdebug_info = false;
+
     return error;
 }
 
 Error
-NativeRegisterContextLinux_arm64::WriteHardwareDebugRegs(lldb::addr_t *addr_buf,
-                                                         uint32_t *cntrl_buf,
-                                                         int type,
-                                                         int count)
+NativeRegisterContextLinux_arm64::WriteHardwareDebugRegs(int hwbType)
 {
     struct iovec ioVec;
     struct user_hwdebug_state dreg_state;

     ioVec.iov_base = &dreg_state;
     ioVec.iov_len = sizeof (dreg_state);
 
-    if (type == 0)
-        type = NT_ARM_HW_WATCH;
+    if (hwbType == 0)
+    {
+        hwbType = 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;
+    {
+        hwbType = 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);
+    return NativeProcessLinux::PtraceWrapper(PTRACE_SETREGSET, m_thread.GetID(), &hwbType, &ioVec, ioVec.iov_len);
 }
 
 Error