Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

-s WASM=0 -s LEGACY_VM_SUPPORT=1 results in JSC_CONSTANT_REASSIGNED_VALUE_ERROR warning during linking #20810

Closed
manxorist opened this issue Nov 30, 2023 · 28 comments · Fixed by #20879

Comments

@manxorist
Copy link
Contributor

manxorist commented Nov 30, 2023

-s WASM=0 -s LEGACY_VM_SUPPORT=1 with emscripten 3.1.50 results in the following warning when linking:

building:WARNING: Closure compiler completed with warnings:

building:WARNING: /tmp/emscripten_temp_qc8uptr2/libopenmpt_test.js.mem.jso4.js:1:4095: WARNING - [JSC_CONSTANT_REASSIGNED_VALUE_ERROR] constant WebAssembly assigned a value more than once.
Original definition at externs.zip//webassembly.js:29

[...]

0 error(s), 1 warning(s)

building:WARNING: (rerun with -g1 linker flag for an unminified output)

(I cut away the 100kB of minified javascript)

Building with -g1 as suggested in the output makes the problem go away.

It does not happen with either -s WASM=2 or -s LEGACY_VM_SUPPORT=0. It also did not happen with emscripten 3.1.49, so this is a regression.

The other LDFLAGS used in this particular build are -Oz -flto -s ALLOW_MEMORY_GROWTH=1 -s DISABLE_EXCEPTION_CATCHING=0 -s ERROR_ON_UNDEFINED_SYMBOLS=1 -s ERROR_ON_MISSING_LIBRARIES=1 -s EXPORT_NAME="'libopenmpt'" -s EXPORTED_FUNCTIONS="['_malloc','_free']"

You can reproduce the problem by building https://github.com/OpenMPT/openmpt/tree/e5a90920223b0648be01b3f2ad917f9f4a375f85 with make CONFIG=emscripten EMSCRIPTEN_TARGET=js VERBOSE=1 TEST=1 ONLY_TEST=1 (on Linux, I have no idea if our build system works correctly on macOS).

Edit: I added -g1 unconditionally as a work-around in a later commit, so it is mandatory to use the precise commit that I linked to reproduce the issue.

@manxorist
Copy link
Contributor Author

https://emscripten.org/docs/tools_reference/emcc.html tells me

-g0: Make no effort to keep code debuggable.
-g1: When linking, preserve whitespace in JavaScript.

so, the only difference should be whitespace in JavaScript. How can removed whitespace result in WebAssembly being supposedly re-assigned?!

manxorist added a commit to OpenMPT/openmpt that referenced this issue Nov 30, 2023
…ipten#20810> but always using -g1. The warning with emscripten 3.1.50 sounds very dangerous, and since it is apparently caused by removing whitespace, additional whitespace is a small price to pay for correctness.

git-svn-id: https://source.openmpt.org/svn/openmpt/trunk/OpenMPT@19943 56274372-70c3-4bfc-bfc3-4c3a0b034d27
manxorist added a commit to OpenMPT/openmpt that referenced this issue Nov 30, 2023
[Fix] build: Makefile: Emscripten: Work-around <emscripten-core/emscripten#20810> but always using -g1. The warning with emscripten 3.1.50 sounds very dangerous, and since it is apparently caused by removing whitespace, additional whitespace is a small price to pay for correctness.
........


git-svn-id: https://source.openmpt.org/svn/openmpt/branches/OpenMPT-1.29@19946 56274372-70c3-4bfc-bfc3-4c3a0b034d27
manxorist added a commit to OpenMPT/openmpt that referenced this issue Nov 30, 2023
[Fix] build: Makefile: Emscripten: Work-around <emscripten-core/emscripten#20810> but always using -g1. The warning with emscripten 3.1.50 sounds very dangerous, and since it is apparently caused by removing whitespace, additional whitespace is a small price to pay for correctness.
........


git-svn-id: https://source.openmpt.org/svn/openmpt/branches/OpenMPT-1.30@19945 56274372-70c3-4bfc-bfc3-4c3a0b034d27
manxorist added a commit to OpenMPT/openmpt that referenced this issue Nov 30, 2023
[Fix] build: Makefile: Emscripten: Work-around <emscripten-core/emscripten#20810> but always using -g1. The warning with emscripten 3.1.50 sounds very dangerous, and since it is apparently caused by removing whitespace, additional whitespace is a small price to pay for correctness.
........


git-svn-id: https://source.openmpt.org/svn/openmpt/branches/OpenMPT-1.31@19944 56274372-70c3-4bfc-bfc3-4c3a0b034d27
@kripken
Copy link
Member

kripken commented Nov 30, 2023

It also did not happen with emscripten 3.1.49, so this is a regression.

Hmm, that is unexpected as we didn't make any significant changes I can think of to WASM=0 mode. I suggest that you bisect to find where this broke, that might be the fastest way to make progress here:

https://emscripten.org/docs/contributing/developers_guide.html#bisecting

@manxorist
Copy link
Contributor Author

