-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
Between 1.39.10 and 1.39.11 ASYNCIFY_IGNORE_INDIRECT and/or SDL2 broke #10746
Comments
Some things I'd try: building with It may also be useful to bisect to find the relevant commit. May need to bisect in all 3 repos though (emscripten, llvm, binaryen). I can't think of a change we made to how indirect calls are handled in asyncify, so this is mysterious. One possibility is that an llvm change (like an optimizer change) affected indirect calls - perhaps there is an indirect call now where there wasn't one earlier, and so your code breaks. |
Thank you for the fast feedback. To really ensure, that my app did not just "by chance" work with 1.39.10, I also downgraded to 1.39.9 down to 1.39.7: Works perfect. It only breaks from 1.39.11 on. For making sure, that the problems are not due to LLVM doing optimizations, I tried deliberately compiling with I also tried Just for being on the safe side: Does it play any role, that I am not working with I will try to strip down my app to have a minimal app. About bisecting to find the relevant commit: I have no idea how to do that; if there are some hints how to, then I could do this (potentially) gruntwork. What I did: I looked through all commits from 1.39.11 and as a beginner and outsider, it is not trivial to guess, what might have gone wrong. The following three commits are my best guesses: |
@kripken I have a minimal.c file that reproduces the problem. You can find it here in pastebin: https://pastebin.com/Yvf0Zeci It seems, there is a problem with When you run the code, you will see in the stdout:
The output Additionally to the pastebin above, here is the code:
|
Thanks @sy2002 ! Looks like this is what's happening here (stack trace from a sleep event right before the assertion is hit):
cc @Beuc @Daft-Freak - I believe that was added intentionally (to get something to work with SDL+Asyncify), so we should probably document this and maybe even error at compile time? Btw, is there a way to do something like |
@kripken it seems that this issue emscripten-ports/SDL2#70 and the related PR emscripten-ports/SDL2#97 is responsible for this new behaviour. I tried the Whitelist from emscripten-ports/SDL2#70 (comment) but no success:
@Beuc writes in emscripten-ports/SDL2#70 (comment)
How can I specify this call path? Is there any workaround for me (other than sticking to 1.39.10)? For me, the general question is: What was this change coming from this PR emscripten-ports/SDL2#97 good for after all? It breaks code that worked fine (and fast) before with 1.39.10 ... And I bet it does not only break mine... Might a general roll-back to the old behaviour in the SDL Port (i.e. a roll back of PR emscripten-ports/SDL2#97) the way forward (for now)? |
Doesn't removing |
@sbc100 yes, it does. But then the code runs 2x slower. And before 1.39.11 everything worked fine. |
This is a good start. What kind of error ("no success") does that produce?
This is the call stack when calling e.g. SDL_RenderPresent. It is displayed when you get the "unreachable" error. By providing a complete whitelist, your program will run as fast as possible.
You don't have to call "emscripten_sleep" yourself anymore, SDL2 will do it at the right time. Most probably you can drop your own "emscripten_sleep" calls. This means desktop SDL2 programs can be run with asyncify without any change. I can see a few way forwards:
|
@Beuc Thank you for your detailed feedback. Very valuable. I will try it tomorrow and report back here. If it works, I will gladly volunteer for updating the documentation to help the community. But please let me ask one last devil's advocate question here: Before the change in 1.39.11, all someone had to do when porting/writing an SDL app was to add a
here and there. And sometimes - just like in "minimal.c" line 22 - "here and there" meant just one single line, one single place in the code. Also in my app (it is an emulator for a 16-bit CPU on an FPGA) it was just one single line (link FYI here). Now, the change in 1.39.11 forces people to invest quite some work, re-run and re-test the app thoroughly and investigate call paths by trial and error and add very lengthy So here comes the devil's advocate question: Was that worth it? (And as a bonus: I am a newby at Emscripten: Is it usual Emscripten policy to break backward-compatibility when increasing a sub-minor version of the SDK e.g. from 1.39.10 to 1.39.11?) Please do not get me wrong; I am thankful that Emscripten exists and that you are helping me. And as said, I will volunteer for the documentation, if I manage to get my app running again and you decide this thing stays as it is. |
Emscripten doesn't do semantic versioning. So yes, if there are breakages there can appear with minor version bumps. Of course we try to avoid unnecessary breakages. We try to weight possible breakages against the number of effected users and the difficulty of the fix/workaround needed. In this case, the users effect are anyone using SDL2 + ASYNCIFY_WHITELIST which I imagine is quite a small number. However, I'm not even sure that the breakage was anticipated. In general it seems that the |
I do not use |
I believe I anticipated this in emscripten-ports/SDL2#70 (comment) This warranted an entry in ChangeLog.md indeed. Anyway, at the time I suggested we could define a new SDL2 API call to toggle auto-calling emscripten_sleep. @sy2002 I understand this change was unexpected, but asyncify is young. This change aims at improving portability, and having to study precisely where emscripten_sleep() is needed (3 precise cases in SDL2), for each ported program. Also from experience Emscripten builds still break regularly as things get more streamlined, expect this to happen from time to time :/ One issue is that while the fix has been sitting in SDL2 for months, it's only being really exposed now with the recent tagging of SDL2 version_19. Not sure what's the best approach here - tag more frequently, have people complain that devs tag without enough testing, or tag less frequently, have people complain devs break things ;) |
Hmm, I think I suggested a hint at the time, |
A hint does seem more appropriate than a new separate API for this quirk. |
I'd forgotten about the hint idea :) SDL2 calls emscripten_sleep when:
so
|
@Beuc: I love your idea about the hint. +1 for |
Not sure how useful disabling the emscripten_sleep in SDL_Delay is, but still not an asyncify user... so no strong opinion either way. |
(@sy2002 not my idea but @Daft-Freak's :)) @Daft-Freak I didn't consider it, but keeping the SDL_Delay/SDL_WaitEvent's emscripten_sleep call unconditional is an option indeed. Another question is whether the hint enables or disables the calls (default behavior). |
@Beuc: I guess the hint would disable the calls. You said:
So if this is the aim of the patch in 1.39.11, then it should continue to be ON by default and people like me and @Akaricchi can use the hint to switch it OFF if we know what we are doing e.g. to avoid problems with |
@Beuc: This minimal test program here (minimal.c) is very short and very simple: https://pastebin.com/Yvf0Zeci
Still, I am getting this "unreachable" error:
Do you have more ideas what I could do (because when I get minimal.c running then I know how to get my app running again)? And/or could you please try to compile this minimal.c (Pastebin Link) on your side using |
I'm not sure asyncify is meant to work with both WHITELIST and IGNORE_INDIRECT. |
Another possible option here: Make the SDL2 port in emscripten add This is not actually possible atm, since the whitelist means "only these" and the blacklist means "ignore these". We'd need to add another list, "instrument these". (Not sure if this is better than the other options already being discussed - it's probably more work, so maybe it's worse...) |
|
@Akaricchi Which brings us back to the discussion of having a hint like |
Yeah, maybe my idea doesn't make sense for the reasons you mentioned. I don't know the SDL side that well. However, about
IIUC the problem happens because Asyncify doesn't know that |
|
Thanks @Akaricchi , then if we agree on that direction, sounds like we need to add both |
By the way, this would be a pretty good case for #10629 if SDL was to be used through the standard pkg-config or sdl2-config mechanism rather than emscripten's built-in port system. SDL could add something like |
That said, we still need a mechanism to disable asyncify usage in SDL, and I think a hint is appropriate for that. |
A hint sounds reasonable to me. I'm also happy to work on the asyncify additions I proposed. Whatever people decide is best here. |
Agreed about inhibiting all calls.
Ideally we should send all emscripten patches upstream, and have the new hint documented on the SDL wiki and in the headers. |
I opened PR emscripten-ports/SDL2#112 |
@Beuc: Thank you! Great! I will test this in the next few days and report back here. |
@Beuc: SUCCESS :-) I tested my original FPGA emulator application by patching my local Great work! Thank you! From my perspective, you might want to merge the PR. I wrote some documentation about the topic and published it on Medium: If anyone has comments on the documentation or wants me to publish it additionally somewhere else, please do not hesitate to tell me. I will monitor the PR and future emscripten releases and as soon as this is officially released, I will close this issue. Thanks again to all of you. |
Thanks for testing! |
Great progress here, thanks everyone! I still hope to find time for the new asyncify option at some point, which I think would still have value (but is not urgent). |
I retested everything using the newly released version 1.39.14, which includes the fix. |
BTW: @Beuc just a small reminder about the https://wiki.libsdl.org/SDL_HINT_EMSCRIPTEN_ASYNCIFY page you wanted to create (I would do it for you if you want and if I had write access to the Wiki) |
Done! |
@Beuc: Thank you. I just read the Wiki Page. Great job! |
And add a fix for emscripten-core/emscripten#10746.
Asyncify does a whole-program analysis to figure out the list of functions to instrument. In emscripten-core/emscripten#10746 (comment) we realized that we need another type of list there, an "add list" which is a list of functions to add to the instrumented functions list, that is, that we should definitely instrument. The use case in that link is that we disable indirect calls, but there is one special indirect call that we do need to instrument. Being able to add just that one can be much more efficient than assuming all indirect calls in a big codebase need instrumentation. Similar issues can come up if we add a profile-guided option to asyncify, which we've discussed. The existing lists were not good enough to allow that, so a new option is needed. I took the opportunity to rename the old ones to something better and more consistent, so after this PR we have 3 lists as follows: * The old "remove list" (previously "blacklist") which removes functions from the list of functions to be instrumented. * The new "add list" which adds to that list (note how add/remove are clearly parallel). * The old "only list" (previously "whitelist") which simply replaces the entire list, and so only those functions are instrumented and no other. This PR temporarily still supports the old names in the commandline arguments, to avoid immediate breakage for our CI.
See emscripten-core/emscripten#10746 and emscripten-ports/SDL2#112 Fixes Bugzilla #4997. --HG-- extra : histedit_source : f9552ea393d4b3752a23c237a6e75cde2b4d5fa9
See emscripten-core/emscripten#10746 and emscripten-ports/SDL2#112 Fixes Bugzilla #4997.
See emscripten-core/emscripten#10746 and emscripten-ports/SDL2#112 Fixes Bugzilla #4997.
See emscripten-core/emscripten#10746 and emscripten-ports/SDL2#112 Fixes Bugzilla #4997.
See emscripten-core/emscripten#10746 and emscripten-ports/SDL2#112 Fixes Bugzilla #4997.
Using emsdk 1.39.10 works like a charm for me and in 1.39.11 I am receiving the following error message when running my SDL2 program:
RuntimeError: unreachable executed
I am compiling using these flags:
-O3 -s ASYNCIFY -s ASYNCIFY_IGNORE_INDIRECT -s USE_SDL=2
If I remove
-s ASYNCIFY_IGNORE_INDIRECT
then it works, also in 1.39.11, but it cuts the performance of my program by factor two to three.Compiling with the debug option
-g
and removing-O3
yields the following, more verbose error:I am an Emscripten beginner, so I do not know how to debug this further. But I am happy to provide more information and/or to debug more, if someone tells me how :-)
The text was updated successfully, but these errors were encountered: