[Lldb-commits] [PATCH] D80112: Check if thread was suspended during previous stop added.

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

Thanks, this is looking good.  I have a bunch of nits, but nothing substantial.



================
Comment at: lldb/source/Target/Process.cpp:3944
+bool Process::ProcessEventData::ShouldStop(Event *event_ptr,
+                                           bool *have_valid_stopinfo_ptr) {
+  ProcessSP process_sp(m_process_wp.lock());
----------------
I would use a reference for `have_valid_stopinfo_ptr` here.  Passing this in isn't optional (and you don't check if the pointer is null...) which is better expressed by taking a reference.  If you do have a reason why you want this to be a pointer, then you need to check if its non-null before setting it.

Also, I think "found_valid_stopinfo" is a better name.  "have" makes it sound like you are asking whether the ProcessEventData has a valid stopinfo pointer, which doesn't really make sense since Events don't have stop info.

What you are saying is the should stop computation found one...


================
Comment at: lldb/source/Target/Process.cpp:3977
+    } else {
+      /*
+       For the sake of logical consistency. For example we have only
----------------
I'm not entirely sure about this part.  Setting the "have_valid_stopinfo_ptr would only matter if we stopped and no non-suspended thread had a valid stop reason.  That's really only going to happen because there was a bug in the stub, but when this happens we really can't figure out what to do.  The suspended thread's StopInfo isn't going to help us because it is stale by now.

I think the right thing to do in this case is say nobody had an opinion, and let the upper layers deal with whether they want to ignore a seemingly spurious stop, or stop and let the user decide what to do.


================
Comment at: lldb/source/Target/Process.cpp:4094
   // If we're stopped and haven't restarted, then do the StopInfo actions here:
   if (m_state == eStateStopped && !m_restarted) {
     bool does_anybody_have_an_opinion = false;
----------------
You could convert this to an early return if you feel like it.  The llvm style purists prefer that.


================
Comment at: lldb/test/API/functionalities/thread/ignore_suspended/TestIgnoreSuspendedThread.py:2
+"""
+Test suspeneded threads.
+"""
----------------
Spelling.  Also, say what you are testing about suspended threads, like "test that a suspended thread doesn't affect should-stop decisions."


================
Comment at: lldb/test/API/functionalities/thread/ignore_suspended/TestIgnoreSuspendedThread.py:14
+    mydir = TestBase.compute_mydir(__file__)
+
+    def setUp(self):
----------------
This test doesn't depend on the details of debug info generation, and doesn't need to be run once for each format.  If you put:

    NO_DEBUG_INFO_TESTCASE = True

it will only get run once.


================
Comment at: lldb/test/API/functionalities/thread/ignore_suspended/TestIgnoreSuspendedThread.py:46
+        #The breakpoint list should show 1 locations.
+        self.expect(
+            "breakpoint list -f",
----------------
What you are testing in this `self.expect` is already all tested by run_break_set_by_file_and_line.  I don't think you need to repeat it.  If you want to assert that the breakpoint was set exactly on the line number requested, just pass `loc_exact = True` as well as num_expected_locations.


================
Comment at: lldb/test/API/functionalities/thread/ignore_suspended/TestIgnoreSuspendedThread.py:62
+        
+        print('First stop:')
+        self.printThreadsStoppedByBreakpoint(process)
----------------
We've been trying to enforce the discipline that tests only emit stdout if tracing is on.  So this print and the subsequent printThreadsStoppedByBreakpoint should be guarded by:


```
if self.TraceOn():
```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80112/new/

https://reviews.llvm.org/D80112