Re: [musl] [PATCH 07/14] Emulate wait4 using waitid

["Stefan O'Rear" <sorear@fastmail.com>]

On Thu, Sep 3, 2020, at 11:49 AM, Rich Felker wrote:
> On Thu, Sep 03, 2020 at 07:23:02AM -0400, Stefan O'Rear wrote:
> > riscv32 and future architectures lack wait4.
> > ---
> >  src/internal/wait4_waitid.h |  1 +
> >  src/linux/__wait4_waitid.c  | 52 +++++++++++++++++++++++++++++++++++++
> >  src/linux/wait4.c           |  5 ++++
> >  src/process/waitpid.c       |  6 +++++
> >  src/stdio/pclose.c          |  6 +++++
> >  src/unistd/faccessat.c      |  6 ++++-
> >  6 files changed, 75 insertions(+), 1 deletion(-)
> >  create mode 100644 src/internal/wait4_waitid.h
> >  create mode 100644 src/linux/__wait4_waitid.c
> 
> I really don't like introducing a new src/internal file for this. If

I agree.

> the code needs to be shared it should probably just be in wait4.c,
> renamed to __wait4, declared in src/include/sys/wait.h according to
> the usual pattern for exposing namespace-safe versions of
> namespace-violating functions.

Currently waitpid and waitid are cancellation points (required by POSIX)
but wait4 is not, which is why I did not have anything else call a function
with exactly wait4 semantics.

> Ideally I'd like to just not need it, but I'm not sure that's
> possible:
> 
> - faccessat throws away the status and doesn't have any need for the
>   status or idtype emulation code. It could easily be just different
>   syscalls in #ifdef/#else paths.

I think that is exactly what my patch does?

> - pclose does need status but not the idtype part. but it's only
>   listening for process termination not all the other weird stuff. I'm
>   not sure if it would make sense to construct status inline here. It
>   might if we had a macro like the glibc "inverse-W" macro (I forget
>   its name) to construct a wait status from parts; if so this could
>   later be considered for a public API (previously requested).

Earlier I had the wait status conversion code broken out separately
but it seemed not worth it for just pclose.

> - waitpid and wait4 at least need the idtype and status construction.
> 
> Unfortunately, wait4 also needs the rusage conversion, which waitpid
> doesn't want. This kinda defeats my idea of just exposing __wait4 from
> wait4.c.
> 
> A side observation to keep in mind is that passing the argument cp
> doesn't really help optimize anything; it only worked well in
> c2feda4e2e because the function there is inline. Of the 4 callers you
> have, faccessat and pclose have hard requirements not to be a
> cancellation point and waitpid has a hard requirement to be one. But

pclose has a hard requirement to not be a cancellation point but it also
has "If a thread is canceled during execution of pclose(), the behavior
is undefined."  The implications of the combination are confusing.

> if the function weren't used for the former, it could just always be
> cancellable -- wait4 probably should have been cancellable all along,
> and almost surely is on other implementations, so to use it safely
> you'd have to not use, block, or handle cancellation.

I have no immediate objection to making wait4 a cancellation point but
I would prefer to make that change consistently on all architectures.
I don't have a good sense of who uses cancellation and wait4 and what
compatibility constraints are imposed by code (as opposed to standards).

> So I'm leaning towards sticking with something like what you've done,
> but with declaration moved to src/include/sys/wait.h, or possibly
> src/internal/syscall.h (since it's essentially emulation of a

I'd be very happy to not have a one-line header file.  I looked at syscall.h
earlier but wasn't sure if it fit; I did not consider reserved namespace in
a public header (would this preclude use of hidden?).

Is src/linux/__exact_name_of_function.c the appropriate name for the file?

> syscall). Inline in src/internal/syscall.h might also be an option;
> one appealing aspect of that is that it would get rid of the #ifdefs
> in source files and allow calling __sys_wait4() unconditionally with
> no codegen regression on existing archs. This is analogous to
> __sys_open[23].
> 
> Rich

-s