WebAssembly: Stop changing MCAsmInfo's ExceptionsType based on flags #146343
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
arsenm
added
backend:WebAssembly
clang:driver
Graphite App
arsenm
requested review from
aheejin,
dschuff,
jansvoboda11,
jhuber6,
mjklemm,
sbc100 and
tlively
|
@llvm/pr-subscribers-clang @llvm/pr-subscribers-backend-webassembly Author: Matt Arsenault (arsenm) ChangesCurrently wasm adds an extra level of options that work backwards These comments about compiling bitcode with clang are describing a toolchain -wasm-enable-eh and -wasm-enable-sjlj should also be removed in favor of the standard Additionally, this was enforcing some rules about the combinations of flags at a random Patch is 25.33 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/146343.diff 16 Files Affected:
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
index 4a7eeece58717..85bfc7f74daed 100644
--- 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
@@ -2,15 +2,16 @@
; 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: %clang_cc1 -triple wasm32 -o - -emit-llvm -exception-model=wasm -mllvm -wasm-enable-eh %s | FileCheck %s
; RUN: not %clang_cc1 -triple wasm32 -o - -emit-llvm -exception-model=invalid %s 2>&1 | FileCheck -check-prefix=ERR %s
+; RUN: not %clang_cc1 -triple wasm32 -o - -emit-llvm -exception-model=dwarf %s 2>&1 | FileCheck -check-prefix=ERR-BE %s
+; RUN: not %clang_cc1 -triple wasm32 -o - -emit-llvm -exception-model=sjlj %s 2>&1 | FileCheck -check-prefix=ERR-BE %s
; CHECK-LABEL: define void @test(
; ERR: error: invalid value 'invalid' in '-exception-model=invalid'
+; ERR-BE: fatal error: error in backend: -exception-model should be either 'none' or 'wasm'
define void @test() {
ret void
}
diff --git a/clang/test/CodeGenCXX/builtins-eh-wasm.cpp b/clang/test/CodeGenCXX/builtins-eh-wasm.cpp
index b0f763d3e54dc..9a7134c48f208 100644
--- a/clang/test/CodeGenCXX/builtins-eh-wasm.cpp
+++ b/clang/test/CodeGenCXX/builtins-eh-wasm.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -triple wasm32-unknown-unknown -fexceptions -fcxx-exceptions -target-feature +reference-types -target-feature +exception-handling -target-feature +multivalue -exception-model=wasm -emit-llvm -o - %s | FileCheck %s
+// RUN: %clang_cc1 -triple wasm32-unknown-unknown -fexceptions -fcxx-exceptions -target-feature +reference-types -target-feature +exception-handling -target-feature +multivalue -mllvm -wasm-enable-eh -exception-model=wasm -emit-llvm -o - %s | FileCheck %s
// Check if __builtin_wasm_throw and __builtin_wasm_rethrow are correctly
// invoked when placed in try-catch.
diff --git a/clang/test/CodeGenCXX/wasm-eh.cpp b/clang/test/CodeGenCXX/wasm-eh.cpp
index faff764878f5d..f243f37ecb435 100644
--- a/clang/test/CodeGenCXX/wasm-eh.cpp
+++ b/clang/test/CodeGenCXX/wasm-eh.cpp
@@ -393,9 +393,9 @@ void noexcept_throw() noexcept {
// CHECK-NEXT: call void @_ZSt9terminatev()
-// RUN: %clang_cc1 %s -triple wasm32-unknown-unknown -fms-extensions -fexceptions -fcxx-exceptions -exception-model=wasm -target-feature +exception-handling -emit-llvm -o - -std=c++11 2>&1 | FileCheck %s --check-prefix=WARNING-DEFAULT
-// RUN: %clang_cc1 %s -triple wasm32-unknown-unknown -fms-extensions -fexceptions -fcxx-exceptions -exception-model=wasm -target-feature +exception-handling -Wwasm-exception-spec -emit-llvm -o - -std=c++11 2>&1 | FileCheck %s --check-prefix=WARNING-ON
-// RUN: %clang_cc1 %s -triple wasm32-unknown-unknown -fms-extensions -fexceptions -fcxx-exceptions -exception-model=wasm -target-feature +exception-handling -Wno-wasm-exception-spec -emit-llvm -o - -std=c++11 2>&1 | FileCheck %s --check-prefix=WARNING-OFF
+// RUN: %clang_cc1 %s -triple wasm32-unknown-unknown -fms-extensions -fexceptions -fcxx-exceptions -mllvm -wasm-enable-eh -exception-model=wasm -target-feature +exception-handling -emit-llvm -o - -std=c++11 2>&1 | FileCheck %s --check-prefix=WARNING-DEFAULT
+// RUN: %clang_cc1 %s -triple wasm32-unknown-unknown -fms-extensions -fexceptions -fcxx-exceptions -mllvm -wasm-enable-eh -exception-model=wasm -target-feature +exception-handling -Wwasm-exception-spec -emit-llvm -o - -std=c++11 2>&1 | FileCheck %s --check-prefix=WARNING-ON
+// RUN: %clang_cc1 %s -triple wasm32-unknown-unknown -fms-extensions -fexceptions -fcxx-exceptions -mllvm -wasm-enable-eh -exception-model=wasm -target-feature +exception-handling -Wno-wasm-exception-spec -emit-llvm -o - -std=c++11 2>&1 | FileCheck %s --check-prefix=WARNING-OFF
// RUN: %clang_cc1 %s -triple wasm32-unknown-unknown -fexceptions -fcxx-exceptions -emit-llvm -o - -std=c++11 2>&1 | FileCheck %s --check-prefix=EM-EH-WARNING
// Wasm EH ignores dynamic exception specifications with types at the moment.
diff --git a/llvm/lib/Target/WebAssembly/MCTargetDesc/WebAssemblyMCAsmInfo.cpp b/llvm/lib/Target/WebAssembly/MCTargetDesc/WebAssemblyMCAsmInfo.cpp
index 3987b086195a4..a783e5fd8521a 100644
--- a/llvm/lib/Target/WebAssembly/MCTargetDesc/WebAssemblyMCAsmInfo.cpp
+++ b/llvm/lib/Target/WebAssembly/MCTargetDesc/WebAssemblyMCAsmInfo.cpp
@@ -55,14 +55,7 @@ WebAssemblyMCAsmInfo::WebAssemblyMCAsmInfo(const Triple &T,
LCOMMDirectiveAlignmentType = LCOMM::Log2Alignment;
SupportsDebugInformation = true;
-
- // When compilation is done on a cpp file by clang, the exception model info
- // is stored in LangOptions, which is later used to set the info in
- // TargetOptions and then MCAsmInfo in CodeGenTargetMachine::initAsmInfo().
- // But this process does not happen when compiling bitcode directly with
- // clang, so we make sure this info is set correctly.
- if (WebAssembly::WasmEnableEH || WebAssembly::WasmEnableSjLj)
- ExceptionsType = ExceptionHandling::Wasm;
+ ExceptionsType = ExceptionHandling::None;
initializeAtSpecifiers(atSpecifiers);
}
diff --git a/llvm/lib/Target/WebAssembly/MCTargetDesc/WebAssemblyMCTargetDesc.cpp b/llvm/lib/Target/WebAssembly/MCTargetDesc/WebAssemblyMCTargetDesc.cpp
index 6c0031f429c6d..8019ae165ec8c 100644
--- a/llvm/lib/Target/WebAssembly/MCTargetDesc/WebAssemblyMCTargetDesc.cpp
+++ b/llvm/lib/Target/WebAssembly/MCTargetDesc/WebAssemblyMCTargetDesc.cpp
@@ -36,35 +36,6 @@ using namespace llvm;
#define GET_REGINFO_MC_DESC
#include "WebAssemblyGenRegisterInfo.inc"
-// Exception handling & setjmp-longjmp handling related options.
-
-// Emscripten's asm.js-style exception handling
-cl::opt<bool> WebAssembly::WasmEnableEmEH(
- "enable-emscripten-cxx-exceptions",
- cl::desc("WebAssembly Emscripten-style exception handling"),
- cl::init(false));
-// Emscripten's asm.js-style setjmp/longjmp handling
-cl::opt<bool> WebAssembly::WasmEnableEmSjLj(
- "enable-emscripten-sjlj",
- cl::desc("WebAssembly Emscripten-style setjmp/longjmp handling"),
- cl::init(false));
-// Exception handling using wasm EH instructions
-cl::opt<bool>
- WebAssembly::WasmEnableEH("wasm-enable-eh",
- cl::desc("WebAssembly exception handling"));
-// setjmp/longjmp handling using wasm EH instrutions
-cl::opt<bool> WebAssembly::WasmEnableSjLj(
- "wasm-enable-sjlj", cl::desc("WebAssembly setjmp/longjmp handling"));
-// If true, use the legacy Wasm EH proposal:
-// https://github.com/WebAssembly/exception-handling/blob/main/proposals/exception-handling/legacy/Exceptions.md
-// And if false, use the standardized Wasm EH proposal:
-// https://github.com/WebAssembly/exception-handling/blob/main/proposals/exception-handling/Exceptions.md
-// Currently set to true by default because not all major web browsers turn on
-// the new standard proposal by default, but will later change to false.
-cl::opt<bool> WebAssembly::WasmUseLegacyEH(
- "wasm-use-legacy-eh", cl::desc("WebAssembly exception handling (legacy)"),
- cl::init(true));
-
static MCAsmInfo *createMCAsmInfo(const MCRegisterInfo & /*MRI*/,
const Triple &TT,
const MCTargetOptions &Options) {
diff --git a/llvm/lib/Target/WebAssembly/MCTargetDesc/WebAssemblyMCTargetDesc.h b/llvm/lib/Target/WebAssembly/MCTargetDesc/WebAssemblyMCTargetDesc.h
index d6a2fe4c7839d..fe9a4bada2430 100644
--- a/llvm/lib/Target/WebAssembly/MCTargetDesc/WebAssemblyMCTargetDesc.h
+++ b/llvm/lib/Target/WebAssembly/MCTargetDesc/WebAssemblyMCTargetDesc.h
@@ -39,13 +39,6 @@ createWebAssemblyWasmObjectWriter(bool Is64Bit, bool IsEmscripten);
namespace WebAssembly {
-// Exception handling / setjmp-longjmp handling command-line options
-extern cl::opt<bool> WasmEnableEmEH; // asm.js-style EH
-extern cl::opt<bool> WasmEnableEmSjLj; // asm.js-style SjLJ
-extern cl::opt<bool> WasmEnableEH; // EH using Wasm EH instructions
-extern cl::opt<bool> WasmEnableSjLj; // SjLj using Wasm EH instructions
-extern cl::opt<bool> WasmUseLegacyEH; // Legacy Wasm EH
-
enum OperandType {
/// Basic block label in a branch construct.
OPERAND_BASIC_BLOCK = MCOI::OPERAND_FIRST_TARGET,
diff --git a/llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp b/llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp
index 44a19e4baaf62..1bf070e9ec9c8 100644
--- a/llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp
+++ b/llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp
@@ -24,6 +24,7 @@
#include "WebAssemblyMachineFunctionInfo.h"
#include "WebAssemblyRegisterInfo.h"
#include "WebAssemblyRuntimeLibcallSignatures.h"
+#include "WebAssemblyTargetMachine.h"
#include "WebAssemblyUtilities.h"
#include "llvm/ADT/MapVector.h"
#include "llvm/ADT/SmallSet.h"
@@ -155,9 +156,11 @@ static std::string getEmscriptenInvokeSymbolName(wasm::WasmSignature *Sig) {
//===----------------------------------------------------------------------===//
MCSymbolWasm *WebAssemblyAsmPrinter::getMCSymbolForFunction(
- const Function *F, bool EnableEmEH, wasm::WasmSignature *Sig,
- bool &InvokeDetected) {
+ const Function *F, wasm::WasmSignature *Sig, bool &InvokeDetected) {
MCSymbolWasm *WasmSym = nullptr;
+
+ const bool EnableEmEH =
+ WebAssembly::WasmEnableEmEH || WebAssembly::WasmEnableEmSjLj;
if (EnableEmEH && isEmscriptenInvokeName(F->getName())) {
assert(Sig);
InvokeDetected = true;
@@ -344,9 +347,7 @@ void WebAssemblyAsmPrinter::emitDecls(const Module &M) {
// will discard it later if it turns out not to be necessary.
auto Signature = signatureFromMVTs(OutContext, Results, Params);
bool InvokeDetected = false;
- auto *Sym = getMCSymbolForFunction(
- &F, WebAssembly::WasmEnableEmEH || WebAssembly::WasmEnableEmSjLj,
- Signature, InvokeDetected);
+ auto *Sym = getMCSymbolForFunction(&F, Signature, InvokeDetected);
// Multiple functions can be mapped to the same invoke symbol. For
// example, two IR functions '__invoke_void_i8*' and '__invoke_void_i32'
@@ -404,7 +405,7 @@ void WebAssemblyAsmPrinter::emitEndOfAsmFile(Module &M) {
if (!F.isIntrinsic() && F.hasAddressTaken()) {
MCSymbolWasm *FunctionTable =
WebAssembly::getOrCreateFunctionTableSymbol(OutContext, Subtarget);
- OutStreamer->emitSymbolAttribute(FunctionTable, MCSA_NoDeadStrip);
+ OutStreamer->emitSymbolAttribute(FunctionTable, MCSA_NoDeadStrip);
break;
}
}
diff --git a/llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.h b/llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.h
index 46063bbe0fba1..aef562e1dd684 100644
--- a/llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.h
+++ b/llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.h
@@ -73,7 +73,7 @@ class LLVM_LIBRARY_VISIBILITY WebAssemblyAsmPrinter final : public AsmPrinter {
MVT getRegType(unsigned RegNo) const;
std::string regToString(const MachineOperand &MO);
WebAssemblyTargetStreamer *getTargetStreamer();
- MCSymbolWasm *getMCSymbolForFunction(const Function *F, bool EnableEmEH,
+ MCSymbolWasm *getMCSymbolForFunction(const Function *F,
wasm::WasmSignature *Sig,
bool &InvokeDetected);
MCSymbol *getOrCreateWasmSymbol(StringRef Name);
diff --git a/llvm/lib/Target/WebAssembly/WebAssemblyCFGStackify.cpp b/llvm/lib/Target/WebAssembly/WebAssemblyCFGStackify.cpp
index 640be5fe8e8c9..acb889f00a48c 100644
--- a/llvm/lib/Target/WebAssembly/WebAssemblyCFGStackify.cpp
+++ b/llvm/lib/Target/WebAssembly/WebAssemblyCFGStackify.cpp
@@ -21,13 +21,13 @@
///
//===----------------------------------------------------------------------===//
-#include "MCTargetDesc/WebAssemblyMCTargetDesc.h"
#include "Utils/WebAssemblyTypeUtilities.h"
#include "WebAssembly.h"
#include "WebAssemblyExceptionInfo.h"
#include "WebAssemblyMachineFunctionInfo.h"
#include "WebAssemblySortRegion.h"
#include "WebAssemblySubtarget.h"
+#include "WebAssemblyTargetMachine.h"
#include "WebAssemblyUtilities.h"
#include "llvm/ADT/Statistic.h"
#include "llvm/BinaryFormat/Wasm.h"
diff --git a/llvm/lib/Target/WebAssembly/WebAssemblyLateEHPrepare.cpp b/llvm/lib/Target/WebAssembly/WebAssemblyLateEHPrepare.cpp
index 254ad5c4f2beb..8ac32f939c5f2 100644
--- a/llvm/lib/Target/WebAssembly/WebAssemblyLateEHPrepare.cpp
+++ b/llvm/lib/Target/WebAssembly/WebAssemblyLateEHPrepare.cpp
@@ -11,9 +11,9 @@
///
//===----------------------------------------------------------------------===//
-#include "MCTargetDesc/WebAssemblyMCTargetDesc.h"
#include "WebAssembly.h"
#include "WebAssemblySubtarget.h"
+#include "WebAssemblyTargetMachine.h"
#include "WebAssemblyUtilities.h"
#include "llvm/ADT/SmallPtrSet.h"
#include "llvm/CodeGen/MachineFunctionPass.h"
diff --git a/llvm/lib/Target/WebAssembly/WebAssemblyMCInstLower.cpp b/llvm/lib/Target/WebAssembly/WebAssemblyMCInstLower.cpp
index d5240959e765e..cc36244e63ff5 100644
--- a/llvm/lib/Target/WebAssembly/WebAssemblyMCInstLower.cpp
+++ b/llvm/lib/Target/WebAssembly/WebAssemblyMCInstLower.cpp
@@ -77,9 +77,7 @@ WebAssemblyMCInstLower::GetGlobalAddressSymbol(const MachineOperand &MO) const {
auto Signature = signatureFromMVTs(Ctx, ResultMVTs, ParamMVTs);
bool InvokeDetected = false;
- auto *WasmSym = Printer.getMCSymbolForFunction(
- F, WebAssembly::WasmEnableEmEH || WebAssembly::WasmEnableEmSjLj,
- Signature, InvokeDetected);
+ auto *WasmSym = Printer.getMCSymbolForFunction(F, Signature, InvokeDetected);
WasmSym->setSignature(Signature);
WasmSym->setType(wasm::WASM_SYMBOL_TYPE_FUNCTION);
return WasmSym;
diff --git a/llvm/lib/Target/WebAssembly/WebAssemblyTargetMachine.cpp b/llvm/lib/Target/WebAssembly/WebAssemblyTargetMachine.cpp
index 378af22f4907c..ad47cb8ea2fe1 100644
--- a/llvm/lib/Target/WebAssembly/WebAssemblyTargetMachine.cpp
+++ b/llvm/lib/Target/WebAssembly/WebAssemblyTargetMachine.cpp
@@ -54,6 +54,35 @@ static cl::opt<bool> WasmDisableFixIrreducibleControlFlowPass(
" irreducible control flow optimization pass"),
cl::init(false));
+// Exception handling & setjmp-longjmp handling related options.
+
+// Emscripten's asm.js-style exception handling
+cl::opt<bool> WebAssembly::WasmEnableEmEH(
+ "enable-emscripten-cxx-exceptions",
+ cl::desc("WebAssembly Emscripten-style exception handling"),
+ cl::init(false));
+// Emscripten's asm.js-style setjmp/longjmp handling
+cl::opt<bool> WebAssembly::WasmEnableEmSjLj(
+ "enable-emscripten-sjlj",
+ cl::desc("WebAssembly Emscripten-style setjmp/longjmp handling"),
+ cl::init(false));
+// Exception handling using wasm EH instructions
+cl::opt<bool>
+ WebAssembly::WasmEnableEH("wasm-enable-eh",
+ cl::desc("WebAssembly exception handling"));
+// setjmp/longjmp handling using wasm EH instrutions
+cl::opt<bool> WebAssembly::WasmEnableSjLj(
+ "wasm-enable-sjlj", cl::desc("WebAssembly setjmp/longjmp handling"));
+// If true, use the legacy Wasm EH proposal:
+// https://github.com/WebAssembly/exception-handling/blob/main/proposals/exception-handling/legacy/Exceptions.md
+// And if false, use the standardized Wasm EH proposal:
+// https://github.com/WebAssembly/exception-handling/blob/main/proposals/exception-handling/Exceptions.md
+// Currently set to true by default because not all major web browsers turn on
+// the new standard proposal by default, but will later change to false.
+cl::opt<bool> WebAssembly::WasmUseLegacyEH(
+ "wasm-use-legacy-eh", cl::desc("WebAssembly exception handling (legacy)"),
+ cl::init(true));
+
extern "C" LLVM_ABI LLVM_EXTERNAL_VISIBILITY void
LLVMInitializeWebAssemblyTarget() {
// Register the target.
@@ -111,6 +140,57 @@ static Reloc::Model getEffectiveRelocModel(std::optional<Reloc::Model> RM,
return *RM;
}
+using WebAssembly::WasmEnableEH;
+using WebAssembly::WasmEnableEmEH;
+using WebAssembly::WasmEnableEmSjLj;
+using WebAssembly::WasmEnableSjLj;
+
+static void basicCheckForEHAndSjLj(TargetMachine *TM) {
+
+ // You can't enable two modes of EH at the same time
+ if (WasmEnableEmEH && WasmEnableEH)
+ report_fatal_error(
+ "-enable-emscripten-cxx-exceptions not allowed with -wasm-enable-eh");
+ // You can't enable two modes of SjLj at the same time
+ if (WasmEnableEmSjLj && WasmEnableSjLj)
+ report_fatal_error(
+ "-enable-emscripten-sjlj not allowed with -wasm-enable-sjlj");
+ // You can't mix Emscripten EH with Wasm SjLj.
+ if (WasmEnableEmEH && WasmEnableSjLj)
+ report_fatal_error(
+ "-enable-emscripten-cxx-exceptions not allowed with -wasm-enable-sjlj");
+
+ if (TM->Options.ExceptionModel == ExceptionHandling::None) {
+ // FIXME: These flags should be removed in favor of directly using the
+ // generically configured ExceptionsType
+ if (WebAssembly::WasmEnableEH || WebAssembly::WasmEnableSjLj)
+ TM->Options.ExceptionModel = ExceptionHandling::Wasm;
+ }
+
+ // Basic Correctness checking related to -exception-model
+ if (TM->Options.ExceptionModel != ExceptionHandling::None &&
+ TM->Options.ExceptionModel != ExceptionHandling::Wasm)
+ report_fatal_error("-exception-model should be either 'none' or 'wasm'");
+ if (WasmEnableEmEH && TM->Options.ExceptionModel == ExceptionHandling::Wasm)
+ report_fatal_error("-exception-model=wasm not allowed with "
+ "-enable-emscripten-cxx-exceptions");
+ if (WasmEnableEH && TM->Options.ExceptionModel != ExceptionHandling::Wasm)
+ report_fatal_error(
+ "-wasm-enable-eh only allowed with -exception-model=wasm");
+ if (WasmEnableSjLj && TM->Options.ExceptionModel != ExceptionHandling::Wasm)
+ report_fatal_error(
+ "-wasm-enable-sjlj only allowed with -exception-model=wasm");
+ if ((!WasmEnableEH && !WasmEnableSjLj) &&
+ TM->Options.ExceptionModel == ExceptionHandling::Wasm)
+ report_fatal_error(
+ "-exception-model=wasm only allowed with at least one of "
+ "-wasm-enable-eh or -wasm-enable-sjlj");
+
+ // Currently it is allowed to mix Wasm EH with Emscripten SjLj as an interim
+ // measure, but some code will error out at compile time in this combination.
+ // See WebAssemblyLowerEmscriptenEHSjLj pass for details.
+}
+
/// Create an WebAssembly architecture model.
///
WebAssemblyTargetMachine::WebAssemblyTargetMachine(
@@ -149,7 +229,7 @@ WebAssemblyTargetMachine::WebAssemblyTargetMachine(
this->Options.UniqueSectionNames = true;
initAsmInfo();
-
+ basicCheckForEHAndSjLj(this);
// Note that we don't use setRequiresStructuredCFG(true). It disables
// optimizations than we're ok with, and want, such as critical edge
// splitting and tail merging.
@@ -400,61 +480,6 @@ FunctionPass *WebAssemblyPassConfig::createTargetRegisterAllocator(bool) {
return nullptr; // No reg alloc
}
-using WebAssembly::WasmEnableEH;
-using WebAssembly::WasmEnableEmEH;
-using WebAssembly::WasmEnableEmSjLj;
-using WebAssembly::WasmEnableSjLj;
-
-static void basicCheckForEHAndSjLj(TargetMachine *TM) {
-
- // You can't enable two modes of EH at the same time
- if (WasmEnableEmEH && WasmEnableEH)
- report_fatal_error(
- "-enable-emscripten-cxx-exceptions not allowed with -wasm-enable-eh");
- // You can't enable two modes of SjLj at the same time
- if (WasmEnableEmSjLj && WasmEnableSjLj)
- report_fatal_error(
- "-enable-emscripten-sjlj not allowed with -wasm-enable-sjlj");
- // You can't mix Emscripten EH with Wasm SjLj.
- if (WasmEnableEmEH && WasmEnableSjLj)
- report_fatal_error(
- "-enable-emscripten-cxx-exceptions not allowed with -wasm-enable-sjlj");
-
- // Here we make sure TargetOptions.ExceptionModel is the same as
- // MCAsmInfo.ExceptionsType. Normally these have to be the same, because clang
- // stores the exception model info in LangOptions, which is later transferred
- // to TargetOptions and MCAsmInfo. But when clang compiles bitcode directly,
- // clang's LangOptions is not ...
[truncated]
|
arsenm
force-pushed
the
users/arsenm/webassembly/stop-overwriting-mcasminfo-exceptions-type
branch
from
9868f97 to
7ac1dc5
Compare
There was a problem hiding this comment.
The removal of MCAsmInfo hack here, which is possible thanks to #146342, looks good to me.
Also moving the basicCheckForEHAndSjLj call to WebAssemblyTargetMachine constructor makes sense. It caught tests that were in fact not using -wasm-enable-eh because they didn't run addIRPasses. Can you split this as a separate PR from the MCAsmInfo fix?
And it was a little confusing that the PR description argues we should remove -wasm-enable-eh/sjlj options, which I disagree, when the code seems to fix something else. Those flags control behaviors, not exception models, so we still need them. For example, someone might want to use only SjLj support but not EH support, but because we implement SjLj using EH constructs, we still need -exception-model=wasm and -wasm-enable-sjlj, but in this case not -wasm-enable-eh.
| @@ -1,4 +1,4 @@ | |||
| # RUN: llc -mtriple=wasm32-unknown-unknown -wasm-use-legacy-eh -exception-model=wasm -mattr=+exception-handling -run-pass wasm-cfg-stackify %s -o - | FileCheck %s | |||
| # RUN: llc -mtriple=wasm32-unknown-unknown -wasm-use-legacy-eh -wasm-enable-sjlj -exception-model=wasm -mattr=+exception-handling -run-pass wasm-cfg-stackify %s -o - | FileCheck %s | |||
There was a problem hiding this comment.
Why does this file need -wasm-enable-sjlj? This file does not use SjLj. I guess you meant -wasm-enable-eh? Ditto for the files below.
There was a problem hiding this comment.
The error checks either, so I just picked one
There was a problem hiding this comment.
They use EH but not SjLj, so I think it's better to use -wasm-enable-eh
There was a problem hiding this comment.
Changed it, but moved this to #146634
There should be no cl::opts that change backend behavior like this. It needs to be part of the triple, or at worst a module flag explicitly in the IR. This is worse than TargetOptions, since it's an ABI option hidden in the backend. We should not have program state hidden in structs outside of the IR. Additionally requiring this second flag on top of the -exception-model flag is an obnoxious user experience |
This was enforcing some rules about the combinations of flags at a random point in the IR pass pipeline configuration. This is a module property that should be handled at TargetMachine construction time at the latest. This required adding flags to a few mir and clang tests which never got this far to avoid hitting the errors. Split out from #146343
I don't understand. They just control code transformations, and every LLVM target has tons of them. And while they are exception-related flags, after this PR, they don't change the exception model.
|
It changes ABI, so it needs to be observable outside of the target. It changes the set of runtime libcalls required and used by the module.
The flag interactions here are unusual. Plus in general LLVM has far too many of these ad-hoc flags and it is a problem. cl::opts should be restricted to debug options, and not part of the backend interface to frontends. Compilation state that is not in the IR or triple-implied-ABI and only captured in command lines is easily lost and interferes with anything beyond the most basic compilation tasks. In this case I don't see why it can't directly interpret the standard -exception-model=none as disable, at least that reduces the number of side-channels to the one which already exists in TargetOptions
Developer quality of life is not unimportant. It's also an obstacle to other frontends. It's making command lines larger and requires test spam, as shown here |
@aheejin said above why -exception-model by itself isn't sufficient. We need to control SJLJ support separately from EH support. Additionally we need to allow for the "emscripten" mode of EH and SJLJ (where these are implemented as calls to JS). For that, IIRC without these flags I think we'd have to plumb "emscripten" as an exception model through from clang. I think we agree that having these backend flags and having them tightly coupled to exception-model the way they are isn't ideal, but when we originally implemented this, we didn't see any options as simple as "just use exception-model", and obviously any improvement over the status quo would have to keep all the modes working. You could also have just asked why things are the way they are, rather than assuming it's unnecessarily clunky and obnoxious for no good reason, which is definitely how you're coming across here. |
Currently wasm adds an extra level of options that work backwards from the standard options, and overwrites them. The ExceptionModel field in TM->Options is the standard user configuration option for the exception model to use. MCAsmInfo's ExceptionsType is a constant for the default to use for the triple if not explicitly set in the TargetOptions ExceptionModel. This was adding 2 custom flags, changing the MCAsmInfo default, and overwriting the ExceptionModel from the custom flags. These comments about compiling bitcode with clang are describing a toolchain bug or user error. TargetOptions is bad, and we should move to eliminating it. It is module state not captured in the IR. Ideally the exception model should either come implied from the triple, or a module flag and not depend on this side state. Currently it is the responsibility of the toolchain and/or user to ensure the same command line flags are used at each phase of the compilation. It is not the backend's responsibilty to try to second guess these options. -wasm-enable-eh and -wasm-enable-sjlj should also be removed in favor of the standard exception control. I'm a bit confused by how all of these fields are supposed to interact, but there are a few uses in the backend that are directly looking at these flags instead of the already parsed ExceptionModel which need to be cleaned up.
arsenm
force-pushed
the
users/arsenm/webassembly/stop-overwriting-mcasminfo-exceptions-type
branch
from
7ac1dc5 to
1a3e23f
Compare
Currently wasm adds an extra level of options that work backwards
from the standard options, and overwrites them. The ExceptionModel
field in TM->Options is the standard user configuration option for the
exception model to use. MCAsmInfo's ExceptionsType is a constant for the
default to use for the triple if not explicitly set in the TargetOptions
ExceptionModel. This was adding 2 custom flags, changing the MCAsmInfo
default, and overwriting the ExceptionModel from the custom flags.
These comments about compiling bitcode with clang are describing a toolchain
bug or user error. TargetOptions is bad, and we should move to eliminating it.
It is module state not captured in the IR. Ideally the exception model should either
come implied from the triple, or a module flag and not depend on this side state.
Currently it is the responsibility of the toolchain and/or user to ensure the same
command line flags are used at each phase of the compilation. It is not the backend's
responsibilty to try to second guess these options.
-wasm-enable-eh and -wasm-enable-sjlj should also be removed in favor of the standard
exception control. I'm a bit confused by how all of these fields are supposed to interact,
but there are a few uses in the backend that are directly looking at these flags instead
of the already parsed ExceptionModel which need to be cleaned up.
Additionally, this was enforcing some rules about the combinations of flags at a random
point in the IR pass pipeline configuration. This is a module property that should
be handled at TargetMachine construction time at the latest. This required adding flags
to a few mir and clang tests which never got this far to avoid hitting the errors.