-
Notifications
You must be signed in to change notification settings - Fork 13.6k
[WebAssembly] Refactor Wasm EH/SjLj error checking #122466
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
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.
@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"
|
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
// Backend needs -wasm-enable-eh to enable Wasm EH | |
CC1Args.push_back("-mllvm"); | |
CC1Args.push_back("-wasm-enable-eh"); |
But this adds checks for it too for symmetry because it's not hard and users can in theory give that to clang too.
// '-fwasm-exceptions' sets +exception-handling, -multivalue, -reference-types, | ||
// "-exception-model=wasm", and '-mllvm -wasm-enable-eh' |
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.
-exception-model=wasm
is also set in
llvm-project/clang/lib/Driver/ToolChains/Clang.cpp
Lines 7469 to 7470 in 9c85cde
if (Opt.matches(options::OPT_fwasm_exceptions)) | |
CmdArgs.push_back("-exception-model=wasm"); |
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.
// '-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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was just moved to gather feature-related errors together.
// '-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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was just moved too.
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.
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...
#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.
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.