[clang] [WebAssembly] Make EH depend on multivalue and reference-types (PR #91299)

2024-05-07 Thread Heejin Ahn via cfe-commits 

https://github.com/aheejin closed 
https://github.com/llvm/llvm-project/pull/91299
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [WebAssembly] Make EH depend on multivalue and reference-types (PR #91299)

2024-05-07 Thread Heejin Ahn via cfe-commits 

aheejin wrote:

According to https://webassembly.org/features/, EH is supported from node v17, 
which is also Emscripten test infra requires: 
https://github.com/emscripten-core/emscripten/blob/2fefc1911e2e6265174c3fd5da38c8e9ab7abb60/test/common.py#L866-L868

So EH tests should have been already being skipped in Chromium CI, so I don't 
think the same problem will happen there.

https://github.com/llvm/llvm-project/pull/91299
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [WebAssembly] Make EH depend on multivalue and reference-types (PR #91299)

2024-05-07 Thread Derek Schuff via cfe-commits 

https://github.com/dschuff approved this pull request.

LGTM.
Are we currently running wasm-eh tests on the Chromium CI with node 16? If so, 
I guess this will have the same issue we had when we tried to turn it on by 
default.
I don't think this needs to be a showstopper in the same way (since this isn't 
the default output of emscripten that we're talking about here, so I think it's 
fine to say "using EH requires node 17+"). But we may have to either disable 
some tests on chromium CI, or just make the test runner add a flag.

https://github.com/llvm/llvm-project/pull/91299
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [WebAssembly] Make EH depend on multivalue and reference-types (PR #91299)

2024-05-06 Thread via cfe-commits 

llvmbot wrote:




@llvm/pr-subscribers-clang

Author: Heejin Ahn (aheejin)


Changes

This PR turns on multivalue and reference-types features when 
exception-handling feature is turned on, and errors out when disabling of those 
dependent features is explicitly requested.

I think doing this would be safe anyway regardless of whether or when we end up 
turning on reference-types by default.

We currently don't yet have a experimental flag for the Clang and LLVM for the 
new experimental EH yet. But I think it should be fine to turn those features 
on even if the LLVM does not yet generate the new EH instructions, for the same 
reason we tried to turn them on by default and the browsers that support EH 
also support multivalue and reference-types anyway.

---
Full diff: https://github.com/llvm/llvm-project/pull/91299.diff


2 Files Affected:

- (modified) clang/lib/Driver/ToolChains/WebAssembly.cpp (+34) 
- (modified) clang/test/Driver/wasm-toolchain.c (+33-6) 


``diff
diff --git a/clang/lib/Driver/ToolChains/WebAssembly.cpp 
b/clang/lib/Driver/ToolChains/WebAssembly.cpp
index b7c6efab83e80..5b763df9b3329 100644
--- a/clang/lib/Driver/ToolChains/WebAssembly.cpp
+++ b/clang/lib/Driver/ToolChains/WebAssembly.cpp
@@ -347,6 +347,23 @@ void WebAssembly::addClangTargetOptions(const ArgList 
,
 // 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.
+if (DriverArgs.hasFlag(options::OPT_mno_multivalue,
+   options::OPT_mmultivalue, false)) {
+  getDriver().Diag(diag::err_drv_argument_not_allowed_with)
+  << "-fwasm-exceptions" << "-mno-multivalue";
+}
+if (DriverArgs.hasFlag(options::OPT_mno_reference_types,
+   options::OPT_mreference_types, false)) {
+  getDriver().Diag(diag::err_drv_argument_not_allowed_with)
+  << "-fwasm-exceptions" << "-mno-reference-types";
+}
+CC1Args.push_back("-target-feature");
+CC1Args.push_back("+multivalue");
+CC1Args.push_back("-target-feature");
+CC1Args.push_back("+reference-types");
   }
 
   for (const Arg *A : DriverArgs.filtered(options::OPT_mllvm)) {
@@ -408,6 +425,23 @@ void WebAssembly::addClangTargetOptions(const ArgList 
,
   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 dabf0ac2433bb..7c26c2c13c0ba 100644
--- a/clang/test/Driver/wasm-toolchain.c
+++ b/clang/test/Driver/wasm-toolchain.c
@@ -120,11 +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 and '-mllvm -wasm-enable-eh'
+// '-fwasm-exceptions' sets +exception-handling, -multivalue, -reference-types
+// 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"
+// 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 \
@@ -132,19 +133,32 @@
 // 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 '-mllvm 
-enable-emscripten-cxx-exceptions'
+// '-fwasm-exceptions' not allowed with
+// '-mllvm -enable-emscripten-cxx-exceptions'
 //

[clang] [WebAssembly] Make EH depend on multivalue and reference-types (PR #91299)

2024-05-06 Thread Heejin Ahn via cfe-commits 

https://github.com/aheejin created 
https://github.com/llvm/llvm-project/pull/91299

This PR turns on multivalue and reference-types features when 
exception-handling feature is turned on, and errors out when disabling of those 
dependent features is explicitly requested.

I think doing this would be safe anyway regardless of whether or when we end up 
turning on reference-types by default.

We currently don't yet have a experimental flag for the Clang and LLVM for the 
new experimental EH yet. But I think it should be fine to turn those features 
on even if the LLVM does not yet generate the new EH instructions, for the same 
reason we tried to turn them on by default and the browsers that support EH 
also support multivalue and reference-types anyway.

>From 527209b600696885ab926cbf09c88feed8a18544 Mon Sep 17 00:00:00 2001
From: Heejin Ahn 
Date: Tue, 7 May 2024 05:31:24 +
Subject: [PATCH] [WebAssembly] Make EH depend on multivalue and
 reference-types

This PR turns on multivalue and reference-types features when
exception-handling feature is turned on, and errors out when disabling
of those dependent features is explicitly requested.

I think doing this would be safe anyway regardless of whether or when we
end up turning on reference-types by default.

We currently don't yet have a experimental flag for the Clang and LLVM
for the new experimental EH yet. But I think it should be fine to turn
those features on even if the LLVM does not yet generate the new EH
instructions, for the same reason we tried to turn them on by default
and the browsers that support EH also support multivalue and
reference-types anyway.
---
 clang/lib/Driver/ToolChains/WebAssembly.cpp | 34 ++
 clang/test/Driver/wasm-toolchain.c  | 39 +
 2 files changed, 67 insertions(+), 6 deletions(-)

diff --git a/clang/lib/Driver/ToolChains/WebAssembly.cpp 
b/clang/lib/Driver/ToolChains/WebAssembly.cpp
index b7c6efab83e806..5b763df9b33293 100644
--- a/clang/lib/Driver/ToolChains/WebAssembly.cpp
+++ b/clang/lib/Driver/ToolChains/WebAssembly.cpp
@@ -347,6 +347,23 @@ void WebAssembly::addClangTargetOptions(const ArgList 
,
 // 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.
+if (DriverArgs.hasFlag(options::OPT_mno_multivalue,
+   options::OPT_mmultivalue, false)) {
+  getDriver().Diag(diag::err_drv_argument_not_allowed_with)
+  << "-fwasm-exceptions" << "-mno-multivalue";
+}
+if (DriverArgs.hasFlag(options::OPT_mno_reference_types,
+   options::OPT_mreference_types, false)) {
+  getDriver().Diag(diag::err_drv_argument_not_allowed_with)
+  << "-fwasm-exceptions" << "-mno-reference-types";
+}
+CC1Args.push_back("-target-feature");
+CC1Args.push_back("+multivalue");
+CC1Args.push_back("-target-feature");
+CC1Args.push_back("+reference-types");
   }
 
   for (const Arg *A : DriverArgs.filtered(options::OPT_mllvm)) {
@@ -408,6 +425,23 @@ void WebAssembly::addClangTargetOptions(const ArgList 
,
   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 dabf0ac2433bbb..7c26c2c13c0baf 100644
--- a/clang/test/Driver/wasm-toolchain.c
+++ b/clang/test/Driver/wasm-toolchain.c
@@ -120,11 +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 and '-mllvm -wasm-enable-eh'
+// '-fwasm-exceptions' sets +exception-handling, -multivalue, -reference-types
+// and '-mllvm -wasm-enable-eh'
 // RUN: %clang -### --target=wasm32-unknown-unknown \
 // RUN:--sysroot=/foo %s