Between 1.39.10 and 1.39.11 ASYNCIFY_IGNORE_INDIRECT and/or SDL2 broke

[sy2002]

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:

13:11:26.047 RuntimeError: unreachable executed
    SDL_RenderPresent http://localhost:8080/qnice.js line 1947          > WebAssembly.instantiate:247773
    vga_one_iteration_screen http://localhost:8080/qnice.js line 1947   > WebAssembly.instantiate:105533
    main http://localhost:8080/qnice.js line 1947                       > WebAssembly.instantiate:55290
    x http://localhost:8080/qnice.js:8816
    handleSleep http://localhost:8080/qnice.js:8900
    safeSetTimeout http://localhost:8080/qnice.js:5181

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 :-)

[kripken]

Some things I'd try: building with ASSERTIONS, and building with SAFE_HEAP. If those don't help, simplifying things codebase into a tiny testcase you can submit would be helpful.

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.

[sy2002]

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 -O0 -g with no positive effect.

I also tried -O0 -g -s SAFE_HEAP=1 -s ASSERTIONS=2 -s ASYNCIFY -s ASYNCIFY_IGNORE_INDIRECT -s USE_SDL=2 with no effect, i.e. not more output on the console than what I printed above in the initial issue.

Just for being on the safe side: Does it play any role, that I am not working with
emscripten_set_main_loop_timing but that I am yielding CPU cycles in my
own main_loop using emscripten_sleep(0)?

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:

fd1ed69

a31a155

6acb404

[sy2002]

@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 SDL_RenderPresent. Might this have been introduced in one of the three commits listed above?

When you run the code, you will see in the stdout:

INIT: OK
Before potentially unreachable SDL_RenderPresent

The output After potentially unreachable SDL_RenderPresent is never reached, though, this is why I suspect SDL_RenderPresent to be "guilty".

Additionally to the pastebin above, here is the code:

#include "emscripten.h"
#include "SDL.h"

SDL_Window*          win;
SDL_Renderer*        renderer;
SDL_Texture*         screen_texture;
Uint32*              screen_pixels;

int action_loop()
{
    unsigned long pixel_amount = 100 * 100 * 4;
    screen_pixels = malloc(pixel_amount * sizeof(Uint32));
    if (!screen_pixels)
    {
        printf("action_loop: malloc screen_pixels failed\n");
        return 5;
    }

    while (1)
    {
        emscripten_sleep(0);
        SDL_UpdateTexture(screen_texture, NULL, screen_pixels, 100 * 4);
        SDL_RenderCopy(renderer, screen_texture, NULL, NULL);
        printf("Before potentially unreachable SDL_RenderPresent\n");
        SDL_RenderPresent(renderer);
        printf("After potentially unreachable SDL_RenderPresent\n");        
    }
    return 0;
}

int main(int argc, char **argv)
{
    if (SDL_Init(SDL_INIT_VIDEO) == 0)
    {
        win = SDL_CreateWindow("QNICE Emulator", SDL_WINDOWPOS_UNDEFINED, SDL_WINDOWPOS_UNDEFINED, 100, 100, SDL_WINDOW_OPENGL | SDL_WINDOW_RESIZABLE);
        if (win)
        {
            renderer = SDL_CreateRenderer(win, -1, SDL_RENDERER_ACCELERATED);
            if (renderer)
            {
                SDL_SetHint(SDL_HINT_RENDER_SCALE_QUALITY, "1");
                SDL_SetRenderDrawColor(renderer, 0, 255, 0, 0);
                screen_texture = SDL_CreateTexture( renderer,
                                                    SDL_PIXELFORMAT_ARGB8888,
                                                    SDL_TEXTUREACCESS_STREAMING,
                                                    100,
                                                    100);
                if (!screen_texture)
                {
                    printf("Unable to screen texture: %s\n", SDL_GetError());
                    return 2;
                }
                else
                {
                    printf("INIT: OK\n");
                    return action_loop();
                }

            }
            else
            {
                printf("Unable to create renderer: %s\n", SDL_GetError());
                return 3;
            }
        }
        else
        {
            printf("Unable to create window: %s\n", SDL_GetError());
            return 4;
        }
    }
    else
        return 1;
}

[kripken]

Thanks @sy2002 !

Looks like this is what's happening here (stack trace from a sleep event right before the assertion is hit):

