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.

Sign up for GitHub

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

Jump to bottom

wasmExports not declared #20145

Closed
walkingeyerobot opened this issue Aug 28, 2023 · 6 comments · Fixed by #20159
Closed

wasmExports not declared #20145

walkingeyerobot opened this issue Aug 28, 2023 · 6 comments · Fixed by #20159

Comments

@walkingeyerobot
Copy link
Collaborator

I'm getting an error about wasmExports not being declared. I believe this is due to #19901 combined with the odd set of linker options I have:

-s MODULARIZE=1
-s EXPORT_NAME=loadWasm
-s ENVIRONMENT=web,node
-s MINIMAL_RUNTIME=2
-s MINIMAL_RUNTIME_STREAMING_WASM_COMPILATION=1
-s FILESYSTEM=0
-s TEXTDECODER=2
-s POLYFILL=0

Oddly, this didn't happen before #19901 because I was running through the js compiler (I'm doing this myself manually and not via emscripten) and asm the compiler decided to declare and rename asm for me, but for some reason it won't do the same for wasmExports.
@sbc100
Copy link
Collaborator

sbc100 commented Aug 28, 2023

Where is the reference to wasmExports coming from? Is it an internal emscripten reference, or is it being referenced perhaps in --pre-js or JS library code?

@walkingeyerobot
Copy link
Collaborator Author

It was erroring out in postamble-minimal.js. If I passed -lexports.js it worked in both cases because of these #ifs:
https://github.com/emscripten-core/emscripten/blob/main/src/postamble_minimal.js#L103-L105
https://github.com/emscripten-core/emscripten/blob/main/src/postamble_minimal.js#L143-L148

@sbc100
Copy link
Collaborator

sbc100 commented Aug 28, 2023

What is the error you are seeing?

sbc100 added a commit that referenced this issue Aug 28, 2023
@sbc100
MINIMAL_RUNTIME: Always declare wasmExports globally
116c132 
This simplifies the code and makes MINIMAL_RUNTIME behave more like
the regular runtime.   It also allows
`-sEXPORTED_RUNTIME_METHODS=wasmExports`, which doesn't work without
this.

This also matches the way we handle other globals such as `wasmTable`
and `wasmMemory`.

This save a few bytes of code size in some tests and cost a few in
others so on average I would say this code size neutral.

Fixes: #20145
@sbc100 sbc100 mentioned this issue Aug 28, 2023
Closed
@juj
Copy link
Collaborator

juj commented Aug 29, 2023

Are you using embind in the project?

Can you post the line in the build that refers to wasmExports that fails? (and some lines of context around that to help locate the construct?)

sbc100 added a commit that referenced this issue Aug 29, 2023
@sbc100
MINIMAL_RUNTIME: Declare wasmExports globally as needed
2141d77 
The conditions under which wasmExports was declared global vs locally
were out of sync.

Also, move the declaration of the global wasmExports alongside the
other wasm globals.

See: #20145
sbc100 added a commit that referenced this issue Aug 29, 2023
@sbc100
MINIMAL_RUNTIME: Declare wasmExports globally as needed
88b2db4 
The conditions under which wasmExports was declared global vs locally
were out of sync.

Also, move the declaration of the global wasmExports alongside the
other wasm globals.

See: #20145
@walkingeyerobot
Copy link
Collaborator Author

Yes, I'm using embind.

The line that fails is this: https://github.com/emscripten-core/emscripten/blob/main/src/postamble_minimal.js#L166

@sbc100 sbc100 mentioned this issue Sep 6, 2023
Merged
@sbc100
Copy link
Collaborator

sbc100 commented Sep 6, 2023 •
edited
Loading

Fix is in #20159

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix wasmExports declaration under MINIMAL_RUNTIME. NFC emscripten-core/emscripten
3 participants
@walkingeyerobot @juj @sbc100