This is an archive of the discontinued LLVM Phabricator instance.

[lldb] [llgs] Fix multi-resume bugs with nonstop mode
ClosedPublic

Authored by mgorny on Jun 28 2022, 1:29 AM.

Details

Summary

Improve handling of multiple successive continue packets in non-stop
mode. More specifically:

  1. Explicitly send error response (instead of crashing on assertion) if the user attempts to resume the same process twice. Since we do not support thread-level non-stop mode, one needs to always stop the process explicitly before resuming another thread set.
  1. Actually stop the process if "vCont;t" is delivered to a running process. Similarly, we only support stopping all the running threads simultaneously and return an error if the action would result in a subset of threads still running.

With this patch, running multiple processes simultaneously is still
unsupported. The patch also employs a hack to avoid enabling stdio
forwarding on "vCont;t" packet. Both of these issues will be addressed
by followup patches.

Sponsored by: The FreeBSD Foundation

Diff Detail

Event Timeline

mgorny created this revision.Jun 28 2022, 1:29 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 28 2022, 1:29 AM
Herald added a subscriber: arichardson. · View Herald Transcript
mgorny updated this revision to Diff 444294.Jul 13 2022, 9:05 AM

Rebase on top of m_debugged_processes changes.

labath added inline comments.Jul 14 2022, 6:03 AM
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
1827–1830

I think this is going to be racy if new threads appear before we are able to stop everything. I mean that in the sense that if the client has listed all (known) threads of the process explicitly, and a new thread appears before we're able to act on it, then (I guess) the expected behavior would be to keep that thread running (but we won't do that).

Do we need to support this kind of stopping right now? Maybe we could only support the pid.-1 syntax for stopping all threads within a process? (which should also make the ResumeActionListStopsAllThreads logic simpler)

3002–3009

Could we factor even more of these functions into a common place? If we e.g. moved this block right before the m_continue_process->Resume, then we could put it, and into a helper function.

mgorny added inline comments.Jul 14 2022, 6:38 AM
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
1827–1830

No, I don't think we need to support it. I'll remove it then and simplify the logic.

3002–3009

Makes sense.

mgorny updated this revision to Diff 444696.Jul 14 2022, 9:48 AM
mgorny marked 2 inline comments as done.

Remove support for t with individual thread numbers and simplify code. Move resuming the process (and checking whether it can be resumed) into a common ResumeProcess() method.

labath accepted this revision.Jul 15 2022, 1:38 AM
This revision is now accepted and ready to land.Jul 15 2022, 1:38 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJul 15 2022, 4:02 AM