_emscripten_sleep@http://localhost:8000/b.js:107101:11
instrumentWasmImports/</imports[x]@http://localhost:8000/b.js:107325:24
Emscripten_GLES_SwapWindow@http://localhost:8000/b.js:92161:21
SDL_GL_SwapWindow@http://localhost:8000/b.js:88221:41
GLES2_RenderPresent@http://localhost:8000/b.js:82140:21
SDL_RenderPresent@http://localhost:8000/b.js:97223:39
action_loop@http://localhost:8000/b.js:86772:23
main@http://localhost:8000/b.js:71869:7

SDL_RenderPresent does an indirect call into SDL2 code, and that SDL2 code does an emscripten_sleep. So there is an indirect call on the stack while sleeping, and ASYNCIFY_IGNORE_INDIRECT will break things.

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 RenderPresent but without the sleep, if the user does a sleep themselves each frame (as in that test case)?

[sy2002]

@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:

-s ASYNCIFY_WHITELIST='["main", "SDL_WaitEvent", "SDL_WaitEventTimeout", "SDL_Delay", "SDL_RenderPresent", "GLES2_RenderPresent", "SDL_GL_SwapWindow", "Emscripten_GLES_SwapWindow", "byn$$fpcast-emu$$Emscripten_GLES_SwapWindow", "SDL_UpdateWindowSurface", "SDL_UpdateWindowSurfaceRects", "Emscripten_UpdateWindowFramebuffer"]'

@Beuc writes in emscripten-ports/SDL2#70 (comment)

Adding these funcs to ASYNCIFY_WHITELIST automatically could help but isn't enough, since the call path between main and SDL_WaitEvent/SDL_Delay/SDL_RenderPresent needs to be specified as well. Maybe there's somewhere to document it.

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)?

[sbc100]

Doesn't removing ASYNCIFY_IGNORE_INDIRECT fix the issue?

[sy2002]

@sbc100 yes, it does. But then the code runs 2x slower. And before 1.39.11 everything worked fine.

[Beuc]

I tried the Whitelist from emscripten-ports/SDL2#70 (comment) but no success:

-s ASYNCIFY_WHITELIST='["main", "SDL_WaitEvent", "SDL_WaitEventTimeout", "SDL_Delay", "SDL_RenderPresent", "GLES2_RenderPresent", "SDL_GL_SwapWindow", "Emscripten_GLES_SwapWindow", "byn$$fpcast-emu$$Emscripten_GLES_SwapWindow", "SDL_UpdateWindowSurface", "SDL_UpdateWindowSurfaceRects", "Emscripten_UpdateWindowFramebuffer"]'

This is a good start. What kind of error ("no success") does that produce?

Adding these funcs to ASYNCIFY_WHITELIST automatically could help but isn't enough, since the call path between main and SDL_WaitEvent/SDL_Delay/SDL_RenderPresent needs to be specified as well. Maybe there's somewhere to document it.

How can I specify this call path?

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.

For me, the general question is: What was this change coming from this PR emscripten-ports/SDL2#97 good for after all?

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:

• provide a complete ASYNCIFY_WHITELIST when compiling this program
• implement a new mode "auto-detect + additional list" (currently whitelisting disables auto-detection): keep asyncify detect functions to instrument automatically with ASYNCIFY_IGNORE_INDIRECT, while still being able to mark a few additional indirect call paths for explicit instrumentation
• specific variant of the above: initially populate the asyncify auto-detection list with the SDL2 call path-s

[sy2002]

@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

#ifdef __EMSCRIPTEN__
emscripten_sleep(0);
#endif

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 -s ASYNCIFY_WHITELIST=[...]statements, which were not necessary before. Just to be again at a status quo, where we happily already were in 1.39.10.

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.

[sbc100]

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 ASYNCIFY_WHITELIST is quite hard to use correctly. I wonder if there is some way we can make this feature less fragile?

[Akaricchi]

In this case, the users effect are anyone using SDL2 + ASYNCIFY_WHITELIST which I imagine is quite a small number

I do not use ASYNCIFY_WHITELIST and I've also been affected by this change. I use Asyncify for fibers only. My code uses emscripten_set_main_loop when compiled for emscripten, so that additional sleep in SDL_GL_SwapWindow is of no benefit to me. Not only that, it actually breaks my program in a bizarre way, making this ccall fail with this assertion (which may actually be an Asyncify bug). Fortunately I can work around this by ifdeffing the SDL_GL_SwapWindow call away, which kind of goes against the goal of running desktop programs unmodified...

