SDL_RWops I/O macros (SDL_RWwrite() et al) are problematic.

[stdjon]

I'm trying to cross compile my codebase with Emscripten, and I ran into the following problem with my File class, which is a (pretty thin) wrapper around an SDL_RWops:

int File::write(const void *ptr, int size, int num) {
    int result = -1;
    if(rwops()) {
        result = SDL_RWwrite(rwops(), ptr, size, num);              // ** problem line **
    }
    if(result < 1) {
        tracef(CH_FILE, 1, "Can't write File - %s", SDL_GetError());
    }
    return result;
}

When this executes, I get the following trace in the Firefox console:

exception thrown: TypeError: FUNCTION_TABLE[$14] is not a function,__ZN2fw2io4File5writeEPKvii@file:///home/jon/devel/fw/BRANCH/CROSS2/bin/debug/emscripten/test.js:320694:6
__ZN5tests2fw9test_fileEv/$10<@file:///home/jon/devel/fw/BRANCH/CROSS2/bin/debug/emscripten/test.js:48016:45
__ZN5tests2fw9test_fileEv@file:///home/jon/devel/fw/BRANCH/CROSS2/bin/debug/emscripten/test.js:48016:6
_TFR_run@file:///home/jon/devel/fw/BRANCH/CROSS2/bin/debug/emscripten/test.js:134653:6
_main@file:///home/jon/devel/fw/BRANCH/CROSS2/bin/debug/emscripten/test.js:134241:6
callMain@file:///home/jon/devel/fw/BRANCH/CROSS2/bin/debug/emscripten/test.js:737025:9
doRun@file:///home/jon/devel/fw/BRANCH/CROSS2/bin/debug/emscripten/test.js:737078:7
run/<@file:///home/jon/devel/fw/BRANCH/CROSS2/bin/debug/emscripten/test.js:737090:19

Here's the (start of the) generated code, up to the end of the line marked as "problem line" above:

function __ZN2fw2io4File5writeEPKvii($this,$ptr,$size,$num){
 var label=0;
 var tempVarArgs=0;
 var sp=STACKTOP;STACKTOP=(STACKTOP+16)|0; (assert((STACKTOP|0) < (STACK_MAX|0))|0);
 label = 1; 
 while(1)switch(label){
 case 1: 
 var $1;
 var $2;
 var $3;
 var $4;
 var $result;
 var $5=sp;
 var $6;
 var $7;
 $1=$this;
 $2=$ptr;
 $3=$size;
 $4=$num;
 var $8=$1;
 $result=-1; //@line 136 "src/fw/io/file.cpp"
 var $9=__ZNK2fw2io4File5rwopsEv($8); //@line 137 "src/fw/io/file.cpp"
 var $10=($9|0)!=0; //@line 137 "src/fw/io/file.cpp"
 if($10){label=2;break;}else{label=3;break;} //@line 137 "src/fw/io/file.cpp"
 case 2: 
 var $12=__ZNK2fw2io4File5rwopsEv($8); //@line 138 "src/fw/io/file.cpp"
 var $13=(($12+8)|0); //@line 138 "src/fw/io/file.cpp"
 var $14=HEAP32[(($13)>>2)]; //@line 138 "src/fw/io/file.cpp"
 var $15=__ZNK2fw2io4File5rwopsEv($8); //@line 138 "src/fw/io/file.cpp"
 var $16=$2; //@line 138 "src/fw/io/file.cpp"
 var $17=$3; //@line 138 "src/fw/io/file.cpp"
 var $18=$4; //@line 138 "src/fw/io/file.cpp"
 var $19=FUNCTION_TABLE[$14]($15,$16,$17,$18); //@line 138 "src/fw/io/file.cpp"
 $result=$19; //@line 138 "src/fw/io/file.cpp"
 label=3;break; //@line 139 "src/fw/io/file.cpp"

I initially thought this might be due to incompatible function pointers (as there are some minor differences between my native SDL headers and the ones Emscripten uses).
But now I think it's an issue with the fact that SDL_RWwrite() is implemented as a macro in "SDL_rwops.h" as follows:

#define SDL_RWwrite(ctx, ptr, size, n)  (ctx)->write(ctx, ptr, size, n)

Looking at the assignments to vars $12, $13, $14, the compiled JS looks like it's trying to treat the 'rwops' variable as a pointer into "heap" memory and pull out the 'write' function pointer, as the native code in the macro would. But in library_sdl.js, rwops objects are currently implemented as JavaScript Objects, and (AFAICT) they don't even have a 'write' member, so I'm not sure how this is going to work.

This issue probably applies to the other macros in "SDL_rwops.h" as well (SDL_RWread, SDL_RWseek, etc.)

As the majority of my I/O is done through RWops, then I don't know how much further I can go with my port for the time being. Depending on the timeline for a full SDL_RWops-compatible version of in Emscripten, I may consider an interim solution, and I'm not sure which is going to be the least amount of work:

1. I can pull in an emcc-compiled version of SDL_rwops.c, and strip out anything in library_sdl.js which is currently using SDL_RWops (I think that's just IMG_Load_RW and Mix_LoadWAV_RW?)
2. Or, I can comment out the macros in SDL_rwops.h and try to implement all of SDL_RWops on the JavaScript side, probably as functions using the FS API. I would guess this is closer to a 'proper' fix for the issue, but I'm not actually sure how much work this might involve.

My temptation right now is go with option #1, but if there is 'official' work planned in this area in the short term, then I might leave this alone for now and wait for a later Emscripten release. On the other hand, if anyone thought it was useful, then I could have a go at putting a patch together if I'm able to come up with a solution that works.

It's a fascinating piece of tech, by the way :)

[kripken]

The safest thing is likely for you to use the SDL2 port, issue #2404 , which should have fully-functional RWops stuff.

Otherwise, this might be solvable in the SDL1 code, but we that would be less preferable.

[stdjon]

OK, I'll take a look at that and see if I can make the SDL2 solution work for me -- there are probably other benefits to an SDL upgrade anyway. If I make any progress either way I'll let you know.
[edit]: Looking at https://github.com/Daft-Freak/emscripten/tree/sdl2, there didn't seem to be any significant changes to the SDL_RWwops code (that I could see anyway). I'll see if I can pull out a subset of my code (the File class) and get it passing tests against that branch.

[stdjon]

OK, I've (finally) got the version of SDL2 from https://github.com/Daft-Freak/SDL-emscripten compiling against a recent emsdk_portable, and everything seems to be working as intended! We can probably flag this as resolved, as SDL1 isn't a priority for me personally any more.

The only minor niggle I have is that I had interference from the old SDL1 headers still being on the include path, so I ended up 'fixing' that by going into tools/shared.py and commenting out the entry in C_INCLUDE_PATHS (i.e. this line: https://github.com/kripken/emscripten/blob/master/tools/shared.py#L722). Is there a more elegant way to do that? :D

[kripken]

Not currently, but when we finish the SDL2 work, the plan is to have -lSDL1 vs -lSDL2 which will affect that. cc @juj

[juj]

Removing the old SDL linking is #2730.