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