[Lldb-commits] [PATCH] D146977: [lldb-server/linux] Use waitpid(-1) to collect inferior events

[Pavel Labath via Phabricator via lldb-commits]

labath marked 2 inline comments as done.
labath added inline comments.


================
Comment at: lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp:376-386
+    bool handled = llvm::any_of(m_processes, [&](NativeProcessLinux *process) {
+      return process->TryHandleWaitStatus(pid, status);
+    });
+    if (!handled) {
+      if (status.type == WaitStatus::Stop && status.status == SIGSTOP) {
+        // Store the thread for later collection.
+        m_unowned_threads.insert(pid);
----------------
yinghuitan wrote:
> Maybe I misunderstood the purpose -- the code stores `pid` into `m_unowned_threads` for later collection if **none** of the process handle it. Instead, I thought we want to store into `m_unowned_threads` if **at least one process** did not handle it? 
> 
> Can you add some comment to clarify the intention? Thanks.
The code is correct. Each event should be handled by exactly one process, but it may not happen immediately (if the process doesn't know about the thread). I've added some comments to clarify.


================
Comment at: lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp:538
+      (status.type == WaitStatus::Exit || status.type == WaitStatus::Signal)) {
+    SetExitStatus(status, true);
+    return true;
----------------
yinghuitan wrote:
> Can you add back the original comment which I find useful? Thanks
> ```
> // The process exited.  We're done monitoring.  Report to delegate.
> ```
Yeah sure. It got lost because I changed the code a couple of times while working on it.


================
Comment at: lldb/source/Plugins/Process/Linux/NativeProcessLinux.h:70-72
+    llvm::SmallPtrSet<NativeProcessLinux *, 2> m_processes;
+
+    llvm::DenseSet<::pid_t> m_unowned_threads;
----------------
yinghuitan wrote:
> I assume no lock is needed? 
Yes. This is all single-threaded.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146977