mgorny marked 10 inline comments as done. mgorny added a comment. Ok, I think I've implemented all the requested changes. I'm going to test it on all three platforms now and attach the patch if it miraculously still works ;-).
================ 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: > > 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. Hmm, though I guess it doesn't matter for FreeBSD right now and since this is entirely platform-specific code, I'll simplify it for now. 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