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
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -14,17 +14,25 @@
//===----------------------------------------------------------------------===//

#include "sanitizer_platform.h"
#include "sanitizer_platform_limits_posix.h"
#include "sanitizer_common.h"
#include "sanitizer_stoptheworld.h"

#include <fcntl.h>
#include <signal.h>
#include <time.h>
#include <unistd.h>
#include <math.h>

#if SANITIZER_EMSCRIPTEN

#include <sys/stat.h>
#include <sys/types.h>

#include <emscripten.h>
#include <emscripten/stack.h>
#include <sys/types.h>
#include <wasi/api.h>
#include <wasi/wasi-helpers.h>

#include "emscripten_internal.h"

Expand Down Expand Up @@ -128,6 +136,92 @@ u64 MonotonicNanoTime() {

void GetMemoryProfile(fill_profile_f cb, uptr *stats) {}

int internal_madvise(uptr addr, uptr length, int advice) {
return 0; // madvise is currently ignored
}

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.

}

uptr internal_open(const char *filename, int flags) {
return open(filename, flags);
}

uptr internal_open(const char *filename, int flags, u32 mode) {
return open(filename, flags, mode);
}

uptr internal_read(fd_t fd, void *buf, uptr count) {
__wasi_iovec_t iov = { (uint8_t*)buf, count };
size_t num;
if (__wasi_syscall_ret(__wasi_fd_read(fd, &iov, 1, &num))) {
return -1;
}
return num;
}

uptr internal_write(fd_t fd, const void *buf, uptr count) {
__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;
}
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.

return num;
}

uptr internal_stat(const char *path, void *buf) {
return stat(path, (struct stat *)buf);
}

uptr internal_fstat(fd_t fd, void *buf) {
return fstat(fd, (struct stat *)buf);
}

uptr internal_filesize(fd_t fd) {
struct stat st;
if (internal_fstat(fd, &st))
return -1;
return (uptr)st.st_size;
}

uptr internal_dup(int oldfd) {
return dup(oldfd);
}

uptr internal_getpid() {
return 42;
}

uptr internal_sched_yield() {
return sched_yield();
}

void internal_sigfillset(__sanitizer_sigset_t *set) {
sigfillset(set);
}

uptr internal_sigprocmask(int how, __sanitizer_sigset_t *set,
__sanitizer_sigset_t *oldset) {
return sigprocmask(how, set, oldset);
}

void internal_usleep(u64 useconds) {
usleep(useconds);
}

void internal__exit(int exitcode) {
__wasi_proc_exit(exitcode);
}

tid_t GetTid() {
return gettid();
}

uptr internal_clock_gettime(__sanitizer_clockid_t clk_id, void *tp) {
return clock_gettime(clk_id, (struct timespec *)tp);
}

} // namespace __sanitizer

#endif
Loading
Loading