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

Using the closure compiler at link time corrupts references to variables in EM_JS blocks #21748

Closed
mlabbe opened this issue Apr 11, 2024 · 4 comments

Comments

@mlabbe
Copy link

mlabbe commented Apr 11, 2024

Please include the following in your bug report:

Version of emscripten/emsdk:
emcc (Emscripten gcc/clang-like replacement + linker emulating GNU ld) 3.1.57 (1df9c19)
clang version 19.0.0git (https:/github.com/llvm/llvm-project ccdebbae4d77d3efc236af92c22941de5d437e01)
Target: wasm32-unknown-emscripten
Thread model: posix

Compiling with --closure=1 causes variables to be renamed at access time that are attached to Module. For instance, Module.HEAPU8 becomes undefined. This happens regardless of whether MODULARIZE is used. This happens regardless of the -O setting passed to the linker or compiler.

The following code prints an error when --closure=1 is used:

EM_JS(void, log_byte_array, (unsigned char *bytes, size_t bytes_len), {
        if (Module.HEAPU8 === undefined) {
            console.error("HEAPU8 is undefined");
        } else {
            console.log("HEAPU8 is defined", Module.HEAPU8);
        }

        let byte_array = new Uint8Array(Module.HEAPU8.buffer, bytes, bytes_len);
        console.log("success: byte_array", byte_array);
    });

I have prepared a minimal repro case at https://github.com/mlabbe/closure-bug . The README contains steps to reproduce the bug.

In addition to the bug proper, I have a couple of ancillary notes/questions:

  1. The official docs on using closure state "Closure is only run if JavaScript opts are being done (-O2 or above)." However, I was able to have Closure run if I passed -O0 to the linker, or didn't specify -O at all. Some minification occurred, but not all, as if I passed -O3. So these docs do not appear to be accurate.

  2. Here is the relevant part of the output from emcc -v:

 dev/emscripten/node/16.20.0_64bit/bin/node dev/emscripten/upstream/emscripten/src/compiler.mjs /tmp/tmp79qg1g8k.json
 dev/emscripten/node/16.20.0_64bit/bin/node --max_old_space_size=8192 dev/emscripten/upstream/emscripten/node_modules/.bin/google-closure-compiler --compilation_level ADVANCED_OPTIMIZATIONS --language_in ECMASCRIPT_2021 --language_out NO_TRANSPILE --emit_use_strict=false --externs dev/emscripten/upstream/emscripten/src/closure-externs/closure-externs.js --js /tmp/emscripten_temp_od6308mj/example.js --js_output_file tmpe_lh7pxj.cc.js --formatting PRETTY_PRINT
 dev/emscripten/node/16.20.0_64bit/bin/node dev/emscripten/upstream/emscripten/tools/preprocessor.mjs /tmp/emscripten_temp_od6308mj/settings.js shell.html

@sbc100
Copy link
Collaborator

sbc100 commented Apr 12, 2024

For code that lives inside the module itself I you probably just want to use HEAPU8 directly and not via the Module object at all.

Is this a recent regression? We did recently stop exporting HEAPU8 when in STRICT mode (See #21439)

@mlabbe
Copy link
Author

mlabbe commented Apr 12, 2024

I'm not running in strict mode, so I don't think it qualifies as a regression. If I've misunderstood, let me know and I'll test the repro with a version < 3.1.55.

Removing the Module prefix works. Thanks - this is my first encounter with closure variable renaming.

@curiousdannii
Copy link
Contributor

curiousdannii commented Apr 18, 2024

I don't use Closure, but I think if you use Module['HEAP8'] then it will also handle renaming it for you. Or it prevents the property from being renamed? I'm not sure..

@sbc100
Copy link
Collaborator

sbc100 commented Apr 18, 2024

If you access a property of the Module you need to use the string name to access it yes. This prevents closure from minifying the access, and Module properties are not minified by closure compiler.

Better to just use HEAP8 on its own though if your code is withing the module itself, since that avoids having to export HEAP8 in the first place and it will get minified resulting in smaller code.

Closing this for now as WAI.

@sbc100 sbc100 closed this as completed Apr 18, 2024
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

No branches or pull requests

3 participants