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 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits