[Lldb-commits] [PATCH] D128710: [lldb] [llgs] Fix multi-resume bugs with nonstop mode

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.


================
Comment at: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:1818-1821
+    // 4. vCont on a running process that requests suspending a subset
+    //    of running threads or resuming a subset of suspended threads.
+    //    Since we do not support full nonstop mode, this is unsupported
+    //    and we return an error.
----------------
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)


================
Comment at: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:3003-3010
+  // In non-stop protocol mode, the process could be running already.
+  // We do not support resuming threads independently, so just error out.
+  if (!m_continue_process->CanResume()) {
+    LLDB_LOG(log, "process cannot be resumed (state={0})",
+             m_continue_process->GetState());
+    return SendErrorResponse(0x37);
+  }
----------------
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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D128710/new/

https://reviews.llvm.org/D128710