[WebAssembly] Warn on exception spec for Emscripten EH

[aheejin]

Summary

It turns out we have not correctly supported exception spec all along in
Emscripten EH. Emscripten EH supports throw() but not throw with
types. See https://bugs.llvm.org/show_bug.cgi?id=50396.

Wasm EH also only supports throw() but not throw with types, and we
have been printing a warning message for the latter. This prints the
same warning message for throw with types when Emscripten EH is used,
or more precisely, when Wasm EH is not used. (So this will print the
warning messsage even when -fno-exceptions is used but I think that
should be fine. It's cumbersome to do a complilcated option checking in
CGException.cpp and options checkings are mostly done in elsewhere.)

[dschuff]

BTW Is there a way to disable this warning?
Since IIUC this could cause code that was not warning (because it used -fno-exceptions or used emscripten's default of -fignore-exceptions) to now start warning, sometimes that makes users who use -Werror unhappy.

clang/lib/CodeGen/CGException.cpp

508

Does emscripten's default of -fignore-exceptions also end up as haveing ExceptionHandlingKind::None even though exceptions aren't disabled?

[aheejin]

In D102791#2769892, @dschuff wrote:

BTW Is there a way to disable this warning?

The warning is like

test.cpp:3:6: warning: dynamic exception specifications with types are currently ignored in wasm [-Wwasm-exception-spec]

So -Wno-wasm-exception-spec will disable the warning.

Since IIUC this could cause code that was not warning (because it used -fno-exceptions or used emscripten's default of -fignore-exceptions) to now start warning, sometimes that makes users who use -Werror unhappy.

-fno-exceptions does not print this warning. -fignore-exceptions does. There is a way to disable this as I said above, but if this makes existing users unhappy, I can abandon this patch. It was just to give users a hint that this feature they are using is in fact not working.

clang/lib/CodeGen/CGException.cpp

508

Yes. This ExceptionHandlingKind has nothing to do with whether it is enabled or disabled. It is defined here: https://github.com/llvm/llvm-project/blob/a647100b4320923b0e9d156cc3872b3be470ad98/clang/include/clang/Basic/LangOptions.h#L233-L234

This is the clang counterpart to -exception-model in LLVM backend. If we don't use -fwasm-exceptions, ExceptionHandlingKind will be None whether Emscripten EH is enabled or not.

[kripken]

I'm not very familiar with this code, but it looks right, and sgtm to add a warning here.

[dschuff]

Got it, this makes sense to me too. And since it can be turned off, I'm not too worried about annoying users.

clang/lib/CodeGen/CGException.cpp

if (isNoexceptExceptionSpec(EST) && Proto->canThrow() == CT_Cannot) {
     // noexcept functions are simple terminate scopes.
     if (!getLangOpts().EHAsynch) // -EHa: HW exception still can occur
       EHStack.pushTerminate();
   } else if (EST == EST_Dynamic || EST == EST_DynamicNone) {
     // TODO: Revisit exception specifications for the MS ABI.  There is a way to
     // encode these in an object file but MSVC doesn't do anything with it.
     if (getTarget().getCXXABI().isMicrosoft())
       return;
-    // In wasm we currently treat 'throw()' in the same way as 'noexcept'. In
+    // In Wasm EH we currently treat 'throw()' in the same way as 'noexcept'. In
     // case of throw with types, we ignore it and print a warning for now.
-    // TODO Correctly handle exception specification in wasm
+    // TODO Correctly handle exception specification in Wasm EH
     if (CGM.getLangOpts().hasWasmExceptions()) {
       if (EST == EST_DynamicNone)
         EHStack.pushTerminate();
       else
         CGM.getDiags().Report(D->getLocation(),
                               diag::warn_wasm_dynamic_exception_spec_ignored)
             << FD->getExceptionSpecSourceRange();
       return;
     }
+    // Currently Emscripten EH only handles 'throw()' but not 'throw' with
+    // types. 'throw()' handling will be done in JS glue code so we don't need
+    // to do anything in that case. Just print a warning message in case of
+    // throw with types.
+    // TODO Correctly handle exception specification in Emscripten EH
+    if (getTarget().getCXXABI() == TargetCXXABI::WebAssembly &&
+        CGM.getLangOpts().getExceptionHandling() ==
+            LangOptions::ExceptionHandlingKind::None &&
+        EST == EST_Dynamic)
+      CGM.getDiags().Report(D->getLocation(),
+                            diag::warn_wasm_dynamic_exception_spec_ignored)
+          << FD->getExceptionSpecSourceRange();
+
     unsigned NumExceptions = Proto->getNumExceptions();
     EHFilterScope *Filter = EHStack.pushFilter(NumExceptions);
 
     for (unsigned I = 0; I != NumExceptions; ++I) {
       QualType Ty = Proto->getExceptionType(I);
       QualType ExceptType = Ty.getNonReferenceType().getUnqualifiedType();
       llvm::Value *EHType = CGM.GetAddrOfRTTIDescriptor(ExceptType,
                                                         /*ForEH=*/true);

clang/test/CodeGenCXX/wasm-eh.cpp

 // CHECK:   %[[CLEANUPPAD1:.*]] = cleanuppad within %[[CATCHPAD0]] []
 // CHECK:   cleanupret from %[[CLEANUPPAD1]] unwind to caller
 
 // CHECK:   unreachable
 
 // 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 -fexceptions -fcxx-exceptions -emit-llvm -o - -std=c++11 2>&1 | FileCheck %s --check-prefix=NOT-WASM-EH
+// 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.
 // This is controlled by -Wwasm-exception-spec, which is on by default. This
 // warning can be suppressed with -Wno-wasm-exception-spec. Checks if a warning
 // message is correctly printed or not printed depending on the options.
 void test9() throw(int) {
 }
 // WARNING-DEFAULT: warning: dynamic exception specifications with types are currently ignored in wasm
 // WARNING-ON: warning: dynamic exception specifications with types are currently ignored in wasm
 // WARNING-OFF-NOT: warning: dynamic exception specifications with types are currently ignored in wasm
-// NOT-WASM-EH-NOT: warning: dynamic exception specifications with types are currently ignored in wasm
+// EM-EH-WARNING: warning: dynamic exception specifications with types are currently ignored in wasm
 
 // Wasm curremtly treats 'throw()' in the same way as 'noexept'. Check if the
 // same warning message is printed as if when a 'noexcept' function throws.
 void test10() throw() {
   throw 3;
 }
 // WARNING-DEFAULT: warning: 'test10' has a non-throwing exception specification but can still throw
 // WARNING-DEFAULT: function declared non-throwing here