Skip to content

Commit

Permalink
Avoid calling poll_oneoff with zero subscriptions. (#162)
Browse files Browse the repository at this point in the history
* Avoid calling `poll_oneoff` with zero subscriptions.

With WebAssembly/WASI#193 merged, WASI is moving
to make `poll_oneoff` with no arguments an error. Even though that's in
ephemeral and not yet in a snapshot, we can start to anticipate it in
libc:
 - Remove the `pause` function, since WASI has no signals and thus no
   way to ever wake it up short of having the host terminate it.
 - Make `poll` and `pselect` return `ENOTSUP` in the case of having no
   events to wait for.

* Remove `pause` from the defined-symbols.txt list.

* Fix __wasilibc_unmodified_upstream markers.

* Check for zero subscriptions, rather than zero events.

Make `poll` and `pselect` return `ENOTSUP` when asked to poll on zero
subscriptions, rather than when the systerm returns zero events.

While here, drop the `__wasilibc_unmodified_upstream` markers, which
were already pretty noisy here, and would be significantly worse with
this change.

* Add comments about the subtle relationship between nfds and nsubscriptions.

* Rewrite the comment.

* Fix code quotes.
  • Loading branch information
sunfishcode committed Jun 2, 2020
1 parent 753cc43 commit 5a7ba74
Show file tree
Hide file tree
Showing 5 changed files with 42 additions and 53 deletions.
1 change: 0 additions & 1 deletion expected/wasm32-wasi/defined-symbols.txt
Original file line number Diff line number Diff line change
Expand Up @@ -805,7 +805,6 @@ optind
optopt
optreset
pathconf
pause
perror
poll
posix_close
Expand Down
39 changes: 20 additions & 19 deletions libc-bottom-half/cloudlibc/src/libc/poll/poll.c
Original file line number Diff line number Diff line change
Expand Up @@ -11,14 +11,14 @@ int poll(struct pollfd *fds, size_t nfds, int timeout) {
// Construct events for poll().
size_t maxevents = 2 * nfds + 1;
__wasi_subscription_t subscriptions[maxevents];
size_t nevents = 0;
size_t nsubscriptions = 0;
for (size_t i = 0; i < nfds; ++i) {
struct pollfd *pollfd = &fds[i];
if (pollfd->fd < 0)
continue;
bool created_events = false;
if ((pollfd->events & POLLRDNORM) != 0) {
__wasi_subscription_t *subscription = &subscriptions[nevents++];
__wasi_subscription_t *subscription = &subscriptions[nsubscriptions++];
*subscription = (__wasi_subscription_t){
.userdata = (uintptr_t)pollfd,
.u.tag = __WASI_EVENTTYPE_FD_READ,
Expand All @@ -27,7 +27,7 @@ int poll(struct pollfd *fds, size_t nfds, int timeout) {
created_events = true;
}
if ((pollfd->events & POLLWRNORM) != 0) {
__wasi_subscription_t *subscription = &subscriptions[nevents++];
__wasi_subscription_t *subscription = &subscriptions[nsubscriptions++];
*subscription = (__wasi_subscription_t){
.userdata = (uintptr_t)pollfd,
.u.tag = __WASI_EVENTTYPE_FD_WRITE,
Expand All @@ -47,7 +47,7 @@ int poll(struct pollfd *fds, size_t nfds, int timeout) {

// Create extra event for the timeout.
if (timeout >= 0) {
__wasi_subscription_t *subscription = &subscriptions[nevents++];
__wasi_subscription_t *subscription = &subscriptions[nsubscriptions++];
*subscription = (__wasi_subscription_t){
.u.tag = __WASI_EVENTTYPE_CLOCK,
.u.u.clock.id = __WASI_CLOCKID_REALTIME,
Expand All @@ -56,15 +56,24 @@ int poll(struct pollfd *fds, size_t nfds, int timeout) {
}

// Execute poll().
__wasi_event_t events[nevents];
size_t nevents;
__wasi_event_t events[nsubscriptions];
__wasi_errno_t error =
#ifdef __wasilibc_unmodified_upstream
__wasi_poll(subscriptions, events, nevents, &nevents);
#else
__wasi_poll_oneoff(subscriptions, events, nevents, &nevents);
#endif
__wasi_poll_oneoff(subscriptions, events, nsubscriptions, &nevents);
if (error != 0) {
errno = error;
// WASI's poll requires at least one subscription, or else it returns
// `EINVAL`. Since a `poll` with nothing to wait for is valid in POSIX,
// return `ENOTSUP` to indicate that we don't support that case.
//
// Wasm has no signal handling, so if none of the user-provided `pollfd`
// elements, nor the timeout, led us to producing even one subscription
// to wait for, there would be no way for the poll to wake up. WASI
// returns `EINVAL` in this case, but for users of `poll`, `ENOTSUP` is
// more likely to be understood.
if (nsubscriptions == 0)
errno = ENOTSUP;
else
errno = error;
return -1;
}

Expand All @@ -80,18 +89,10 @@ int poll(struct pollfd *fds, size_t nfds, int timeout) {
if (event->type == __WASI_EVENTTYPE_FD_READ ||
event->type == __WASI_EVENTTYPE_FD_WRITE) {
struct pollfd *pollfd = (struct pollfd *)(uintptr_t)event->userdata;
#ifdef __wasilibc_unmodified_upstream // generated constant names
if (event->error == __WASI_EBADF) {
#else
if (event->error == __WASI_ERRNO_BADF) {
#endif
// Invalid file descriptor.
pollfd->revents |= POLLNVAL;
#ifdef __wasilibc_unmodified_upstream // generated constant names
} else if (event->error == __WASI_EPIPE) {
#else
} else if (event->error == __WASI_ERRNO_PIPE) {
#endif
// Hangup on write side of pipe.
pollfd->revents |= POLLHUP;
} else if (event->error != 0) {
Expand Down
39 changes: 20 additions & 19 deletions libc-bottom-half/cloudlibc/src/libc/sys/select/pselect.c
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,7 @@

int pselect(int nfds, fd_set *restrict readfds, fd_set *restrict writefds,
fd_set *restrict errorfds, const struct timespec *restrict timeout,
#ifdef __wasilibc_unmodified_upstream
...) {
#else
const sigset_t *sigmask) {
#endif
// Negative file descriptor upperbound.
if (nfds < 0) {
errno = EINVAL;
Expand All @@ -40,13 +36,13 @@ int pselect(int nfds, fd_set *restrict readfds, fd_set *restrict writefds,
// Determine the maximum number of events.
size_t maxevents = readfds->__nfds + writefds->__nfds + 1;
__wasi_subscription_t subscriptions[maxevents];
size_t nevents = 0;
size_t nsubscriptions = 0;

// Convert the readfds set.
for (size_t i = 0; i < readfds->__nfds; ++i) {
int fd = readfds->__fds[i];
if (fd < nfds) {
__wasi_subscription_t *subscription = &subscriptions[nevents++];
__wasi_subscription_t *subscription = &subscriptions[nsubscriptions++];
*subscription = (__wasi_subscription_t){
.userdata = fd,
.u.tag = __WASI_EVENTTYPE_FD_READ,
Expand All @@ -59,7 +55,7 @@ int pselect(int nfds, fd_set *restrict readfds, fd_set *restrict writefds,
for (size_t i = 0; i < writefds->__nfds; ++i) {
int fd = writefds->__fds[i];
if (fd < nfds) {
__wasi_subscription_t *subscription = &subscriptions[nevents++];
__wasi_subscription_t *subscription = &subscriptions[nsubscriptions++];
*subscription = (__wasi_subscription_t){
.userdata = fd,
.u.tag = __WASI_EVENTTYPE_FD_WRITE,
Expand All @@ -70,7 +66,7 @@ int pselect(int nfds, fd_set *restrict readfds, fd_set *restrict writefds,

// Create extra event for the timeout.
if (timeout != NULL) {
__wasi_subscription_t *subscription = &subscriptions[nevents++];
__wasi_subscription_t *subscription = &subscriptions[nsubscriptions++];
*subscription = (__wasi_subscription_t){
.u.tag = __WASI_EVENTTYPE_CLOCK,
.u.u.clock.id = __WASI_CLOCKID_REALTIME,
Expand All @@ -82,15 +78,24 @@ int pselect(int nfds, fd_set *restrict readfds, fd_set *restrict writefds,
}

// Execute poll().
__wasi_event_t events[nevents];
size_t nevents;
__wasi_event_t events[nsubscriptions];
__wasi_errno_t error =
#ifdef __wasilibc_unmodified_upstream
__wasi_poll(subscriptions, events, nevents, &nevents);
#else
__wasi_poll_oneoff(subscriptions, events, nevents, &nevents);
#endif
__wasi_poll_oneoff(subscriptions, events, nsubscriptions, &nevents);
if (error != 0) {
errno = error;
// WASI's poll requires at least one subscription, or else it returns
// `EINVAL`. Since a `pselect` with nothing to wait for is valid in POSIX,
// return `ENOTSUP` to indicate that we don't support that case.
//
// Wasm has no signal handling, so if none of the user-provided `pollfd`
// elements, nor the timeout, led us to producing even one subscription
// to wait for, there would be no way for the poll to wake up. WASI
// returns `EINVAL` in this case, but for users of `poll`, `ENOTSUP` is
// more likely to be understood.
if (nsubscriptions == 0)
errno = ENOTSUP;
else
errno = error;
return -1;
}

Expand All @@ -99,11 +104,7 @@ int pselect(int nfds, fd_set *restrict readfds, fd_set *restrict writefds,
const __wasi_event_t *event = &events[i];
if ((event->type == __WASI_EVENTTYPE_FD_READ ||
event->type == __WASI_EVENTTYPE_FD_WRITE) &&
#ifdef __wasilibc_unmodified_upstream // generated constant names
event->error == __WASI_EBADF) {
#else
event->error == __WASI_ERRNO_BADF) {
#endif
errno = EBADF;
return -1;
}
Expand Down
14 changes: 0 additions & 14 deletions libc-bottom-half/sources/pause.c

This file was deleted.

2 changes: 2 additions & 0 deletions libc-top-half/musl/include/unistd.h
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,9 @@ char *getcwd(char *, size_t);
unsigned alarm(unsigned);
#endif
unsigned sleep(unsigned);
#ifdef __wasilibc_unmodified_upstream /* WASI has no pause */
int pause(void);
#endif

#ifdef __wasilibc_unmodified_upstream /* WASI has no fork/exec */
pid_t fork(void);
Expand Down

0 comments on commit 5a7ba74

Please sign in to comment.