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