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
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
