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

[Michał Górny via Phabricator via lldb-commits]

mgorny 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.
----------------
labath wrote:
> 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)
No, I don't think we need to support it. I'll remove it then and simplify the logic.


================
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);
+  }
----------------
labath wrote:
> 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.
Makes sense.


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

https://reviews.llvm.org/D128710