Skip to content
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

EMTERPRETIFY + EMULATED_FUNCTION_POINTER_CASTS + WASM can mess up f32 indirect call args #6759

Open
Beuc opened this issue Jun 27, 2018 · 9 comments

Comments

@Beuc
Copy link
Contributor

@Beuc Beuc commented Jun 27, 2018

After many hours of debugging I'm facing the following situation:

  • a Cython function for a static module like this:
    cdef void color(self, float r, float g, float b, float a):
        glUniform4f(self.Color, r, g, b, a)
  • is called like this from another static module:
        mod1.color(1.0, 1.0, 1.0, 1.0)
  • both modules compiled as .o
  • when linked with WASM, the arguments are: 1.0,1.0,1.0,1.0
  • when linked with Emterpreter, the arguments are: 0.0,0.0,0.0,0.0 (resulting in an extremely puzzling black screen)

I tried to reproduce the issue with a simple test case but it works correctly.
From what I see in the Cython-generated code, the function signatures match (I suspected some double->float mismatch but that doesn't seem to be the case).

I'll try and investigate further, meanwhile any tip on how to debug this are appreciated :)

@Beuc

This comment has been minimized.

Copy link
Contributor Author

@Beuc Beuc commented Jun 27, 2018

Got it. It involves calling a function pointer in an Emterpreter binary with EMULATE_FUNCTION_POINTER_CASTS=1.

Here's a minimal test case:

embug-call.c:

static void (*pf)(float, float, float, float);
void call_pf() {
  pf(1.0, 1.0, 1.0, 1.0);
}
void set_pf(void (*f)(float, float, float, float)) {
  pf = f;
}

embug-set.c:

#include <stdio.h>
void set_pf(void (*f)(float, float, float, float));
void call_pf(void);

void f(float r, float g, float b, float a) {
  printf("%f,%f,%f,%f\n", r,g,b,a);
}
int main(void) {
  set_pf(&f);
  call_pf();
}

compile:

emcc embug-set.c -o embug-set.o
emcc embug-call.c -o embug-call.o
emcc -s EMULATE_FUNCTION_POINTER_CASTS=1 -s TOTAL_MEMORY=$((48*1024*1024)) -s EMTERPRETIFY=1 -s EMTERPRETIFY_ASYNC=1 -s EMTERPRETIFY_FILE=embug.em embug-set.o embug-call.o -o embug.html

Expected:

1.000000,1.000000,1.000000,1.000000

Result:

0.000000,0.000000,0.000000,0.000000

This is nasty o_O

@Beuc

This comment has been minimized.

Copy link
Contributor Author

@Beuc Beuc commented Jun 27, 2018

Note: when switching all parameters to double the issue is not present.

@kripken

This comment has been minimized.

Copy link
Member

@kripken kripken commented Jun 27, 2018

Thanks! Here's a slightly smaller testcase:

// build with EMULATE_FUNCTION_POINTER_CASTS=1 -s EMTERPRETIFY=1

#include <stdio.h>

typedef void (*T)(float);

void call(T f) {
  f(1.0);
}

void f(float r) {
  printf("1337 %f\n", r);
}

int main(void) {
  call(&f);
}

Turns out the issue here is that the emterpreter assumes asm.js types: i32 and f64, nothing else. So it converts float params to double. Normally this doesn't matter (using doubles to represent floats always does so with full precision), but emulated function pointers actually does care about matching those up, as it uses an ABI that is sensitive to that - in our case here, the call (created by emterpretify.py) uses an f64, and the ABI conversion shim (created by src/passes/FuncCastEmulation.cpp in binaryen) reinterprets those bits to an i64, then it is received as an i64 but assumed to be an f32, so we reinterpret the lower 32 bits as that, and they are all zero.

I suspect we've just never tested the combination of EMTERPRETIFY + EMULATED_FUNCTION_POINTER_CASTS + WASM (wasm matters since the ABI is different in asm.js, in fact exactly because asm.js only has i32s and f64s - so it can use f64s for everything, with no f32s or i64s to worry about).

For your immediate issue, I'd suggest disabling wasm (-s WASM=0).

But it would be good to fix this. One option is to disallow f32s in function pointer calls, perhaps (that would mean changing src/passes/FuncCastEmulation.cpp in binaryen). Another is to make the emterpreter aware of wasm having f32s, and not assume everything is an i32 or f64.

Btw, you can do -s TOTAL_MEMORY=48MB instead of -s TOTAL_MEMORY=$((48*1024*1024)) :)

@kripken kripken changed the title Emterpreter can reset function arguments to zero EMTERPRETIFY + EMULATED_FUNCTION_POINTER_CASTS + WASM can mess up f32 indirect call args Jun 27, 2018
@kripken kripken added the help wanted label Jun 27, 2018
@Beuc

This comment has been minimized.

Copy link
Contributor Author

@Beuc Beuc commented Jun 28, 2018

I confirm the asm.js version works correctly (just needing 3x more time to compile T_T).

When you say "One option is to disallow f32s in function pointer calls", would that mean that my current code with float-s would be unsupported?
(not necessarily a problem, just checking)

@kripken

This comment has been minimized.

Copy link
Member

@kripken kripken commented Jun 28, 2018

Sorry, I wasn't clear there: I meant that we'd automatically translate f32s to f64s for such calls. That's what asm.js does anyhow, and it's probably not that much overhead compared to what the emulation does overall.

renpytom added a commit to renpy/renpy that referenced this issue Apr 28, 2019
This is because of a bug in emscripten, see
emscripten-core/emscripten#6759 for
the details.
renpytom added a commit to renpy/renpy that referenced this issue Apr 28, 2019
This is because of a bug in emscripten, see
emscripten-core/emscripten#6759 for
the details.
@stale

This comment has been minimized.

Copy link

@stale stale bot commented Sep 18, 2019

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.

@stale stale bot added the wontfix label Sep 18, 2019
@Beuc

This comment has been minimized.

Copy link
Contributor Author

@Beuc Beuc commented Sep 19, 2019

The issue is still present in Emterpreter.

@stale stale bot removed the wontfix label Sep 19, 2019
@sbc100 sbc100 added the fastcomp label Sep 19, 2019
@sbc100

This comment has been minimized.

Copy link
Collaborator

@sbc100 sbc100 commented Sep 19, 2019

@kripken do you think this is worth fixing? Labeling as fastcomp only.

@kripken

This comment has been minimized.

Copy link
Member

@kripken kripken commented Sep 19, 2019

I think it's probably too late in fastcomp's lifecycle to be worth fixing - wasm backend + Asyncify is the replacement. But if someone needs this and has a patch I wouldn't object.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.