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

build: Allow building C compiler-rt fallbacks for wasm #566

Merged
merged 3 commits into from
Jan 30, 2024

Conversation

trevyn
Copy link
Contributor

@trevyn trevyn commented Jan 8, 2024

The comment disabling wasm is almost 5 years old, and is outdated:

// * wasm - clang for wasm is somewhat hard to come by and it's
//   unlikely that the C is really that much better than our own Rust.

Clang for wasm is now much easier to come by, and C fallbacks are needed for e.g. #561 and rusqlite/rusqlite#1010

For the __builtin_sadd_overflow thing, see llvm/llvm-project#69305 and #167

@bjorn3
Copy link
Member

bjorn3 commented Jan 8, 2024

wasm32-unknown-unknown is not ABI compatible with C and currently doesn't depend on a C compiler in any way.

@trevyn
Copy link
Contributor Author

trevyn commented Jan 8, 2024

The ABI surface actually used by compiler-rt is compatible with Rust, and per rust-lang/rust#35437, the scope of compiler-builtins is "each and every compiler-rt intrinsic".

@bjorn3
Copy link
Member

bjorn3 commented Jan 9, 2024

You need this for linking against sqlite, right? Sqlite is written in C and thus almost certainly abi incompatible with wasm32-unknown-unknown. compiler-builtins should already support everything rust only wasm modules need, and if not, the missing intrinsics should be ported to rust.

I'm not a reviewer of this repo, but IMHO compiler-builtins should remain rust only on wasm32-unknown-unknown until it is actually possible to use C code as user without risking an abi incompatibility and thus we would need the extra intrinsics.

Edit: I guess if you don't pass any structs by value it will happen to work with current LLVM versions. (the abi depends on llvm internals)

@Amanieu
Copy link
Member

Amanieu commented Jan 10, 2024

I'm not too familiar with the wasm situation. cc @alexcrichton

@alexcrichton
Copy link
Member

There's some more discussion of related intrinsics over at WebAssembly/wasi-sdk#361 as well (linked from #561). I think there's two questions here:

  • Should the compiler-builtins crate be reponsible for bringing along intrinsics that Rust itself doesn't use and only C uses? (which I believe is the case here since AFAIK Rust doesn't use __builtin_sadd_overflow)
  • Should compiler-builtins build the C code of the compiler-rt library?

For the first question that's a bit beyond myself personally, although lots has changed in the 8 years since the issue you linked was posted and AFAIK there was no "official" decision one way or another when I was working on Rust, but things may have changed in the meantime.

For the second question it's unfortunately a bit tied up in the first as well. If compiler-builtins provides only Rust-used intrinsics then there's no need for C because I think compiler-builtins already supplies everything. There might still be performance differences but it seems that's not the question here.

Otherwise if Rust's compiler-builtins does want to provide intrinsics it doesn't use, but instead C might use (as I believe is the case here), then I'm at least personally less worried than @bjorn3 about the ABI issues here. While it's true wasm32-unknown-unknown is not exactly matching clang it's not like every signature is irrationally different. It should be possible to manually confirm that the signatures here all match and since these aren't general ABIs, just fixed ones, it should work out ok. Additionally as @trevyn points out the comment here is indeed outdated, so there shouldn't be any issue compiling C intrinsics if requested, although they should probably still be opt-in since folks compiling for wasm32-unknown-unknown often don't have a clang lying around.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants