[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/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");
+  }
----------------
labath wrote:
> 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.
In the long run, we would probably also want a separate event for `posix_spawn` on NetBSD. I've figured out that reusing native constants avoids reinventing the wheel unnecessarily.


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

https://reviews.llvm.org/D98822