-
Notifications
You must be signed in to change notification settings - Fork 3.4k
[llvm-libc] Import setjmp from llvm-libc #24765
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
Conversation
@@ -26,7 +26,7 @@ typedef struct __jmp_buf_tag { | |||
|| defined(_BSD_SOURCE) | |||
typedef jmp_buf sigjmp_buf; | |||
/* XXX EMSCRIPTEN: No signals support, alias sigsetjmp and siglongjmp to their non-signals counterparts. */ | |||
#if __EMSCRIPTEN__ | |||
#if __EMSCRIPTEN__ && !defined(LLVM_LIBC) |
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.
I wonder if we could remove this musl patch and instead do this redirection in source files, then our musl patch would be smaller and we could avoid the need for this new LLVM_LIBC macro.
I'm fine landing this patch either way, we can always clean this up as a followup.
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.
I am fine either way as well.
On the hand, this should no longer be of any issue once we switch over to use llvm-libc public headers over the musl ones.
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.
True, but I honestly think that process will take O(years). We can't delete musl until llvm-libc is has complete parity, or at least close enough that we accept some minor regressions.
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.
Certainly. As I expect to see more of these as we import more llvm-libc, I feel like we should probably try to address this properly.
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.
PTAL. The forwarding is now done in source files.
1ed883d
to
c58148b
Compare
tools/system_libs.py
Outdated
@@ -1363,6 +1366,7 @@ def get_files(self): | |||
filenames=['thread_profiler.c']) | |||
|
|||
libc_files += glob_in_path('system/lib/libc/compat', '*.c') | |||
libc_files += glob_in_path('system/lib/libc/musl/setjmp', '*.c') |
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.
Can you put these two wrappers into the existing emscripten_libc_stubs.c
instead?
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.
Actually it looks like system/lib/libc/musl/src/signal/siglongjmp.c
already exists and does the right thing.
Can you update the existing system/lib/libc/musl/src/signal/sigsetjmp.c
to include this stub and then add those two files to list on line 1330 in system_libs.py
.
ce52e32
to
c1c862b
Compare
Defined a wasm-specific wasm/sigsetjmp.cpp that just fowards the call to setjmp.
Defines in the musl header don't quite work with llvm-libc's setjmp.