emscripten-core / emscripten

Emscripten: An LLVM-to-WebAssembly Compiler
Other
25.62k stars 3.28k forks source link

wasm function pointers are saved in the wrong function table when float is a parameter #5623

Closed waterlike86 closed 4 years ago

waterlike86 commented 6 years ago

Hi, we have a function with signature static void draw(_Data const device, Ren const , Point const &, Point const &, float start, float end, float tilt, RGB32 const *);

when built with Asmjs without PRECISE_F32=1

when built with ASMJS With PRECISE_F32=1

when built with Wasm

i tried to reproduce the problem with a simple app but couldnt. my build flags are -Oz -s WASM=1 -s ALLOW_MEMORY_GROWTH=1 -g -s DEMANGLE_SUPPORT=1 -s SAFE_HEAP=1 -s EVAL_CTORS=0 -s ASSERTIONS=2 -s DISABLE_EXCEPTION_CATCHING=0 -s "BINARYEN_TRAP_MODE='js'"

<>

include

include

static void setme(float a, float a1){ printf("setme called"); } class FOO { public: void (bar)(float, float); void setFuncpointer(void (funct) (float f, float f1) ){ bar = funct; } void callme(float a, float b){ if(NULL != bar) bar(a, b); } }; void test(float a, float b) { FOO fo; fo.setFuncpointer(setme); fo.callme(a,b); } int main() { test(5.1654f, 0.0f); }

what could be the reason?

kripken commented 6 years ago

One possibility is that the app casts function pointers in an unsafe way (it is undefined behavior to cast and call void (*func)(float) as void (*func)(double), for example).

You say you can't reproduce the problem in a small testcase, but also provide a small testcase - does it not show the bug? I'm confused.

waterlike86 commented 6 years ago

Sorry for the confusion Alon. please ignore the earlier testcase. i managed to repro the problem in the testcase below.

the problem is as you described where the function pointer was cast to something else, and in this particular case, it worked on ASMJS and didnt work when using WASM. these sort of cases is particularly hard to find when the app is big. is there anyway we can spot these errors at build time?

---- test case here---

include

include

typedef void ( Action_Routine)(...); typedef void ( Double_Routine)(double, double);

static void setme(float a, float a1){ printf("setme called\n"); }

class Action_Entry { public: void set (Action_Routine extra){ m_extra = extra; } Action_Routine m_extra; };

class FOO { public: Action_Entry bar; void setFuncpointer(Action_Routine xtra ){ bar.set(xtra); } void callme(float a, float b){ ((Double_Routine)bar.m_extra)(a, b); } };

void test(float a, float b) { FOO fo; fo.setFuncpointer((Action_Routine)&setme); fo.callme(a,b); }

int main() { test(5.1654f, 0.0f); }

kripken commented 6 years ago

Ok, yeah, then this is that known issue of function pointer casting. In wasm it's a little larger than in asm.js because of the additional double/float difference (which in asm.js by default doesn't exist since we just use doubles).

That suggests that we could add an option to standardize on doubles at the function call level - so all arguments to functions would be cast to doubles etc. Would add some overhead but would prevent double/float function pointer issues. I'm not sure if it's worth it, as double/float is just one set of problems here. But it would be easy to add.

There isn't a good way to detect this at build time. The compiler can see function pointer casts, but it can't tell how the cast may be used later (unless it's cast and immediately used right there). But we do have the runtime ASSERTIONS=2 mode which I think you're already aware of, which should print out useful details in a crash.

You do have the EMULATE_FUNCTION_POINTER_CASTS option, which will add runtime overhead to convert arguments so things work.

stale[bot] commented 4 years ago

This issue has been automatically marked as stale because there has been no activity in the past year. It will be closed automatically if no further activity occurs in the next 7 days. Feel free to re-open at any time if this issue is still relevant.