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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
