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

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.


================
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;
----------------
mgorny wrote:
> labath wrote:
> > Is the loop still necessary?
> I am not sure but I think there's no harm in keeping it, and it might save us from some hard-to-debug corner conditions in the future.
I don't  mind that much, though I would be surprised if this can help with anything. IIUC, the BSD kernel always sends one notification when the process stops, and I believe this code is written in a way which expects that (only linux has special code to delay some actions until every thread registers as stopped). If, for whatever reason, we end up getting two notifications here, I expect things would blow up spectacularly...


================
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");
+  }
----------------
mgorny wrote:
> labath wrote:
> > `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..
> The distinction is necessary to know how to deal with software breakpoints, and it will be used in the followup patch (D99864, when I extend it to non-Linux targets). I think removing `event` at this point to reintroduce it would be a wasted effort.
> 
> That said, I guess this is yet another case for `llvm_unreachable()`. Also, I think it's worth keeping `assert()`s like this as they make sure we don't break stuff accidentally when changing code on the caller's end.
True, but it's even better when the code is written such that the asserts/unreachables are unnecessary. One way to do that might be to do the decoding in the caller:
```
  if (info.pl_flags & PL_FLAG_FORKED) {
    MonitorClone(info.pl_child_pid, /*bool is_vfork*/ fork_flags & PL_FLAG_VFORKED);
    return;
  }
```
It's hard to say whether that would work without seeing the real implementation.


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

https://reviews.llvm.org/D98822