[libunwind] Exlcude wasm build from using __declspec export#168449
[libunwind] Exlcude wasm build from using __declspec export#168449
Conversation
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
|
@llvm/pr-subscribers-libunwind Author: Yerzhan Zhamashev (yerzham) ChangesMost minimal changes to enable libunwind to build for wasm by excluding Full diff: https://github.com/llvm/llvm-project/pull/168449.diff 2 Files Affected:
diff --git a/libunwind/src/assembly.h b/libunwind/src/assembly.h
index 84c9d526f1d75..9c35b6050a2bf 100644
--- a/libunwind/src/assembly.h
+++ b/libunwind/src/assembly.h
@@ -254,6 +254,8 @@ aliasname: \
#define NO_EXEC_STACK_DIRECTIVE
// clang-format on
+#elif defined(__wasm__)
+
#else
#error Unsupported target
diff --git a/libunwind/src/config.h b/libunwind/src/config.h
index f017403fa2234..73162995f9293 100644
--- a/libunwind/src/config.h
+++ b/libunwind/src/config.h
@@ -75,13 +75,14 @@
#define _LIBUNWIND_EXPORT
#define _LIBUNWIND_HIDDEN
#else
- #if !defined(__ELF__) && !defined(__MACH__) && !defined(_AIX)
- #define _LIBUNWIND_EXPORT __declspec(dllexport)
- #define _LIBUNWIND_HIDDEN
- #else
- #define _LIBUNWIND_EXPORT __attribute__((visibility("default")))
- #define _LIBUNWIND_HIDDEN __attribute__((visibility("hidden")))
- #endif
+#if !defined(__ELF__) && !defined(__MACH__) && !defined(_AIX) && \
+ !defined(__wasm__)
+#define _LIBUNWIND_EXPORT __declspec(dllexport)
+#define _LIBUNWIND_HIDDEN
+#else
+#define _LIBUNWIND_EXPORT __attribute__((visibility("default")))
+#define _LIBUNWIND_HIDDEN __attribute__((visibility("hidden")))
+#endif
#endif
#define STR(a) #a
|
4b07878 to
8baec64
Compare
|
Compared to yamt's gist why do we not have After fixing that I then ran into the following errors when linking Are there further changes needed to fix this? |
|
I needed to clarify wasi-sdk had to use latest version of src/llvm-project, at least I tested since v21.1.5. Otherwise, usage of NO_EXEC_STACK_DIRECTIVE in UnwindRegistersRestore.S should be gated out since #92192 (then some modification at #162581). I do not think NO_EXEC_STACK_DIRECTIVE is ever used for wasm target after that, so it would be an unused declaration. As for the consequent errors, probably still related to outdated llvm source? |
|
Oh right, I should have directly used your branch instead of locally applying the patch -- thanks! I can report that with this patch (and the one for WASI-SDK) I did get working several large (100K+ LoC) programs from SPEC that use C++ exceptions. I did encounter build issues when using LTO+exceptions ( |
|
Ping |
1 similar comment
|
Ping |
aheejin
left a comment
There was a problem hiding this comment.
Sorry for the delay! My mail filter wasn't set up right..
8baec64 to
0f23156
Compare
0f23156 to
856fee9
Compare
This commit adds initial configuration support for building wasi-sdk with support for C++ exceptions in WebAssembly. A small test is included which is exercised in CI at this time, but otherwise this does not update CI to actually ship sdk builds with C++ exceptions enabled. Instead the intention here is to make it easier to test builds with support for C++ exceptions and centralize planning/development around this. The goal here is to get things compiling to the point that applications can be compiled. I haven't thoroughly tested C++ exceptions and as evident in this PR it still requires changes in LLVM. Some small logic is added here to apply a `*.patch` file to LLVM to avoid needing a new submodule fork or waiting for upstream changes. The intention is that this `*.patch` is short-lived once the changes are officially merged into LLVM itself. The `*.patch` included here contains llvm/llvm-project#168449 as well as another minor edit I found was necessary to get things compiling locally. Given the discussion on that PR it looks like once LLVM is updated with that merged the extra part of the `*.patch` won't be necessary. This PR notably does not enable shared libraries when exceptions are enabled. I don't know enough about how things are supposed to work to be able to fully diagnose the compilation errors I'm seeing if shared libraries are enabled. This is something I'd hope would be fixed before actually shipping exceptions support. This PR then additionally folds in [this gist][gist] for various bits of build logic to this repository itself. Finally, this PR includes some documentation about the current status of exceptions and links to various tracking issues too. [gist]: https://gist.github.com/yerzham/302efcec6a2e82c1e8de4aed576ea29d
> **Note**: this PR is me weaving together the work of many others. For example @yerzham's and @cpetig's work on #565 pretty much shaped this PR. Thank you! This commit adds initial configuration support for building wasi-sdk with support for C++ exceptions in WebAssembly. A small test is included which is exercised in CI at this time, but otherwise this does not update CI to actually ship sdk builds with C++ exceptions enabled. Instead the intention here is to make it easier to test builds with support for C++ exceptions and centralize planning/development around this. The goal here is to get things compiling to the point that applications can be compiled. I haven't thoroughly tested C++ exceptions and as evident in this PR it still requires changes in LLVM. Some small logic is added here to apply a `*.patch` file to LLVM to avoid needing a new submodule fork or waiting for upstream changes. The intention is that this `*.patch` is short-lived once the changes are officially merged into LLVM itself. The `*.patch` included here contains llvm/llvm-project#168449 as well as another minor edit I found was necessary to get things compiling locally. Given the discussion on that PR it looks like once LLVM is updated with that merged the extra part of the `*.patch` won't be necessary. This PR notably does not enable shared libraries when exceptions are enabled. I don't know enough about how things are supposed to work to be able to fully diagnose the compilation errors I'm seeing if shared libraries are enabled. This is something I'd hope would be fixed before actually shipping exceptions support. This PR then additionally folds in [this gist][gist] for various bits of build logic to this repository itself. Finally, this PR includes some documentation about the current status of exceptions and links to various tracking issues too. [gist]: https://gist.github.com/yerzham/302efcec6a2e82c1e8de4aed576ea29d
777761a to
852c8a2
Compare
|
I wanted to ping here and ask -- was there anything remaining for this? I think this is the last piece needed for C++ exceptions in wasi-sdk, so I was curious to ask about and possibly help get this landed. |
|
Same here, I've been using this change as part of packaging WASI support in nixpkgs, but we'd like to see this merged upstream rather than applying a patch. |
| #define _LIBUNWIND_HIDDEN | ||
| #else | ||
| #if !defined(__ELF__) && !defined(__MACH__) && !defined(_AIX) | ||
| #if !defined(__ELF__) && !defined(__MACH__) && !defined(_AIX) && \ |
There was a problem hiding this comment.
Perhaps __has_declspec_attribute(dllexport) @mstorsjo
There was a problem hiding this comment.
That doesn't quite do the right thing here, unfortunately. For <arch>-windows-msvc, it works, but for <arch>-windows-gnu it doesn't. Using declspec on the mingw targets works in practice (and still is the correct idiomatic way of doing it) because the windows-gnu targets have an implicit #define __declspec(a) __attribute__((a)), but they don't support declspec natively as such.
The existing code does seem quite backwards though, we could just replace the list of #if !defined(everything) with #if defined(_WIN32) instead.
This commit collects together some LLVM PRs, some changes in the build configuration here, and some thoughts from WebAssembly#565 and related issues. Specifically the changes here are: * The patch for llvm/llvm-project#168449 is updated to its upstream (unlanded) form. * Patches for the (landed) llvm/llvm-project#185770 and llvm/llvm-project#185775 are added. * The `WASI_SDK_EXCEPTIONS` configuration is now either `ON`, `OFF`, or `DUAL`. The default depends on the version of Clang in use, where 23.0.0+ (which isn't released officially yet) will be `DUAL` and otherwise it's `OFF`. CI for our custom-built patched toolchain defaults to `DUAL`. * In `DUAL` mode libcxx is built twice into two different directories, once with exceptions and once without. This is supported by LLVM patches and means that Clang will select the right set of libraries based on compiler flags. The end result here is that the produced toolchain from this repository, by default, supports C++ exceptions. Additionally if exceptions-related flags are not passed then the final binary will not use C++ exceptions nor require the wasm exception-handling proposal. There's still follow-up work from WebAssembly#565, such as: * Subjectively it feels wordy to pass `-fwasm-exceptions` vs `-fexceptions`. * Personally I think `-mllvm -wasm-use-legacy-eh=false` should become the default upstream. * Subjectively I don't think that `-lunwind` should be necessary and it should be injected automatically with `-fwasm-exceptions` (or `-fexceptions`). * Shared libraries for exceptions remain disabled due to build errors I do not personally know how to resolve. I'll file follow-up issues for these once this has landed since they're more minor compared to the main body of "anything works". Closes WebAssembly#334 Closes WebAssembly#565
This commit collects together some LLVM PRs, some changes in the build configuration here, and some thoughts from WebAssembly#565 and related issues. Specifically the changes here are: * The patch for llvm/llvm-project#168449 is updated to its upstream (unlanded) form. * Patches for the (landed) llvm/llvm-project#185770 and llvm/llvm-project#185775 are added. * The `WASI_SDK_EXCEPTIONS` configuration is now either `ON`, `OFF`, or `DUAL`. The default depends on the version of Clang in use, where 23.0.0+ (which isn't released officially yet) will be `DUAL` and otherwise it's `OFF`. CI for our custom-built patched toolchain defaults to `DUAL`. * In `DUAL` mode libcxx is built twice into two different directories, once with exceptions and once without. This is supported by LLVM patches and means that Clang will select the right set of libraries based on compiler flags. The end result here is that the produced toolchain from this repository, by default, supports C++ exceptions. Additionally if exceptions-related flags are not passed then the final binary will not use C++ exceptions nor require the wasm exception-handling proposal. There's still follow-up work from WebAssembly#565, such as: * Subjectively it feels wordy to pass `-fwasm-exceptions` vs `-fexceptions`. * Personally I think `-mllvm -wasm-use-legacy-eh=false` should become the default upstream. * Subjectively I don't think that `-lunwind` should be necessary and it should be injected automatically with `-fwasm-exceptions` (or `-fexceptions`). * Shared libraries for exceptions remain disabled due to build errors I do not personally know how to resolve. I'll file follow-up issues for these once this has landed since they're more minor compared to the main body of "anything works". Closes WebAssembly#334 Closes WebAssembly#565
This commit collects together some LLVM PRs, some changes in the build configuration here, and some thoughts from #565 and related issues. Specifically the changes here are: * The patch for llvm/llvm-project#168449 is updated to its upstream (unlanded) form. * Patches for the (landed) llvm/llvm-project#185770 and llvm/llvm-project#185775 are added. * The `WASI_SDK_EXCEPTIONS` configuration is now either `ON`, `OFF`, or `DUAL`. The default depends on the version of Clang in use, where 23.0.0+ (which isn't released officially yet) will be `DUAL` and otherwise it's `OFF`. CI for our custom-built patched toolchain defaults to `DUAL`. * In `DUAL` mode libcxx is built twice into two different directories, once with exceptions and once without. This is supported by LLVM patches and means that Clang will select the right set of libraries based on compiler flags. The end result here is that the produced toolchain from this repository, by default, supports C++ exceptions. Additionally if exceptions-related flags are not passed then the final binary will not use C++ exceptions nor require the wasm exception-handling proposal. There's still follow-up work from #565, such as: * Subjectively it feels wordy to pass `-fwasm-exceptions` vs `-fexceptions`. * Personally I think `-mllvm -wasm-use-legacy-eh=false` should become the default upstream. * Subjectively I don't think that `-lunwind` should be necessary and it should be injected automatically with `-fwasm-exceptions` (or `-fexceptions`). * Shared libraries for exceptions remain disabled due to build errors I do not personally know how to resolve. I'll file follow-up issues for these once this has landed since they're more minor compared to the main body of "anything works". Closes #334 Closes #565 --------- Co-authored-by: Joel Dice <joel.dice@akamai.com>
Most minimal change to enable libunwind to build for wasm by excluding
__wasm__from the Windows-style __declspec path in config.h. This unblocks building an exceptions-enabled wasi-sdk sysroot (see WebAssembly/wasi-sdk#565) and builds on prior attempts including yamt’s gist, #79667, and #140365.