[Beuc]

I believe I anticipated this in emscripten-ports/SDL2#70 (comment)
At the time fiber.h didn't exist yet but this fits the "case 2" there.

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 ;)

[Daft-Freak]

Hmm, I think I suggested a hint at the time, SDL_HINT_EMSCRIPTEN_SLEEP_IN_PRESENT maybe?

[Akaricchi]

A hint does seem more appropriate than a new separate API for this quirk.

[Beuc]

I'd forgotten about the hint idea :)

SDL2 calls emscripten_sleep when:

• refreshing the software graphics context,
• refreshing the GPU graphics context,
• using SDL_Delay,
• polling events (through SDL_Delay) - aka support SDL_WaitEvent

so SDL_HINT_EMSCRIPTEN_SLEEP_IN_PRESENT would be a misnomer.

SDL_HINT_EMSCRIPTEN_ASYNCIFY or
SDL_HINT_EMSCRIPTEN_PSEUDO_SYNCHRONOUS maybe?

[sy2002]

@Beuc: I love your idea about the hint. +1 for SDL_HINT_EMSCRIPTEN_ASYNCIFY

[Daft-Freak]

Not sure how useful disabling the emscripten_sleep in SDL_Delay is, but still not an asyncify user... so no strong opinion either way.

[Beuc]

(@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).

[sy2002]

@Beuc: I guess the hint would disable the calls. You said:

This change aims at improving portability, and [not] having to study precisely where emscripten_sleep() is needed (3 precise cases in SDL2), for each ported program.

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 -s ASYNCIFY_IGNORE_INDIRECT.

[sy2002]

@Beuc: This minimal test program here (minimal.c) is very short and very simple: https://pastebin.com/Yvf0Zeci
I removed the line #22 containing emscripten_sleep(0) and then used this here to compile (I enhanced your whitelist with the call path of the error message):

emcc minimal.c -g -O0 -s ASYNCIFY -s ASYNCIFY_IGNORE_INDIRECT -s USE_SDL=2 -o minimal.html -s ASYNCIFY_WHITELIST='["main", "SDL_WaitEvent", "SDL_WaitEventTimeout", "SDL_Delay", "SDL_RenderPresent", "GLES2_RenderPresent", "SDL_GL_SwapWindow", "Emscripten_GLES_SwapWindow", "byn$$fpcast-emu$$Emscripten_GLES_SwapWindow", "SDL_UpdateWindowSurface", "SDL_UpdateWindowSurfaceRects", "Emscripten_UpdateWindowFramebuffer", "action_loop", "x", "handleSleep", "safeSetTimeout"]'

Still, I am getting this "unreachable" error:

RuntimeError: unreachable executed minimal.js line 1770 > WebAssembly.instantiate:13453:1
    action_loop http://localhost:8080/minimal.js line 1770 > WebAssembly.instantiate:13453
    main http://localhost:8080/minimal.js line 1770 > WebAssembly.instantiate:18121
    x http://localhost:8080/minimal.js:8606
    handleSleep http://localhost:8080/minimal.js:8690
    safeSetTimeout http://localhost:8080/minimal.js:5004

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 -s ASYNCIFY_IGNORE_INDIRECT?

[Beuc]

I'm not sure asyncify is meant to work with both WHITELIST and IGNORE_INDIRECT.
In this minimal example, just drop IGNORE_INDIRECT and it'll work, with good perfs since WHITELIST contains all the functions to instrument. I believe the same approach can be used for the full program.

[kripken]

Another possible option here: Make the SDL2 port in emscripten add SDL_RenderPresent to the list of functions that definitely need asyncify instrumentation. That is, even if the user uses a whitelist or blacklist or disables indirect calls, we do know that specific function will do an indirect call that will sleep, to definitely instrument that.

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]

Another possible option here: Make the SDL2 port in emscripten add SDL_RenderPresent to the list of functions that definitely need asyncify instrumentation. That is, even if the user uses a whitelist or blacklist or disables indirect calls, we do know that specific function will do an indirect call that will sleep, to definitely instrument that.

