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

Method with i64 return type fails on Safari 13+14 #127

Closed
yonihemi opened this issue Oct 8, 2020 · 21 comments · Fixed by #131
Closed

Method with i64 return type fails on Safari 13+14 #127

yonihemi opened this issue Oct 8, 2020 · 21 comments · Fixed by #131
Labels
bug Something isn't working

Comments

@yonihemi
Copy link
Member

yonihemi commented Oct 8, 2020

Using JavaScriptKit as only dependency, trying to instantiate a Date() fails on Safari (13.1.1, 14) with:
TypeError: i64 not allowed as return type or argument to an imported function

Stacktrace

TypeError: i64 not allowed as return type or argument to an imported function

wasm-stub
<?>.wasm-function[gettimeofday]
<?>.wasm-function[CFAbsoluteTimeGetCurrent]
<?>.wasm-function[$s10Foundation4DateVACycfC]
<?>.wasm-function[main]
<?>.wasm-function[__original_main]
<?>.wasm-function[_start]
wasm-stub
_start
(anonymous function) — e8900d1b3895f276.js:1:47960
(anonymous function) — e8900d1b3895f276.js:1:184568
asyncFunctionResume
[native code]

Same code runs fine on Chrome and Firefox. Tried with latest blessed carton versions and with latest swiftwasm+JavaScriptKit, same results.

Assuming it's related to swiftwasm/JavaScriptKit#56. Could/should we incorporate this polyfill in JavaScriptKit?

@yonihemi yonihemi changed the title Method with i64 return type fail on Safari 13+14 Method with i64 return type fails on Safari 13+14 Oct 8, 2020
@MaxDesiatov
Copy link
Collaborator

MaxDesiatov commented Oct 8, 2020

Is this reproducible with carton? I thought there was a polyfill for this in Wasmer-JS which carton ships in dev.js and bundle.js entrypoints.

@yonihemi
Copy link
Member Author

yonihemi commented Oct 8, 2020

Unfortunately it is, getting same results with carton init --template basic, then in main.swift:

import Foundation
print("Hello, world!")
let date = Date()

(Running with carton dev/bundle)

@MaxDesiatov
Copy link
Collaborator

Here's an issue that seems related to me WebAssembly/WASI#334

@MaxDesiatov
Copy link
Collaborator

What people mention there is possible optimization with wasm-opt --i64-to-i32-lowering. If it does work, maybe we could integrate it in carton? It already requires wasm-opt for carton bundle.

@kateinoigakukun
Copy link
Member

I addressed this issue by pollyfilling i64 into two i32 values https://github.com/kateinoigakukun/swiftwasm-pad/tree/master/Frontend/Sources/i64_polyfill

@MaxDesiatov
Copy link
Collaborator

This would require generating additional SwiftPM modules that contain the polyfills for our users. What do you think about the wasm-opt solution, which if works would be much less intrusive?

@kateinoigakukun
Copy link
Member

kateinoigakukun commented Oct 10, 2020

$ wasm-opt --i64-to-i32-lowering .build/wasm32-unknown-wasi/release/Hello -o Bundle/main.wasm
Fatal: IR must be flat: run --flatten beforehand (instructions must only have constant expressions, local.get, or unreachable as children, in 3)

Hmm, that option doesn't work well. If it works well, it would be an option to recommend.

@yonihemi
Copy link
Member Author

I didn't have luck with wasm-opt either. There's also wasmer's lowering pass which we can use at runtime, presumably only if needed by browser detection.

@MaxDesiatov
Copy link
Collaborator

BTW, I hope JSDate works well in Safari, and would be enough to substitute the functionality of Foundation's Date in the meantime.

@yonihemi
Copy link
Member Author

I've had some success running the i64->i32 transformer directly in rust on the pre-optimized bundle. It fails on optimized bundle and when running in javascript.
JSDate does work in Safari, but naturally untenable for reusing 'real' Swift code. I guess it also affects a bunch of other WASI methods.

@MaxDesiatov
Copy link
Collaborator

Do I understand correctly that polyfilling this purely on JS side or even within the Swift codebase of JavaScriptKit is not possible? Maybe this issue should be transferred to either toolchain or carton repositories then? Depends on where exactly we're going to apply the final i64->i32 transformation .

@MaxDesiatov
Copy link
Collaborator

MaxDesiatov commented Oct 12, 2020

(For posterity, this is also tracked in WebKit).

@MaxDesiatov
Copy link
Collaborator

