Page MenuHomePhabricator

Update the thread list before setting stop reasons with an OS plugin
ClosedPublic

Authored by jingham on Jun 4 2019, 6:50 PM.

Details

Summary

When talking to a gdb-remote server that didn't support the newer thread info packets, we noticed that lldb would hit a breakpoint in target, but then continue on without stopping. This was because the only indication of why you stopped in this scenario was the initial "T05thread:01;" packet. Unfortunately, lldb read the stop reasons from the stop packet and created the new thread & set its stop reason BEFORE calling UpdateThreadListIfNeeded. So when you finally got around to calling UpdateThreadListIfNeeded, we would notice that the ThreadList StopID was out of date, wipe the stop reasons, and then update. Normally some other thread info packet would tell us the stop reason again, and we we'd get back on track. But if they weren't supported, we'd be left thinking that the process had stopped for no reason, and auto-continue it.

The main part of the patch just adjusts the call to UpdateThreadListIfNeeded so it happens before we try to set StopInfo's.

I also added a test that captures this failure.

Diff Detail

Repository
rL LLVM

Event Timeline

jingham created this revision.Jun 4 2019, 6:50 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 4 2019, 6:50 PM
clayborg added inline comments.Jun 7 2019, 2:03 PM
source/Target/Process.cpp
3037–3041 ↗(On Diff #203053)

Should this be in the above if statement?

if (!m_os_up) {
    LoadOperatingSystemPlugin(false);
    // Somebody might have gotten threads before now, but we need to force the
    // update after we've loaded the OperatingSystem plugin or it won't get a
    // chance to process the threads.
    m_thread_list.Clear();
    UpdateThreadListIfNeeded();
}

And if we do this in the if statement, do we need to clear the m_thread_list?

jingham updated this revision to Diff 206980.Jun 27 2019, 6:04 PM

Addresses Greg's question about what happens when we load a new OS plugin. Indeed we should limit the work we do only to the case where we didn't have an OS plugin, then tried to load one and succeeded. Only then do we need to clear and update the thread list.

jingham marked 2 inline comments as done.Jun 27 2019, 6:06 PM
jingham added inline comments.
source/Target/Process.cpp
3037–3041 ↗(On Diff #203053)

You're right. Actually, we should try to get the OS plugin and only if we succeed do we need to do any work. I changed the code to do that.

clayborg accepted this revision.Jun 27 2019, 9:32 PM
This revision is now accepted and ready to land.Jun 27 2019, 9:32 PM
This revision was automatically updated to reflect the committed changes.
jingham marked an inline comment as done.
Herald added a project: Restricted Project. · View Herald TranscriptJun 28 2019, 10:58 AM