Skip to content

[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

Merged
merged 1 commit into from
Jan 10, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
111 changes: 39 additions & 72 deletions clang/lib/Driver/ToolChains/WebAssembly.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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)) {
Expand Down Expand Up @@ -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"}) {
Copy link
Member Author

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:

// 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.

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");
}
}
}
Expand Down
53 changes: 30 additions & 23 deletions clang/test/Driver/wasm-toolchain.c
Original file line number Diff line number Diff line change
Expand Up @@ -120,26 +120,33 @@
// 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'
Comment on lines +123 to +124
Copy link
Member Author

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

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.

// 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'
// 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'
// 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'
Comment on lines +145 to +149
Copy link
Member Author

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.


// '-fwasm-exceptions' not allowed with '-mno-multivalue'
// RUN: not %clang -### --target=wasm32-unknown-unknown \
Expand All @@ -154,46 +161,46 @@
// 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'
// RUN: not %clang -### --target=wasm32-unknown-unknown \
// 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'
Comment on lines +185 to +190
Copy link
Member Author

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.


// '-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"
Expand Down
Loading