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

AsmJs/WAsm calling convention issues on xplat #2378

Closed
Penguinwizzard opened this issue Jan 13, 2017 · 13 comments
Closed

AsmJs/WAsm calling convention issues on xplat #2378

Penguinwizzard opened this issue Jan 13, 2017 · 13 comments

Comments

@Penguinwizzard
Copy link
Contributor

Penguinwizzard commented Jan 13, 2017

When attempting to pass a floating-point argument to an asmjs/wasm function on xplat, the caller and callee don't agree on the calling convention. This causes correctness issues, as we end up often using the wrong value. This occurs in JITted code.

Repro:
Remove these lines from test/runtest.py (link):

not_compile_flags = set(['-simdjs']) \  
    if sys.platform != 'win32' else None

Run runtests.py -d AsmJSFloat.

Note: There's a separate bug filed related to removing the lines hiding this issue from runtest.py at #2412.

Repro file:
Here's a separate file by @Cellule that reproduces the issue with -maic:0

function AsmModuleDouble() {
        "use asm";

        function add(x) {
                    x = +x;
                    return +sub(1.5);
                }
        function sub(x) {
                    x = +x;
                    return +(x);
                }
        return {
                    add: add
                };
}

var asmModuleDouble = AsmModuleDouble();     // produces AOT-compiled version
print(asmModuleDouble.add(2, 1))
@jianchun
Copy link

@Penguinwizzard Indeed, didn't get to complete Simd when I worked on xplat JIT. That's why Simd tests were disabled on xplat by that line.

@Cellule
Copy link
Contributor

Cellule commented Jan 13, 2017

From what I was able to gather this happens because LowererMDArch::LowerCallArgs calls in LowererMDArch::GetArgSlotOpnd which tries to use xmm registers for float argument when we are supposed to put them on the stack on xplat

@Cellule
Copy link
Contributor

Cellule commented Jan 13, 2017

@jianchun The problem is almost all asm.js test have that flag even though simd is not used (I am not entirely sure the reason why, but I think there's a good one).

@Penguinwizzard
Copy link
Contributor Author

Penguinwizzard commented Jan 13, 2017

@jianchun If Simd doesn't work on xplat, should we disable building Simd/not recognize the flag/error out on use on xplat? It looks like a lot of tests just fail with "SIMD is undefined", which seems fine - those tests can just be excluded.

@jianchun
Copy link

@Penguinwizzard @Cellule At the time I felt Simd was close to completion, but I was not exactly sure of the calling conventions. Left Simd code in build so it would not diverge further (Simd code kept introducing build failures at the time).

@MikeHolman
Copy link
Contributor

Is wrong calling convention still an issue?

@Cellule
Copy link
Contributor

Cellule commented Mar 3, 2017

@MikeHolman AFAIK it hasn't been fixed yet, but maybe @obastemur has more info on this

@MikeHolman
Copy link
Contributor

Then can we increase the priority on this issue? It is essentially blocking our wasm fuzzing and generally is a pretty high impact issue for wasm/asm.js code.

@obastemur
Copy link
Collaborator

#2716 is merged and should fix this issue.

@Penguinwizzard
Copy link
Contributor Author

Problems still occur; remove exclude_xplat on most wasm test cases to repro.

@Penguinwizzard Penguinwizzard reopened this Jun 8, 2017
chakrabot pushed a commit that referenced this issue Jun 9, 2017
Merge pull request #3129 from Penguinwizzard:fix_asan_ci

Disable one test in ASAN due to a known issue with modules in ch (bug #2735), disable one test on xplat due to wasm calling convention issues (bug #2378), keep a loop from accessing data beyond an array bound in the pal code, and fix the casing on one test file.
chakrabot pushed a commit that referenced this issue Jun 9, 2017
…ASAN runs

Merge pull request #3129 from Penguinwizzard:fix_asan_ci

Disable one test in ASAN due to a known issue with modules in ch (bug #2735), disable one test on xplat due to wasm calling convention issues (bug #2378), keep a loop from accessing data beyond an array bound in the pal code, and fix the casing on one test file.
@obastemur
Copy link
Collaborator

@Penguinwizzard the problem shouldn't be calling conventions. At least it was fixed for asm.js and others. About WASM, we have couple of disable stuff (SIMD) for xplat. Maybe we should close this issue and open a new one : Enable WASM on xplat ?

@Cellule
Copy link
Contributor

Cellule commented Jul 13, 2017

This issue is also hitting in asm.js not just wasm.
One of the problem is how we make calls in jitted code which is shared by both asm.js and wasm

@Cellule
Copy link
Contributor

Cellule commented Aug 21, 2017

Closing in favor of #3561

@Cellule Cellule closed this as completed Aug 21, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants