feat: add wasm32-unknown-unknown support and memvfs feature #935
Conversation
Codecov Report
@@ Coverage Diff @@
## master #935 +/- ##
==========================================
- Coverage 77.72% 77.28% -0.44%
==========================================
Files 48 49 +1
Lines 5621 5653 +32
==========================================
Hits 4369 4369
- Misses 1252 1284 +32
Continue to review full report at Codecov.
|
|
| @@ -0,0 +1,207 @@ | |||
| // import implementations from android bionic libc | |||
There was a problem hiding this comment.
@thomcc It appears to be BSD 3-clause, is this acceptable? https://en.wikipedia.org/wiki/Bionic_%28software%29
There was a problem hiding this comment.
Yes, I think that should be fine. It would need to include the license somewhere nearby though, but that's not hard to arrange.
|
Wow, nice. I wonder if this would make us one of the -sys crates that has a story on wasm32-unknown-unknown.
Agreed 100%.
No, sadly. It's because there's no libc on the web. This has historically been a huge challenge for C code used by rust libraries on the web. Typically, C code uses the wasm32-unknown-emscripten target, which Is there anywhere in the SQLite api where structures are passed or returned by value? Unfortunately, clang and rustc disagree about wasm32-unknown-unknown's ABI, and so it would be incompatible. Stuff like this is on the path to solving it, but that path is just starting: rust-lang/rust#83763 A lot of the declarations in the header don't have definitions. What's the deal there? We also should at least test building for this target in CI. |
| _p_out_flags: *mut raw::c_int, | ||
| ) -> raw::c_int { | ||
| let name = cchar_to_str(z_name) | ||
| .expect("meet non-utf8 db name") |
There was a problem hiding this comment.
Panics over FFI are UB. We should be guarding all of these that could possibly panic with something like https://gist.github.com/thomcc/70bcee940215ef9dae8a43744a597abe (feel free to use that code)
| lazy_static = { version = "1.4.0", optional = true } | ||
| parking_lot = { version = "0.11.1", optional = true } | ||
| log = { version = "0.4", optional = true } | ||
| anyhow = { version = "1.0.40", optional = true } |
There was a problem hiding this comment.
Why do we need anyhow — doesn't seem to be used?
| } | ||
|
|
||
| #[cfg(all(target_arch = "wasm32", target_os = "unknown"))] | ||
| mod allocator { |
There was a problem hiding this comment.
This allocator seems broken. sqlite_free assumes that the first 8 bytes of the pointer are going to be the size, but sqlite_malloc doesn't allocate space for that, nor store it, etc. I'm surprised this works at all (it seems like if it does, it's a coincidence based on an implementation detail of SQLite allocations).
You need something more along the lines of https://shift.click/blog/on-dealloc/#appendix-how-to-wrap-allocators-which-need-layout-info-in-dealloc, where the information about the allocation is stored before the pointer. Note that that code also handles the full layout, e.g. alignment too, whereas for this we can assume the needed alignment is always the same, and only handle the size.
There was a problem hiding this comment.
The default allocator implements memsize() by always allocating 8 extra bytes on each malloc() request and storing the size of the allocation in that 8-byte header.
| use std::alloc::{alloc, dealloc, realloc as rs_realloc, Layout}; | ||
|
|
||
| #[no_mangle] | ||
| pub unsafe fn sqlite_malloc(len: usize) -> *mut u8 { |
There was a problem hiding this comment.
These must be extern "C".
I feel like we should either be overriding malloc, or should be using the normal config-based mechanism for overriding the allocator globally, or we should have a big comment explaining why we're doing it this way.
|
@passchaos This is very cool, do you intend to continue working on this PR to get it merged? If not, do you mind if others pick up the work? Also, is the memvfs part critical, or can one just be restricted to the |
|
@trevyn Recently I am not dealing with WASM related things, welcome to continue this work; The reason for using a separate memvfs is that SQLite's ':memory:' is connnection-local, data can't be shared between database connections, my implementation allows different db connections to share data by db name, even if used from different threads (However, it seems that the latest SQLite version solves this problem, without confirming whether it can). |
|
@passchaos Thank you!! For reference, I have a very rough WIP of using a very small subset of the OpenBSD libc with clean licensing here: https://github.com/trevyn/rusqlite/tree/wasm32-unknown-unknown Will submit a separate PR if I can get it working and address the rest of @thomcc 's reviews. |
4 participants
Content:
With these three features: linking sqlite3 to Rust's memory allocator, using an in-memory virtual file system, linking to a separate implementation of string functions and qsort functions, along with fine-tuned WASI headers, our implementation works under
wasm32-unknown-unknownRemind:
Time zone function temporarily missing