Asyncify: Support for new add-list, and updates for binaryen list changes

ChangeLog.md

@@ -17,6 +17,16 @@ See docs/process.md for how version tagging works.
 
 Current Trunk
 -------------
+- Add support for the new `ASYNCIFY_ADD_LIST`, and update existing list names
+  following the updates in Binaryen, so that now we have `ASYNCIFY_ADD_LIST` to
+  add a function, `ASYNCIFY_REMOVE_LIST` to remove one (previously this was
+  called `ASYNCIFY_BLACKLIST`), and `ASYNCIFY_ONLY_LIST` to set a list of the
+  only functions to instrument and no others (previously this was called
+  `ASYNCIFY_WHITELIST`). The updated lists also handle indirect calls properly,
+  so that if you use `ASYNCIFY_IGNORE_INDIRECT` and then add (using either the
+  add-list or the only-list) all the functions that are on the stack when
+  pausing, then things will work (for more, see
+  https://github.com/WebAssembly/binaryen/pull/2913).
 
 1.39.18: 06/12/2020
 -------------------

emcc.py

@@ -656,7 +656,6 @@ def backend_binaryen_passes():
     # be able to whitelist them etc.
     passes += ['--fpcast-emu']
   if shared.Settings.ASYNCIFY:
-    # TODO: allow whitelist as in asyncify
     passes += ['--asyncify']
     if shared.Settings.ASSERTIONS:
       passes += ['--pass-arg=asyncify-asserts']
@@ -694,15 +693,18 @@ def check_human_readable_list(items):
           logger.warning('''emcc: ASYNCIFY list contains an item without balanced parentheses ("(", ")"):''')
           logger.warning('''   ''' + item)
           logger.warning('''This may indicate improper escaping that led to splitting inside your names.''')
-          logger.warning('''Try to quote the entire argument, like this: -s 'ASYNCIFY_WHITELIST=["foo(int, char)", "bar"]' ''')
+          logger.warning('''Try to quote the entire argument, like this: -s 'ASYNCIFY_ONLY_LIST=["foo(int, char)", "bar"]' ''')
           break
 
-    if shared.Settings.ASYNCIFY_BLACKLIST:
-      check_human_readable_list(shared.Settings.ASYNCIFY_BLACKLIST)
-      passes += ['--pass-arg=asyncify-blacklist@%s' % ','.join(shared.Settings.ASYNCIFY_BLACKLIST)]
-    if shared.Settings.ASYNCIFY_WHITELIST:
-      check_human_readable_list(shared.Settings.ASYNCIFY_WHITELIST)
-      passes += ['--pass-arg=asyncify-whitelist@%s' % ','.join(shared.Settings.ASYNCIFY_WHITELIST)]
+    if shared.Settings.ASYNCIFY_REMOVE_LIST:
+      check_human_readable_list(shared.Settings.ASYNCIFY_REMOVE_LIST)
+      passes += ['--pass-arg=asyncify-removelist@%s' % ','.join(shared.Settings.ASYNCIFY_REMOVE_LIST)]
+    if shared.Settings.ASYNCIFY_ADD_LIST:
+      check_human_readable_list(shared.Settings.ASYNCIFY_ADD_LIST)
+      passes += ['--pass-arg=asyncify-addlist@%s' % ','.join(shared.Settings.ASYNCIFY_ADD_LIST)]
+    if shared.Settings.ASYNCIFY_ONLY_LIST:
+      check_human_readable_list(shared.Settings.ASYNCIFY_ONLY_LIST)
+      passes += ['--pass-arg=asyncify-onlylist@%s' % ','.join(shared.Settings.ASYNCIFY_ONLY_LIST)]
   if shared.Settings.BINARYEN_IGNORE_IMPLICIT_TRAPS:
     passes += ['--ignore-implicit-traps']
 
@@ -3177,7 +3179,7 @@ def do_binaryen(target, asm_target, options, memfile, wasm_binary_target,
   debug_info = shared.Settings.DEBUG_LEVEL >= 2 or options.profiling_funcs
   # whether we need to emit -g in the intermediate binaryen invocations (but not necessarily at the very end).
   # this is necessary for emitting a symbol map at the end.
-  intermediate_debug_info = bool(debug_info or options.emit_symbol_map or shared.Settings.ASYNCIFY_WHITELIST or shared.Settings.ASYNCIFY_BLACKLIST)
+  intermediate_debug_info = bool(debug_info or options.emit_symbol_map or shared.Settings.ASYNCIFY_ONLY_LIST or shared.Settings.ASYNCIFY_REMOVE_LIST or shared.Settings.ASYNCIFY_ADD_LIST)
   emit_symbol_map = options.emit_symbol_map or shared.Settings.CYBERDWARF
   # finish compiling to WebAssembly, using asm2wasm, if we didn't already emit WebAssembly directly using the wasm backend.
   if not shared.Settings.WASM_BACKEND:

emscripten.py

@@ -2092,7 +2092,7 @@ def finalize_wasm(temp_files, infile, outfile, memfile, DEBUG):
 
   # tell binaryen to look at the features section, and if there isn't one, to use MVP
   # (which matches what llvm+lld has given us)
-  if shared.Settings.DEBUG_LEVEL >= 2 or shared.Settings.PROFILING_FUNCS or shared.Settings.EMIT_SYMBOL_MAP or shared.Settings.ASYNCIFY_WHITELIST or shared.Settings.ASYNCIFY_BLACKLIST:
+  if shared.Settings.DEBUG_LEVEL >= 2 or shared.Settings.PROFILING_FUNCS or shared.Settings.EMIT_SYMBOL_MAP or shared.Settings.ASYNCIFY_ONLY_LIST or shared.Settings.ASYNCIFY_REMOVE_LIST or shared.Settings.ASYNCIFY_ADD_LIST:
     args.append('-g')
   if shared.Settings.WASM_BIGINT:
     args.append('--bigint')

site/source/docs/porting/asyncify.rst

@@ -243,11 +243,15 @@ you can tell Asyncify to ignore indirect calls using
 If you know that some indirect calls matter and others do not, then you
 can provide a manual list of functions to Asyncify:
 
-* ``ASYNCIFY_BLACKLIST`` is a list of functions that do not unwind the stack.
-  Asyncify will do it's normal whole-program analysis under the assumption
-  that those do not unwind.
-* ``ASYNCIFY_WHITELIST`` is a list of the **only** functions that can unwind
-  the stack. Asyncify will instrument those and no others.
+* ``ASYNCIFY_REMOVE_LIST`` is a list of functions that do not unwind the stack.
+  Asyncify will do its normal whole-program analysis, then remove these
+  functions from the list of instrumented functions.
+* ``ASYNCIFY_ADD_LIST`` is a list of functions that do unwind the stack, and
+  are added after doing the normal whole-program analysis. This is mostly useful
+  if you use ``ASYNCIFY_IGNORE_INDIRECT`` but want to also mark some additional
+  functions that need to unwind.
+* ``ASYNCIFY_ONLY_LIST`` is a list of the **only** functions that can unwind
+  the stack. Asyncify will instrument exactly those and no others.
 
 For more details see ``settings.js``. Note that the manual settings
 mentioned here are error-prone - if you don't get things exactly right,

src/settings.js

@@ -691,7 +691,7 @@ var NODEJS_CATCH_REJECTION = 1;
 // This allows to inject some async functions to the C code that appear to be sync
 // e.g. emscripten_sleep
 // On fastcomp this uses the Asyncify IR transform.
-// On upstream this uses the Asyncify pass in Binaryen. TODO: whitelist, coroutines
+// On upstream this uses the Asyncify pass in Binaryen.
 var ASYNCIFY = 0;
 
 // Imports which can do a sync operation, in addition to the default ones that
@@ -714,7 +714,7 @@ var ASYNCIFY_IGNORE_INDIRECT = 0;
 // In that case, you should increase this size.
 var ASYNCIFY_STACK_SIZE = 4096;
 
-// If the Asyncify blacklist is provided, then the functions in it will not
+// If the Asyncify remove-list is provided, then the functions in it will not
 // be instrumented even if it looks like they need to. This can be useful
 // if you know things the whole-program analysis doesn't, like if you
 // know certain indirect calls are safe and won't unwind. But if you
@@ -735,13 +735,22 @@ var ASYNCIFY_STACK_SIZE = 4096;
 // builds, etc.). You can inspect the wasm binary to look for the actual names,
 // either directly or using wasm-objdump or wasm-dis, etc.
 // Simple '*' wildcard matching is supported.
-var ASYNCIFY_BLACKLIST = [];
-
-// If the Asyncify whitelist is provided, then *only* the functions in the list
-// will be instrumented. Like the blacklist, getting this wrong will break
+var ASYNCIFY_REMOVE_LIST = [];
+
+// Functions in the Asyncify add-list are added to the list of instrumented
+// functions, that is, they will be instrumented even if otherwise asyncify
+// thinks they don't need to be. As by default everything will be instrumented
+// in the safest way possible, this is only useful if you use IGNORE_INDIRECT
+// and use this list to fix up some indirect calls that *do* need to be
+// instrumented.
+// See notes on ASYNCIFY_REMOVE_LIST about the names.
+var ASYNCIFY_ADD_LIST = [];
+
+// If the Asyncify only-list is provided, then *only* the functions in the list
+// will be instrumented. Like the remove-list, getting this wrong will break
 // your application.
-// See notes on ASYNCIFY_BLACKLIST about the names.
-var ASYNCIFY_WHITELIST = [];
+// See notes on ASYNCIFY_REMOVE_LIST about the names.
+var ASYNCIFY_ONLY_LIST = [];
 
 // Allows lazy code loading: where emscripten_lazy_load_code() is written, we
 // will pause execution, load the rest of the code, and then resume.
@@ -1817,4 +1826,6 @@ var LEGACY_SETTINGS = [
   ['BINARYEN_MEM_MAX', 'MAXIMUM_MEMORY'],
   ['BINARYEN_PASSES', [''], 'Use BINARYEN_EXTRA_PASSES to add additional passes'],
   ['SWAPPABLE_ASM_MODULE', [0], 'Fully swappable asm modules are no longer supported'],
+  ['ASYNCIFY_WHITELIST', 'ASYNCIFY_ONLY_LIST'],
+  ['ASYNCIFY_BLACKLIST', 'ASYNCIFY_REMOVE_LIST'],
 ];

tests/core/test_asyncify_indirect_lists.cpp

@@ -0,0 +1,33 @@
+#include <assert.h>
+#include <stdio.h>
+#include <emscripten.h>
+
+int x = 0;
+
+struct Structy {
+  virtual int virty() {
+    if (x == 1337) return virty(); // don't inline me
+    emscripten_sleep(1);
+    return 42;
+  }
+};
+
+Structy y;
+
+__attribute__((noinline))
+void virt() {
+  if (x == 1337) {
+    // don't inline me
+    virt();
+  }
+  Structy *z = &y;
+  printf("virt: %d\n", z->virty()); // but the indirect call itself!
+}
+
+int main() {
+  EM_ASM({
+    Module.counter = (Module.counter || 0) + 1;
+    if (Module.counter > 10) throw "infinite loop due to asyncify bug?";
+  });
+  virt();
+}

tests/core/test_asyncify_indirect_lists.txt

@@ -0,0 +1 @@
+virt: 42

tests/test_browser.py

@@ -3307,8 +3307,8 @@ def test_async_stack_overflow(self):
     self.btest('browser/async_stack_overflow.cpp', '0', args=['-s', 'ASYNCIFY', '-s', 'ASYNCIFY_STACK_SIZE=4'])
 
   @no_fastcomp('wasm backend asyncify specific')
-  def test_async_bad_whitelist(self):
-    self.btest('browser/async_bad_whitelist.cpp', '0', args=['-s', 'ASYNCIFY', '-s', 'ASYNCIFY_WHITELIST=["waka"]', '--profiling'])
+  def test_async_bad_list(self):
+    self.btest('browser/async_bad_list.cpp', '0', args=['-s', 'ASYNCIFY', '-s', 'ASYNCIFY_ONLY_LIST=["waka"]', '--profiling'])
 
   # Tests that when building with -s MINIMAL_RUNTIME=1, the build can use -s MODULARIZE=1 as well.
   def test_minimal_runtime_modularize(self):

tests/test_core.py

@@ -7862,22 +7862,22 @@ def test_asyncify_unused(self):
 
   @parameterized({
     'normal': ([], True),
-    'blacklist_a': (['-s', 'ASYNCIFY_BLACKLIST=["foo(int, double)"]'], False),
-    'blacklist_b': (['-s', 'ASYNCIFY_BLACKLIST=["bar()"]'], True),
-    'blacklist_c': (['-s', 'ASYNCIFY_BLACKLIST=["baz()"]'], False),
-    'whitelist_a': (['-s', 'ASYNCIFY_WHITELIST=["main","__original_main","foo(int, double)","baz()","c_baz","Structy::funcy()","bar()"]'], True),
-    'whitelist_b': (['-s', 'ASYNCIFY_WHITELIST=["main","__original_main","foo(int, double)","baz()","c_baz","Structy::funcy()"]'], True),
-    'whitelist_c': (['-s', 'ASYNCIFY_WHITELIST=["main","__original_main","foo(int, double)","baz()","c_baz"]'], False),
-    'whitelist_d': (['-s', 'ASYNCIFY_WHITELIST=["foo(int, double)","baz()","c_baz","Structy::funcy()"]'], False),
-    'whitelist_b_response': ([], True,  '["main","__original_main","foo(int, double)","baz()","c_baz","Structy::funcy()"]'),
-    'whitelist_c_response': ([], False, '["main","__original_main","foo(int, double)","baz()","c_baz"]'),
+    'removelist_a': (['-s', 'ASYNCIFY_REMOVE_LIST=["foo(int, double)"]'], False),
+    'removelist_b': (['-s', 'ASYNCIFY_REMOVE_LIST=["bar()"]'], True),
+    'removelist_c': (['-s', 'ASYNCIFY_REMOVE_LIST=["baz()"]'], False),
+    'onlylist_a': (['-s', 'ASYNCIFY_ONLY_LIST=["main","__original_main","foo(int, double)","baz()","c_baz","Structy::funcy()","bar()"]'], True),
+    'onlylist_b': (['-s', 'ASYNCIFY_ONLY_LIST=["main","__original_main","foo(int, double)","baz()","c_baz","Structy::funcy()"]'], True),
+    'onlylist_c': (['-s', 'ASYNCIFY_ONLY_LIST=["main","__original_main","foo(int, double)","baz()","c_baz"]'], False),
+    'onlylist_d': (['-s', 'ASYNCIFY_ONLY_LIST=["foo(int, double)","baz()","c_baz","Structy::funcy()"]'], False),
+    'onlylist_b_response': ([], True,  '["main","__original_main","foo(int, double)","baz()","c_baz","Structy::funcy()"]'),
+    'onlylist_c_response': ([], False, '["main","__original_main","foo(int, double)","baz()","c_baz"]'),
   })
   @no_asan('asan is not compatible with asyncify stack operations; may also need to not instrument asan_c_load_4, TODO')
   @no_fastcomp('new asyncify only')
   def test_asyncify_lists(self, args, should_pass, response=None):
     if response is not None:
       create_test_file('response.file', response)
-      self.emcc_args += ['-s', 'ASYNCIFY_WHITELIST[email protected]']
+      self.emcc_args += ['-s', 'ASYNCIFY_ONLY_LIST[email protected]']
     self.set_setting('ASYNCIFY', 1)
     self.emcc_args += args
     try:
@@ -7889,6 +7889,25 @@ def test_asyncify_lists(self, args, should_pass, response=None):
       if should_pass:
         raise
 
+  @parameterized({
+    'normal': ([], True),
+    'ignoreindirect': (['-s', 'ASYNCIFY_IGNORE_INDIRECT'], False),
+    'add': (['-s', 'ASYNCIFY_IGNORE_INDIRECT', '-s', 'ASYNCIFY_ADD_LIST=["main","virt()"]'], True),
+  })
+  @no_asan('asan is not compatible with asyncify stack operations; may also need to not instrument asan_c_load_4, TODO')
+  @no_fastcomp('new asyncify only')
+  def test_asyncify_indirect_lists(self, args, should_pass):
+    self.set_setting('ASYNCIFY', 1)
+    self.emcc_args += args
+    try:
+      self.do_run_in_out_file_test('tests', 'core', 'test_asyncify_indirect_lists', assert_identical=True)
+      if not should_pass:
+        should_pass = True
+        raise Exception('should not have passed')
+    except Exception:
+      if should_pass:
+        raise
+
   @no_asan('asyncify stack operations confuse asan')
   @no_fastcomp('wasm-backend specific feature')
   def test_emscripten_scan_registers(self):

tests/test_other.py

@@ -8931,7 +8931,7 @@ def test_emcc_parsing(self):
 
   @no_fastcomp('uses new ASYNCIFY')
   def test_asyncify_escaping(self):
-    proc = run_process([EMCC, path_from_root('tests', 'hello_world.c'), '-s', 'ASYNCIFY=1', '-s', "ASYNCIFY_WHITELIST=[DOS_ReadFile(unsigned short, unsigned char*, unsigned short*, bool)]"], stdout=PIPE, stderr=PIPE)
+    proc = run_process([EMCC, path_from_root('tests', 'hello_world.c'), '-s', 'ASYNCIFY=1', '-s', "ASYNCIFY_ONLY_LIST=[DOS_ReadFile(unsigned short, unsigned char*, unsigned short*, bool)]"], stdout=PIPE, stderr=PIPE)
     self.assertContained('emcc: ASYNCIFY list contains an item without balanced parentheses', proc.stderr)
     self.assertContained('   DOS_ReadFile(unsigned short', proc.stderr)
     self.assertContained('Try to quote the entire argument', proc.stderr)
@@ -8943,10 +8943,10 @@ def test_asyncify_response_file(self):
   "DOS_ReadFile(unsigned short, unsigned char*, unsigned short*, bool)"
 ]
 ''')
-    proc = run_process([EMCC, path_from_root('tests', 'hello_world.c'), '-s', 'ASYNCIFY=1', '-s', "ASYNCIFY_WHITELIST[email protected]"], stdout=PIPE, stderr=PIPE)
+    proc = run_process([EMCC, path_from_root('tests', 'hello_world.c'), '-s', 'ASYNCIFY=1', '-s', "ASYNCIFY_ONLY_LIST[email protected]"], stdout=PIPE, stderr=PIPE)
     # we should parse the response file properly, and then issue a proper warning for the missing function
     self.assertContained(
-        'Asyncify whitelist contained a non-matching pattern: DOS_ReadFile(unsigned short, unsigned char*, unsigned short*, bool)',
+        'Asyncify onlylist contained a non-matching pattern: DOS_ReadFile(unsigned short, unsigned char*, unsigned short*, bool)',
         proc.stderr)
 
   # Sockets and networking