-
Notifications
You must be signed in to change notification settings - Fork 3.4k
[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
base: main
Are you sure you want to change the base?
[compiler-rt] Avoid using musl's syscall layer. NFC #24655
Conversation
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; | ||
} |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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:
emscripten/system/lib/compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp
Lines 344 to 351 in b0c461c
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:
emscripten/system/lib/libc/musl/src/unistd/write.c
Lines 6 to 16 in b0c461c
#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.
There was a problem hiding this comment.
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:
emscripten/system/lib/libc/musl/src/unistd/read.c
Lines 6 to 16 in b0c461c
#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()
:
emscripten/system/lib/libc/musl/src/exit/_Exit.c
Lines 6 to 8 in b0c461c
#ifdef __EMSCRIPTEN__ | |
__wasi_proc_exit(ec); | |
#else |
Commit ba629f8 should fix this.
} | ||
|
||
uptr internal_close(fd_t fd) { | ||
return __wasi_fd_close(fd); |
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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:
emscripten/system/lib/compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp
Lines 303 to 305 in b0c461c
uptr internal_close(fd_t fd) { | |
# if SANITIZER_EMSCRIPTEN | |
return __wasi_fd_close(fd); |
Looking at:
emscripten/system/lib/libc/musl/src/unistd/close.c
Lines 16 to 20 in b0c461c
#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()
).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
system/lib/compiler-rt/lib/sanitizer_common/sanitizer_platform_interceptors.h
Show resolved
Hide resolved
Looking at the test failures, I think we need to land PR #24652 first. |
Use libc calls instead, similar to the NetBSD and macOS implementations.