clang: Forward exception_model flag for bitcode inputs #146342
Merged
+76
−0
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
This will enable removal of a hack from the wasm backend in a future change. This feels unnecessarily clunky. I would assume something was automatically parsing this and propagating it in the C++ case, but I can't seem to find it. In particular it feels wrong that I need to parse out the individual values, given they are listed in the options.td file. We should also be parsing and forwarding every flag that corresponds to something else in TargetOptions, which requires auditing.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
arsenm
added
the
clang:driver
Graphite App
|
@llvm/pr-subscribers-clang @llvm/pr-subscribers-clang-driver Author: Matt Arsenault (arsenm) ChangesThis will enable removal of a hack from the wasm backend This feels unnecessarily clunky. I would assume something was Full diff: https://github.com/llvm/llvm-project/pull/146342.diff 4 Files Affected:
diff --git a/clang/lib/Frontend/CompilerInvocation.cpp b/clang/lib/Frontend/CompilerInvocation.cpp
index f366e90945dac..d8916a6b15f58 100644
--- a/clang/lib/Frontend/CompilerInvocation.cpp
+++ b/clang/lib/Frontend/CompilerInvocation.cpp
@@ -3679,6 +3679,22 @@ static StringRef GetInputKindName(InputKind IK) {
llvm_unreachable("unknown input language");
}
+static StringRef getExceptionHandlingName(unsigned EHK) {
+ switch (static_cast<LangOptions::ExceptionHandlingKind>(EHK)) {
+ case LangOptions::ExceptionHandlingKind::None:
+ default:
+ return "none";
+ case LangOptions::ExceptionHandlingKind::SjLj:
+ return "sjlj";
+ case LangOptions::ExceptionHandlingKind::DwarfCFI:
+ return "dwarf";
+ case LangOptions::ExceptionHandlingKind::Wasm:
+ return "wasm";
+ }
+
+ llvm_unreachable("covered switch");
+}
+
void CompilerInvocationBase::GenerateLangArgs(const LangOptions &Opts,
ArgumentConsumer Consumer,
const llvm::Triple &T,
@@ -3694,6 +3710,10 @@ void CompilerInvocationBase::GenerateLangArgs(const LangOptions &Opts,
GenerateArg(Consumer, OPT_pic_is_pie);
for (StringRef Sanitizer : serializeSanitizerKinds(Opts.Sanitize))
GenerateArg(Consumer, OPT_fsanitize_EQ, Sanitizer);
+ if (Opts.ExceptionHandling) {
+ GenerateArg(Consumer, OPT_exception_model,
+ getExceptionHandlingName(Opts.ExceptionHandling));
+ }
return;
}
@@ -4002,6 +4022,24 @@ bool CompilerInvocation::ParseLangArgs(LangOptions &Opts, ArgList &Args,
parseSanitizerKinds("-fsanitize=", Args.getAllArgValues(OPT_fsanitize_EQ),
Diags, Opts.Sanitize);
+ if (const Arg *A = Args.getLastArg(options::OPT_exception_model)) {
+ std::optional<LangOptions::ExceptionHandlingKind> EMValue =
+ llvm::StringSwitch<std::optional<LangOptions::ExceptionHandlingKind>>(
+ A->getValue())
+ .Case("dwarf", LangOptions::ExceptionHandlingKind::DwarfCFI)
+ .Case("sjlj", LangOptions::ExceptionHandlingKind::SjLj)
+ .Case("wineh", LangOptions::ExceptionHandlingKind::WinEH)
+ .Case("wasm", LangOptions::ExceptionHandlingKind::Wasm)
+ .Case("none", LangOptions::ExceptionHandlingKind::None)
+ .Default(std::nullopt);
+ if (EMValue) {
+ Opts.ExceptionHandling = static_cast<unsigned>(*EMValue);
+ } else {
+ Diags.Report(diag::err_drv_invalid_value)
+ << A->getAsString(Args) << A->getValue();
+ }
+ }
+
return Diags.getNumErrors() == NumErrorsBefore;
}
diff --git a/clang/test/CodeGen/WebAssembly/wasm-exception-model-flag-parse-ir-input.ll b/clang/test/CodeGen/WebAssembly/wasm-exception-model-flag-parse-ir-input.ll
new file mode 100644
index 0000000000000..4a7eeece58717
--- /dev/null
+++ b/clang/test/CodeGen/WebAssembly/wasm-exception-model-flag-parse-ir-input.ll
@@ -0,0 +1,16 @@
+; REQUIRES: webassembly-registered-target
+
+; Check all the options parse
+; RUN: %clang_cc1 -triple wasm32 -o - -emit-llvm -exception-model=none %s | FileCheck %s
+; RUN: %clang_cc1 -triple wasm32 -o - -emit-llvm -exception-model=wasm %s | FileCheck %s
+; RUN: %clang_cc1 -triple wasm32 -o - -emit-llvm -exception-model=dwarf %s | FileCheck %s
+; RUN: %clang_cc1 -triple wasm32 -o - -emit-llvm -exception-model=sjlj %s | FileCheck %s
+
+; RUN: not %clang_cc1 -triple wasm32 -o - -emit-llvm -exception-model=invalid %s 2>&1 | FileCheck -check-prefix=ERR %s
+
+; CHECK-LABEL: define void @test(
+
+; ERR: error: invalid value 'invalid' in '-exception-model=invalid'
+define void @test() {
+ ret void
+}
diff --git a/clang/test/CodeGen/WebAssembly/wasm-invalid-exception-kinds.ll b/clang/test/CodeGen/WebAssembly/wasm-invalid-exception-kinds.ll
new file mode 100644
index 0000000000000..27fb696c92499
--- /dev/null
+++ b/clang/test/CodeGen/WebAssembly/wasm-invalid-exception-kinds.ll
@@ -0,0 +1,8 @@
+; RUN: not %clang_cc1 -triple wasm32 -exception-model=arst -S %s 2>&1 | FileCheck -check-prefix=INVALID-VALUE %s
+
+; Make sure invalid values are rejected for -exception-model when the
+; input is IR.
+
+; INVALID-VALUE: error: invalid value 'arst' in '-exception-model=arst'
+
+target triple = "wasm32"
diff --git a/clang/test/Driver/ir-exception-model.c b/clang/test/Driver/ir-exception-model.c
new file mode 100644
index 0000000000000..9e8f998de0d6b
--- /dev/null
+++ b/clang/test/Driver/ir-exception-model.c
@@ -0,0 +1,14 @@
+// RUN: %clang -### -target wasm32-unknown-unknown -fwasm-exceptions -c -S -o - %S/Inputs/file.ll 2>&1 | FileCheck %s
+// RUN: %clang -### -target wasm32-unknown-unknown -Xclang -exception-model=wasm -c -S -o - %S/Inputs/file.ll 2>&1 | FileCheck %s
+// RUN: %clang -### -target wasm32-unknown-unknown -Xclang -exception-model=dwarf -c -S -o - %S/Inputs/file.ll 2>&1 | FileCheck -check-prefix=DWARF %s
+// RUN: %clang -### -target wasm32-unknown-unknown -Xclang -exception-model=sjlj -c -S -o - %S/Inputs/file.ll 2>&1 | FileCheck -check-prefix=SJLJ %s
+// RUN: %clang -### -target wasm32-unknown-unknown -Xclang -exception-model=wineh -c -S -o - %S/Inputs/file.ll 2>&1 | FileCheck -check-prefix=WINEH %s
+// RUN: %clang -### -target wasm32-unknown-unknown -Xclang -exception-model=arst -c -S -o - %S/Inputs/file.ll 2>&1 | FileCheck -check-prefix=INVALID %s
+
+// Check that -fwasm-exceptions propagates -exception-model to cc1
+
+// CHECK: "-exception-model=wasm"
+// DWARF: "-exception-model=dwarf"
+// SJLJ: "-exception-model=sjlj"
+// WINEH: "-exception-model=wineh"
+// INVALID: "-exception-model=arst"
|
arsenm
requested review from
aheejin,
dschuff,
jansvoboda11,
jhuber6,
mjklemm,
sbc100 and
tlively
jhuber6
approved these changes
Jun 30, 2025
Comment on lines
+4035
to
+4040
| if (EMValue) { | ||
| Opts.ExceptionHandling = static_cast<unsigned>(*EMValue); | ||
| } else { | ||
| Diags.Report(diag::err_drv_invalid_value) | ||
| << A->getAsString(Args) << A->getValue(); | ||
| } |
There was a problem hiding this comment.
Suggested change
| if (EMValue) { | |
| Opts.ExceptionHandling = static_cast<unsigned>(*EMValue); | |
| } else { | |
| Diags.Report(diag::err_drv_invalid_value) | |
| << A->getAsString(Args) << A->getValue(); | |
| } | |
| if (EMValue) | |
| Opts.ExceptionHandling = static_cast<unsigned>(*EMValue); | |
| else | |
| Diags.Report(diag::err_drv_invalid_value) | |
| << A->getAsString(Args) << A->getValue(); |
There was a problem hiding this comment.
Multiple lines implies braces, and then keep on all if one does
Comment on lines
+3713
to
+3716
| if (Opts.ExceptionHandling) { | ||
| GenerateArg(Consumer, OPT_exception_model, | ||
| getExceptionHandlingName(Opts.ExceptionHandling)); | ||
| } |
There was a problem hiding this comment.
Suggested change
| if (Opts.ExceptionHandling) { | |
| GenerateArg(Consumer, OPT_exception_model, | |
| getExceptionHandlingName(Opts.ExceptionHandling)); | |
| } | |
| if (Opts.ExceptionHandling) | |
| GenerateArg(Consumer, OPT_exception_model, | |
| getExceptionHandlingName(Opts.ExceptionHandling)); |
Comment on lines
+3694
to
+3695
|
|
||
| llvm_unreachable("covered switch"); |
There was a problem hiding this comment.
Is unreachable necessary here? The compiler is smart enough to know that this function always has a return value I think.
There was a problem hiding this comment.
I'm pretty sure you need it to keep the full set of compilers happy. I think MSVC is the annoying one
jansvoboda11
approved these changes
Jun 30, 2025
|
(FWIW I have the alternative fix here: jansvoboda11@45f21d8; happy to create a PR after yours lands.) |
aheejin
reviewed
Jul 1, 2025
Comment on lines
+4
to
+7
| ; RUN: %clang_cc1 -triple wasm32 -o - -emit-llvm -exception-model=none %s | FileCheck %s | ||
| ; RUN: %clang_cc1 -triple wasm32 -o - -emit-llvm -exception-model=wasm %s | FileCheck %s | ||
| ; RUN: %clang_cc1 -triple wasm32 -o - -emit-llvm -exception-model=dwarf %s | FileCheck %s | ||
| ; RUN: %clang_cc1 -triple wasm32 -o - -emit-llvm -exception-model=sjlj %s | FileCheck %s |
There was a problem hiding this comment.
Given that modes other than none/wasm do not make any sense in Wasm, what's the point of testing them? (Also #146343 somehow seems to remove them?)
There was a problem hiding this comment.
They still should parse. The other patch also doesn't remove them, just changes the error
aheejin
reviewed
Jul 1, 2025
| @@ -0,0 +1,14 @@ | |||
| // RUN: %clang -### -target wasm32-unknown-unknown -fwasm-exceptions -c -S -o - %S/Inputs/file.ll 2>&1 | FileCheck %s | |||
aheejin
approved these changes
Jul 2, 2025
arsenm
deleted the
users/arsenm/clang/driver-forward-exception-handling-argument-ir-input
branch
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/207/builds/3225 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/23/builds/11788 Here is the relevant piece of the build log for the reference |
|
This broke building C++ modules for mingw targets. Repro:
export module empty;$ clang -target x86_64-windows-gnu -x c++-module empty.cppm -c -o empty.cppm.obj -std=gnu++23
error: invalid value 'seh' in '-exception-model=seh' |
arsenm
added a commit
that referenced
this pull request
Jul 2, 2025
Fixes regression reported #146342 (comment) The test could probably be better. I'm not sure what special is happening with the module compile, but I can't seem to reproduce this with just a plain -cc1 run.
I for some reason have to repeat the reproducer exactly this way; if I just run cc1 with the same triple and flags it seems to work? |
Yeah, I have no idea what's being passed through at what level here, but the fix looks reasonable to me. Thanks! |
mstorsjo
pushed a commit
that referenced
this pull request
Jul 2, 2025
Fixes regression reported #146342 (comment) The test could probably be better. I'm not sure what special is happening with the module compile, but I can't seem to reproduce this with just a plain -cc1 run.
llvm-sync bot
pushed a commit
to arm/arm-toolchain
that referenced
this pull request
Jul 2, 2025
Fixes regression reported llvm/llvm-project#146342 (comment) The test could probably be better. I'm not sure what special is happening with the module compile, but I can't seem to reproduce this with just a plain -cc1 run.
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 will enable removal of a hack from the wasm backend
in a future change.
This feels unnecessarily clunky. I would assume something was
automatically parsing this and propagating it in the C++ case,
but I can't seem to find it. In particular it feels wrong that
I need to parse out the individual values, given they are listed
in the options.td file. We should also be parsing and forwarding
every flag that corresponds to something else in TargetOptions,
which requires auditing.