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?