-
Notifications
You must be signed in to change notification settings - Fork 265
For panic=unwind on Wasm targets, define __cpp_exception tag #1077
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
Closed
+26
−0
Closed
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,23 @@ | ||
| //! Builtins for WebAssembly targets. | ||
|
|
||
| // Define the __cpp_exception tag that LLVM's wasm exception handling requires. | ||
| // This must be provided since LLVM commit | ||
| // aee99e8015daa9f53ab1fd4e5b24cc4c694bdc4a which changed the tag from being | ||
| // weakly defined in each object file to being an external reference that must | ||
| // be linked from somewhere. | ||
| // | ||
| // In LLVM's compiler-rt this is provided by | ||
| // compiler-rt/lib/builtins/wasm/__cpp_exception.S | ||
| // | ||
| // Emscripten provides this tag in its runtime libraries, so we don't need to | ||
| // define it for Emscripten targets. Including this in Emscripten would break | ||
| // the wasm-cpp-exception-tag test. | ||
| #[cfg(not(target_os = "emscripten"))] | ||
| core::arch::global_asm!( | ||
| ".globl __cpp_exception", | ||
| #[cfg(target_pointer_width = "64")] | ||
| ".tagtype __cpp_exception i64", | ||
| #[cfg(target_pointer_width = "32")] | ||
| ".tagtype __cpp_exception i32", | ||
| "__cpp_exception:", | ||
| ); | ||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
For wasi should this be provided by wasi-libc in case C++ code is used?
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 not sure what is happening with wasi cc @alexcrichton . But I'm happy to guard this to also be skipped on wasi if we think that's the right choice. I think the only case where there could be problems is with dynamic linking.
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'd agree with bjorn3 that this shouldn't be defined here, but in general with dynamic linking I don't think this should be defined in compiler-builtins either because that would mean that there'd be an exception-type-per-shared-object which I don't think is what's desired.
Is this just for Emscripten targets? If so, shouldn't Emscripten be the one providing this definition? If this isn't for Emscripten, is it for
wasm32-unknown-unknown? If so where does Rust end up needing__cpp_exceptionfrom?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 is for wasm32-unknown-unknown. I have a cfg on this code that excludes Emscripten because Emscripten is already handling it. Rust uses cpp_exception for panics and catch_unwind. It is emitted by llvm, and by default it will import it from
env.__cpp_exceptionunless you define the symbol explicitly. wasm-bindgen is currently using walrus to transform the generated module and convert the__cpp_exceptionimport into a tag definition:https://github.com/wasm-bindgen/wasm-bindgen/pull/4938/changes#diff-8d702d8da596b2105e4e94a3855489a49792843bd621954a3feacf845b6d42afR375-R389
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.
Specifically,
panicuses thewasm_throwllvm intrinsic andcatch_unwinduses thewasm_catchllvm intrinsic. Both of these intrinsics generate a reference to this symbol:wasm_throw definition
wasm_catch definition
__cpp_exception symbol generated 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.
Makes sense, thanks for the info. In that situation though this definition I think might be best to reside in the
unwindcrate in the standard library? There's no need to have it compiler-builtins specifically and unwinding is the true source of the symbol as well?Additionally I think it would be good to restrict it to
target_os="unknown"to only affectwasm32-unknown-unknown. The responsibility for defining this symbol on other toolchains lies elsewhere (e.g. with Emscripten or WASI targets). External toolchains will have more knowledge of what to do in the face of shared libraries, for example.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.
Yeah, that's fine with me.