[Lldb-commits] [PATCH] D11899: Fix AArch64 watchpoint handlers in NativeRegisterContextLinux_arm64

[Tamas Berghammer via lldb-commits]

tberghammer added a comment.

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.


================
Comment at: source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp:514-519
@@ -507,8 +513,8 @@
     
     // 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;
     }
 		
----------------
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.

================
Comment at: source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp:537
@@ -529,1 +536,3 @@
+    // Flip watchpoint flag bits to match AArch64 write-read bit configuration.
+    watch_flags = ((watch_flags >> 1) | (watch_flags << 1));
 
----------------
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.

================
Comment at: source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp:753
@@ -737,6 +752,3 @@
 Error
-NativeRegisterContextLinux_arm64::WriteHardwareDebugRegs(lldb::addr_t *addr_buf,
-                                                         uint32_t *cntrl_buf,
-                                                         int type,
-                                                         int count)
+NativeRegisterContextLinux_arm64::WriteHardwareDebugRegs(int type)
 {
----------------
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)


http://reviews.llvm.org/D11899