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

Fix SINGLE_FILE with wasm2js #9407

Merged
merged 1 commit into from Sep 11, 2019
Merged

Fix SINGLE_FILE with wasm2js #9407

merged 1 commit into from Sep 11, 2019

Conversation

kripken
Copy link
Member

@kripken kripken commented Sep 6, 2019

The relevant test was disabled internally, which is why I missed it when I triaged all the disabled tests (I was looking for decorators etc.).

Fix is pretty simple, the logic for when to use a mem init file was not aware of SINGLE_FILE. (There may be an even simpler way to rewrite that code, but let's wait until after fastcomp is removed for any major refactoring.)

Fixes #9380

@Beuc
Copy link
Contributor

Beuc commented Sep 7, 2019

Hi. Recompiling zee.js with this fix works (no more external dependency) but the resulting zee.js does not decompress data anymore. I'll check whether this is a separate issue when time permits.

@Beuc
Copy link
Contributor

Beuc commented Sep 8, 2019

I confirm, with 1.38.43 + this patch:

  • Without -s SINGLE_FILE=1, zee.js requires libz.raw.js.mem but otherwise works.
  • With -s SINGLE_FILE=1, zee.js is standalone but fails on ccall(gzread, ...), and I get weird errors such as TypeError: h[k[((Le + 36) >> 2)]] is not a function while running printf.
    So I suspect something is going wrong internally.

@buu700
Copy link
Contributor

buu700 commented Sep 8, 2019

@kripken, I'm seeing a related issue: even when using --memory-init-file 0 as a quick workaround, it still fails in cases where a Content Security Policy blocks fetching data: URIs, whereas I would expect it to fall back to using tryParseAsDataURI as in this case.

buu700 added a commit to cyph/mceliece.js that referenced this pull request Sep 9, 2019
buu700 added a commit to cyph/ntru.js that referenced this pull request Sep 9, 2019
buu700 added a commit to cyph/rlwe.js that referenced this pull request Sep 9, 2019
buu700 added a commit to cyph/rsasign.js that referenced this pull request Sep 9, 2019
buu700 added a commit to cyph/sidh.js that referenced this pull request Sep 9, 2019
buu700 added a commit to cyph/sphincs.js that referenced this pull request Sep 9, 2019
buu700 added a commit to cyph/xkcd-passphrase that referenced this pull request Sep 9, 2019
buu700 added a commit to cyph/mceliece.js that referenced this pull request Sep 9, 2019
buu700 added a commit to cyph/ntru.js that referenced this pull request Sep 9, 2019
buu700 added a commit to cyph/rlwe.js that referenced this pull request Sep 9, 2019
buu700 added a commit to cyph/rsasign.js that referenced this pull request Sep 9, 2019
buu700 added a commit to cyph/sidh.js that referenced this pull request Sep 9, 2019
buu700 added a commit to cyph/sphincs.js that referenced this pull request Sep 9, 2019
buu700 added a commit to cyph/xkcd-passphrase that referenced this pull request Sep 9, 2019
buu700 added a commit to cyph/supersphincs that referenced this pull request Sep 9, 2019
@kripken
Copy link
Member Author

kripken commented Sep 10, 2019

@Beuc: that's odd, but I guess there's another issue then... if debugging it directly doesn't work, maybe running the test suite with that flag turned on would find some failure that is easier to debug.

@buu700: not sure I follow, but if there's a network access it should be practical to find where it's coming from?

@buu700
Copy link
Contributor

buu700 commented Sep 10, 2019

There's no network access needed; SINGLE_FILE preferentially uses XMLHttpRequest to parse strings of base64 for performance reasons, then falls back to a JS implementation (tryParseAsDataURI) when that fails. A website's Content Security Policy is just one (I'd guess the most common?) reason XHR might fail.

The issue is that the fallback didn't seem to be working in my testing, which is odd because the code looks correct. I'll dig into this a bit more and see if I can figure out what's going on.

@Beuc
Copy link
Contributor

Beuc commented Sep 10, 2019

This occurs with -02 and -s WASM=0.

Also, with zee's nodejs-based test suite, whatever value for SINGLE_FILE and/or WASM and/or 0, I get:
Assertion failed: you need to wait for the runtime to be ready (e.g. wait for main() to be called)
when ccall-ing gzcompress.
-- Is this related? Maybe it still expects some async loading to be completed, even with SINGLE_FILE? Given that there's no main in this project I'm not sure how to proceed :/

@buu700
Copy link
Contributor

buu700 commented Sep 10, 2019

Whoops, sorry, you can disregard my comments here!

I totally misunderstood the problem because I didn't realize that with mem init method 0 and wasm2js asm.js no longer had a separate mem init file. Turns out this last issue with upgrading to the new backend was just an issue on my end (hadn't fully handled that asm.js no longer initializes synchronously).

@kripken
Copy link
Member Author

kripken commented Sep 11, 2019

@Beuc: SINGLE_FILE doesn't mean synchronous compilation. There is a separate flag for that with wasm, WASM_ASYNC_COMPILATION=0.

@kripken kripken merged commit 1c05a94 into incoming Sep 11, 2019
@kripken kripken deleted the singlewasm2js branch September 11, 2019 21:49
belraquib pushed a commit to belraquib/emscripten that referenced this pull request Dec 23, 2020
The relevant test was disabled internally, which is why I missed it when I triaged all the disabled tests (I was looking for decorators etc.).

Fix is pretty simple, the logic for when to use a mem init file was not aware of SINGLE_FILE. (There may be an even simpler way to rewrite that code, but let's wait until after fastcomp is removed for any major refactoring.)

Fixes emscripten-core#9380
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 this pull request may close these issues.

WASM=0 + SINGLE_FILE=1 not a stand-alone file
4 participants