[WebAssembly] Refactor Wasm EH/SjLj error checking #122466
Merged
Conversation
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
There were many overlaps between error checking and feature enabling routines for Wasm EH and Wasm SjLj. This tries to factor out those common routines in separate lambda functions. This is not NFC because this ends up disallowing a few new combinations (e.g. `-fwasm-exceptions` and `-emscripten-cxx-exceptions-allowed`), and also deletes `-mllvm` from the error messages to share the same lambda function between options with `-mllvm` and those without it. This adds a few more tests but does not try to cover every single possible disallowed combination.
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-driver @llvm/pr-subscribers-clang Author: Heejin Ahn (aheejin) ChangesThere were many overlaps between error checking and feature enabling routines for Wasm EH and Wasm SjLj. This tries to factor out those common routines in separate lambda functions. This is not NFC because this ends up disallowing a few new combinations (e.g. This adds a few more tests but does not try to cover every single possible disallowed combination. Full diff: https://github.com/llvm/llvm-project/pull/122466.diff 2 Files Affected:
diff --git a/clang/lib/Driver/ToolChains/WebAssembly.cpp b/clang/lib/Driver/ToolChains/WebAssembly.cpp
index 44a6894d30fb29..e338b0d2398e04 100644
--- a/clang/lib/Driver/ToolChains/WebAssembly.cpp
+++ b/clang/lib/Driver/ToolChains/WebAssembly.cpp
@@ -344,44 +344,53 @@ void WebAssembly::addClangTargetOptions(const ArgList &DriverArgs,
}
}
- if (DriverArgs.getLastArg(options::OPT_fwasm_exceptions)) {
- // '-fwasm-exceptions' is not compatible with '-mno-exception-handling'
+ // Bans incompatible options for Wasm EH / SjLj. We don't allow using
+ // different modes for EH and SjLj.
+ auto BanIncompatibleOptionsForWasmEHSjLj = [&](StringRef CurOption) {
if (DriverArgs.hasFlag(options::OPT_mno_exception_handing,
options::OPT_mexception_handing, false))
getDriver().Diag(diag::err_drv_argument_not_allowed_with)
- << "-fwasm-exceptions"
- << "-mno-exception-handling";
- // '-fwasm-exceptions' is not compatible with
- // '-mllvm -enable-emscripten-cxx-exceptions'
- for (const Arg *A : DriverArgs.filtered(options::OPT_mllvm)) {
- if (StringRef(A->getValue(0)) == "-enable-emscripten-cxx-exceptions")
- getDriver().Diag(diag::err_drv_argument_not_allowed_with)
- << "-fwasm-exceptions"
- << "-mllvm -enable-emscripten-cxx-exceptions";
- }
- // '-fwasm-exceptions' implies exception-handling feature
- CC1Args.push_back("-target-feature");
- CC1Args.push_back("+exception-handling");
- // 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.
+ << CurOption << "-mno-exception-handling";
+ // The standardized Wasm EH spec requires multivalue and reference-types.
if (DriverArgs.hasFlag(options::OPT_mno_multivalue,
- options::OPT_mmultivalue, false)) {
+ options::OPT_mmultivalue, false))
getDriver().Diag(diag::err_drv_argument_not_allowed_with)
- << "-fwasm-exceptions" << "-mno-multivalue";
- }
+ << CurOption << "-mno-multivalue";
if (DriverArgs.hasFlag(options::OPT_mno_reference_types,
- options::OPT_mreference_types, false)) {
+ options::OPT_mreference_types, false))
getDriver().Diag(diag::err_drv_argument_not_allowed_with)
- << "-fwasm-exceptions" << "-mno-reference-types";
+ << CurOption << "-mno-reference-types";
+
+ for (const Arg *A : DriverArgs.filtered(options::OPT_mllvm)) {
+ for (const auto *Option :
+ {"-enable-emscripten-cxx-exceptions", "-enable-emscripten-sjlj",
+ "-emscripten-cxx-exceptions-allowed"}) {
+ if (StringRef(A->getValue(0)) == Option)
+ getDriver().Diag(diag::err_drv_argument_not_allowed_with)
+ << CurOption << Option;
+ }
}
+ };
+
+ // Enable necessary features for Wasm EH / SjLj in the backend.
+ auto EnableFeaturesForWasmEHSjLj = [&]() {
+ CC1Args.push_back("-target-feature");
+ CC1Args.push_back("+exception-handling");
+ // The standardized Wasm EH spec requires multivalue and reference-types.
CC1Args.push_back("-target-feature");
CC1Args.push_back("+multivalue");
CC1Args.push_back("-target-feature");
CC1Args.push_back("+reference-types");
+ // Backend needs '-exception-model=wasm' to use Wasm EH instructions
+ CC1Args.push_back("-exception-model=wasm");
+ };
+
+ if (DriverArgs.getLastArg(options::OPT_fwasm_exceptions)) {
+ BanIncompatibleOptionsForWasmEHSjLj("-fwasm-exceptions");
+ EnableFeaturesForWasmEHSjLj();
+ // Backend needs -wasm-enable-eh to enable Wasm EH
+ CC1Args.push_back("-mllvm");
+ CC1Args.push_back("-wasm-enable-eh");
}
for (const Arg *A : DriverArgs.filtered(options::OPT_mllvm)) {
@@ -413,53 +422,11 @@ void WebAssembly::addClangTargetOptions(const ArgList &DriverArgs,
}
}
- if (Opt.starts_with("-wasm-enable-sjlj")) {
- // '-mllvm -wasm-enable-sjlj' is not compatible with
- // '-mno-exception-handling'
- if (DriverArgs.hasFlag(options::OPT_mno_exception_handing,
- options::OPT_mexception_handing, false))
- getDriver().Diag(diag::err_drv_argument_not_allowed_with)
- << "-mllvm -wasm-enable-sjlj"
- << "-mno-exception-handling";
- // '-mllvm -wasm-enable-sjlj' is not compatible with
- // '-mllvm -enable-emscripten-cxx-exceptions'
- // because we don't allow Emscripten EH + Wasm SjLj
- for (const Arg *A : DriverArgs.filtered(options::OPT_mllvm)) {
- if (StringRef(A->getValue(0)) == "-enable-emscripten-cxx-exceptions")
- getDriver().Diag(diag::err_drv_argument_not_allowed_with)
- << "-mllvm -wasm-enable-sjlj"
- << "-mllvm -enable-emscripten-cxx-exceptions";
+ for (const auto *Option : {"-wasm-enable-eh", "-wasm-enable-sjlj"}) {
+ if (Opt.starts_with(Option)) {
+ BanIncompatibleOptionsForWasmEHSjLj(Option);
+ EnableFeaturesForWasmEHSjLj();
}
- // '-mllvm -wasm-enable-sjlj' is not compatible with
- // '-mllvm -enable-emscripten-sjlj'
- for (const Arg *A : DriverArgs.filtered(options::OPT_mllvm)) {
- if (StringRef(A->getValue(0)) == "-enable-emscripten-sjlj")
- getDriver().Diag(diag::err_drv_argument_not_allowed_with)
- << "-mllvm -wasm-enable-sjlj"
- << "-mllvm -enable-emscripten-sjlj";
- }
- // '-mllvm -wasm-enable-sjlj' implies exception-handling feature
- CC1Args.push_back("-target-feature");
- 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 7c26c2c13c0baf..84c1b4f6efe662 100644
--- a/clang/test/Driver/wasm-toolchain.c
+++ b/clang/test/Driver/wasm-toolchain.c
@@ -120,18 +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, -multivalue, -reference-types
-// and '-mllvm -wasm-enable-eh'
+// '-fwasm-exceptions' sets +exception-handling, -multivalue, -reference-types,
+// "-exception-model=wasm", 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" "-target-feature" "+multivalue" "-target-feature" "+reference-types"
-
-// '-fwasm-exceptions' not allowed with '-mno-exception-handling'
-// RUN: not %clang -### --target=wasm32-unknown-unknown \
-// RUN: --sysroot=/foo %s -fwasm-exceptions -mno-exception-handling 2>&1 \
-// RUN: | FileCheck -check-prefix=WASM_EXCEPTIONS_NO_EH %s
-// WASM_EXCEPTIONS_NO_EH: invalid argument '-fwasm-exceptions' not allowed with '-mno-exception-handling'
+// WASM_EXCEPTIONS: "-cc1" {{.*}} "-target-feature" "+exception-handling" "-target-feature" "+multivalue" "-target-feature" "+reference-types" "-exception-model=wasm" "-mllvm" "-wasm-enable-eh"
// '-fwasm-exceptions' not allowed with
// '-mllvm -enable-emscripten-cxx-exceptions'
@@ -139,7 +133,20 @@
// 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'
+// WASM_EXCEPTIONS_EMSCRIPTEN_EH: invalid argument '-fwasm-exceptions' not allowed with '-enable-emscripten-cxx-exceptions'
+
+// '-fwasm-exceptions' not allowed with '-mllvm -enable-emscripten-sjlj'
+// RUN: not %clang -### --target=wasm32-unknown-unknown \
+// RUN: --sysroot=/foo %s -fwasm-exceptions \
+// RUN: -mllvm -enable-emscripten-sjlj 2>&1 \
+// RUN: | FileCheck -check-prefix=WASM_EXCEPTIONS_EMSCRIPTEN_SJLJ %s
+// WASM_EXCEPTIONS_EMSCRIPTEN_SJLJ: invalid argument '-fwasm-exceptions' not allowed with '-enable-emscripten-sjlj'
+
+// '-fwasm-exceptions' not allowed with '-mno-exception-handling'
+// RUN: not %clang -### --target=wasm32-unknown-unknown \
+// RUN: --sysroot=/foo %s -fwasm-exceptions -mno-exception-handling 2>&1 \
+// 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 '-mno-multivalue'
// RUN: not %clang -### --target=wasm32-unknown-unknown \
@@ -154,18 +161,11 @@
// 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'
+// +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" "-target-feature" "+multivalue" "-target-feature" "+reference-types"
-
-// '-mllvm -wasm-enable-sjlj' not allowed with '-mno-exception-handling'
-// RUN: not %clang -### --target=wasm32-unknown-unknown \
-// RUN: --sysroot=/foo %s -mllvm -wasm-enable-sjlj -mno-exception-handling \
-// RUN: 2>&1 \
-// RUN: | FileCheck -check-prefix=WASM_SJLJ_NO_EH %s
-// WASM_SJLJ_NO_EH: invalid argument '-mllvm -wasm-enable-sjlj' not allowed with '-mno-exception-handling'
+// WASM_SJLJ: "-cc1" {{.*}} "-target-feature" "+exception-handling" "-target-feature" "+multivalue" "-target-feature" "+reference-types" "-exception-model=wasm"
// '-mllvm -wasm-enable-sjlj' not allowed with
// '-mllvm -enable-emscripten-cxx-exceptions'
@@ -173,27 +173,34 @@
// RUN: --sysroot=/foo %s -mllvm -wasm-enable-sjlj \
// RUN: -mllvm -enable-emscripten-cxx-exceptions 2>&1 \
// RUN: | FileCheck -check-prefix=WASM_SJLJ_EMSCRIPTEN_EH %s
-// WASM_SJLJ_EMSCRIPTEN_EH: invalid argument '-mllvm -wasm-enable-sjlj' not allowed with '-mllvm -enable-emscripten-cxx-exceptions'
+// WASM_SJLJ_EMSCRIPTEN_EH: invalid argument '-wasm-enable-sjlj' not allowed with '-enable-emscripten-cxx-exceptions'
// '-mllvm -wasm-enable-sjlj' not allowed with '-mllvm -enable-emscripten-sjlj'
// RUN: not %clang -### --target=wasm32-unknown-unknown \
// RUN: --sysroot=/foo %s -mllvm -wasm-enable-sjlj \
// RUN: -mllvm -enable-emscripten-sjlj 2>&1 \
// 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'
+// WASM_SJLJ_EMSCRIPTEN_SJLJ: invalid argument '-wasm-enable-sjlj' not allowed with '-enable-emscripten-sjlj'
+
+// '-mllvm -wasm-enable-sjlj' not allowed with '-mno-exception-handling'
+// RUN: not %clang -### --target=wasm32-unknown-unknown \
+// RUN: --sysroot=/foo %s -mllvm -wasm-enable-sjlj -mno-exception-handling \
+// RUN: 2>&1 \
+// RUN: | FileCheck -check-prefix=WASM_SJLJ_NO_EH %s
+// WASM_SJLJ_NO_EH: invalid argument '-wasm-enable-sjlj' not allowed with '-mno-exception-handling'
// '-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'
+// WASM_SJLJ_NO_MULTIVALUE: invalid argument '-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'
+// WASM_SJLJ_NO_REFERENCE_TYPES: invalid argument '-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"
|
aheejin
commented
Jan 10, 2025
| getDriver().Diag(diag::err_drv_argument_not_allowed_with) | ||
| << "-mllvm -wasm-enable-sjlj" | ||
| << "-mllvm -enable-emscripten-cxx-exceptions"; | ||
| for (const auto *Option : {"-wasm-enable-eh", "-wasm-enable-sjlj"}) { |
There was a problem hiding this comment.
We were not checking -wasm-enable-eh before because emscripten does not set it directly; it sets -fwasm-exceptions, which sets -wasm-enable-eh:
llvm-project/clang/lib/Driver/ToolChains/WebAssembly.cpp
Lines 365 to 367 in 9c85cde
But this adds checks for it too for symmetry because it's not hard and users can in theory give that to clang too.
Comment on lines
+123
to
+124
| // '-fwasm-exceptions' sets +exception-handling, -multivalue, -reference-types, | ||
| // "-exception-model=wasm", and '-mllvm -wasm-enable-eh' |
There was a problem hiding this comment.
-exception-model=wasm is also set in
llvm-project/clang/lib/Driver/ToolChains/Clang.cpp
Lines 7469 to 7470 in 9c85cde
which is the reason we set this only for Wasm SjLj and not Wasm EH here. But now
EnableFeaturesForWasmEHSjLj includes it so it is set for both, which I think is harmless anyway.
Comment on lines
+145
to
+149
| // '-fwasm-exceptions' not allowed with '-mno-exception-handling' | ||
| // RUN: not %clang -### --target=wasm32-unknown-unknown \ | ||
| // RUN: --sysroot=/foo %s -fwasm-exceptions -mno-exception-handling 2>&1 \ | ||
| // RUN: | FileCheck -check-prefix=WASM_EXCEPTIONS_NO_EH %s | ||
| // WASM_EXCEPTIONS_NO_EH: invalid argument '-fwasm-exceptions' not allowed with '-mno-exception-handling' |
There was a problem hiding this comment.
This was just moved to gather feature-related errors together.
Comment on lines
+185
to
+190
| // '-mllvm -wasm-enable-sjlj' not allowed with '-mno-exception-handling' | ||
| // RUN: not %clang -### --target=wasm32-unknown-unknown \ | ||
| // RUN: --sysroot=/foo %s -mllvm -wasm-enable-sjlj -mno-exception-handling \ | ||
| // RUN: 2>&1 \ | ||
| // RUN: | FileCheck -check-prefix=WASM_SJLJ_NO_EH %s | ||
| // WASM_SJLJ_NO_EH: invalid argument '-wasm-enable-sjlj' not allowed with '-mno-exception-handling' |
There was a problem hiding this comment.
This was just moved too.
dschuff
approved these changes
Jan 10, 2025
BaiXilin
pushed a commit
to BaiXilin/llvm-fix-vnni-instr-types
that referenced
this pull request
Jan 12, 2025
There were many overlaps between error checking and feature enabling routines for Wasm EH and Wasm SjLj. This tries to factor out those common routines in separate lambda functions. This is not NFC because this ends up disallowing a few new combinations (e.g. `-fwasm-exceptions` and `-emscripten-cxx-exceptions-allowed`), and also deletes `-mllvm` from the error messages to share the same lambda function between options with `-mllvm` and those without it. This adds a few more tests but does not try to cover every single possible disallowed combination.
aheejin
added a commit
to aheejin/llvm-project
that referenced
this pull request
Jan 23, 2025
llvm#122466 had an unexpected side effect that, `EnableFeaturesForWasmEHSjLj` and `BanIncompatibleOptionsForWasmEHSjLj` can be called multiple times now, every time a Wasm EH flag (`-fwasm-exceptions, `-wasm-enable-eh`, `-wasm-enable-sjlj`, ..) was checked and handled. This resulted in unnecessarily adding the same feature-enabling arguments multiple times to the command line, for example, `-target-feature +exception-handling` could be added as many as three times, which didn't cause any errors but unnecessary. Also we ran `BanIncompatibleOptionsForWasmEHSjLj` more than once, which was harmless but unnecessary. This guards these functions with a static variable so that we only run them once. This does not add any new tests because honestly I don't see any value of having a test that checks `-target-feature +exception-handling` is only added once...
aheejin
added a commit
that referenced
this pull request
Jan 23, 2025
#122466 had an unexpected side effect that, `EnableFeaturesForWasmEHSjLj` and `BanIncompatibleOptionsForWasmEHSjLj` can be called multiple times now, every time a Wasm EH flag (`-fwasm-exceptions`, `-wasm-enable-eh`, `-wasm-enable-sjlj`, ..) was checked and handled. This resulted in unnecessarily adding the same feature-enabling arguments multiple times to the command line, for example, `-target-feature +exception-handling` could be added as many as three times, which didn't cause any errors but unnecessary. Also we ran `BanIncompatibleOptionsForWasmEHSjLj` more than once, which was harmless but unnecessary. This guards these functions with a static variable so that we only run them once.
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.
There were many overlaps between error checking and feature enabling routines for Wasm EH and Wasm SjLj. This tries to factor out those common routines in separate lambda functions.
This is not NFC because this ends up disallowing a few new combinations (e.g.
-fwasm-exceptionsand-emscripten-cxx-exceptions-allowed), and also deletes-mllvmfrom the error messages to share the same lambda function between options with-mllvmand those without it.This adds a few more tests but does not try to cover every single possible disallowed combination.