-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Hello WebAssembly! (MVP implementation) #10870
Hello WebAssembly! (MVP implementation) #10870
Conversation
Would it be better to implement more WebAssembly-related features here before this gets merged or to focus on merging this minimally as is (hardly useful by itself) and bring improvements later? The included smoke test should at least avoid regressions. |
IMO I Would strongly caution against trying to get more features in and maintaining a long-running branch for a new platform (it'll be harder to keep momentum if you have to play catch-up with upstream) Each of the pending things is easily enough extra time and work for their own PR (and likely multiples) |
The scope of this is perfect, thank you. It just seems that nobody has gotten around reviewing it, due to the release cycle, conference and other more urgent tasks. |
My only concern is there are a lot of stub methods, which is fine, but there is no indication that they need to be completed later, which might get confusing as this is iteratively worked on. I'm wondering if those stub methods should at least have a |
Yes, a couple of TODO or |
Co-authored-by: Sijawusz Pur Rahnama <sija@sija.pl>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks really good! Very organized! I found a couple of small things, most of which can probably be deferred. All pretty minor concerns, save for maybe the ABI stuff.
src/regex/lib_pcre.cr
Outdated
{% unless flag?(:wasm32) %} | ||
LibPCRE.pcre_malloc = ->GC.malloc(LibC::SizeT) | ||
LibPCRE.pcre_free = ->GC.free(Void*) | ||
{% end %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
with gc/none
, these will just point to regular C malloc/free, so you can probably leave this untouched for wasm32
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left it here because I didn't build libpcre and didn't test it. I can remove this check before merging.
…stly made by @maxfierke, with small changes.
…panded macro: spawn:6:18 has no type (Exception)
Do you have the build instructions for this? And how big uncompressed? |
22kb gzipped / 54kb before compression. Here is my code and instructions on running it: https://github.com/lbguilherme/crystal-web |
Current status:
|
@@ -43,7 +43,7 @@ module SystemError | |||
end | |||
|
|||
# The original system error wrapped by this exception | |||
getter os_error : Errno | WinError | Nil | |||
getter os_error : Errno | WinError | WasiError | Nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💭 Maybe at this point we should create an alias for this union (optional in this PR!)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like much the fact that we have this union, since a program will only ever ever have one of these kinds of errors. It doesn't make sense to have all of those defined, they should be guarded by flags or moved to the Crystal::System, I don't know.
…ead of wasm32 directly. This provides better support for the wasm32-unknown-unknown target.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe these should also be wasi
?
There are more flag?(:wasm32)
left. They should probably all be wasi
instead, except for those directly related to codegen.
Actually, those were correct. We can think about three wasm targets:
The first two targets are quite important to have. I'm using Wasm doesn't have a way of implementing FFI with or without wasi. It could be done with js, but I'm leaving this to the future. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀 🦾
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is amazing, thank you!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💯 After this get merged maybe we should move more stuff to Crystal::System regarding sockets, right? It seems that there where many tweaks of flags on networking that might be good to encapsulate so a next platform could start with all the NotImplemented as this PR.
We meet again my dear 32 bits!
Basic WebAssembly support has been added in crystal-lang/crystal#10870 and was released in Crystal 1.4 The second commit applies uniform format for the libc description.
See #829.
This PR adds minimal support for compiling a Crystal program into WebAssembly and then linking it with a WASI-based LibC. Browsers are still not supported.
What does not work:
How to use:
puts "Hello WebAssembly!"
and put it tomain.cr
.make
).bin/crystal build main.cr --cross-compile --target wasm32-unknown-wasi
. This will generate amain.o
file that is actually a binary wasm file.wasm-ld main.o -o main.wasm wasi-sysroot-14.0/wasi-sysroot/lib/wasm32-wasi/libc.a wasi-sysroot-14.0/wasi-sysroot/lib/wasm32-wasi/crt1.o
.main.wasm
file. It is already a final module that can run on any WASI-capable runtime, like Wasmer.This PR is the bare minimum necessary to get it working. Unfortunately, I had to add flag checks on more places than I would like, but these can go away once other features get ported.