Page MenuHomePhabricator

[lldb-server] Reset stop reason of all threads when resuming
ClosedPublic

Authored by jarin on May 3 2020, 1:55 PM.

Details

Summary

This patch makes the stop reason reset logic similar to MacOS' debugserver, where exceptions are reset for all threads when resuming process for stepping or continuing (see MachThreadList::ProcessWillResume and MachThread::ThreadWillResume).

Resetting stop reasons on resume fixes problems where LLDB spuriously reports SIGTRAP signal stop reason for deleted breakpoints (both internal and public) and where LLDB stops on an internal breakpoint while stepping over while a breakpoint is hit in another thread. See PR45642 for details.

Diff Detail

Event Timeline

jarin created this revision.May 3 2020, 1:55 PM
labath added a subscriber: labath.May 4 2020, 3:57 AM

The test setup here seems unnecessarily complex. Wouldn't an inferior like this work better?

void thread1() {
  pseudo_barrier_wait(g_barrier); // See other tests how this works.
  g_foo = 0; // break_here
}
int main() {
  pseudo_barrier_init(g_barrier1, 2);
  std::thread t1(thread1);
  pseudo_barrier_wait(g_barrier);
  for (int i = 0; i<10000; ++i) g_bar = i; // empty loop to have something to step over
  t1.join();
}

That way you always know only one thread will hit a breakpoint, and and you can just pick the "other" thread as the target for stepping.

jarin added a comment.May 4 2020, 4:47 AM

The test setup here seems unnecessarily complex. Wouldn't an inferior like this work better?

void thread1() {
  pseudo_barrier_wait(g_barrier); // See other tests how this works.
  g_foo = 0; // break_here
}
int main() {
  pseudo_barrier_init(g_barrier1, 2);
  std::thread t1(thread1);
  pseudo_barrier_wait(g_barrier);
  for (int i = 0; i<10000; ++i) g_bar = i; // empty loop to have something to step over
  t1.join();
}

That way you always know only one thread will hit a breakpoint, and and you can just pick the "other" thread as the target for stepping.

Yeah, I considered something like that, but then I thought it would be better if the "other" thread only runs code that we completely control. In my patch, the "other" thread is guaranteed to be in thread_func after we hit the breakpoint. In your suggested inferior, it could be still in pseudo_barrier_wait. If you feel stepping in external code is safe, I am happy to rewrite the test to the simpler version.

jarin updated this revision to Diff 261805.May 4 2020, 7:11 AM

Simplify the test based on the suggestion from labath@.

labath accepted this revision.May 5 2020, 5:01 AM

Yeah, I considered something like that, but then I thought it would be better if the "other" thread only runs code that we completely control. In my patch, the "other" thread is guaranteed to be in thread_func after we hit the breakpoint. In your suggested inferior, it could be still in pseudo_barrier_wait. If you feel stepping in external code is safe, I am happy to rewrite the test to the simpler version.

The main reason that "pseudo_barrier_wait" even exists is so that we can drive multiple test threads to very precise points in the code, so I wouldn't really call it "external" code. If we were doing something more complicated to the thread (like playing with the line numbers for instance, then I might get worried, but given that all we need is to do an instruction step, I think this is perfectly safe, and a lot more understandable than the previous version.

lldb/test/API/functionalities/thread/break_step_other/TestThreadBreakStepOther.py
22

You can add NO_DEBUG_INFO_TESTCASE = True here.

lldb/test/API/functionalities/thread/break_step_other/main.cpp
24–26

Just a small tweak to ensure the inferior exits if anything happens to the debugger (e.g., it crashes)

volatile int i = 0;
while (g_foo == 0)
  ++i;
t.join();

(and change g_foo = 0 to g_foo = 1 in thread_func above).

This revision is now accepted and ready to land.May 5 2020, 5:01 AM
jarin updated this revision to Diff 262081.May 5 2020, 5:47 AM
jarin marked an inline comment as done.

Addressed reviewer comments.

jarin updated this revision to Diff 262082.May 5 2020, 5:50 AM

... and remove the extra braces.

jarin updated this revision to Diff 262087.May 5 2020, 5:58 AM
jarin marked 2 inline comments as done.

... now also fixed the 'volatile'. It took only three patches to copy four lines of code by hand. Not bad, huh?

jarin added inline comments.May 5 2020, 6:07 AM
lldb/test/API/functionalities/thread/break_step_other/TestThreadBreakStepOther.py
22

Out of curiosity, what does that do?

labath added inline comments.May 5 2020, 6:32 AM
lldb/test/API/functionalities/thread/break_step_other/TestThreadBreakStepOther.py
22

It prevents the test suite from forking the test for different debug info "formats" ("regular dwarf", split dwarf, dsym, etc.).
That does not seem relevant/useful here as this test is mainly about the stop reason machinery and debug info is only used incidentally.

jarin added a comment.May 6 2020, 11:24 PM

Jim, do you think this is good to go?

jingham accepted this revision.May 7 2020, 10:40 AM

LGTM too!

jarin added a comment.May 19 2020, 3:13 PM

Pavel, could you possibly land this for us?

Sure.

Btw, at this point, I think one of you guys should get commit-after-aproval access to the repository, so you can handle these things yourselves.

This revision was automatically updated to reflect the committed changes.