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

Run fpcast-emu before asyncify in the wasm backend #9408

Merged
merged 3 commits into from Sep 19, 2019
Merged

Conversation

kripken
Copy link
Member

@kripken kripken commented Sep 6, 2019

So that the asyncify whitelist can affect the fpcast helper functions.

Regrettable that the fastcomp and upstream paths diverge a little here, but asyncify is totally different between them... when we get rid of fastcomp it'll all be better.

@Beuc
Copy link
Contributor

Beuc commented Sep 8, 2019

Following https://groups.google.com/d/msg/emscripten-discuss/YWdLNgCqIjk/54XnVv7vAAAJ I tested the patch.

Functions such as byn$fpcast-emu$__pyx_pw_11pygame_sdl2_7display_6Window_13flip aren't reported as missing anymore (note: fpcast-emu not fpcast_emu as I wrote in the mailing list).

I don't experience any difference in the resulting executable's behavior though, possibly those functions aren't crucial in the whitelist.

(incidentally these functions are head-truncated in console.trace at 'emu': emu$__pyx_pw_11pygame_sdl2_7display_6Window_13flip http://localhost:8000/index.js line 97 > WebAssembly.instantiate:8823207)

@kripken kripken requested a review from sbc100 September 9, 2019 16:36
@kripken
Copy link
Member Author

kripken commented Sep 9, 2019

Thanks for testing @Beuc!

console.trace is the normal JS feature, I assume? I guess it truncates very long names, although it seem odd to truncate from the head...

@Beuc
Copy link
Contributor

Beuc commented Sep 9, 2019

I guess it truncates very long names, although it seem odd to truncate from the head...

I don't believe so, because all function names are head-truncated at 'emu' independently of their length. I was thinking maybe some parsing went awry due to the '-'.

@kripken
Copy link
Member Author

kripken commented Sep 10, 2019

@Beuc Interesting theory, seems plausible. I did a manual test now with a function with - in the middle, and didn't see that, though, so not sure what's going on.

@AjayP13
Copy link
Contributor

AjayP13 commented Sep 17, 2019

What does this fix? If fpcast-emu functions were on the stack when unwinding, would it not unwind properly? I'm running into an issue where I'm getting unreachable errors after running an asyncified function, when looking at the stack when unwinding, I do see a fpcast-emu on the stack.

@kripken
Copy link
Member Author

kripken commented Sep 17, 2019

@AjayP13 If one of these functions is on the stack when unwinding, but the blacklist/whitelist mechanism incorrectless told us to not prepare it for unwinding, then that would be a problem. Otherwise, though, this would just work (these functions would be instrumented just like any other).

This PR fixes the ability to whitelist/blacklist these functions, but nothing else. So if you aren't using the whitelist/blacklist, it will have no effect.

@kripken kripken merged commit b7e0950 into incoming Sep 19, 2019
@kripken kripken deleted the fpcast_asyncify branch September 19, 2019 21:13
belraquib pushed a commit to belraquib/emscripten that referenced this pull request Dec 23, 2020
)

So that the asyncify whitelist can affect the fpcast helper functions.

Regrettable that the fastcomp and upstream paths diverge a little here, but asyncify is totally different between them... when we get rid of fastcomp it'll all be better.
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.

None yet

4 participants