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

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D98822#2666257 <https://reviews.llvm.org/D98822#2666257>, @mgorny wrote:

> @labath, I think I've made all the requested changes.

This looks pretty good now. Just some small nits inline...



================
Comment at: lldb/include/lldb/Host/linux/Host.h:13
+#include "llvm/ADT/Optional.h"
+
+#include "lldb/lldb-types.h"
----------------
mgorny wrote:
> 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.
And it should do that -- that's part of the point :)


================
Comment at: lldb/source/Plugins/Process/FreeBSD/NativeProcessFreeBSD.cpp:723
   // Process all pending waitpid notifications.
-  int status;
-  ::pid_t wait_pid =
-      llvm::sys::RetryAfterSignal(-1, waitpid, GetID(), &status, WNOHANG);
+  while (true) {
+    int status;
----------------
Is the loop still necessary?


================
Comment at: lldb/source/Plugins/Process/FreeBSD/NativeProcessFreeBSD.cpp:981-987
+  switch (event) {
+  case PL_FLAG_FORKED:
+  case PL_FLAG_FORKED | PL_FLAG_VFORKED:
+    break;
+  default:
+    assert(false && "unknown clone_info.event");
+  }
----------------
`assert(event&PL_FLAG_FORKED)` would be shorter, but I am not sure if we need even that, as this is already checked in the caller. You could just drop the entire `event` argument, if it is unused..


================
Comment at: lldb/source/Plugins/Process/FreeBSD/NativeProcessFreeBSD.h:124
+
+  void MonitorClone(::pid_t child_pid, int event);
 };
----------------
move this next to the other `Monitor` functions ?


================
Comment at: lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp:670
+      data = 0;
+
+    MonitorFork(data, true, PTRACE_EVENT_FORK);
----------------
mgorny wrote:
> labath wrote:
> > I guess we should also do something like the `WaitForNewThread` call in the clone case.
> Yeah, I was wondering about it. On the other hand, won't the main loop handle it anyway?
> 
> Does doing it explicitly have any real gain? One thing I was worried about is that the case of parent signal first is much more common than child signal first, so having a different logic on both branches would make it more likely for bugs in the child-first branch not to be noticed.
Maybe obsolete by now, but I just realized what you actually meant here.

There will always be some divergence between the two sequence of events. Letting this be handled from the main loop may make it smaller though. I don't really mind doing it that way, as long as the (v)fork and clone cases do it the same way.


================
Comment at: lldb/source/Plugins/Process/NetBSD/NativeProcessNetBSD.cpp:760
   // Process all pending waitpid notifications.
-  int status;
-  ::pid_t wait_pid = llvm::sys::RetryAfterSignal(-1, waitpid, GetID(), &status,
-                                                 WALLSIG | WNOHANG);
-
-  if (wait_pid == 0)
-    return; // We are done.
+  while (true) {
+    int status;
----------------
same here


================
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);
----------------
mgorny wrote:
> 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...)`.
I think that the looping would be the cleanest solution for the BSDs. For linux, we will need to call waitpid centrally, and then somehow choose which instance to notify.


================
Comment at: lldb/source/Plugins/Process/NetBSD/NativeProcessNetBSD.h:117
+
+  void MonitorClone(::pid_t child_pid, int event);
 };
----------------
same here


================
Comment at: lldb/test/API/functionalities/gdb_remote_client/TestMultiprocess.py:3
 import lldb
+<<<<<<< HEAD
 import unittest
----------------
bad merge


================
Comment at: lldb/test/Shell/Subprocess/Inputs/fork.cpp:30-42
+#if defined(TEST_CLONE)
+  pid_t pid = clone(child_func, &stack[sizeof(stack)], 0, NULL);
+#elif defined(TEST_FORK)
+  pid_t pid = fork();
+#elif defined(TEST_VFORK)
+  pid_t pid = vfork();
+#endif
----------------
How about:
```
#if def TEST_CLONE
  pid_t pid = clone(child_func, &stack[sizeof(stack)], 0, NULL);
#elif defined TEST_FORK
  pid_t pid = TEST_FORK(); // defined to fork of vfork
  if (pid == 0)
    _exit(child_func(NULL));
#endif
```
?


================
Comment at: lldb/test/Shell/Subprocess/clone-follow-parent.test:2
+# REQUIRES: native && (system-linux || system-netbsd)
+# UNSUPPORTED: system-windows
+# RUN: %clangxx_host %p/Inputs/fork.cpp -DTEST_CLONE -o %t
----------------
The UNSUPPORTED:windows is _not_ required if you explicitly list supported operating systems. :)


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

https://reviews.llvm.org/D98822