dac4fd71aa0ef9efc0dde7b20108ea91e048a6e2 is the first bad commit
commit dac4fd71aa0ef9efc0dde7b20108ea91e048a6e2
Author: chromium-autoroll <[email protected]>
Date:   Wed Nov 15 21:17:48 2023 +0000

    Roll emscripten from 084611f22cc2 to 9429b4893c5e (2 revisions)

    https://chromium.googlesource.com/external/github.com/emscripten-core/emscripten.git/+log/084611f22cc2..9429b4893c5e

    2023-11-15 [email protected] Remove limits of ES6 features in standard library code (#20700)
    2023-11-15 [email protected] Add support for -no-pthread (#20723)

    If this roll has caused a breakage, revert this CL and stop the roller
    using the controls here:
    https://autoroll.skia.org/r/emscripten-emscripten-releases
    Please CC [email protected],[email protected] on the revert to ensure that a human
    is aware of the problem.

    To report a problem with the AutoRoller itself, please file a bug:
    https://issues.skia.org/issues/new?component=1389291&template=1850622

    Documentation for the AutoRoller is here:
    https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

    Tbr: [email protected]
    Change-Id: Id780307bb980c9b3590d694bf708ad2cde114e7e
    Reviewed-on: https://chromium-review.googlesource.com/c/emscripten-releases/+/5034965
    Commit-Queue: chromium-autoroll <[email protected]>
    Bot-Commit: chromium-autoroll <[email protected]>

 DEPS | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

@sbc100
Copy link
Collaborator

sbc100 commented Nov 30, 2023

I think perhaps the changes in #20700 meant that we now run closure compiler in a slightly different mode when transpiling. This means that most likely this closure warning was always there but closure was not being run.

The fact that building withiout LEGACY_VM_SUPPORT (which disables trasnpiling) fixes to issue would seem to confirm this.

@manxorist can you double check by explictly adding --closure=1 to the previously working build (e.g. with 3.1.49(. I'm guessing that will cause the closure warning to show up in the older build too.

The quick fix for this is to simply ignore the warning using -Wno-closure. The long term fix is to find out where the warning is coming from and fix it.

@manxorist
Copy link
Contributor Author

manxorist commented Nov 30, 2023

@manxorist can you double check by explictly adding --closure=1 to the previously working build (e.g. with 3.1.49(. I'm guessing that will cause the closure warning to show up in the older build too.

3.1.49 with --closure=1 does not reproduce the problem.

Edit: I had tried with -closure=1 (missing the second dash),

@manxorist
Copy link
Contributor Author

Sorry, ignore the last comment.

3.1.49 with --closure=1 results in:

building:ERROR: Closure compiler run failed:

building:ERROR: /tmp/emscripten_temp_f694z38_/libopenmpt_test.js.mem.jso4.js:37:220: ERROR - [JSC_VAR_MULTIPLY_DECLARED_ERROR] Variable Module declared more than once. First occurrence: /tmp/emscripten_temp_f694z38_/libopenmpt_test.js.mem.jso4.js:12:3
  37|    */Object.assign=function(target,source){for(var i=1;i<arguments.length;i++){var source=arguments[i];if(!source)continue;for(var key in source){if(source.hasOwnProperty(key))target[key]=source[key]}}return target}}var Module={"preInit":function(text){FS.mkdir("/test");FS.mount(NODEFS,{"root":"../test/"},"/test");FS.mkdir("/libopenmpt");FS.mount(NODEFS,{"root":"../libopenmpt/"},"/libopenmpt")}};var moduleOverrides=Object.assign({},Module);var arguments_=[];var thisProgram="./this.program";var quit_=(status,toThrow)=>{throw toThrow};var ENVIRONMENT_IS_WEB=typeof window=="object";var ENVIRONMENT_IS_WORKER=typeof importScripts=="function";var ENVIRONMENT_IS_NODE=typeof process=="object"&&typeof process.versions=="object"&&typeof process.versions.node=="string";var ENVIRONMENT_IS_SHELL=!ENVIRONMENT_IS_WEB&&!ENVIRONMENT_IS_NODE&&!ENVIRONMENT_IS_WORKER;var scriptDirectory="";function locateFile(path){if(Module["locateFile"]){return Module["locateFile"](path,scriptDirectory)}return scriptDirectory+path}var read_,readAsync,readBinary;if(ENVIRONMENT_IS_NODE){var fs=require("fs");var nodePath=require("path");if(ENVIRONMENT_IS_WORKER){scriptDirectory=nodePath.dirname(scriptDirectory)+"/"}else{scriptDirectory=__dirname+"/"}/**
                                                                                                                                                                                                                                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

1 error(s), 0 warning(s)

em++: error: closure compiler failed (rc: 1): /home/manx/opt/emsdk/node/16.20.0_64bit/bin/node --max_old_space_size=8192 /home/manx/opt/emsdk/upstream/emscripten/node_modules/.bin/google-closure-compiler --compilation_level ADVANCED_OPTIMIZATIONS --language_in ECMASCRIPT_2021 --language_out ES5 --emit_use_strict=false --externs /home/manx/opt/emsdk/upstream/emscripten/src/closure-externs/closure-externs.js --externs /home/manx/opt/emsdk/upstream/emscripten/src/closure-externs/node-externs.js --externs /home/manx/opt/emsdk/upstream/emscripten/third_party/closure-compiler/node-externs/stream.js --externs /home/manx/opt/emsdk/upstream/emscripten/third_party/closure-compiler/node-externs/dns.js --externs /home/manx/opt/emsdk/upstream/emscripten/third_party/closure-compiler/node-externs/zlib.js --externs /home/manx/opt/emsdk/upstream/emscripten/third_party/closure-compiler/node-externs/path.js --externs /home/manx/opt/emsdk/upstream/emscripten/third_party/closure-compiler/node-externs/fs.js --externs /home/manx/opt/emsdk/upstream/emscripten/third_party/closure-compiler/node-externs/tty.js --externs /home/manx/opt/emsdk/upstream/emscripten/third_party/closure-compiler/node-externs/tls.js --externs /home/manx/opt/emsdk/upstream/emscripten/third_party/closure-compiler/node-externs/os.js --externs /home/manx/opt/emsdk/upstream/emscripten/third_party/closure-compiler/node-externs/cluster.js --externs /home/manx/opt/emsdk/upstream/emscripten/third_party/closure-compiler/node-externs/url.js --externs /home/manx/opt/emsdk/upstream/emscripten/third_party/closure-compiler/node-externs/dgram.js --externs /home/manx/opt/emsdk/upstream/emscripten/third_party/closure-compiler/node-externs/core.js --externs /home/manx/opt/emsdk/upstream/emscripten/third_party/closure-compiler/node-externs/events.js --externs /home/manx/opt/emsdk/upstream/emscripten/third_party/closure-compiler/node-externs/repl.js --externs /home/manx/opt/emsdk/upstream/emscripten/third_party/closure-compiler/node-externs/process.js --externs /home/manx/opt/emsdk/upstream/emscripten/third_party/closure-compiler/node-externs/string_decoder.js --externs /home/manx/opt/emsdk/upstream/emscripten/third_party/closure-compiler/node-externs/http.js --externs /home/manx/opt/emsdk/upstream/emscripten/third_party/closure-compiler/node-externs/punycode.js --externs /home/manx/opt/emsdk/upstream/emscripten/third_party/closure-compiler/node-externs/util.js --externs /home/manx/opt/emsdk/upstream/emscripten/third_party/closure-compiler/node-externs/vm.js --externs /home/manx/opt/emsdk/upstream/emscripten/third_party/closure-compiler/node-externs/domain.js --externs /home/manx/opt/emsdk/upstream/emscripten/third_party/closure-compiler/node-externs/child_process.js --externs /home/manx/opt/emsdk/upstream/emscripten/third_party/closure-compiler/node-externs/buffer.js --externs /home/manx/opt/emsdk/upstream/emscripten/third_party/closure-compiler/node-externs/querystring.js --externs /home/manx/opt/emsdk/upstream/emscripten/third_party/closure-compiler/node-externs/readline.js --externs /home/manx/opt/emsdk/upstream/emscripten/third_party/closure-compiler/node-externs/https.js --externs /home/manx/opt/emsdk/upstream/emscripten/third_party/closure-compiler/node-externs/net.js --js /tmp/emscripten_temp_f694z38_/libopenmpt_test.js.mem.jso4.js --js_output_file tmpprje_bhs.cc.js the error message may be clearer with -g1 and EMCC_DEBUG=2 set

@manxorist
Copy link
Contributor Author

The problem is actually way worse.

Note that originally I had set -s LEGACY_VM_SUPPORT=1 -Wno-transpile to silence a warning about running closure.
If I replace that -Wno-transpile by --closure=1 as recommended by the warning, I am getting a hard error, even with 3.1.50 and -g1, and even with -s WASM=2:

building:ERROR: Closure compiler run failed:

building:ERROR: /tmp/emscripten_temp_znbjrx69/libopenmpt_test.js.mem.jso4.js:45:4: ERROR - [JSC_VAR_MULTIPLY_DECLARED_ERROR] Variable Module declared more than once. First occurrence: /tmp/emscripten_temp_znbjrx69/libopenmpt_test.js.mem.jso4.js:12:4
  45| var Module = {
          ^^^^^^^^^^
  46|  "preInit": function(text) {
      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
...
  55|  }
      ^^
  56| };
      ^

1 error(s), 0 warning(s)

em++: error: closure compiler failed (rc: 1): /home/openmpt/opt/emsdk/node/16.20.0_64bit/bin/node --max_old_space_size=8192 /home/openmpt/opt/emsdk/upstream/emscripten/node_modules/.bin/google-closure-compiler --compilation_level ADVANCED_OPTIMIZATIONS --language_in ECMASCRIPT_2021 --language_out ES5 --emit_use_strict=false --externs /home/openmpt/opt/emsdk/upstream/emscripten/src/closure-externs/closure-externs.js --externs /home/openmpt/opt/emsdk/upstream/emscripten/src/closure-externs/node-externs.js --externs /home/openmpt/opt/emsdk/upstream/emscripten/third_party/closure-compiler/node-externs/http.js --externs /home/openmpt/opt/emsdk/upstream/emscripten/third_party/closure-compiler/node-externs/util.js --externs /home/openmpt/opt/emsdk/upstream/emscripten/third_party/closure-compiler/node-externs/readline.js --externs /home/openmpt/opt/emsdk/upstream/emscripten/third_party/closure-compiler/node-externs/events.js --externs /home/openmpt/opt/emsdk/upstream/emscripten/third_party/closure-compiler/node-externs/fs.js --externs /home/openmpt/opt/emsdk/upstream/emscripten/third_party/closure-compiler/node-externs/core.js --externs /home/openmpt/opt/emsdk/upstream/emscripten/third_party/closure-compiler/node-externs/tls.js --externs /home/openmpt/opt/emsdk/upstream/emscripten/third_party/closure-compiler/node-externs/process.js --externs /home/openmpt/opt/emsdk/upstream/emscripten/third_party/closure-compiler/node-externs/path.js --externs /home/openmpt/opt/emsdk/upstream/emscripten/third_party/closure-compiler/node-externs/net.js --externs /home/openmpt/opt/emsdk/upstream/emscripten/third_party/closure-compiler/node-externs/string_decoder.js --externs /home/openmpt/opt/emsdk/upstream/emscripten/third_party/closure-compiler/node-externs/url.js --externs /home/openmpt/opt/emsdk/upstream/emscripten/third_party/closure-compiler/node-externs/buffer.js --externs /home/openmpt/opt/emsdk/upstream/emscripten/third_party/closure-compiler/node-externs/dgram.js --externs /home/openmpt/opt/emsdk/upstream/emscripten/third_party/closure-compiler/node-externs/punycode.js --externs /home/openmpt/opt/emsdk/upstream/emscripten/third_party/closure-compiler/node-externs/stream.js --externs /home/openmpt/opt/emsdk/upstream/emscripten/third_party/closure-compiler/node-externs/zlib.js --externs /home/openmpt/opt/emsdk/upstream/emscripten/third_party/closure-compiler/node-externs/https.js --externs /home/openmpt/opt/emsdk/upstream/emscripten/third_party/closure-compiler/node-externs/child_process.js --externs /home/openmpt/opt/emsdk/upstream/emscripten/third_party/closure-compiler/node-externs/cluster.js --externs /home/openmpt/opt/emsdk/upstream/emscripten/third_party/closure-compiler/node-externs/domain.js --externs /home/openmpt/opt/emsdk/upstream/emscripten/third_party/closure-compiler/node-externs/querystring.js --externs /home/openmpt/opt/emsdk/upstream/emscripten/third_party/closure-compiler/node-externs/vm.js --externs /home/openmpt/opt/emsdk/upstream/emscripten/third_party/closure-compiler/node-externs/os.js --externs /home/openmpt/opt/emsdk/upstream/emscripten/third_party/closure-compiler/node-externs/dns.js --externs /home/openmpt/opt/emsdk/upstream/emscripten/third_party/closure-compiler/node-externs/repl.js --externs /home/openmpt/opt/emsdk/upstream/emscripten/third_party/closure-compiler/node-externs/tty.js --js /tmp/emscripten_temp_znbjrx69/libopenmpt_test.js.mem.jso4.js --js_output_file tmpz6g8bt13.cc.js --formatting PRETTY_PRINT

@sbc100
Copy link
Collaborator

sbc100 commented Dec 2, 2023

It this JSC_VAR_MULTIPLY_DECLARED_ERROR issue with --closure=1 specific for 3.1.50 and/or specific to-sWASM=2.

Hopefully we can create a new closure test when we fix this issue.

@manxorist
Copy link
Contributor Author

It this JSC_VAR_MULTIPLY_DECLARED_ERROR issue with --closure=1 specific for 3.1.50 and/or specific to-sWASM=2.

That's with 3.1.50 and it happens with any of -sWASM=0/-sWASM=1/-sWASM=2.

@sbc100
Copy link
Collaborator

sbc100 commented Dec 2, 2023

It this JSC_VAR_MULTIPLY_DECLARED_ERROR issue with --closure=1 specific for 3.1.50 and/or specific to-sWASM=2.

That's with 3.1.50 and it happens with any of -sWASM=0/-sWASM=1/-sWASM=2.

But it doesn't happen with 3.1.49?

I wonder if there is some way I can repro this locally to help me debug? Can you share your full link command line? From the settings you shared it looks like you are using -sEXPORT_NAME but without -sMODULARIZE. Is that correct? Do you know why -sEXPORT_NAME is being used?

@manxorist
Copy link
Contributor Author

It this JSC_VAR_MULTIPLY_DECLARED_ERROR issue with --closure=1 specific for 3.1.50 and/or specific to-sWASM=2.

That's with 3.1.50 and it happens with any of -sWASM=0/-sWASM=1/-sWASM=2.

But it doesn't happen with 3.1.49?

Also happens with 3.1.49 if I pass --closure=1 explicitly. (I did only test with -sWASM=0 so far).

I wonder if there is some way I can repro this locally to help me debug?

I have not yet found the time to provide a minimal reproducer. It may take me some days or weeks to find the time.

Can you share your full link command line?

em++ -std=c++20 -fPIC -Oz -flto -fno-inline-functions -s DISABLE_EXCEPTION_CATCHING=0 -fvisibility=hidden -Wall -Wextra -Wpedantic -Wcast-align -Wcast-qual -Wdouble-promotion -Wfloat-conversion -Wmissing-prototypes -Wshift-count-negative -Wshift-count-overflow -Wshift-op-parentheses -Wshift-overflow -Wshift-sign-overflow -Wundef -Wdeprecated -Wextra-semi -Wframe-larger-than=16000 -Wglobal-constructors -Wimplicit-fallthrough -Wmissing-declarations -Wnon-virtual-dtor -Wreserved-id-macro -Isrc -Icommon -I. -D MPT_SVNURL=\"https://source.openmpt.org/svn/openmpt/trunk/OpenMPT\" -D MPT_SVNVERSION=\"19963M\" -D MPT_SVNDATE=\"2023-12-02T14:48:00.530286Z\" -DLIBOPENMPT_BUILD -DMPT_WITH_MINIZ -Iinclude -DMPT_WITH_MINIMP3 -Iinclude -DMPT_WITH_STBVORBIS -DSTB_VORBIS_NO_PULLDATA_API -DSTB_VORBIS_NO_STDIO -Iinclude -Oz -flto -g1 -s WASM=0 -s LEGACY_VM_SUPPORT=1 --closure=1 -fno-inline-functions -s ALLOW_MEMORY_GROWTH=1 -s DISABLE_EXCEPTION_CATCHING=0 -s ERROR_ON_UNDEFINED_SYMBOLS=1 -s ERROR_ON_MISSING_LIBRARIES=1 -s EXPORT_NAME="'libopenmpt'" --pre-js build/make/test-pre.js -lnodefs.js test/libopenmpt_test.test.o common/ComponentManager.test.o common/Logging.test.o common/Profiler.test.o common/mptFileIO.test.o common/mptFileTemporary.test.o common/mptFileType.test.o common/mptPathString.test.o common/mptRandom.test.o common/mptStringBuffer.test.o common/mptTime.test.o common/serialization_utils.test.o common/version.test.o soundlib/AudioCriticalSection.test.o soundlib/ContainerMMCMP.test.o soundlib/ContainerPP20.test.o soundlib/ContainerUMX.test.o soundlib/ContainerXPK.test.o soundlib/Dlsbank.test.o soundlib/Fastmix.test.o soundlib/ITCompression.test.o soundlib/ITTools.test.o soundlib/InstrumentExtensions.test.o soundlib/Load_667.test.o soundlib/Load_669.test.o soundlib/Load_amf.test.o soundlib/Load_ams.test.o soundlib/Load_c67.test.o soundlib/Load_dbm.test.o soundlib/Load_digi.test.o soundlib/Load_dmf.test.o soundlib/Load_dsm.test.o soundlib/Load_dsym.test.o soundlib/Load_dtm.test.o soundlib/Load_far.test.o soundlib/Load_fmt.test.o soundlib/Load_gdm.test.o soundlib/Load_gt2.test.o soundlib/Load_imf.test.o soundlib/Load_it.test.o soundlib/Load_itp.test.o soundlib/Load_mdl.test.o soundlib/Load_med.test.o soundlib/Load_mid.test.o soundlib/Load_mo3.test.o soundlib/Load_mod.test.o soundlib/Load_mt2.test.o soundlib/Load_mtm.test.o soundlib/Load_mus_km.test.o soundlib/Load_okt.test.o soundlib/Load_plm.test.o soundlib/Load_psm.test.o soundlib/Load_ptm.test.o soundlib/Load_s3m.test.o soundlib/Load_sfx.test.o soundlib/Load_stm.test.o soundlib/Load_stp.test.o soundlib/Load_symmod.test.o soundlib/Load_uax.test.o soundlib/Load_ult.test.o soundlib/Load_wav.test.o soundlib/Load_xm.test.o soundlib/Load_xmf.test.o soundlib/MIDIEvents.test.o soundlib/MIDIMacros.test.o soundlib/MPEGFrame.test.o soundlib/Message.test.o soundlib/MixFuncTable.test.o soundlib/MixerLoops.test.o soundlib/MixerSettings.test.o soundlib/ModChannel.test.o soundlib/ModInstrument.test.o soundlib/ModSample.test.o soundlib/ModSequence.test.o soundlib/OPL.test.o soundlib/OggStream.test.o soundlib/Paula.test.o soundlib/RowVisitor.test.o soundlib/S3MTools.test.o soundlib/SampleFormatBRR.test.o soundlib/SampleFormatFLAC.test.o soundlib/SampleFormatMP3.test.o soundlib/SampleFormatMediaFoundation.test.o soundlib/SampleFormatOpus.test.o soundlib/SampleFormatSFZ.test.o soundlib/SampleFormatVorbis.test.o soundlib/SampleFormats.test.o soundlib/SampleIO.test.o soundlib/Snd_flt.test.o soundlib/Snd_fx.test.o soundlib/Sndfile.test.o soundlib/Sndmix.test.o soundlib/SoundFilePlayConfig.test.o soundlib/Tables.test.o soundlib/Tagging.test.o soundlib/TinyFFT.test.o soundlib/UMXTools.test.o soundlib/UpgradeModule.test.o soundlib/WAVTools.test.o soundlib/WindowedFIR.test.o soundlib/XMTools.test.o soundlib/load_j2b.test.o soundlib/mod_specifications.test.o soundlib/modcommand.test.o soundlib/modsmp_ctrl.test.o soundlib/pattern.test.o soundlib/patternContainer.test.o soundlib/tuning.test.o soundlib/tuningCollection.test.o soundlib/plugins/DigiBoosterEcho.test.o soundlib/plugins/LFOPlugin.test.o soundlib/plugins/PlugInterface.test.o soundlib/plugins/PluginManager.test.o soundlib/plugins/SymMODEcho.test.o soundlib/plugins/dmo/Chorus.test.o soundlib/plugins/dmo/Compressor.test.o soundlib/plugins/dmo/DMOPlugin.test.o soundlib/plugins/dmo/DMOUtils.test.o soundlib/plugins/dmo/Distortion.test.o soundlib/plugins/dmo/Echo.test.o soundlib/plugins/dmo/Flanger.test.o soundlib/plugins/dmo/Gargle.test.o soundlib/plugins/dmo/I3DL2Reverb.test.o soundlib/plugins/dmo/ParamEq.test.o soundlib/plugins/dmo/WavesReverb.test.o sounddsp/AGC.test.o sounddsp/DSP.test.o sounddsp/EQ.test.o sounddsp/Reverb.test.o test/mpt_tests_base.test.o test/mpt_tests_binary.test.o test/mpt_tests_crc.test.o test/mpt_tests_endian.test.o test/mpt_tests_format.test.o test/mpt_tests_io.test.o test/mpt_tests_parse.test.o test/mpt_tests_random.test.o test/mpt_tests_string.test.o test/mpt_tests_string_transcode.test.o test/mpt_tests_uuid.test.o test/test.test.o test/TestToolsLib.test.o include/miniz/miniz.test.o include/minimp3/minimp3.test.o include/stb_vorbis/stb_vorbis.test.o -o bin/libopenmpt_test.js

From the settings you shared it looks like you are using -sEXPORT_NAME but without -sMODULARIZE. Is that correct?

That's correct.

Do you know why -sEXPORT_NAME is being used?

I think because some user requested us to do that. However looking at the output we are getting and diffing it against not providing that option, I doubt it is actually doing anything useful in our case?! I am by no means fluent in JavaScript though.

However, I doubt we want to use -sMODULARIZE=1, neither for the unit tests nor for the library. For the unit tests (which we run via node.js), this requires us to additionally provide JavaScript code to instantiate the module and run it. This moves the behaviour of emscripten even further away from any other platform we are targeting. And I honestly do not want to use it for the library either because that introduces a difference between the unit test build options and the library build options. Emscripten has bitten us for similar minor build option changes in the past, and that frankly makes running our unit tests kind of pointless.

@sbc100
Copy link
Collaborator

sbc100 commented Dec 4, 2023

Yes, I was going to suggest that you drop EXPORT_NAME if you not using MODULARIZE. It was a little curious why it was there. (My hope it make using EXPORT_NAME without MODULARIZE into an error at some point)

@sbc100
Copy link
Collaborator

sbc100 commented Dec 4, 2023

Could you maybe share the contents of build/make/test-pre.js? Or describe what its doing?

@manxorist
Copy link
Contributor Author

Could you maybe share the contents of build/make/test-pre.js? Or describe what its doing?

https://github.com/OpenMPT/openmpt/blob/47e417851537d256522ec4a567afa1891cdc0019/build/make/test-pre.js#L1C1-L9C3

var Module = {
 'preInit': function(text) {
  FS.mkdir('/test');
  FS.mount(NODEFS, {'root': '../test/'}, '/test');
  FS.mkdir('/libopenmpt');
  FS.mount(NODEFS, {'root': '../libopenmpt/'}, '/libopenmpt');
 }
};

So maybe this is wrong. But it worked for the last 7 years with -sLEGACY_VM_SUPPORT=1 -Wno-transpile which should imply --closure=1 as far as I understand the ´-Wtranspile` warning.

I guess a related question from my side would why there always appears to exist a hard-coded Module variable in the JavaScript. Shouldn't that magic name be changeable, i.e. by EXPORT_NAME so that we can build multiple things with emscripten and use them from JavaScript independently?

The diff between setting EXPORT_NAME and not setting it is:

--- trunk-2/bin/libopenmpt.js   2023-12-04 08:02:48.980642452 +0100
+++ trunk-3/bin/libopenmpt.js   2023-12-04 08:05:38.940615223 +0100
@@ -1,4 +1,4 @@
-var Module = typeof libopenmpt != "undefined" ? libopenmpt : {};
+var Module = typeof Module != "undefined" ? Module : {};
 var Promise = function() {
   function noop() {
   }

@sbc100
Copy link
Collaborator

sbc100 commented Dec 4, 2023

I guess a related question from my side would why there always appears to exist a hard-coded Module variable in the JavaScript. Shouldn't that magic name be changeable, i.e. by EXPORT_NAME so that we can build multiple things with emscripten and use them from JavaScript independently?

This is what the MODULARIZE setting is for.

Setting EXPORT_NAME on its own does not do this and never has, which I think is perhaps somewhat confusing and why I propose to only allow EXPORT_NAME in combination with MODULARIZE

@sbc100
Copy link
Collaborator

sbc100 commented Dec 4, 2023

So maybe this is wrong. But it worked for the last 7 years with -sLEGACY_VM_SUPPORT=1 -Wno-transpile which should imply --closure=1 as far as I understand the ´-Wtranspile` warning.

I think what you have written is valid and as you say has always been valid in the past.

However, I think this is likely never been valid when using closure.. and we recently started using closure in a different way to perform transpilation which is why this only effecting you now. See #20700.

@manxorist
Copy link
Contributor Author

I guess a related question from my side would why there always appears to exist a hard-coded Module variable in the JavaScript. Shouldn't that magic name be changeable, i.e. by EXPORT_NAME so that we can build multiple things with emscripten and use them from JavaScript independently?

This is what the MODULARIZE setting is for.

Yeah, but then I cannot trivially run the resulting "binary" any more because it very fundamentally behaves completely differently from any other C++ target platform on the planet. Should I write additional JavaScript just to make the C++ program behave like the C++ program that I have actually written, to be able to run it via node.js?

I guess I could set MODULARIZE and EXPORT_NAME only for building the actual library and not for building the unit tests, but I strongly fear to get different behaviour in some obscure corner cases again, as has been the case in the past for e.g. #13224.

Setting EXPORT_NAME on its own does not do this and never has, which I think is perhaps somewhat confusing and why I propose to only allow EXPORT_NAME in combination with MODULARIZE

Yeah, that's certainly confusing, I agree :).

But EXPORT_NAME is not even the problem here. --closure=1 fails with exactly the same error in both 3.1.49 and 3.1.50 even when I do not set EXPORT_NAME.

Can you maybe explain why there always appears to be a variable named Module getting generated? What is the motivation behind this non-changeable name in the first place? I honestly want to understand this aspect.

So maybe this is wrong. But it worked for the last 7 years with -sLEGACY_VM_SUPPORT=1 -Wno-transpile which should imply --closure=1 as far as I understand the ´-Wtranspile` warning.

I think what you have written is valid and as you say has always been valid in the past.

However, I think this is likely never been valid when using closure.. and we recently started using closure in a different way to perform transpilation which is why this only effecting you now. See #20700.

So, how am I supposed to use --pre-js with --closure=1 then?

Maybe I am confused about the details here, but why did it work when closure was only implied by LEGACY_VM_SUPPORT, but not when I explicitly enabled it?

@sbc100
Copy link
Collaborator

sbc100 commented Dec 4, 2023

So, how am I supposed to use --pre-js with --closure=1 then?

I think the problem is that declaring var Module in a pre-js file is currently not compatible with --closure=1. Indeed IIUC it never has been. That is the crux of bug here that we need to fix now I beleive.

Maybe I am confused about the details here, but why did it work when closure was only implied by LEGACY_VM_SUPPORT, but not when I explicitly enabled it?

I believe what happened here is that -sLEGACY_VM_SUPPORT now implies the the use of closure in way that it didn't before #20700. Before this change we transpiled using closure's WHITESPACE_ONLY mode, which doesn't trigger this error. After #20700 we transpile with using SIMPLE_OPTIMIZATIONS which is closure to the full --closure=1 in the type of errors it reports.

@sbc100
Copy link
Collaborator

sbc100 commented Dec 4, 2023

Can you maybe explain why there always appears to be a variable named Module getting generated?

Within the module code Module is always a valid name. We use this all over the place in our internal library code, so something called Module always exists within the compiled code.

EXPORT_NAME changes the name for outside code but not for inside code.

@manxorist
Copy link
Contributor Author

Can you maybe explain why there always appears to be a variable named Module getting generated?

Within the module code Module is always a valid name. We use this all over the place in our internal library code, so something called Module always exists within the compiled code.

And MODULARIZE wraps that into its own scope so that multiple different "Module"s can co-exist, is my understand correct in that aspect?

So, why does MODULARIZE additionally wrap it as a promise? To me, this looks like 2 separate concerns, but I may obviously be missing important details as I am no JavaScript expert.

I think what I want here is basically the "namespace" wrapping, but not the promise/function indirection. Or maybe I want the modularized function getting called implicitly, or something like that.

@sbc100
Copy link
Collaborator

sbc100 commented Dec 4, 2023

So, why does MODULARIZE additionally wrap it as a promise? To me, this looks like 2 separate concerns, but I may obviously be missing important details as I am no JavaScript expert.

Indeed MODULARIZE does both two things:

  1. Wraps the generated code in its own namespace
  2. Allows more than one instance of a given module to exist be exposing a factory function

In the past we had an additional option called MODULARIZE_INSTANCE which only did (1) and we have be discussing bring that back, and maybe even making it the default (since my impression is that most programs don't need multiple instance of the same module on the same page).

You can think of the current options as being more like MODULARIZE_FACTORY since it exposes a factor function for stamping out instances.

@manxorist
Copy link
Contributor Author

Indeed MODULARIZE does both two things:

1. Wraps the generated code in its own namespace

2. Allows more than one instance of a given module to exist be exposing a factory function

In the past we had an additional option called MODULARIZE_INSTANCE which only did (1) and we have be discussing bring that back, and maybe even making it the default (since my impression is that most programs don't need multiple instance of the same module on the same page).

Yeah, making that the default sounds like a great idea, as it would best match the traditional C/C++ model. Having multiple instances as an optional feature would be nice to have (i.e. simulating multiple Unix-like processes in a single web page, or something like that), but I think the most common use case is probably instantiating a single set of WASM or JS libraries and driving them from the page (or from C main()).

@manxorist
Copy link
Contributor Author

As a matter of fact, I am currently building with -s EXPORT_NAME="'libopenmpt'" -s LEGACY_VM_SUPPORT=1 -Wno-transpile -g1 which just so happens to put closure into a mode where it does not complain for any of our other build option combinations.

So for now, this appears to be a work-around for me, at least for getting a clean build. It feels very fragile nonetheless, as any minor change in closure or how it is run with that particular combination of options is likely to break things again.

@manxorist
Copy link
Contributor Author

manxorist commented Dec 4, 2023

I think we concluded that the root cause for JSC_CONSTANT_REASSIGNED_VALUE_ERROR or JSC_VAR_MULTIPLY_DECLARED_ERROR is my use of --pre-js. So feel free to rename this issue to a more appropriate title. We can move other discussion elsewhere if you prefer (please just direct me where to go).

@manxorist
Copy link
Contributor Author

I am now testing replacing the --pre-js with just inlining the JavaScript via EM_ASM, which would be a perfectly fine solution for me. I actually prefer that to specifying additional build options like --pre-js.

@manxorist
Copy link
Contributor Author

manxorist commented Dec 4, 2023

~I think we concluded that the root cause for JSC_CONSTANT_REASSIGNED_VALUE_ERROR or JSC_VAR_MULTIPLY_DECLARED_ERROR is my use of --pre-js.

Uhm, well, sorry, I was wrong. I am still seeing JSC_CONSTANT_REASSIGNED_VALUE_ERROR even without --pre-js, so are we chasing 2 different problems here?! (-s WASM=0 -s LEGACY_VM_SUPPORT=1 -Wno-transpile).

@sbc100
Copy link
Collaborator

sbc100 commented Dec 4, 2023

~I think we concluded that the root cause for JSC_CONSTANT_REASSIGNED_VALUE_ERROR or JSC_VAR_MULTIPLY_DECLARED_ERROR is my use of --pre-js.

Uhm, well, sorry, I was wrong. I am still seeing JSC_CONSTANT_REASSIGNED_VALUE_ERROR even without --pre-js, so are we chasing 2 different problems here?! (-s WASM=0, -s LEGACY_VM_SUPPORT=1 -Wno-transpile).

Two symptoms of the same problem I imagine.

The problem being the transpilation now requires code that is closure compatible, whereas is didn't before #20700

manxorist added a commit to OpenMPT/openmpt that referenced this issue Dec 4, 2023
…ilds and use inline EM_ASM() instead, as this is more compatible with closure. See <emscripten-core/emscripten#20810 (comment)> and <emscripten-core/emscripten#20810 (comment)>.

git-svn-id: https://source.openmpt.org/svn/openmpt/trunk/OpenMPT@19964 56274372-70c3-4bfc-bfc3-4c3a0b034d27
sbc100 added a commit to sbc100/emscripten that referenced this issue Dec 8, 2023
We recently switch from using closure-compiler with `WHITESPACE_ONLY` to
closure-compiler with `SIMPLE_OPTIMIZATIONS`.  However this had the
negative side effect that all input need to be free of closure compiler
warnings, and this turned out not to be practical for all users.
See emscripten-core#20810 for more on this

When selecting a transpilation tool use I also evaluated `swx` (written
in rust) and `esbuild` (written in go).  `esbuild` was rejected
because the simply don't support ES5
(evanw/esbuild#297).  `swx` was rejected
because it almost doubled the side of our `node_modules` directory by
adding two 50mb binary files.
sbc100 added a commit to sbc100/emscripten that referenced this issue Dec 8, 2023
We recently switch from using closure-compiler with `WHITESPACE_ONLY` to
closure-compiler with `SIMPLE_OPTIMIZATIONS`.  However this had the
negative side effect that all input need to be free of closure compiler
warnings, and this turned out not to be practical for all users.
See emscripten-core#20810 for more on this

When selecting a transpilation tool use I also evaluated `swx` (written
in rust) and `esbuild` (written in go).  `esbuild` was rejected
because the simply don't support ES5
(evanw/esbuild#297).  `swx` was rejected
because it almost doubled the side of our `node_modules` directory by
adding two 50mb binary files.
sbc100 added a commit to sbc100/emscripten that referenced this issue Dec 8, 2023
We recently switch from using closure-compiler with `WHITESPACE_ONLY` to
closure-compiler with `SIMPLE_OPTIMIZATIONS`.  However this had the
negative side effect that all input need to be free of closure compiler
warnings, and this turned out not to be practical for all users.
See emscripten-core#20810 for more on this

When selecting a transpilation tool use I also evaluated `swx` (written
in rust) and `esbuild` (written in go).  `esbuild` was rejected
because the simply don't support ES5
(evanw/esbuild#297).  `swx` was rejected
because it almost doubled the side of our `node_modules` directory by
adding two 50mb binary files.
sbc100 added a commit to sbc100/emscripten that referenced this issue Dec 8, 2023
We recently switch from using closure-compiler with `WHITESPACE_ONLY` to
closure-compiler with `SIMPLE_OPTIMIZATIONS`.  However this had the
negative side effect that all input need to be free of closure compiler
warnings, and this turned out not to be practical for all users.
See emscripten-core#20810 for more on this

When selecting a transpilation tool use I also evaluated `swx` (written
in rust) and `esbuild` (written in go).  `esbuild` was rejected
because the simply don't support ES5
(evanw/esbuild#297).  `swx` was rejected
because it almost doubled the side of our `node_modules` directory by
adding two 50mb binary files.
sbc100 added a commit to sbc100/emscripten that referenced this issue Dec 8, 2023
We recently switch from using closure-compiler with `WHITESPACE_ONLY` to
closure-compiler with `SIMPLE_OPTIMIZATIONS`.  However this had the
negative side effect that all input need to be free of closure compiler
warnings, and this turned out not to be practical for all users.
See emscripten-core#20810 for more on this

When selecting a transpilation tool use I also evaluated `swx` (written
in rust) and `esbuild` (written in go).  `esbuild` was rejected
because the simply don't support ES5
(evanw/esbuild#297).  `swx` was rejected
because it almost doubled the side of our `node_modules` directory by
adding two 50mb binary files.
sbc100 added a commit to sbc100/emscripten that referenced this issue Dec 8, 2023
We recently switch from using closure-compiler with `WHITESPACE_ONLY` to
closure-compiler with `SIMPLE_OPTIMIZATIONS`.  However this had the
negative side effect that all input need to be free of closure compiler
warnings, and this turned out not to be practical for all users.
See emscripten-core#20810 for more on this

When selecting a transpilation tool use I also evaluated `swx` (written
in rust) and `esbuild` (written in go).  `esbuild` was rejected
because the simply don't support ES5
(evanw/esbuild#297).  `swx` was rejected
because it almost doubled the side of our `node_modules` directory by
adding two 50mb binary files.
sbc100 added a commit to sbc100/emscripten that referenced this issue Dec 8, 2023
We recently switch from using closure-compiler with `WHITESPACE_ONLY` to
closure-compiler with `SIMPLE_OPTIMIZATIONS`.  However this had the
negative side effect that all input need to be free of closure compiler
warnings, and this turned out not to be practical for all users.
See emscripten-core#20810 for more on this

When selecting a transpilation tool use I also evaluated `swx` (written
in rust) and `esbuild` (written in go).  `esbuild` was rejected
because the simply don't support ES5
(evanw/esbuild#297).  `swx` was rejected
because it almost doubled the side of our `node_modules` directory by
adding two 50mb binary files.
sbc100 added a commit to sbc100/emscripten that referenced this issue Dec 8, 2023
We recently switch from using closure-compiler with `WHITESPACE_ONLY` to
closure-compiler with `SIMPLE_OPTIMIZATIONS`.  However this had the
negative side effect that all input need to be free of closure compiler
warnings, and this turned out not to be practical for all users.
See emscripten-core#20810 for more on this

When selecting a transpilation tool use I also evaluated `swx` (written
in rust) and `esbuild` (written in go).  `esbuild` was rejected
because the simply don't support ES5
(evanw/esbuild#297).  `swx` was rejected
because it almost doubled the side of our `node_modules` directory by
adding two 50mb binary files.
sbc100 added a commit to sbc100/emscripten that referenced this issue Dec 8, 2023
We recently switch from using closure-compiler with `WHITESPACE_ONLY` to
closure-compiler with `SIMPLE_OPTIMIZATIONS`.  However this had the
negative side effect that all input need to be free of closure compiler
warnings, and this turned out not to be practical for all users.
See emscripten-core#20810 for more on this

When selecting a transpilation tool use I also evaluated `swx` (written
in rust) and `esbuild` (written in go).  `esbuild` was rejected
because the simply don't support ES5
(evanw/esbuild#297).  `swx` was rejected
because it almost doubled the side of our `node_modules` directory by
adding two 50mb binary files.
sbc100 added a commit to sbc100/emscripten that referenced this issue Dec 8, 2023
We recently switch from using closure-compiler with `WHITESPACE_ONLY` to
closure-compiler with `SIMPLE_OPTIMIZATIONS`.  However this had the
negative side effect that all input need to be free of closure compiler
warnings, and this turned out not to be practical for all users.
See emscripten-core#20810 for more on this

When selecting a transpilation tool use I also evaluated `swx` (written
in rust) and `esbuild` (written in go).  `esbuild` was rejected
because the simply don't support ES5
(evanw/esbuild#297).  `swx` was rejected
because it almost doubled the side of our `node_modules` directory by
adding two 50mb binary files.
sbc100 added a commit to sbc100/emscripten that referenced this issue Dec 8, 2023
We recently switch from using closure-compiler with `WHITESPACE_ONLY` to
closure-compiler with `SIMPLE_OPTIMIZATIONS`.  However this had the
negative side effect that all input need to be free of closure compiler
warnings, and this turned out not to be practical for all users.
See emscripten-core#20810 for more on this

When selecting a transpilation tool use I also evaluated `swx` (written
in rust) and `esbuild` (written in go).  `esbuild` was rejected
because the simply don't support ES5
(evanw/esbuild#297).  `swx` was rejected
because it almost doubled the side of our `node_modules` directory by
adding two 50mb binary files.
sbc100 added a commit to sbc100/emscripten that referenced this issue Dec 8, 2023
We recently switch from using closure-compiler with `WHITESPACE_ONLY` to
closure-compiler with `SIMPLE_OPTIMIZATIONS`.  However this had the
negative side effect that all input need to be free of closure compiler
warnings, and this turned out not to be practical for all users.
See emscripten-core#20810 for more on this

When selecting a transpilation tool use I also evaluated `swx` (written
in rust) and `esbuild` (written in go).  `esbuild` was rejected
because the simply don't support ES5
(evanw/esbuild#297).  `swx` was rejected
because it almost doubled the side of our `node_modules` directory by
adding two 50mb binary files.
sbc100 added a commit to sbc100/emscripten that referenced this issue Dec 8, 2023
We recently switch from using closure-compiler with `WHITESPACE_ONLY` to
closure-compiler with `SIMPLE_OPTIMIZATIONS`.  However this had the
negative side effect that all input need to be free of closure compiler
warnings, and this turned out not to be practical for all users.
See emscripten-core#20810 for more on this

When selecting a transpilation tool use I also evaluated `swx` (written
in rust) and `esbuild` (written in go).  `esbuild` was rejected
because the simply don't support ES5
(evanw/esbuild#297).  `swx` was rejected
because it almost doubled the side of our `node_modules` directory by
adding two 50mb binary files.
sbc100 added a commit to sbc100/emscripten that referenced this issue Dec 8, 2023
We recently switch from using closure-compiler with `WHITESPACE_ONLY` to
closure-compiler with `SIMPLE_OPTIMIZATIONS`.  However this had the
negative side effect that all input need to be free of closure compiler
warnings, and this turned out not to be practical for all users.
See emscripten-core#20810 for more on this

When selecting a transpilation tool use I also evaluated `swx` (written
in rust) and `esbuild` (written in go).  `esbuild` was rejected
because the simply don't support ES5
(evanw/esbuild#297).  `swx` was rejected
because it almost doubled the side of our `node_modules` directory by
adding two 50mb binary files.
sbc100 added a commit that referenced this issue Dec 9, 2023
We recently switch from using closure-compiler with `WHITESPACE_ONLY` to
closure-compiler with `SIMPLE_OPTIMIZATIONS`.  However this had the
negative side effect that all input need to be free of closure compiler
warnings, and this turned out not to be practical for all users.
See #20810 for more on this

When selecting a transpilation tool use I also evaluated `swx` (written
in rust) and `esbuild` (written in go).  `esbuild` was rejected
because the simply don't support ES5
(evanw/esbuild#297).  `swx` was rejected
because it almost doubled the side of our `node_modules` directory by
adding two 50mb binary files.
@manxorist
Copy link
Contributor Author

Thanks. I have tested current tot, and it appears to work without any work-around or warning for all cases I care about. My tests also pass.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants