Skip to content

[compiler-rt] Avoid using musl's syscall layer. NFC #24655

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

kleisauke
Copy link
Collaborator

Use libc calls instead, similar to the NetBSD and macOS implementations.

kleisauke added 2 commits July 7, 2025 12:51
Use libc calls instead, similar to the NetBSD and macOS implementations.
size_t num;
if (__wasi_syscall_ret(__wasi_fd_write(fd, &iov, 1, &num))) {
return -1;
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If calling libc functions is OK in these internal_xx function why not just call libc write here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question. I left it this way to ensure this change is still NFC, just like the previous implementation:

uptr internal_write(fd_t fd, const void *buf, uptr count) {
# if SANITIZER_EMSCRIPTEN
__wasi_ciovec_t iov = {(const uint8_t *)buf, count};
size_t num;
if (__wasi_syscall_ret(__wasi_fd_write(fd, &iov, 1, &num))) {
return -1;
}
return num;

Looking at:

#if __EMSCRIPTEN__
__wasi_ciovec_t iov = {
.buf = buf,
.buf_len = count
};
size_t num;
if (__wasi_syscall_ret(__wasi_fd_write(fd, &iov, 1, &num))) {
return -1;
}
return num;
#else

It looks like doing write() here has the same effect.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The above comment also applies to internal_read(), see:

#if __EMSCRIPTEN__
__wasi_iovec_t iov = {
.buf = buf,
.buf_len = count
};
size_t num;
if (__wasi_syscall_ret(__wasi_fd_read(fd, &iov, 1, &num))) {
return -1;
}
return num;
#else

And internal__exit():

#ifdef __EMSCRIPTEN__
__wasi_proc_exit(ec);
#else

Commit ba629f8 should fix this.

}

uptr internal_close(fd_t fd) {
return __wasi_fd_close(fd);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems odd to use the lower lever __wasi_fd_close but the higher level open for internal_open.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed. I left it this way to ensure this change is still NFC, just like the previous implementation:

uptr internal_close(fd_t fd) {
# if SANITIZER_EMSCRIPTEN
return __wasi_fd_close(fd);

Looking at:

#ifdef __EMSCRIPTEN__
int r = __wasi_fd_close(fd);
if (r == __WASI_ERRNO_INTR) r = __WASI_ERRNO_SUCCESS;
return __wasi_syscall_ret(r);
#else

It sounds like doing close() here is more appropriate (as it wraps the errno with __wasi_syscall_ret()).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keeping the pre-existing behaviour (for now) seems reasonable. We can do cleanups / improvements separately and a followups.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like the callers of internal_close() aren't checking its return code anyway, so using close() here should be safe. Commit ba629f8 addresses this.

@kleisauke
Copy link
Collaborator Author

Looking at the test failures, I think we need to land PR #24652 first.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants