[WebAssembly] Make EH depend on multivalue and reference-types #91299
Conversation
This file contains 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 PR turns on multivalue and reference-types features when exception-handling feature is turned on, and errors out when disabling of those dependent features is explicitly requested. I think doing this would be safe anyway regardless of whether or when we end up turning on reference-types by default. We currently don't yet have a experimental flag for the Clang and LLVM for the new experimental EH yet. But I think it should be fine to turn those features on even if the LLVM does not yet generate the new EH instructions, for the same reason we tried to turn them on by default and the browsers that support EH also support multivalue and reference-types anyway.
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-driver @llvm/pr-subscribers-clang Author: Heejin Ahn (aheejin) ChangesThis PR turns on multivalue and reference-types features when exception-handling feature is turned on, and errors out when disabling of those dependent features is explicitly requested. I think doing this would be safe anyway regardless of whether or when we end up turning on reference-types by default. We currently don't yet have a experimental flag for the Clang and LLVM for the new experimental EH yet. But I think it should be fine to turn those features on even if the LLVM does not yet generate the new EH instructions, for the same reason we tried to turn them on by default and the browsers that support EH also support multivalue and reference-types anyway. Full diff: https://github.com/llvm/llvm-project/pull/91299.diff 2 Files Affected:
diff --git a/clang/lib/Driver/ToolChains/WebAssembly.cpp b/clang/lib/Driver/ToolChains/WebAssembly.cpp
index b7c6efab83e80..5b763df9b3329 100644
--- a/clang/lib/Driver/ToolChains/WebAssembly.cpp
+++ b/clang/lib/Driver/ToolChains/WebAssembly.cpp
@@ -347,6 +347,23 @@ void WebAssembly::addClangTargetOptions(const ArgList &DriverArgs,
// Backend needs -wasm-enable-eh to enable Wasm EH
CC1Args.push_back("-mllvm");
CC1Args.push_back("-wasm-enable-eh");
+
+ // New Wasm EH spec (adopted in Oct 2023) requires multivalue and
+ // reference-types.
+ if (DriverArgs.hasFlag(options::OPT_mno_multivalue,
+ options::OPT_mmultivalue, false)) {
+ getDriver().Diag(diag::err_drv_argument_not_allowed_with)
+ << "-fwasm-exceptions" << "-mno-multivalue";
+ }
+ if (DriverArgs.hasFlag(options::OPT_mno_reference_types,
+ options::OPT_mreference_types, false)) {
+ getDriver().Diag(diag::err_drv_argument_not_allowed_with)
+ << "-fwasm-exceptions" << "-mno-reference-types";
+ }
+ CC1Args.push_back("-target-feature");
+ CC1Args.push_back("+multivalue");
+ CC1Args.push_back("-target-feature");
+ CC1Args.push_back("+reference-types");
}
for (const Arg *A : DriverArgs.filtered(options::OPT_mllvm)) {
@@ -408,6 +425,23 @@ void WebAssembly::addClangTargetOptions(const ArgList &DriverArgs,
CC1Args.push_back("+exception-handling");
// Backend needs '-exception-model=wasm' to use Wasm EH instructions
CC1Args.push_back("-exception-model=wasm");
+
+ // New Wasm EH spec (adopted in Oct 2023) requires multivalue and
+ // reference-types.
+ if (DriverArgs.hasFlag(options::OPT_mno_multivalue,
+ options::OPT_mmultivalue, false)) {
+ getDriver().Diag(diag::err_drv_argument_not_allowed_with)
+ << "-mllvm -wasm-enable-sjlj" << "-mno-multivalue";
+ }
+ if (DriverArgs.hasFlag(options::OPT_mno_reference_types,
+ options::OPT_mreference_types, false)) {
+ getDriver().Diag(diag::err_drv_argument_not_allowed_with)
+ << "-mllvm -wasm-enable-sjlj" << "-mno-reference-types";
+ }
+ CC1Args.push_back("-target-feature");
+ CC1Args.push_back("+multivalue");
+ CC1Args.push_back("-target-feature");
+ CC1Args.push_back("+reference-types");
}
}
}
diff --git a/clang/test/Driver/wasm-toolchain.c b/clang/test/Driver/wasm-toolchain.c
index dabf0ac2433bb..7c26c2c13c0ba 100644
--- a/clang/test/Driver/wasm-toolchain.c
+++ b/clang/test/Driver/wasm-toolchain.c
@@ -120,11 +120,12 @@
// RUN: | FileCheck -check-prefix=EMSCRIPTEN_EH_ALLOWED_WO_ENABLE %s
// EMSCRIPTEN_EH_ALLOWED_WO_ENABLE: invalid argument '-mllvm -emscripten-cxx-exceptions-allowed' only allowed with '-mllvm -enable-emscripten-cxx-exceptions'
-// '-fwasm-exceptions' sets +exception-handling and '-mllvm -wasm-enable-eh'
+// '-fwasm-exceptions' sets +exception-handling, -multivalue, -reference-types
+// and '-mllvm -wasm-enable-eh'
// RUN: %clang -### --target=wasm32-unknown-unknown \
// RUN: --sysroot=/foo %s -fwasm-exceptions 2>&1 \
// RUN: | FileCheck -check-prefix=WASM_EXCEPTIONS %s
-// WASM_EXCEPTIONS: "-cc1" {{.*}} "-target-feature" "+exception-handling" "-mllvm" "-wasm-enable-eh"
+// WASM_EXCEPTIONS: "-cc1" {{.*}} "-target-feature" "+exception-handling" "-mllvm" "-wasm-enable-eh" "-target-feature" "+multivalue" "-target-feature" "+reference-types"
// '-fwasm-exceptions' not allowed with '-mno-exception-handling'
// RUN: not %clang -### --target=wasm32-unknown-unknown \
@@ -132,19 +133,32 @@
// RUN: | FileCheck -check-prefix=WASM_EXCEPTIONS_NO_EH %s
// WASM_EXCEPTIONS_NO_EH: invalid argument '-fwasm-exceptions' not allowed with '-mno-exception-handling'
-// '-fwasm-exceptions' not allowed with '-mllvm -enable-emscripten-cxx-exceptions'
+// '-fwasm-exceptions' not allowed with
+// '-mllvm -enable-emscripten-cxx-exceptions'
// RUN: not %clang -### --target=wasm32-unknown-unknown \
// RUN: --sysroot=/foo %s -fwasm-exceptions \
// RUN: -mllvm -enable-emscripten-cxx-exceptions 2>&1 \
// RUN: | FileCheck -check-prefix=WASM_EXCEPTIONS_EMSCRIPTEN_EH %s
// WASM_EXCEPTIONS_EMSCRIPTEN_EH: invalid argument '-fwasm-exceptions' not allowed with '-mllvm -enable-emscripten-cxx-exceptions'
-// '-mllvm -wasm-enable-sjlj' sets +exception-handling and
-// '-exception-model=wasm'
+// '-fwasm-exceptions' not allowed with '-mno-multivalue'
+// RUN: not %clang -### --target=wasm32-unknown-unknown \
+// RUN: --sysroot=/foo %s -fwasm-exceptions -mno-multivalue 2>&1 \
+// RUN: | FileCheck -check-prefix=WASM_EXCEPTIONS_NO_MULTIVALUE %s
+// WASM_EXCEPTIONS_NO_MULTIVALUE: invalid argument '-fwasm-exceptions' not allowed with '-mno-multivalue'
+
+// '-fwasm-exceptions' not allowed with '-mno-reference-types'
+// RUN: not %clang -### --target=wasm32-unknown-unknown \
+// RUN: --sysroot=/foo %s -fwasm-exceptions -mno-reference-types 2>&1 \
+// RUN: | FileCheck -check-prefix=WASM_EXCEPTIONS_NO_REFERENCE_TYPES %s
+// WASM_EXCEPTIONS_NO_REFERENCE_TYPES: invalid argument '-fwasm-exceptions' not allowed with '-mno-reference-types'
+
+// '-mllvm -wasm-enable-sjlj' sets +exception-handling, +multivalue,
+// +reference-types and '-exception-model=wasm'
// RUN: %clang -### --target=wasm32-unknown-unknown \
// RUN: --sysroot=/foo %s -mllvm -wasm-enable-sjlj 2>&1 \
// RUN: | FileCheck -check-prefix=WASM_SJLJ %s
-// WASM_SJLJ: "-cc1" {{.*}} "-target-feature" "+exception-handling" "-exception-model=wasm"
+// WASM_SJLJ: "-cc1" {{.*}} "-target-feature" "+exception-handling" "-exception-model=wasm" "-target-feature" "+multivalue" "-target-feature" "+reference-types"
// '-mllvm -wasm-enable-sjlj' not allowed with '-mno-exception-handling'
// RUN: not %clang -### --target=wasm32-unknown-unknown \
@@ -168,6 +182,19 @@
// RUN: | FileCheck -check-prefix=WASM_SJLJ_EMSCRIPTEN_SJLJ %s
// WASM_SJLJ_EMSCRIPTEN_SJLJ: invalid argument '-mllvm -wasm-enable-sjlj' not allowed with '-mllvm -enable-emscripten-sjlj'
+// '-mllvm -wasm-enable-sjlj' not allowed with '-mno-multivalue'
+// RUN: not %clang -### --target=wasm32-unknown-unknown \
+// RUN: --sysroot=/foo %s -mllvm -wasm-enable-sjlj -mno-multivalue 2>&1 \
+// RUN: | FileCheck -check-prefix=WASM_SJLJ_NO_MULTIVALUE %s
+// WASM_SJLJ_NO_MULTIVALUE: invalid argument '-mllvm -wasm-enable-sjlj' not allowed with '-mno-multivalue'
+
+// '-mllvm -wasm-enable-sjlj' not allowed with '-mno-reference-types'
+// RUN: not %clang -### --target=wasm32-unknown-unknown \
+// RUN: --sysroot=/foo %s -mllvm -wasm-enable-sjlj \
+// RUN: -mno-reference-types 2>&1 \
+// RUN: | FileCheck -check-prefix=WASM_SJLJ_NO_REFERENCE_TYPES %s
+// WASM_SJLJ_NO_REFERENCE_TYPES: invalid argument '-mllvm -wasm-enable-sjlj' not allowed with '-mno-reference-types'
+
// RUN: %clang -### %s -fsanitize=address --target=wasm32-unknown-emscripten 2>&1 | FileCheck -check-prefix=CHECK-ASAN-EMSCRIPTEN %s
// CHECK-ASAN-EMSCRIPTEN: "-fsanitize=address"
// CHECK-ASAN-EMSCRIPTEN: "-fsanitize-address-globals-dead-stripping"
|
dschuff
approved these changes
May 7, 2024
There was a problem hiding this comment.
LGTM.
Are we currently running wasm-eh tests on the Chromium CI with node 16? If so, I guess this will have the same issue we had when we tried to turn it on by default.
I don't think this needs to be a showstopper in the same way (since this isn't the default output of emscripten that we're talking about here, so I think it's fine to say "using EH requires node 17+"). But we may have to either disable some tests on chromium CI, or just make the test runner add a flag.
|
According to https://webassembly.org/features/, EH is supported from node v17, which is also Emscripten test infra requires: https://github.com/emscripten-core/emscripten/blob/2fefc1911e2e6265174c3fd5da38c8e9ab7abb60/test/common.py#L866-L868 So EH tests should have been already being skipped in Chromium CI, so I don't think the same problem will happen there. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This PR turns on multivalue and reference-types features when exception-handling feature is turned on, and errors out when disabling of those dependent features is explicitly requested.
I think doing this would be safe anyway regardless of whether or when we end up turning on reference-types by default.
We currently don't yet have a experimental flag for the Clang and LLVM for the new experimental EH yet. But I think it should be fine to turn those features on even if the LLVM does not yet generate the new EH instructions, for the same reason we tried to turn them on by default and the browsers that support EH also support multivalue and reference-types anyway.