[Lldb-commits] [PATCH] D9703: Adds support for ARM hardware watchpoints

[Tamas Berghammer via lldb-commits]

tberghammer accepted this revision.
tberghammer added a comment.
This revision is now accepted and ready to land.

Looks good with a few minor comments inline


================
Comment at: source/Plugins/Process/Linux/NativeRegisterContextLinux_arm.cpp:397
@@ +396,3 @@
+
+    uint32_t control_value, bp_index;
+
----------------
(nit): Please initialize these variables

================
Comment at: source/Plugins/Process/Linux/NativeRegisterContextLinux_arm.cpp:434
@@ +433,3 @@
+     if (bp_index == LLDB_INVALID_INDEX32)
+        return LLDB_INVALID_INDEX32;
+
----------------
(nit): Indentation

================
Comment at: source/Plugins/Process/Linux/NativeRegisterContextLinux_arm.cpp:444
@@ +443,3 @@
+        // PTRACE call to set corresponding hardware breakpoint register.
+        WriteHardwareDebugRegs(eDREGTypeBREAK, bp_index);
+    }
----------------
Please check for error

================
Comment at: source/Plugins/Process/Linux/NativeRegisterContextLinux_arm.cpp:484
@@ +483,3 @@
+        // PTRACE call to clear corresponding hardware breakpoint register.
+        WriteHardwareDebugRegs(eDREGTypeBREAK, hw_idx);
+    }
----------------
I think a return true is missing here

================
Comment at: source/Plugins/Process/Linux/NativeRegisterContextLinux_arm.cpp:484
@@ +483,3 @@
+        // PTRACE call to clear corresponding hardware breakpoint register.
+        WriteHardwareDebugRegs(eDREGTypeBREAK, hw_idx);
+    }
----------------
tberghammer wrote:
> I think a return true is missing here
Please check for error

================
Comment at: source/Plugins/Process/Linux/NativeRegisterContextLinux_arm.cpp:525
@@ +524,3 @@
+		
+    uint32_t control_value, wp_index, addr_word_offset, byte_mask;
+
----------------
(nit): Please initialize

================
Comment at: source/Plugins/Process/Linux/NativeRegisterContextLinux_arm.cpp:599
@@ +598,3 @@
+        // PTRACE call to set corresponding watchpoint register.
+        WriteHardwareDebugRegs(eDREGTypeWATCH, wp_index);
+    }
----------------
Please check for error

================
Comment at: source/Plugins/Process/Linux/NativeRegisterContextLinux_arm.cpp:640
@@ +639,3 @@
+        // Ptrace call to update hardware debug registers
+        WriteHardwareDebugRegs(eDREGTypeWATCH, wp_index);
+        return true;
----------------
Please check for error

================
Comment at: source/Plugins/Process/Linux/NativeRegisterContextLinux_arm.cpp:673
@@ +672,3 @@
+            // Ptrace call to update hardware debug registers
+            WriteHardwareDebugRegs(eDREGTypeWATCH, i);
+        }
----------------
Please check for error


http://reviews.llvm.org/D9703