[Lldb-commits] [PATCH] D98822: [lldb] [Process] Watch for fork/vfork notifications

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

mgorny added inline comments.


================
Comment at: lldb/include/lldb/Host/linux/Host.h:13
+#include "llvm/ADT/Optional.h"
+
+#include "lldb/lldb-types.h"
----------------
labath wrote:
> These spaces are pretty common in lldb, but that's not actually consistent with how the rest of llvm formats them...
Will remove. However, note that this causes `clang-format` to reorder the includes.


================
Comment at: lldb/source/Plugins/Process/NetBSD/NativeProcessNetBSD.cpp:784
+    int status;
+    ::pid_t wait_pid = llvm::sys::RetryAfterSignal(-1, waitpid, -1, &status,
+                                                   WALLSIG | WNOHANG);
----------------
labath wrote:
> For NetBSD, it might be cleaner to keep waiting for the parent only, and then do a separate waitpid for the child once you get its process id. That would make the sequence of events (and the relevant code) predictable.
> 
> I think this is how this interface was meant to be used, even on linux, but linux makes it tricky because it's not possible to wait for all threads belonging to a given process in a single call -- one would have to iterate through the threads and wait for each one separately (it might be interesting to try to implement and benchmark that).
I've been thinking about it and I suppose it makes sense. I was considering what would be better to future-proof it for the future full multiprocess support but I suppose we could just loop over all process classes and let every one wait for its own process rather than creating a dedicated dispatcher that calls `waitpid(-1...)`.


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

https://reviews.llvm.org/D98822