Deprecate DISABLE_EXCEPTION_CATCHING=2

We can instead imply this mode by the presence of the
EXCEPTION_CATCHING_ALLOWED` list.

This turns DISABLE_EXCEPTION_CATCHING into a binary option
which is easier to deal with both internally and externally.

Author: sbc100

ChangeLog.md

@@ -22,6 +22,11 @@ Current Trunk
 -------------
 - Values returned from `sysconf` now more closely match the definitions found in
   header files and in upstream musl (#13713).
+- `DISABLE_EXCEPTION_CATCHING=2` is now deprecated since it can be inferred from
+  the presence of the `EXCEPTION_CATCHING_ALLOWED` list.  This makes
+  `DISABLE_EXCEPTION_CATCHING` a simple binary option (0 or 1) which defaults to
+  0 which will be set to 1 internally if `EXCEPTION_CATCHING_ALLOWED` list is
+  specified.
 - Values returned from `pathconf` now match the definitions found in header files
   and/or upstream musl:
     _PC_LINK_MAX 3200 -> 8

emcc.py

@@ -751,7 +751,7 @@ def get_cflags(options, user_args):
 
   # if exception catching is disabled, we can prevent that code from being
   # generated in the frontend
-  if shared.Settings.DISABLE_EXCEPTION_CATCHING == 1 and not shared.Settings.EXCEPTION_HANDLING:
+  if shared.Settings.DISABLE_EXCEPTION_CATCHING and not shared.Settings.EXCEPTION_HANDLING:
     cflags.append('-fignore-exceptions')
 
   if shared.Settings.INLINING_LIMIT:
@@ -1588,6 +1588,18 @@ def default_setting(name, new_default):
       default_setting('ERROR_ON_UNDEFINED_SYMBOLS', 0)
       default_setting('WARN_ON_UNDEFINED_SYMBOLS', 0)
 
+    if 'DISABLE_EXCEPTION_CATCHING' in settings_key_changes and 'EXCEPTION_CATCHING_ALLOWED' in settings_key_changes:
+      # If we get here then the user specified both DISABLE_EXCEPTION_CATCHING and EXCEPTION_CATCHING_ALLOWED
+      # on the command line.  This is no longer valid so report either an error or a warning (for
+      # backwards compat with the old `DISABLE_EXCEPTION_CATCHING=2`
+      if settings_key_changes['DISABLE_EXCEPTION_CATCHING'] in ('0', '2'):
+        diagnostics.warning('deprecated', 'DISABLE_EXCEPTION_CATCHING=X is no longer needed when specifying EXCEPTION_CATCHING_ALLOWED')
+      else:
+        exit_with_error('DISABLE_EXCEPTION_CATCHING and EXCEPTION_CATCHING_ALLOWED are mutually exclusive')
+
+    if shared.Settings.EXCEPTION_CATCHING_ALLOWED:
+      shared.Settings.DISABLE_EXCEPTION_CATCHING = 0
+
     if shared.Settings.DISABLE_EXCEPTION_THROWING and not shared.Settings.DISABLE_EXCEPTION_CATCHING:
       exit_with_error("DISABLE_EXCEPTION_THROWING was set (probably from -fno-exceptions) but is not compatible with enabling exception catching (DISABLE_EXCEPTION_CATCHING=0). If you don't want exceptions, set DISABLE_EXCEPTION_CATCHING to 1; if you do want exceptions, don't link with -fno-exceptions")
 
@@ -1640,7 +1652,7 @@ def default_setting(name, new_default):
 
     if shared.Settings.USE_PTHREADS:
       if shared.Settings.USE_PTHREADS == 2:
-        exit_with_error('USE_PTHREADS=2 is not longer supported')
+        exit_with_error('USE_PTHREADS=2 is no longer supported')
       if shared.Settings.ALLOW_MEMORY_GROWTH:
         diagnostics.warning('pthreads-mem-growth', 'USE_PTHREADS + ALLOW_MEMORY_GROWTH may run non-wasm code slowly, see https://github.com/WebAssembly/design/issues/1271')
       # UTF8Decoder.decode may not work with a view of a SharedArrayBuffer, see https://github.com/whatwg/encoding/issues/172
@@ -1965,7 +1977,7 @@ def check_memory_setting(setting):
        shared.Settings.USE_PTHREADS or \
        shared.Settings.OFFSCREENCANVAS_SUPPORT or \
        shared.Settings.LEGACY_GL_EMULATION or \
-       shared.Settings.DISABLE_EXCEPTION_CATCHING != 1 or \
+       not shared.Settings.DISABLE_EXCEPTION_CATCHING or \
        shared.Settings.ASYNCIFY or \
        shared.Settings.ASMFS or \
        shared.Settings.DEMANGLE_SUPPORT or \
@@ -1978,7 +1990,7 @@ def check_memory_setting(setting):
        sanitize:
       shared.Settings.EXPORTED_FUNCTIONS += ['_malloc', '_free']
 
-    if shared.Settings.DISABLE_EXCEPTION_CATCHING != 1:
+    if not shared.Settings.DISABLE_EXCEPTION_CATCHING:
       # If not for LTO builds, we could handle these by adding deps_info.py
       # entries for __cxa_find_matching_catch_* functions.  However, under
       # LTO these symbols don't exist prior the linking.

src/parseTools.js

@@ -873,8 +873,8 @@ function makeStructuralReturn(values) {
 }
 
 function makeThrow(what) {
-  if (ASSERTIONS && DISABLE_EXCEPTION_CATCHING == 1) {
-    what += ' + " - Exception catching is disabled, this exception cannot be caught. Compile with -s DISABLE_EXCEPTION_CATCHING=0 or DISABLE_EXCEPTION_CATCHING=2 to catch."';
+  if (ASSERTIONS && DISABLE_EXCEPTION_CATCHING) {
+    what += ' + " - Exception catching is disabled, this exception cannot be caught. Compile with -s NO_DISABLE_EXCEPTION_CATCHING or -s EXCEPTION_CATCHING_ALLOWED=[..] to catch."';
     if (MAIN_MODULE) {
       what += ' + " (note: in dynamic linking, if a side module wants exceptions, the main module must be built with that support)"';
     }

src/preamble.js

@@ -658,11 +658,12 @@ function createExportWrapper(name, fixedasm) {
 #endif
 
 #if ABORT_ON_WASM_EXCEPTIONS
-// When DISABLE_EXCEPTION_CATCHING != 1 `abortWrapperDepth` counts the recursion
-// level of the wrapper function so that we only handle exceptions at the top level
-// letting the exception mechanics work uninterrupted at the inner level.
-// Additionally, `abortWrapperDepth` is also manually incremented in callMain so that
-// we know to ignore exceptions from there since they're handled by callMain directly.
+// When exception catching is enabled (!DISABLE_EXCEPTION_CATCHING)
+// `abortWrapperDepth` counts the recursion level of the wrapper function so
+// that we only handle exceptions at the top level letting the exception
+// mechanics work uninterrupted at the inner level.  Additionally,
+// `abortWrapperDepth` is also manually incremented in callMain so that we know
+// to ignore exceptions from there since they're handled by callMain directly.
 var abortWrapperDepth = 0;
 
 // Creates a wrapper in a closure so that each wrapper gets it's own copy of 'original'
@@ -673,7 +674,7 @@ function makeAbortWrapper(original) {
       throw "program has already aborted!";
     }
 
-#if DISABLE_EXCEPTION_CATCHING != 1
+#if !DISABLE_EXCEPTION_CATCHING
     abortWrapperDepth += 1;
 #endif
     try {
@@ -692,7 +693,7 @@ function makeAbortWrapper(original) {
 
       abort("unhandled exception: " + [e, e.stack]);
     }
-#if DISABLE_EXCEPTION_CATCHING != 1
+#if !DISABLE_EXCEPTION_CATCHING
     finally {
       abortWrapperDepth -= 1;
     }

src/settings.js

@@ -643,22 +643,23 @@ var LZ4 = 0;
 // currently (in the future, wasm should improve that). When exceptions are
 // disabled, if an exception actually happens then it will not be caught
 // and the program will halt (so this will not introduce silent failures).
-// There are 3 specific modes here:
-// DISABLE_EXCEPTION_CATCHING = 0 - generate code to actually catch exceptions
-// DISABLE_EXCEPTION_CATCHING = 1 - disable exception catching at all
-// DISABLE_EXCEPTION_CATCHING = 2 - disable exception catching, but enables
-//                                  catching in list of allowed functions
+//
 // XXX note that this removes *catching* of exceptions, which is the main
 //     issue for speed, but you should build source files with
 //     -fno-exceptions to really get rid of all exceptions code overhead,
 //     as it may contain thrown exceptions that are never caught (e.g.
 //     just using std::vector can have that). -fno-rtti may help as well.
 //
+// This option is mutually exclusive with EXCEPTION_CATCHING_ALLOWED.
+//
 // [compile+link] - affects user code at compile and system libraries at link
 var DISABLE_EXCEPTION_CATCHING = 1;
 
-// Enables catching exception in the listed functions only, if
-// DISABLE_EXCEPTION_CATCHING = 2 is set
+// Enables catching exception but only in the listed functions.  This
+// option acts like a more precise version of `DISABLE_EXCEPTION_CATCHING=0`.
+//
+// This option is mutually exclusive with DISABLE_EXCEPTION_CATCHING.
+//
 // [compile+link] - affects user code at compile and system libraries at link
 var EXCEPTION_CATCHING_ALLOWED = [];
 

tests/test_core.py

@@ -1236,7 +1236,6 @@ def test_exceptions_3(self):
     self.do_run('src.js', 'Caught exception: Hello\nDone.', args=['2'], no_build=True)
 
   def test_exceptions_allowed(self):
-    self.set_setting('DISABLE_EXCEPTION_CATCHING', 2)
     # Wasm does not add an underscore to function names. For wasm, the
     # mismatches are fixed in fixImports() function in JS glue code.
     self.set_setting('EXCEPTION_CATCHING_ALLOWED', ["_Z12somefunctionv"])
@@ -1267,7 +1266,7 @@ def test_exceptions_allowed(self):
       fake_size += os.path.getsize('test_exceptions_allowed.wasm')
     shutil.copyfile('test_exceptions_allowed.js', 'fake.js')
 
-    self.set_setting('DISABLE_EXCEPTION_CATCHING')
+    self.clear_setting('EXCEPTION_CATCHING_ALLOWED')
     self.do_run_from_file(src, empty_output, assert_returncode=NON_ZERO)
     disabled_size = os.path.getsize('test_exceptions_allowed.js')
     if self.is_wasm():
@@ -1278,33 +1277,55 @@ def test_exceptions_allowed(self):
     print('empty_size: %d' % empty_size)
     print('fake_size: %d' % fake_size)
     print('disabled_size: %d' % disabled_size)
-    self.assertEqual(empty_size, fake_size)
+    # empty list acts the same as fully disabled
+    self.assertEqual(empty_size, disabled_size)
     # big change when we disable exception catching of the function
     self.assertGreater(size - empty_size, 0.01 * size)
     # full disable can remove a little bit more
-    self.assertLess(disabled_size, empty_size)
+    self.assertLess(disabled_size, fake_size)
 
   def test_exceptions_allowed_2(self):
-    self.set_setting('DISABLE_EXCEPTION_CATCHING', 2)
     # Wasm does not add an underscore to function names. For wasm, the
     # mismatches are fixed in fixImports() function in JS glue code.
-    self.set_setting('EXCEPTION_CATCHING_ALLOWED', ["main"])
+    self.set_setting('EXCEPTION_CATCHING_ALLOWED', ['main'])
     # otherwise it is inlined and not identified
     self.set_setting('INLINING_LIMIT')
 
     self.do_core_test('test_exceptions_allowed_2.cpp')
 
   def test_exceptions_allowed_uncaught(self):
     self.emcc_args += ['-std=c++11']
-    self.set_setting('DISABLE_EXCEPTION_CATCHING', 2)
     # Wasm does not add an underscore to function names. For wasm, the
     # mismatches are fixed in fixImports() function in JS glue code.
-    self.set_setting('EXCEPTION_CATCHING_ALLOWED', ["_Z4testv"])
+    self.set_setting('EXCEPTION_CATCHING_ALLOWED', ['_Z4testv'])
     # otherwise it is inlined and not identified
     self.set_setting('INLINING_LIMIT')
 
     self.do_core_test('test_exceptions_allowed_uncaught.cpp')
 
+  def test_exceptions_allowed_misuse(self):
+    self.set_setting('EXCEPTION_CATCHING_ALLOWED', ['foo'])
+
+    # Test old =2 setting for DISABLE_EXCEPTION_CATCHING
+    self.set_setting('DISABLE_EXCEPTION_CATCHING', 2)
+    err = self.expect_fail([EMCC, test_file('hello_world.c')] + self.get_emcc_args())
+    self.assertContained('error: DISABLE_EXCEPTION_CATCHING=X is no longer needed when specifying EXCEPTION_CATCHING_ALLOWED [-Wdeprecated] [-Werror]', err)
+
+    # =0 should also be a warning
+    self.set_setting('DISABLE_EXCEPTION_CATCHING', 0)
+    err = self.expect_fail([EMCC, test_file('hello_world.c')] + self.get_emcc_args())
+    self.assertContained('error: DISABLE_EXCEPTION_CATCHING=X is no longer needed when specifying EXCEPTION_CATCHING_ALLOWED [-Wdeprecated] [-Werror]', err)
+
+    # =1 should be a hard error
+    self.set_setting('DISABLE_EXCEPTION_CATCHING', 1)
+    err = self.expect_fail([EMCC, test_file('hello_world.c')] + self.get_emcc_args())
+    self.assertContained('error: DISABLE_EXCEPTION_CATCHING and EXCEPTION_CATCHING_ALLOWED are mutually exclusive', err)
+
+    # even setting an empty list should trigger the error;
+    self.set_setting('EXCEPTION_CATCHING_ALLOWED', [])
+    err = self.expect_fail([EMCC, test_file('hello_world.c')] + self.get_emcc_args())
+    self.assertContained('error: DISABLE_EXCEPTION_CATCHING and EXCEPTION_CATCHING_ALLOWED are mutually exclusive', err)
+
   @with_both_exception_handling
   def test_exceptions_uncaught(self):
     # needs to flush stdio streams

tools/building.py

@@ -483,10 +483,10 @@ def llvm_backend_args():
   args = ['-combiner-global-alias-analysis=false']
 
   # asm.js-style exception handling
-  if Settings.DISABLE_EXCEPTION_CATCHING != 1:
+  if not Settings.DISABLE_EXCEPTION_CATCHING:
     args += ['-enable-emscripten-cxx-exceptions']
-  if Settings.DISABLE_EXCEPTION_CATCHING == 2:
-    allowed = ','.join(Settings.EXCEPTION_CATCHING_ALLOWED or ['__fake'])
+  if Settings.EXCEPTION_CATCHING_ALLOWED:
+    allowed = ','.join(Settings.EXCEPTION_CATCHING_ALLOWED)
     args += ['-emscripten-cxx-exceptions-allowed=' + allowed]
 
   if Settings.SUPPORT_LONGJMP: