Make js library functions compatible with LEGALIZE_JS_FFI=0

[kripken]

The LEGALIZE_JS_FFI flag can't be used in many cases, e.g.

./emcc tests/hello_libcxx.cpp -s MAIN_MODULE=1 -s LEGALIZE_JS_FFI=0

crashes. The reason is that e.g. for rintf we emit f32 calls in the backend, since we are not legalizing. But the js library implementation for rintf has hardcoded doubles in src/library.js. So we end up with the wrong type. That method and others would need to be ifdefed for the non-legalizing case, or we could implement them in C.

cc @awtcode

[waterlike86]

@kripken

while trying to use LEGALIZE_JS_FFI with the following flags

main.cpp -O3 --profiling -s ASSERTIONS=1 -s WASM=1 -s DISABLE_EXCEPTION_CATCHING=0
-s MAIN_MODULE=2 -s BINARYEN_TRAP_MODE='js' -s LEGALIZE_JS_FFI=0
-o %config%\index.html

and code below

#include <stdio.h>
#include <stdlib.h>
#include <math.h>
//#include <string>
//#include <dlfcn.h>

using namespace std;

const float y = 0.05000000074505806;
const int  x = 7;
int main()
{   
    int ran = rand() % 100;// v1 in the range 0 to 99
    float safeY = fmax((float) x / ran , y);
    printf("In main.cpp now %f\n", safeY);
    return 0;
}

yields this error

error: asm2wasm seeing an invalid argument type at index 0 (this will not validate) in call from $_main to $_llvm_maxnum_f32 (this is likely due to undefined behavior in C, like defining a function one way and calling it in another, which is important to fix)
error: asm2wasm seeing an invalid argument type at index 1 (this will not validate) in call from $_main to $_llvm_maxnum_f32 (this is likely due to undefined behavior in C, like defining a function one way and calling it in another, which is important to fix)
[wasm-validator error in function $_main] 3 != 4: call param types must match, on
[f64] (call $_llvm_maxnum_f32
 [f32] (f32.div
  [f32] (f32.const 7)
  [f32] (f32.convert_s/i32
   [i32] (call $i32s-rem
    [i32] (call $_rand)
    [i32] (i32.const 100)
   )
  )
 )
 [f32] (f32.const 0.05000000074505806)
)
(on argument 0)
[wasm-validator error in function $_main] 3 != 4: call param types must match, on
[f64] (call $_llvm_maxnum_f32
 [f32] (f32.div
  [f32] (f32.const 7)
  [f32] (f32.convert_s/i32
   [i32] (call $i32s-rem
    [i32] (call $_rand)
    [i32] (i32.const 100)
   )
  )
 )
 [f32] (f32.const 0.05000000074505806)
)
(on argument 1)

if i change float safeY = fmax((float) x / ran , y); to float safeY = fmax( x / ran , y);
the wasm generated will end up calling _llvm_maxnum_f64 and the build will pass.

what should be the expected behaviour?
cc @awtcode

[kripken]

Looks like the same issue, yes - f32/f64 differences with that flag.

[awtcode]

Sorry @kripken , I didn't really understand the crash. From what I see, there is an implementation of rintf in musl that uses float:
https://github.com/kripken/emscripten/blob/6e4b98636618989bcd99308391e51aa1b81f4c61/system/lib/libc/musl/src/math/rintf.c#L14

I also didn't manage to see the hardcoded doubles in library.js:
https://github.com/kripken/emscripten/blob/6d5b777ffd41b53ff2a6aaab3c354c4dc99af240/src/library.js#L1663

Did I miss something here?

[kripken]

I think the double comes in on the return. Stuff like +(x) convert to a double, which i see on rintf, maxnum, etc.. it should be fround(x) instead. I'd recommend changing that and seeing if it helps and if other tests break.

[waterlike86]

@kripken so if we removed -s BINARYEN_TRAP_MODE='js' from the link flags we were able to make the build pass because likely it didnt perform the checks, but this gave us problems at runtime.

consider this
with build flags
main.cpp -O3 --profiling -s ASSERTIONS=1 -s WASM=1 -s DISABLE_EXCEPTION_CATCHING=0
-s MAIN_MODULE=2 -s LEGALIZE_JS_FFI=0 -o %config%\index.html

and code below

#include <stdio.h>
#include <stdlib.h>
#include <math.h>
#include <stdexcept>
#include <cstdint>

using namespace std;

