[Lldb-commits] [PATCH] D21280: Allow installing watchpoints at less than 8-byte alligned addresses for AArch64 targets

[Pavel Labath via lldb-commits]

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

Thanks for adding the test. I have some nits about some details of the test. If you agree with them, you can commit the updated version without additional review.


================
Comment at: packages/Python/lldbsuite/test/functionalities/watchpoint/watchpoint_size/TestWatchpointSizes.py:2
@@ +1,3 @@
+"""
+Test watchpoint size cases (1-byte, 2-byte, 4-byte or 8-byte).
+"""
----------------
Add: "Also make sure that the watchpoints work when the variables are not placed at 8-byte aligned addresses." or something to that effect. Otherwise, someone in the future will wonder why the test is so complicated.

Also, you don't seem to test 8-byte watchpoints (which is fine, but the comment is not correct then).

================
Comment at: packages/Python/lldbsuite/test/functionalities/watchpoint/watchpoint_size/TestWatchpointSizes.py:15
@@ +14,3 @@
+
+    mydir = TestBase.compute_mydir(__file__)
+
----------------
How about adding NO_DEBUG_INFO_TESTCASE = True here? We are not testing debug info, so it should be enough to run the test once.

================
Comment at: packages/Python/lldbsuite/test/functionalities/watchpoint/watchpoint_size/TestWatchpointSizes.py:49
@@ +48,3 @@
+
+    def run_watchpoint_size_test(self, arrayName, rangelimit, wsize):
+        self.build(dictionary=self.d)
----------------
this would a lot more natural if you renamed this into `array_size` or something and used 0-based indexes.

================
Comment at: packages/Python/lldbsuite/test/functionalities/watchpoint/watchpoint_size/TestWatchpointSizes.py:66
@@ +65,3 @@
+
+        for i in range(1,rangelimit):
+            # We should be stopped again due to the breakpoint.
----------------
`range(array_size)`

================
Comment at: packages/Python/lldbsuite/test/functionalities/watchpoint/watchpoint_size/TestWatchpointSizes.py:73
@@ +72,3 @@
+            # Set a read_write type watchpoint arrayName
+            watch_loc=arrayName+"[" + str(i-1) + "]"
+            self.expect("watchpoint set variable -w read_write " + watch_loc,
----------------
`str(i)`

================
Comment at: packages/Python/lldbsuite/test/functionalities/watchpoint/watchpoint_size/TestWatchpointSizes.py:104
@@ +103,3 @@
+            self.expect("watchpoint list -v",
+                substrs = ['hit_count = 2'])
+
----------------
I guess the intention here is to test both read and write watchpoints. If so, could you state that somewhere (in a comment at least, if it's possible to somehow verify the watchpoint type in code, even better). 

================
Comment at: packages/Python/lldbsuite/test/functionalities/watchpoint/watchpoint_size/main.c:46
@@ +45,3 @@
+        pad1++;
+        localWord = wordArray[i]; // Here onwards we should'nt be stopped in loop
+        wordArray[i]++; 
----------------
shouldn't (typo)


http://reviews.llvm.org/D21280