[Lldb-commits] [PATCH] D29669: Hardware breakpoints implementation for Arm/AArch64 targets

[Pavel Labath via Phabricator via lldb-commits]

labath requested changes to this revision.
labath added a comment.
This revision now requires changes to proceed.

I am sorry about the delay - I was busy last week and then this kinda fell off my radar.

The change looks good, but I want to make the test more stable - we are running these in CI and I've already seen some very unlikely things happen, so I don't want this to be flaky.

Also, it looks like you didn't catch all instances of using printf-style logging.



================
Comment at: packages/Python/lldbsuite/test/functionalities/breakpoint/hardware_breakpoints/hardware_breakpoint_on_multiple_threads/TestHWBreakMultiThread.py:78
+
+        while count < 4 :
+         
----------------
This is quite racy.

There is nothing that guarantees that we will stop on the breakpoint at least four times. In particular, it can happen that all 8 threads hit the breakpoint simultaneously, and then you have only one stop of the whole process. If you want to have more than one stop you need to either:
- make sure that no two threads can hit the breakpoint simultaneously (e.g., add some kind of a mutex in the inferior),
- or count the stops more diligently (after each stop, count the number of threads stopped at the location, as in e.g. TestConcurrentEvents)

However, if the purpose of this test is to make sure the breakpoint applies to newly-created threads, then I think you don't need than many threads, and one thread would suffice. Then the test could be:
- set a breakpoint
- inferior spins up a thread and hits it
- remove/disable the breakpoint
- inferior joins the first thread
- inferior spins up another thread which does *not* hit the breakpoint
This should make debugging any failures easier as the test is more deterministic.


================
Comment at: packages/Python/lldbsuite/test/functionalities/breakpoint/hardware_breakpoints/hardware_breakpoint_on_multiple_threads/TestHWBreakMultiThread.py:108
+        # Continue. Program should exit without stopping anywhere.
+        self.runCmd("process continue")
----------------
It looks like you are missing an assertion to verify that the exit actually happened here.


================
Comment at: source/Plugins/Process/Linux/NativeRegisterContextLinux_arm.cpp:362
+  if (log)
+    log->Printf("NativeRegisterContextLinux_arm::%s()", __FUNCTION__);
+
----------------
LLDB_LOG (or just remove the log statement as it does not convey much information).


================
Comment at: source/Plugins/Process/Linux/NativeRegisterContextLinux_arm.cpp:474
+  if (log)
+    log->Printf("NativeRegisterContextLinux_arm64::%s()", __FUNCTION__);
 
----------------
LLDB_LOG (and in below function as well)


https://reviews.llvm.org/D29669