Run wasm-emscripten-finalize only when needed, and add ERROR_ON_WASM_CHANGES_AFTER_LINK option (#12173)

emscripten.py now tracks whether we actually need to run wasm-emscripten-finalize.
Currently we don't need to in a specific subset of -O0 builds, in which we don't
need legalization (by using BigInt support) and we don't need invokes (which means
we don't use longjmp or C++ exceptions; this restriction will be lifted once
WebAssembly/binaryen#3081 is fixed).

This new option will show an error if any binaryen work
is done after wasm-ld. That is, it ensures that we do not run wasm-emscripten-finalize
or wasm-opt. That is useful to verify that the link is as fast as possible and also that
it does not rewrite DWARF, which is necessary to support split DWARF (binaryen can
rewrite normal DWARF, but it's simpler and better to support split DWARF by just
not doing any binaryen work after wasm-ld).

See WebAssembly/binaryen#3043

Author: kripken

ChangeLog.md

@@ -17,6 +17,10 @@ See docs/process.md for how version tagging works.
 
 Current Trunk
 -------------
+- Add `ERROR_ON_WASM_CHANGES_AFTER_LINK` option that errors if we need to do
+  any work in `wasm-emscripten-finalize` or `wasm-opt` after linking. This
+  can verify the link is maximally fast and also does no DWARF rewriting.
+  (#12173)
 
 2.0.3: 09/10/2020
 -----------------

emscripten.py

@@ -14,6 +14,7 @@
 import os
 import json
 import subprocess
+import shutil
 import time
 import logging
 import pprint
@@ -509,31 +510,49 @@ def finalize_wasm(temp_files, infile, outfile, memfile, DEBUG):
 
   args = ['--detect-features', '--minimize-wasm-changes']
 
+  # if we don't need to modify the wasm, don't tell finalize to emit a wasm file
+  modify_wasm = False
+
+  # C++ exceptions and longjmp require invoke processing,
+  # https://github.com/WebAssembly/binaryen/issues/3081
+  if shared.Settings.SUPPORT_LONGJMP or shared.Settings.DISABLE_EXCEPTION_CATCHING != 1:
+    modify_wasm = True
+  if shared.Settings.WASM2JS:
+    # wasm2js requires full legalization (and will do extra wasm binary
+    # later processing later anyhow)
+    modify_wasm = True
   write_source_map = shared.Settings.DEBUG_LEVEL >= 4
   if write_source_map:
     building.emit_wasm_source_map(base_wasm, base_wasm + '.map')
     building.save_intermediate(base_wasm + '.map', 'base_wasm.map')
     args += ['--output-source-map-url=' + shared.Settings.SOURCE_MAP_BASE + os.path.basename(shared.Settings.WASM_BINARY_FILE) + '.map']
-
+    modify_wasm = True
   # 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_ONLY or shared.Settings.ASYNCIFY_REMOVE or shared.Settings.ASYNCIFY_ADD:
     args.append('-g')
   if shared.Settings.WASM_BIGINT:
     args.append('--bigint')
-
-  if not shared.Settings.USE_LEGACY_DYNCALLS:
+  if shared.Settings.USE_LEGACY_DYNCALLS:
+    # we need to add all dyncalls to the wasm
+    modify_wasm = True
+  else:
     if shared.Settings.WASM_BIGINT:
       args.append('--no-dyncalls')
     else:
       args.append('--dyncalls-i64')
-
-  if shared.Settings.LEGALIZE_JS_FFI != 1:
+      # we need to add some dyncalls to the wasm
+      modify_wasm = True
+  if not shared.Settings.LEGALIZE_JS_FFI:
     args.append('--no-legalize-javascript-ffi')
+  else:
+    modify_wasm = True
   if not shared.Settings.MEM_INIT_IN_WASM:
     args.append('--separate-data-segments=' + memfile)
+    modify_wasm = True
   if shared.Settings.SIDE_MODULE:
     args.append('--side-module')
+    modify_wasm = True
   else:
     # --global-base is used by wasm-emscripten-finalize to calculate the size
     # of the static data used.  The argument we supply here needs to match the
@@ -548,23 +567,30 @@ def finalize_wasm(temp_files, infile, outfile, memfile, DEBUG):
       args.append('--global-base=%s' % shared.Settings.GLOBAL_BASE)
   if shared.Settings.STACK_OVERFLOW_CHECK >= 2:
     args.append('--check-stack-overflow')
+    modify_wasm = True
   if shared.Settings.STANDALONE_WASM:
     args.append('--standalone-wasm')
+    modify_wasm = True
   # When we dynamically link our JS loader adds functions from wasm modules to
   # the table. It must add the original versions of them, not legalized ones,
   # so that indirect calls have the right type, so export those.
   if shared.Settings.RELOCATABLE:
     args.append('--pass-arg=legalize-js-interface-export-originals')
+    modify_wasm = True
   if shared.Settings.DEBUG_LEVEL >= 3:
     args.append('--dwarf')
-  stdout = building.run_binaryen_command('wasm-emscripten-finalize',
-                                         infile=base_wasm,
-                                         outfile=wasm,
-                                         args=args,
-                                         stdout=subprocess.PIPE)
+  stdout = building.run_binaryen_command(
+      'wasm-emscripten-finalize',
+      infile=base_wasm,
+      outfile=wasm if modify_wasm else None,
+      args=args,
+      stdout=subprocess.PIPE)
+  if modify_wasm:
+    building.save_intermediate(wasm, 'post_finalize.wasm')
+  else:
+    shutil.copyfile(base_wasm, wasm)
   if write_source_map:
     building.save_intermediate(wasm + '.map', 'post_finalize.map')
-  building.save_intermediate(wasm, 'post_finalize.wasm')
 
   if not shared.Settings.MEM_INIT_IN_WASM:
     # we have a separate .mem file. binaryen did not strip any trailing zeros,

src/settings.js

@@ -1591,6 +1591,17 @@ var WASM2C = 0;
 // in a custom location.
 var SEPARATE_DWARF_URL = '';
 
+// Emscripten runs wasm-ld to link, and in some cases will do further changes to
+// the wasm afterwards, like running wasm-opt to optimize the binary in
+// optimized builds. However, in some builds no wasm changes are necessary after
+// link. This can make the entire link step faster, and can also be important
+// for other reasons, like in debugging if the wasm is not modified then the
+// DWARF info from LLVM is preserved (wasm-opt can rewrite it in some cases, but
+// not in others like split-dwarf).
+// When this flag is turned on, we error at link time if the build requires any
+// changes to the wasm after link. This can be useful in testing, for example.
+var ERROR_ON_WASM_CHANGES_AFTER_LINK = 0;
+
 // Whether the program should abort when an unhandled WASM exception is encountered.
 // This makes the Emscripten program behave more like a native program where the OS
 // would terminate the process and no further code can be executed when an unhandled

tests/hello_world_main_loop.cpp

@@ -0,0 +1,20 @@
+// Copyright 2020 The Emscripten Authors.  All rights reserved.
+// Emscripten is available under two separate licenses, the MIT license and the
+// University of Illinois/NCSA Open Source License.  Both these licenses can be
+// found in the LICENSE file.
+
+#include <stdio.h>
+#include <emscripten.h>
+
+void looper() {
+  static int frame = 0;
+  frame++;
+  if (frame == 10) {
+    puts("hello, world!");
+    emscripten_cancel_main_loop();
+  }
+}
+
+int main() {
+  emscripten_set_main_loop(looper, 0, 0);
+}

tests/test_other.py

@@ -9327,6 +9327,44 @@ def test_stdout_link(self):
     self.assertContained('invalid output filename: `-foo`', err)
     self.assertNotExists('-foo')
 
+  def test_immutable_after_link(self):
+    # some builds are guaranteed to not require any binaryen work after wasm-ld
+    def ok(args, filename='hello_world.cpp', expected='hello, world!'):
+      print('ok', args, filename)
+      args += ['-sERROR_ON_WASM_CHANGES_AFTER_LINK']
+      self.run_process([EMCC, path_from_root('tests', filename)] + args)
+      self.assertContained(expected, self.run_js('a.out.js'))
+
+    # -O0 with BigInt support (to avoid the need for legalization) and without
+    # longjmp
+    required_flags = ['-sWASM_BIGINT', '-sSUPPORT_LONGJMP=0']
+    ok(required_flags)
+    # Same with DWARF
+    ok(required_flags + ['-g'])
+    # Function pointer calls from JS work too
+    ok(required_flags, filename='hello_world_main_loop.cpp')
+
+    # other builds fail with a standard message + extra details
+    def fail(args, details):
+      print('fail', args, details)
+      args += ['-sERROR_ON_WASM_CHANGES_AFTER_LINK']
+      err = self.expect_fail([EMCC, path_from_root('tests', 'hello_world.cpp')] + args)
+      self.assertContained('changes to the wasm are required after link, but disallowed by ERROR_ON_WASM_CHANGES_AFTER_LINK', err)
+      self.assertContained(details, err)
+
+    # plain -O0
+    legalization_message = 'to disable legalization (which requires changes after link) use -s WASM_BIGINT'
+    longjmp_message = 'to disable longjmp support (which requires changes after link) use -s SUPPORT_LONGJMP=0'
+    fail([], legalization_message)
+    fail(['-sWASM_BIGINT'], longjmp_message)
+    fail(['-sSUPPORT_LONGJMP=0'], legalization_message)
+    # optimized builds even without legalization
+    optimization_message = 'optimizations always require changes, build with -O0 instead'
+    fail(required_flags + ['-O1'], optimization_message)
+    fail(required_flags + ['-O2'], optimization_message)
+    # exceptions fails until invokes are fixed
+    fail(required_flags + ['-fexceptions'], 'C++ exceptions always require changes')
+
   def test_output_to_nowhere(self):
     self.run_process([EMCC, path_from_root('tests', 'hello_world.cpp'), '-o', os.devnull, '-c'])
 

tools/building.py

@@ -1568,6 +1568,21 @@ def run_binaryen_command(tool, infile, outfile=None, args=[], debug=False, stdou
     cmd += [infile]
   if outfile:
     cmd += ['-o', outfile]
+    if Settings.ERROR_ON_WASM_CHANGES_AFTER_LINK:
+      # emit some extra helpful text for common issues
+      extra = ''
+      # a plain -O0 build *almost* doesn't need post-link changes, except for
+      # legalization and longjmp. show a clear error for those (as the flags
+      # the user passed in are not enough to see what went wrong)
+      if shared.Settings.LEGALIZE_JS_FFI:
+        extra += '\nnote: to disable legalization (which requires changes after link) use -s WASM_BIGINT'
+      if shared.Settings.SUPPORT_LONGJMP:
+        extra += '\nnote: to disable longjmp support (which requires changes after link) use -s SUPPORT_LONGJMP=0'
+      if shared.Settings.OPT_LEVEL > 0:
+        extra += '\nnote: optimizations always require changes, build with -O0 instead'
+      if shared.Settings.DISABLE_EXCEPTION_CATCHING != 1:
+        extra += '\nnote: C++ exceptions always require changes'
+      exit_with_error('changes to the wasm are required after link, but disallowed by ERROR_ON_WASM_CHANGES_AFTER_LINK: ' + str(cmd) + extra)
   if debug:
     cmd += ['-g'] # preserve the debug info
   # if the features are not already handled, handle them