Add wasm32-unknown-unknown support
#1010
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1010 +/- ##
==========================================
- Coverage 81.31% 80.62% -0.69%
==========================================
Files 51 52 +1
Lines 10402 10493 +91
==========================================
+ Hits 8458 8460 +2
- Misses 1944 2033 +89 ☔ View full report in Codecov by Sentry. |
|
I reviewed https://sqlite.org/capi3ref.html, and did not see any instances of structs being passed or returned by value. @thomcc Does this PR look roughly acceptable, if I sort out the time and randomness-related bits? |
|
Give me some time to review it (probably sometime over the weekend), but thanks for the work. |
There was a problem hiding this comment.
Great effort, hope it makes it into rusqlite soon!
Reading up on https://github.com/jlongster/absurd-sql, I wonder how feasible it would be to implement a pluggable persistence backend like jslongster did in sql-js/sql.js#481 for rusqlite.
Have you seen that? What are your thoughts on it?
|
@wngr Thanks! It does seem plausible to make a VFS that uses IndexedDB as a backend; that would be very cool. I don't expect to personally have time to work on it in the near future, but would love to see it happen. |
|
A concern from the previous PR:
I believe what's happening (in the few cases of this that still exist) is that the amalgamation is relying on certain declarations existing, but never actually uses them in the ultimately-generated code. If they were used, the WASM file would fail to load with its cryptic version of an "undefined symbol" error, see for example rustwasm/wasm-bindgen#2570 This is essentially how I decided which libc files to include; provide the minimal declarations that allowed compilation, and then track down the symbols that needed to be added that would allow the WASM to load. Edit: Also, it seems clang provides builtins for memcpy & friends. |
trevyn
force-pushed
the
wasm32-unknown-unknown
branch
from
24b1454
to
94d45d3
Compare
|
@thomcc Ready for review.
|
trevyn
force-pushed
the
wasm32-unknown-unknown
branch
from
94d45d3
to
004cf26
Compare
|
@thomcc Any chance you could review this PR? wasm32-unknown-unknown support would be awesome 😄 |
|
I tried using this and ran into the following (click to expand)
|
|
For what it's worth, this is what happens if I don't set
|
|
@paulyoung For an Anyway, this still works for me with: rusqlite = {git = "https://github.com/trevyn/rusqlite", branch = "wasm32-unknown-unknown", features = ["bundled"]}Does this work for you? Are you able to link to the project that you're actually building? |
|
@trevyn I manage my projects and their dependencies using nix (not homebrew) so it's probably a matter of figuring out what versions are needed and making sure they're being used. I could create a minimal example project that exhibits this issue if it would help. |
|
I created a standalone project here: https://github.com/paulyoung/rusqlite-wasm32-unknown-unknown-nix and asked about it on the Nix forums here: https://discourse.nixos.org/t/help-building-rusqlite-for-wasm32-unknown-unknown-with-crane/21724 |
|
Fancy! In that case, |
|
hey, is this waiting for something? can it be merged? |
|
This built fine on my m1 mac, then it failed on docker: Would love to see this merged into master 🎉 |
Co-authored-by: Andelf <[email protected]>
|
If merged this would be a pretty big win for the Rust ecosystem imo |
|
Absolutely massive. Especially since sled is dead. |
This reverts commit 7c6cd3b.
|
@trevyn I am trying to get this branch working with I don't want to hijack your pull request discussion for a support request, I was just hoping there was somewhere you could point me that would be a good place to discuss this. Thanks! (And thanks for this work, this is very exciting -- I'm hoping to use |
|
I suggest moving that line of questioning to wasm-pack land. Here is a similar issue: rustwasm/wasm-pack#1328 You'll need to figure out how to convince wasm-pack + cargo + rustc to recompile everything with the atomics and bulk-memory features in order to support shared memory. I did a bunch of investigation into this recently and it's a huge PITA (for now). I also suggest taking a look at the new wasm32-wasi-preview1-threads target which may make this easier (or harder): https://doc.rust-lang.org/rustc/platform-support/wasm32-wasi-preview1-threads.html |
|
@carlsverre After investigating a bit, this seems to be distinct from the wasm-pack issue -- it's not about rust code, but specifically about sqlite3.o, which is produced by libsqlite3-sys. The issue seems to be that the target feature flags set for rust are not being forwarded to clang when building sqlite3.c. I proposed a fix to @trevyn in a pull request to his fork's pull request branch (😅), not sure if that's the best approach. |
This allows code that e.g. relies on `-Ctarget-feature=+atomics,+bulk-memory` to build libsqlite3-sys.
|
FWIW, the list of supported features was pulled from https://github.com/rust-lang/rust/blob/master/compiler/rustc_target/src/target_features.rs#L312 -- I'm not sure if there's a clean way to depend on |
I'm seeing this same issue as well on the latest commit of this branch. Upon some investigation, it seems like I run into this if I am executing statements via |
|
Sorry I'm not deep enough into understanding sqlite3 to propose a fix, but I have found out that this happens when journaling is enabled. When a transaction is started a |
|
Hey @trevyn thanks for the PR, this branch correctly compiles to However, I get a I've also tried older commits from this branch but the problem persists... |
IIUC, this PR only supports an in memory db. |
Yeah, I've now found out that in order to access the filesystem you need to target |
There was a problem hiding this comment.
Hi there,
This PR came up for discussion at $EMPLOYER today so I thought I would take a cursory look at what was here so far. I am not the maintainer but I tried being leaving some helpful comments on things I would have noticed if this fork was checked-in internally.
| @@ -37,6 +37,12 @@ wasm32-wasi-vfs = [] | |||
| [dependencies] | |||
| openssl-sys = { version = "0.9", optional = true } | |||
|
|
|||
| [target.wasm32-unknown-unknown.dependencies] | |||
There was a problem hiding this comment.
Should this include a not(unix) condition here because js-sys is a web-only dependency? wasm32-unknown-unknown doesn't always mean web. I did something similar, along with a js feature and it's worked pretty well for quite a few downstream WASM users.
There was a problem hiding this comment.
It sounds like you might be conflating two different things here:
-
To my understanding,
wasm32-unknown-unknownwill never report itself asunix, so your proposed condition is unnecessary.unixis set bywasm32-unknown-emscripten, which is explicitly not matched by[target.wasm32-unknown-unknown.dependencies], and is outside the scope of this PR. -
The other thing is that
wasm32-unknown-unknowndoesn't always mean web, which is true. I think you actually meant to say thatwasm32-unknown-unknowndoesn't always mean a Javascript environment, which is also true. PR welcome for supporting that use case.
| @@ -261,6 +261,57 @@ mod build_bundled { | |||
| cfg.file("sqlite3/wasm32-wasi-vfs.c"); | |||
| } | |||
| } | |||
| if env::var("TARGET") == Ok("wasm32-unknown-unknown".to_string()) { | |||
| // Apple clang doesn't support wasm32, so use Homebrew clang by default. | |||
There was a problem hiding this comment.
IMO, this block should be reverted and be replaced with documentation stating how to setup a functional cross-compiling environment on Apple's hosts. Not everyone uses Homebrew (this wouldn't work with Nix, for example) and it would result in different errors about the path not existing vs clang saying "unknown target".
I see it in wasm32-unknown-unknown-openbsd-libc's build script as well?
There was a problem hiding this comment.
I definitely welcome any improvements to documentation or error messages. I think that it is reasonable to find and use a Homebrew installation by default if available.
There was a problem hiding this comment.
Yeah, it's pretty weird for us to automatically do this for the user, seconding the suggestion to instead document how to set things up appropriately.
| - run: npm install -g wasm-pack | ||
|
|
||
| - name: Test wasm-pack ${{ matrix.host }} | ||
| run: wasm-pack test --node --features bundled |
There was a problem hiding this comment.
This should have a browser-bound test as well, no? Otherwise there's no way to know in CI if the js_sys dependency or another browser-specific one breaks.
There was a problem hiding this comment.
A browser test is certainly reasonable to add, but note that there are no web dependencies, only Javascript dependencies, which node fulfills just as well.
| @@ -37,6 +37,12 @@ wasm32-wasi-vfs = [] | |||
| [dependencies] | |||
| openssl-sys = { version = "0.9", optional = true } | |||
|
|
|||
| [target.wasm32-unknown-unknown.dependencies] | |||
| compiler-rt-builtins = "0.1" | |||
There was a problem hiding this comment.
Is it documented which part of the requirements compiler-rt-builtins provides vs the stripped down OpenBSD libc source?
There was a problem hiding this comment.
Also, I haven't tested things since rust-lang/compiler-builtins#566 got merged, it's possible that might already provide the required functions or get us closer to a cleaner solution in some way.
| let target_date = js_sys::Date::now() + (microseconds as f64 / 1000.0); | ||
| while js_sys::Date::now() < target_date {} |
There was a problem hiding this comment.
If this PR supports being built with the atomics WASM feature (and people are intending to), maybe allow this to be optimized with memory_atomic_wait32 instead of a spinloop? This is what the standard library does when built with atomics support too.
There was a problem hiding this comment.
This is a good suggestion, thanks.
There was a problem hiding this comment.
It's worth noting that atomic waits don't work on the browser's main thread (only in workers). So IDK if this is a good idea.
OTOH, there are JS environments where the timestamp will never advance (due to concerns around Spectre). This at least used to be the case for Cloudflare workers, and may still be the case there.
|
Not for nothing, this PR is requested more frequently than any other from people in the Discord (not that that means it's requested that frequently, but I did just get another request about it). It doesn't appear to pull in a big chunk of libc anymore, so the previous main objection I had might be fixed now. |
| @@ -261,6 +261,57 @@ mod build_bundled { | |||
| cfg.file("sqlite3/wasm32-wasi-vfs.c"); | |||
| } | |||
| } | |||
| if env::var("TARGET") == Ok("wasm32-unknown-unknown".to_string()) { | |||
| // Apple clang doesn't support wasm32, so use Homebrew clang by default. | |||
There was a problem hiding this comment.
Yeah, it's pretty weird for us to automatically do this for the user, seconding the suggestion to instead document how to set things up appropriately.
| [a, b][(a < b) as usize] | ||
| } | ||
|
|
||
| const ALIGN: usize = max( |
There was a problem hiding this comment.
This is subtly wrong (too low) for targets other than wasm32-unknown-unknown (in particular, 64 bit targets).
For example, I could imagine this code not getting updated if wasm64-unknown-unknown support is added.
It's probably better to use 16 instead of 8 for the hard-coded alignment bound, which is a somewhat safer value to hard-code (it's a very common max_align_t value for 64bit targets).
Also add a comment about this, please.
| } | ||
|
|
||
| const fn max(a: usize, b: usize) -> usize { | ||
| [a, b][(a < b) as usize] |
There was a problem hiding this comment.
This goofy way of implementing max shouldn't be needed anymore.
| return null_mut(); | ||
| } | ||
|
|
||
| *(ptr as *mut usize) = size; |
There was a problem hiding this comment.
Given that we don't need to provide posix_memalign, only storing size is fine (otherwise we'd need to store both size/align for free).
| let target_date = js_sys::Date::now() + (microseconds as f64 / 1000.0); | ||
| while js_sys::Date::now() < target_date {} |
There was a problem hiding this comment.
It's worth noting that atomic waits don't work on the browser's main thread (only in workers). So IDK if this is a good idea.
OTOH, there are JS environments where the timestamp will never advance (due to concerns around Spectre). This at least used to be the case for Cloudflare workers, and may still be the case there.
|
@thomcc is there any news regarding this? What's the plan here? Is there help needed to get this over the finish line? |
All credit to @passchaos in #935 for the insight that SQLite only uses a very small subset of libc, that can easily be copied in-tree.
Significant changes from that PR:
.cfiles from OpenBSDlibsqlite3-sys/sqlite3/sqlite3.ckept pristinememvfs, replaced by a thin VFS that liberally returnsSQLITE_IOERR. This target is intended to be used only withConnection::open_in_memory()or equivalent.wasm-pack.The allocator'sfree()method is still a little weird, but it is documented that SQLite's default allocator stores the allocation length in the first 8 bytes: https://sqlite.org/malloc.html#the_default_memory_allocator . If we want to wrap it again, I'm happy to do that, just let me know.To do:
Closes #488, closes #827, closes #935