Today and yesterday I've been playing with Binaryen bindings, and binaryen is what powers wasm-opt. If we can get its i64-to-i32-lowering pass to work, it could be easily integrated in carton and applied during the build.

@yonihemi
Copy link
Member Author

I'll try to look into that. Meanwhile I realized the wasmer js transformer is not working because... the rust->wasm module it's running itself has i64 calls. I've been trying to transform the transformer offline but got bogged down in rust and npm failures.

@MaxDesiatov
Copy link
Collaborator

the wasmer js transformer is not working because... the rust->wasm module it's running itself has i64 calls.

oh dear

@yonihemi
Copy link
Member Author

I've seen your great progress on https://github.com/MaxDesiatov/binaryen. But not sure it's the right way forward:

  1. We already fork to wasm-opt, so not sure of the benefit of building it into carton.
  2. More importantly, binaryen's lowering pass doesn't seem to handle our output wasm well.
    • On the most simple example, yields Fatal: IR must be flat: run --flatten beforehand (instructions must only have constant expressions, local.get, or unreachable as children, in _start)
    • Running flatten as suggested crashes with
warning: invalid abbreviation code 23 (range: 61 : 0..69)
compile unit size was incorrect
UNREACHABLE executed

Wasmer's pass does seem to handle our output, though I've only ran some rudimentary tests. We can incorporate it either:

  • Bundling wasm_transform_cli, though it seems more of a demo than a fully fetured tool;
  • Linking the rust code into carton, which is doable (though I'm not familiar with the Linux mechanics of that), or
  • Fix the i64 issue and use the rust->wasm->js version, (1) in runtime or (2) as our own pass in carton bundle, say with wasmer cli.

@MaxDesiatov
Copy link
Collaborator

Thanks for the detailed update!

I've seen your great progress on https://github.com/MaxDesiatov/binaryen.

To clarify, there's not much there, and I'm thinking of abandoning that repository for now and merging my work into previously available bindings, which I weren't fully aware of. I knew there were some, but didn't know their API coverage was much greater than what I managed to get on my own.

My foray into Binaryen bindings was provoked by dreaming of a Swift to WebAssembly compiler running directly in the browser. This is not something I plan to work on very soon, but rather an exploration in background. LLVM is too heavy, so I wonder how difficult it would be to take the Swift compiler frontend as is, get SIL output from it, and then transform SIL (at least some very limited subset of it as PoC) to Wasm with Binaryen. This is a long-term project, and fixing high priority issues we have in SwiftWasm right now is what I'm focused on. I only hoped that Binaryen bindings could bring me one step closer to that Binaryen compiler backend PoC.

1. We already fork to `wasm-opt`, so not sure of the benefit of building it into carton.

Forking has some overhead, especially for frequent builds as we have to run for carton dev too. Additionaly, we have more control with direct bindings, maybe even allowing us to write our own optimization passes in Swift.

But you've convinced me that trying wasm_transform_cli in some form is probably the best approach for now.

@MaxDesiatov
Copy link
Collaborator

MaxDesiatov commented Oct 14, 2020

wasm_transform_cli works for me, but it will require Rust to build this dependency for carton, which is not ideal. The transformer itself is ~400 LoC, I wonder how hard it would be to rewrite it in Swift with Binaryen bindings...

@MaxDesiatov
Copy link
Collaborator

Moving it to the carton repo in the meantime, I don't think any changes in JavaScriptKit will be necessary.

@MaxDesiatov MaxDesiatov transferred this issue from swiftwasm/JavaScriptKit Oct 14, 2020
@kateinoigakukun
Copy link
Member

I'll work on porting wasm_transform_cli in Swift world

@MaxDesiatov
Copy link
Collaborator

Looks like Binaryen bindings with full API coverage I was referring to are in a bit broken state, I'm fixing those right now.

@MaxDesiatov MaxDesiatov added the bug Something isn't working label Oct 14, 2020
MaxDesiatov added a commit that referenced this issue Oct 14, 2020
`TestApp/.swift-version` is removed as it was mostly duplicating the default toolchain version. Downloads of recent toolchains is fixed now with the new `macos` tarball suffix that replaces `osx`. `TestApp/main.swift` is updated to make #127 easily reproducible.
MaxDesiatov added a commit that referenced this issue Oct 21, 2020
Resolves #127.

* Add separate Builder class, use WasmTransformer

* Don't apply the transformer in non-browser environments

* Fix macOS Swift 5.2 build failure

* Fix Package.swift errors

* Fix cyclic dependency in `Package.swift`

* Bump WasmTransformer requirement to 0.0.1

* Rename `case wasmer` to `case other`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants