Emscripten: MODULARIZE + Closure Compiler + --pre-js breaks output

[Stuk]

The Closure compiler runs before the MODULARIZE code. This means that the Module variable will be reduced by closure (e.g. to d), however the MODULARIZE code assumes it is still called Module, and returns that variable.

This can be reproduced with any file, and the command line:

emcc test.c -o test.js -O2 --closure 1 -s MODULARIZE=1

Have I missed something? I would be happy to create a PR to fix this.

[saschanaz]

What version do you see when emcc --version?

[Stuk]

emcc (Emscripten gcc/clang-like replacement) 1.37.22 ()

[kripken]

This looks ok to me in 1.37.22, file begins with
var Module = function(Module) { Module = Module || {}; var Module = Module; var b;b||(b=eval("(function() { try { return Module || {} } catch(e) { return {} } })()"));
A little ugly (there is a PR to get rid of the eval), but it should work. Then b and Module are referring to the same object, and it's ok to return Module.

But maybe I'm reading the code wrong. Do you have a testcase showing something not working?

[Stuk]

Thank you for looking into this! After taking a look I realized I also have a --pre-js flag, that uses the Module object before that line runs. Hence I was getting errors.

I have a couple of questions:

1. Is it wrong to use the Module object in a --pre-js file?
2. Is it necessary to use eval to get the Module variable when in MODULARIZE mode? It seems like this could be skipped as the Module object should always get passed in instead of existing in the global scope, then the whole file could be processed with Closure.

I'll close this issue as it isn't accurate.

[kripken]

Interesting, yes, there is an issue with --pre-js in this context, which I didn't realize. The --pre-js appears before the
b || (b = eval("(function() { try { return Module || {} } catch(e) { return {} } })()"));
line, so it doesn't work.

It's not immediately obvious to me how we should fix it. Perhaps we should move that line higher up, so it's before the --pre-js, but I worry that would affect other things.

I'll reopen this and edit the title.

[saschanaz]

I'm seeing that the latest incoming branch does not replace Module.

python2 emscripten/emcc.py test.c -o test.js -O2 --closure 1 -s MODULARIZE=1 --pre-js prejs.js

var Module = function(Module) {
  Module = Module || {};
  var Module = Module; // included code may refer to Module (e.g. from file packager), so alias it

Module.arbitrary="thisismymodule";var Module;Module||(Module=eval("(function() { try { return Module || {} } catch(e) { return {} } })()"));

So probably no problem for 1.37.23+.

[kripken]

Hmm, yes... this changed in the big closure compiler update in 93479ecbd390aec4f8a3765fe04bcb365d0b31b2

I didn't realize that update made it no longer minify Module. That does fix this bug, which is nice, but it also increases code size by 3% on hello world with your command. That can be fixed by removing var Module; from src/closure-externs.js, which we should probably do (after fixing the duplicate Module vars, which is why it is in that file, that I believe is fixed in #5751). At the same time, we'd need to fix this bug...

[kripken]

Ah, it's even in that PR already, https://github.com/kripken/emscripten/pull/5751/files#diff-28cd435d49a673d644ba5877fc595dfcL1035

[saschanaz]

We should get a test for this issue so that any future PR won't break it, if it's not already there.

[nazar-pc]

3% of code size increase by one word will be negligible if you use gzip or some alternative, which you should use anyway.

[kripken]

@saschanaz Yes, we should add a test when we fix this. With that PR, or soon after.

@nazar-pc Good point, it should be a smaller difference after gzip. I checked and it's 0.5% on that testcase. Still, it's nice to have 3% smaller JS for JS parse times, and we'll be fixing this with your PR anyhow (when it removes Module from the closure externs).

[nazar-pc]

To make it easier for me to work on this, can someone prepare a proper test case that I can test against? Possibly just push it on top of #5751.

[saschanaz]

I'll do it.

[kripken]

Already did it, sorry @saschanaz :)

See https://github.com/kripken/emscripten/commit/4d0443f353f56ca975d4f6af35dd2548cb388ff6

That currently passes, but fails once var Module; is removed from closure-externs.js.

[nazar-pc]

Great! Will merge it locally and push whenever the fix is ready, should be today.

[saschanaz]

The test added by 4d0443f passes even on 1.37.22.

I'm not sure I'm fully understanding this issue. The Module reference in pre-js file should be anyway okay as the MODULARIZE already defines Module and the closure will use it, no? No, as the pre-js file will also compiled by Closure. Sorry 😅

[nazar-pc]

-O2 was missing, already added it in mentioned PR, made potential fix for the issue and waiting for CI to confirm.

[saschanaz]

Cherry-picking 9001924481a7f23d9d77d13f1dd5b6a6704f6f15, the test still passes on 1.37.22.

[nazar-pc]

I've cherry-picked 4d0443f & 9001924 and on tag 1.37.22 got the following:

nazar-pc@nazar-pc /m/D/emscripten> python tests/runner.py test_modularize_closure_pre
WARNING:root:use EM_ALL_ENGINES=1 in the env to run against all JS engines, which is slower but provides more coverage
Test suites:
['test_core']
Running test_core: (1 tests)
test_modularize_closure_pre (test_core.default) ... (checking sanity from test runner)
INFO:root:(Emscripten: Running sanity checks)
(test did not pass in JS engine: ['/media/Data/emsdk-portable/node/4.1.1_64bit/bin/node'])
ERROR

======================================================================
ERROR: test_modularize_closure_pre (test_core.default)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/media/Data/emscripten/tests/test_core.py", line 6997, in test_modularize_closure_pre
    post_build=(None, post))
  File "/media/Data/emscripten/tests/runner.py", line 675, in do_run
    raise e
Exception: Expected to find 'waka
' in '/tmp/emscripten_test_default_tpVrhc/src.cpp.o.js:5
b.on_module = "waka";
            ^

TypeError: Cannot set property 'on_module' of undefined
    at Module (/tmp/emscripten_test_default_tpVrhc/src.cpp.o.js:5:13)
    at Object.<anonymous> (/tmp/emscripten_test_default_tpVrhc/src.cpp.o.js:3319:17)
    at Module._compile (module.js:434:26)
    at Object.Module._extensions..js (module.js:452:10)
    at Module.load (module.js:355:32)
    at Function.Module._load (module.js:310:12)
    at Function.Module.runMain (module.js:475:10)
    at startup (node.js:117:18)
    at node.js:951:3
', diff:

--- expected
+++ actual
@@ -1,2 +1,15 @@
-waka
+/tmp/emscripten_test_default_tpVrhc/src.cpp.o.js:5
+b.on_module = "waka";
+            ^

+TypeError: Cannot set property 'on_module' of undefined
+    at Module (/tmp/emscripten_test_default_tpVrhc/src.cpp.o.js:5:13)
+    at Object.<anonymous> (/tmp/emscripten_test_default_tpVrhc/src.cpp.o.js:3319:17)
+    at Module._compile (module.js:434:26)
+    at Object.Module._extensions..js (module.js:452:10)
+    at Module.load (module.js:355:32)
+    at Function.Module._load (module.js:310:12)
+    at Function.Module.runMain (module.js:475:10)
+    at startup (node.js:117:18)
+    at node.js:951:3
+



----------------------------------------------------------------------
Ran 1 test in 2.966s

FAILED (errors=1)

Make sure you don't mix anything else in there

[saschanaz]

Ah yes, my bad, I wasn't correctly cherry-picking your commit. Thanks!

[nazar-pc]

This should be fixed now since #5751 is merged

[Stuk]

Thank you everyone for following up on this!