mgorny marked 3 inline comments as done. mgorny added a comment. Answered where answer was due, will update the rest once I finish retesting the multiprocess patch.
================ 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; ---------------- 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. ================ 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: > `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. 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