uint64_t getbigint(){
    int ran = rand() % 100;// v1 in the range 0 to 99
    ++ran;
    if(ran > -1)
        throw new std::runtime_error("error!!");

    return 1152921504606846975 + ran;
}
int main()
{   
    uint64_t mybig = 0;
    printf("getbigint--\n");
    try{
        mybig = getbigint();
    }
    catch(std::runtime_error){};
    printf("getbigint++ %lld\n", mybig);
    return 0;
}

this was crashing at runtime with error

Uncaught TypeError: wasm function signature contains illegal type
    at _main (wasm-function[315]:63)
    at Object._main (index.js:6291)
    at Object.callMain (index.js:6650)
    at doRun (index.js:6686)
    at index.js:6695

this seemed to be harmless and i expected a direct call to the getbigint function, but it ended up generating an invoke_j function.

do we need to LEGALIZE invoke functions as well?

[kripken]

@waterlike86 I think you're right, thanks. I opened WebAssembly/binaryen#1883 , does that fix things for you?

[waterlike86]

@kripken no unfortunately no. now i get a different call stack

exception thrown: TypeError: wasm function signature contains illegal type,TypeError: wasm function signature contains illegal type
    at Object.ftCall_j [as dynCall_j] (http://localhost:8080/index.js:5906:31)
    at invoke_j (http://localhost:8080/index.js:5548:29)
    at legalfunc$invoke_j (wasm-function[289]:3)
    at _main (wasm-function[332]:63)
    at Object._main (http://localhost:8080/index.js:6328:29)
    at Object.callMain (http://localhost:8080/index.js:6687:28)
    at doRun (http://localhost:8080/index.js:6723:58)
    at http://localhost:8080/index.js:6732:4

seems like original invoke_j call got turned into a $legalfunc$invoke_j

  (func $legalfunc$invoke_j (type $t51) (param $p0 i32) (result i64)
    get_local $p0
    call $legalimport$invoke_j
    i64.extend_u/i32
    call $getTempRet0
    i64.extend_u/i32
    i64.const 32
    i64.shl
    i64.or)

but shouldnt $legalfunc$invoke_j result type be i32? eventually this calls into dynCall_j where i dont see it being legalized as well. am i missing something?

[kripken]

Ah, must be emscripten needs to legalize the invokes on the JS side too. I was hoping that would just work, but I guess not.

[awtcode]

@kripken , are you referring to the cases whereby we call the invokes from Wasm?

[kripken]

I believe we always call invokes from wasm - wasm calls the invoke, which is in JS, and the invoke uses JS try-finally around a call back into wasm.

I think the issue is that that binaryen PR legalizes the invoke functions, but emscripten's JS is not yet aware of that and is calling the wrong ones back. Not sure though.

[awtcode]

Thanks for your clarification @kripken :) Do you happen to have the bandwidth to help us resolve this issue? I believe this would most probably be the last one that's blocking us from using LEGALIZE_JS_FFI=0. Please let us know if you need more info or enhancement on the test case. Thanks :)

[waterlike86]

@kripken to get the legalstub generated, we made a change to this line ->
https://github.com/WebAssembly/binaryen/blob/c98b8170d6419a659f9b45a41d399879f023621b/src/passes/LegalizeJSInterface.cpp#L60
from
if (isIllegal(func) && shouldBeLegalized(ex.get(), func))
to
if (isIllegal(func) || shouldBeLegalized(ex.get(), func))

does this make sense?

after this we got
In our main function when we call getbigint,

   ...
   ...
   i32.const 361
   i32.add
   call $legalfunc$invoke_j
   set_local $l4
   ...
   ...

(func $legalfunc$invoke_j (type $t52) (param $p0 i32) (result i64)
   get_local $p0
   call $legalimport$invoke_j
   i64.extend_u/i32
   call $getTempRet0
   i64.extend_u/i32
   i64.const 32
   i64.shl
   i64.or
)

In JS, it is mapped to invoke_j -> "$legalimport$invoke_j": invoke_j,

`invoke_j` calls `dyncall_j` which calls `ftcall_j` which should eventally call the $legalstub$__Z9getbigintv however now is still calling the $__Z9getbigintv function


(func $legalstub$__Z9getbigintv (type $t9) (result i32)
   (local $l0 i64)
   call $__Z9getbigintv
   set_local $l0
   get_local $l0
   i64.const 32
   i64.shr_u
   i32.wrap/i64
   call $setTempRet0
   get_local $l0
   i32.wrap/i64
)

Is our understanding of this flow correct? If so, do you have an idea how we can point ftcall_j to the $legalstub$__Z9getbigintv instead of the $__Z9getbigintv function

[kripken]

I don't think if (isIllegal(func) || shouldBeLegalized(ex.get(), func)) is right - in the logic there, shouldBeLegalized decides if we should legalize it at all, so it is affected by whether we are doing full legalization or not, etc.

I don't think the binaryen side needs to be changed - my guess is the issue is on the emscripten side. Running your testcase, it looks like the error happens on

function ftCall_j(x) {
  return wasmTable.get(x)();
}

The problem is that we call into the table from JS, and that is only possible if the type is legal for JS. So all the legalization we did does not help here. To solve this, we should use dynCalls normally - that is, have a dynCall_j function in the wasm module, like we do without dynamic linking I believe. That would get legalized properly, and then things should work - we actually recently added testing for it, #7953

Avoiding the ftCall approach also makes sense for perf, I think we discussed in the past - table calls are not that optimized for code size currently.

To do this, emscripten.py is where we emit ftCalls etc. This probably isn't a tiny change, but hopefully it's mostly just making dynamic linking do what normal code does (it might not be dynamic linking directly - perhaps it's emulated function pointers, which is enabled by dynamic linking - looking for where ftCalls are emitted in emscripten.py should show).

[waterlike86]

I see that the invoke calls in this case were a result of handling exceptions in fastcomp how would the later passes know to generate a dynCall_j for the function replaced by invoke?

[kripken]

In the normal way, I think? Maybe I misunderstood your question. But this should work normally, that is, without dynamic linking (see #7953 as mentioned earlier). I think we just need to make dynamic linking do the same thing, by not using the custom ftCall mechanism.

[waterlike86]

just to set the context right, this is the state of 1.38.22 with your binaryen patch from WebAssembly/binaryen#1883

Legalize +  getbigint in side module
_main -> call_indirect -> __Z9getbigintv(result i32) -> works

Minimal  Legalize +  getbigint in side module
_main -> call_indirect -> __Z9getbigintv(result i64) -> works

Minimal Legalize + throws + getbigint in side module
_main -> $legalfunc$invoke_j ->  $legalimport$invoke_j ->  invoke_j -> dynCall_j ->  
 ftCall_j -> __Z9getbigintv(result i64) -> exception thrown: TypeError: cannot pass i64 to or from JS

Legalize + throws + getbigint  within same module
_main -> invoke_i -> __Z9getbigintv(result i32) -> works

Minimal Legalize + throws + getbigint within same module (this is what we are facing )
_main -> $legalfunc$invoke_j ->  $legalimport$invoke_j ->  invoke_j -> dynCall_j ->   
ftCall_j -> __Z9getbigintv(result i64) -> exception thrown: TypeError: cannot pass i64 to or from JS

it doesnt seem that dyncalls are legalized anywhere, is this output expected?

[kripken]

How are you checking if something is legalized or not?

See the last 2 files in this PR for an example of legalized dynCalls https://github.com/WebAssembly/binaryen/pull/1824/files

[waterlike86]

I actually created the test app for all 4 scenarios and built it, ran it and inspected the wast result, and index.js to look at how getbigint was called from _main

[waterlike86]

@kripken see #8039 for a testcase for what I expect should be there.

[kripken]

Thanks, the testcase was very helpful. So I think, as I commented there, that we should land my PR that legalizes invokes, and we should also make the ftCall change i suggested here (to stop using them). I might not have time for that part myself.

[kripken]

I wrote up a quick patch to show what I mean,

1bdf398

That seems to pass most tests, but e.g. test_dlfcn_longjmp is broken. The patch might be a good starting point if you want to investigate this further.

[waterlike86]

Thanks @kripken I think we are getting closer. it seems that the testcase works only if i set EMULATED_FUNCTION_POINTERS=0 with WASM=1. still trying to understand why if we need EMULATED_FUNCTION_POINTERS and why is it set to 2 by default when WASM=1 is set

[waterlike86]

@kripken Theres another case we didnt handle, when the side module tries to call into a main module function which contains a 64bit param, when legalization turned off it errors out with
exception thrown: RuntimeError: function signature mismatch
i created a test case to show this. see -> #8291
any idea where this might be going wrong?

[stale]

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.