Page MenuHomePhabricator

If an interrupt fails, don't try to fetch any more packets from the server
ClosedPublic

Authored by jingham on May 5 2021, 11:49 AM.

Details

Summary

When a gdb-remote plugin process doesn't respond to an interrupt, it returns from
SendContinuePacketAndWaitForResponse with eStateInvalid. The AsyncThread
code responds to this return by setting the state to eStateExited. But it doesn't
immediately exit the ProcessGDBRemote::AsyncThread's packet fetch loop.

This looks like just an oversight in the AsyncThread function. It's important not to
go back to wait for another packet. If it arrives while you are tearing down the
process, the internal-state-thread might try to handle it when the process in not
in a good state.

We weren't going to do anything useful with the extra packet anyway, we've already
declared the process exited. So rather than put more checks into all the shutdown paths
to make sure this extra packet doesn't cause problems, just don't fetch it.

The main part of the patch is setting "done = true" when we get the eStateInvalid.
I also added a check at the beginning of the while(done) loop to prevent another error
from getting us to fetch packets for an exited process.

I also added a test case to ensure that if an Interrupt fails, we call the process
exited. I can't test exactly the error I'm fixing, there's no good way to know
that the stop reply for the failed interrupt wasn't fetched. But at least this
asserts that the overall behavior is correct.

Event Timeline

jingham requested review of this revision.May 5 2021, 11:49 AM
jingham created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptMay 5 2021, 11:49 AM
clayborg accepted this revision.May 5 2021, 2:43 PM
clayborg added inline comments.
lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
3698–3701 ↗(On Diff #343131)

You could put this up into the while condition?

while (!done && process->GetPrivateState() != eStateExited)
This revision is now accepted and ready to land.May 5 2021, 2:43 PM
JDevlieghere accepted this revision.May 6 2021, 10:43 AM
This revision was landed with ongoing or failed builds.May 6 2021, 2:12 PM
This revision was automatically updated to reflect the committed changes.

I moved the eStateExited test as Greg suggested on commit.