1. IIRC SDL_RenderPresent itself does not do the sleep, it calls SDL_GL_SwapWindow (the GLES implementation of it) which does the sleep.
2. SDL_RenderPresent is only useful to projects that actually use SDL's 2D rendering subsystem. Many projects (mine included) have custom OpenGL renderers and these must call SDL_GL_SwapWindow instead. Although SDL_GL_SwapWindow is not actually required in the case of a WebGL canvas.
3. As said in my first comment, sleeping during SDL_GL_SwapWindow may not actually be what you want. If the project uses the emscripten_set_main_loop mechanism, any sort of asyncify usage in SDL is only detrimental. It's only useful when porting some code that does not have the web limitations in mind.

[sy2002]

@Akaricchi Which brings us back to the discussion of having a hint like SDL_HINT_EMSCRIPTEN_ASYNCIFY to be able to switch off the new behaviour introduced in 1.39.11?

[kripken]

@Akaricchi

Yeah, maybe my idea doesn't make sense for the reasons you mentioned. I don't know the SDL side that well. However, about

IIRC SDL_RenderPresent itself does not do the sleep, it calls SDL_GL_SwapWindow (the GLES implementation of it) which does the sleep.

IIUC the problem happens because Asyncify doesn't know that SDL_RenderPresent needs to be instrumented. It needs to be because it's on the stack during a sleep. If we instrument all such functions, everything would work. And specifically the problem here is that SDL_RenderPresent does an indirect call, which normally we assume means we need to instrument it, but with ignore-indirect we don't. So manually forcing it to be instrumented would fix this. (We'd also need to instrument all other functions doing indirect calls that can lead eventually to a sleep.)

[Akaricchi]

And specifically the problem here is that SDL_RenderPresent does an indirect call, which normally we assume means we need to instrument it, but with ignore-indirect we don't.

SDL_GL_SwapWindow also does an indirect call into Emscripten_GLES_SwapWindow, which is where the asyncify sleep is. So it needs to be instrumented as well.

[kripken]

Thanks @Akaricchi , then if we agree on that direction, sounds like we need to add both SDL_RenderPresent and SDL_GL_SwapWindow. (Actually, the safe thing might be to instrument all SDL_* calls, but maybe that's excessive...)

[Akaricchi]

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 -s ASYNCIFY_DEFINITELY_INSTRUMENT_THESE+=['_SDL_RenderPresent', '_SDL_GL_SwapWindow', '_Internal_Function'] to its link flags. An attribute annotation in the source code would once again be better, but probably harder to implement.

[Akaricchi]

That said, we still need a mechanism to disable asyncify usage in SDL, and I think a hint is appropriate for that.

[kripken]

A hint sounds reasonable to me. I'm also happy to work on the asyncify additions I proposed. Whatever people decide is best here.

[Akaricchi]

Agreed about inhibiting all calls.

Another question to the group: I would like to help updating the documentation and I was wondering, where would be the best place to write about it

Ideally we should send all emscripten patches upstream, and have the new hint documented on the SDL wiki and in the headers.

[Beuc]

I opened PR emscripten-ports/SDL2#112
This appears not to break RenPyWeb nor minimal.c.
Additional testing/feedback welcome (instructions in PR) :)

[sy2002]

@Beuc: Thank you! Great! I will test this in the next few days and report back here.

[sy2002]

@Beuc: SUCCESS :-) I tested my original FPGA emulator application by patching my local sdl2.py as described in your Pull Request comment (emscripten-ports/SDL2#112 (comment)), running emcc --clear-ports and then recompiling: Works like a charm.

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:
https://medium.com/@mirkoholzer/emscripten-sdl2-unreachable-executed-cebd8f034c32

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.

[Beuc]

Thanks for testing!
About documentation, I have access to wiki.libsdl.org and I think I'll add more info there e.g.at https://wiki.libsdl.org/SDL_HINT_EMSCRIPTEN_ASYNCIFY (not created yet)

[kripken]

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).

[sy2002]

I retested everything using the newly released version 1.39.14, which includes the fix.
Everything works perfectly! :-D
Therefore, I am closing this issue. Thanks, everyone!

[sy2002]

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)

[Beuc]

just a small reminder about the https://wiki.libsdl.org/SDL_HINT_EMSCRIPTEN_ASYNCIFY page you wanted to create

Done!
You can send remarks here or using the wiki's feedback form at the top-right.

[sy2002]

@Beuc: Thank you. I just read the Wiki Page. Great job!