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

Added support for compiling against wasm32-wasi #1499

Closed
wants to merge 1 commit into from

Conversation

john-sharratt
Copy link

You will need to compile wasi-sdk and set the export environment variable WASI_SDK_DIR which is fairly standard these days.
https://github.com/WebAssembly/wasi-sdk

@rjzak
Copy link

rjzak commented Jun 8, 2022

I think the env variable is CC_wasm32_wasi, where I set it to /opt/wasi-sdk/bin/clang which worked for me when trying to figure this out.

@john-sharratt
Copy link
Author

Do you mean we that this bit isn't needed?

ring/build.rs

Line 499 in 019299e

&std::env::var("WASI_SDK_DIR").expect("missing environment variable: WASI_SDK_DIR");

let target_os = std::env::var("CARGO_CFG_TARGET_OS").unwrap();
    if target_os == "wasi" {
        let wasi_sdk_path =
            &std::env::var("WASI_SDK_DIR").expect("missing environment variable: WASI_SDK_DIR");
        c.flag(format!("--sysroot={}", wasi_sdk_path).as_str());
    }

If so - the WASI_SDK_DIR env is how some of the other crates asked to have the variable - we for sure need to make sure its standardized as otherwise it will become annoying for users.

Open to suggestions (unless of course I completely misunderstood your comment @rjzak :-) )

@john-sharratt
Copy link
Author

Pushed another update that fixes some externals that were leaked into the web assembly manifest

@rjzak
Copy link

rjzak commented Jun 9, 2022

When I was trying to work on Wasi support, I noticed that it would fail initially, and have output about some flags not being defined: CC_wasm32_wasi, CFLAGS_wasm32_wasi, etc. So when I set CC_wasm32_wasi to /opt/wasi-sdk/bin/clang, it worked. I think both approaches do the same thing. If you set the variable for Wasi SDK, it'll find that clang and sysroot. If you set the CC, it'll use that clang, which already knows what to do. But if Ring was already asking about CC_wasm32_wasi, then maybe that's a better way is my guess. I also couldn't find in the code where it would ask for that env var, but it worked when I set it. Can it use both (an either-or situation)?

@rjzak
Copy link

rjzak commented Jun 9, 2022

I'm getting this error even with WASI_SDK_DIR set:

 include/ring-core/check.h:27:11: fatal error: 'assert.h' file not found
  # include <assert.h>
            ^~~~~~~~~~
  1 error generated.

@john-sharratt
Copy link
Author

@rjzak does this file exist for you?

ls $WASI_SDK_DIR/include/assert.h

@rjzak
Copy link

rjzak commented Jun 10, 2022

@rjzak does this file exist for you?

ls $WASI_SDK_DIR/include/assert.h

It's here: /opt/wasi-sdk/share/wasi-sysroot/include/assert.h
Using /opt/wasi-sdk/bin/clang should be able to figure it out, I think.

@john-sharratt
Copy link
Author

When I was trying to work on Wasi support, I noticed that it would fail initially, and have output about some flags not being defined: CC_wasm32_wasi, CFLAGS_wasm32_wasi, etc. So when I set CC_wasm32_wasi to /opt/wasi-sdk/bin/clang, it worked. I think both approaches do the same thing. If you set the variable for Wasi SDK, it'll find that clang and sysroot. If you set the CC, it'll use that clang, which already knows what to do. But if Ring was already asking about CC_wasm32_wasi, then maybe that's a better way is my guess. I also couldn't find in the code where it would ask for that env var, but it worked when I set it. Can it use both (an either-or situation)?

Using both might be possible but are you sure these are the same thing? sysroot does not have clang in it - its the libc libraries
and headers so I am unsure how the environment variables are directly related

@john-sharratt
Copy link
Author

@rjzak does this file exist for you?

ls $WASI_SDK_DIR/include/assert.h

It's here: /opt/wasi-sdk/share/wasi-sysroot/include/assert.h Using /opt/wasi-sdk/bin/clang should be able to figure it out, I think.

I'm not sure that's how it works but happy to learn something new.
Why don't you try dropping the environment variable CC_wasm32_wasi then set WASM_SDK_DIR to /opt/wasi-sdk/share/wasi-sysroot and see what happens.
(The native clang from normal package managed can already compile wasm32-wasi so it should only need the libc)

@rjzak
Copy link

rjzak commented Jun 10, 2022

When I was trying to work on Wasi support, I noticed that it would fail initially, and have output about some flags not being defined: CC_wasm32_wasi, CFLAGS_wasm32_wasi, etc. So when I set CC_wasm32_wasi to /opt/wasi-sdk/bin/clang, it worked. I think both approaches do the same thing. If you set the variable for Wasi SDK, it'll find that clang and sysroot. If you set the CC, it'll use that clang, which already knows what to do. But if Ring was already asking about CC_wasm32_wasi, then maybe that's a better way is my guess. I also couldn't find in the code where it would ask for that env var, but it worked when I set it. Can it use both (an either-or situation)?

Using both might be possible but are you sure these are the same thing? sysroot does not have clang in it - its the libc libraries and headers so I am unsure how the environment variables are directly related

But the wasi-sdk has it's own clang, which is aware of the sysroot. I had made this: https://github.com/rjzak/ring/tree/wasi_wip
and it compiles with CC_wasm32_wasi=/opt/wasi-sdk/bin/clang cargo build --target=wasm32-wasi but trying to use it shows a missing definition at runtime: env::ring_core_0_17_0_not_released_yet_p256_point_mul_base.

@john-sharratt
Copy link
Author

john-sharratt commented Jun 10, 2022

When I was trying to work on Wasi support, I noticed that it would fail initially, and have output about some flags not being defined: CC_wasm32_wasi, CFLAGS_wasm32_wasi, etc. So when I set CC_wasm32_wasi to /opt/wasi-sdk/bin/clang, it worked. I think both approaches do the same thing. If you set the variable for Wasi SDK, it'll find that clang and sysroot. If you set the CC, it'll use that clang, which already knows what to do. But if Ring was already asking about CC_wasm32_wasi, then maybe that's a better way is my guess. I also couldn't find in the code where it would ask for that env var, but it worked when I set it. Can it use both (an either-or situation)?

Using both might be possible but are you sure these are the same thing? sysroot does not have clang in it - its the libc libraries and headers so I am unsure how the environment variables are directly related

But the wasi-sdk has it's own clang, which is aware of the sysroot. I had made this: https://github.com/rjzak/ring/tree/wasi_wip and it compiles with CC_wasm32_wasi=/opt/wasi-sdk/bin/clang cargo build --target=wasm32-wasi but trying to use it shows a missing definition at runtime: env::ring_core_0_17_0_not_released_yet_p256_point_mul_base.

Yeah I had a similar problem and fixed it in a branch here:
https://github.com/john-sharratt/ring/tree/v0.16.20

I didn't reimplement it in this merge request yet (v0.17.0)

The issue is that its exporting functions that are not implemented (hence get exposed under env) - the real fix is to implement them (most of them are coded in assembly which won't work with web assembly). In that branch I just dropped in implementations that are not implemented so they at least are not unrealisable imports

@rjzak
Copy link

rjzak commented Jun 10, 2022

Neat, that's awesome. I wasn't sure what was going on or how to remove the assembly code.

What's the command you used to get this code to compile?

@john-sharratt
Copy link
Author

john-sharratt commented Jun 10, 2022

Neat, that's awesome. I wasn't sure what was going on or how to remove the assembly code.

What's the command you used to get this code to compile?

git clone https://github.com/john-sharratt/ring.git
git checkout v0.16.20
cargo install wasi
cargo wasi build

(after your export the environment variables of course)

@rjzak
Copy link

rjzak commented Jun 10, 2022

Wow, you've made my day. Thank you!
https://chat.enarx.dev/channel/development?msg=GX9t75YvQDvMPRHib

@john-sharratt
Copy link
Author

Wow, you've made my day. Thank you! https://chat.enarx.dev/channel/development?msg=GX9t75YvQDvMPRHib

you are very welcome!

@lu-zero
Copy link

lu-zero commented Aug 25, 2022

I tried and I'm getting:

rustc --crate-name ring --edition=2018 src/lib.rs --error-format=json --json=diagnostic-rendered-ansi,artifacts,future-incompat --crate-type lib --emit=dep-info,metadata,link -C embed-bitcode=no -C debuginfo=2 --cfg 'feature="alloc"' --cfg 'feature="default"' --cfg 'feature="dev_urandom_fallback"' --cfg 'feature="once_cell"' -C metadata=f8d4d4c0fee9bf79 -C extra-filename=-f8d4d4c0fee9bf79 --out-dir /Users/lu_zero/Sources/rust/ring/target/wasm32-wasi/debug/deps --target wasm32-wasi -C linker=/opt/wasi-sdk-16.0/bin/clang -C incremental=/Users/lu_zero/Sources/rust/ring/target/wasm32-wasi/debug/incremental -L dependency=/Users/lu_zero/Sources/rust/ring/target/wasm32-wasi/debug/deps -L dependency=/Users/lu_zero/Sources/rust/ring/target/debug/deps --extern chacha20poly1305=/Users/lu_zero/Sources/rust/ring/target/wasm32-wasi/debug/deps/libchacha20poly1305-3cddf5b12433a1f2.rmeta --extern untrusted=/Users/lu_zero/Sources/rust/ring/target/wasm32-wasi/debug/deps/libuntrusted-06cb463d0e692f49.rmeta --extern wasi=/Users/lu_zero/Sources/rust/ring/target/wasm32-wasi/debug/deps/libwasi-c38bbc2ea6b6a973.rmeta -L native=/Users/lu_zero/Sources/rust/ring/target/wasm32-wasi/debug/build/ring-929d3c687b99509b/out -l static=ring-core -l static=ring-test`
error: failed to build archive: section too large

is it a known problem?

@rjzak
Copy link

rjzak commented Aug 25, 2022

Thought: perhaps Ring should not be ported to Wasi, as Wasi-Crypto (goals) should be used instead.

  • Private key security, as it only sits on the machine side
  • Actual cryptographic code run on the machine side, making it faster than interpreting Wasm.

@lu-zero
Copy link

lu-zero commented Aug 25, 2022

If you have a crate using a crate that uses ring, it would be quite a problem getting things going.

(btw setting AR_wasm32_wasi to a sane value fixed the problem)

@briansmith
Copy link
Owner

See the draft PR #1531 where I propose we delegate the randomness stuff to the getrandom crate.

@briansmith
Copy link
Owner

Thought: perhaps Ring should not be ported to Wasi, as Wasi-Crypto (goals) should be used instead.

Probably ring will need to be implemented on top of some parts of the WASI crypto API, similarly to how we use processor's crypto instructions when they are available.

Copy link
Owner

@briansmith briansmith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR.

The most important contribution I need to enable WASI support is the changes to .github/workflows/ci.yml to test wasm32-wasi in CI. Check out how the getrandom crate does it at https://github.com/rust-random/getrandom/blob/8e44d13ebf7e53f8d2b32c9b63caab8a993d30e4/.github/workflows/tests.yml#L230-L245.

It seems like this PR is trying to do three things:

  • Add wasm32-wasi support.
  • Add wasm64 support
  • Enable additional ring functionality for wasm32 targets.

It would be much easier for me to have those things be separated into at least 3 separate PRs.

When adding wasm32-wasi support, we need to add testing with a wasm32 environment as mentioned above.

When adding wasm64-wasi support, we need to add testing with a wasm64 environment to CI.

When exposing additional functionality to wasm32 environments that currently isn't exposed for wasm32-unknown-unknown, we need to verify during review that all the tests for that additional functionality are modified to be tested in both wasm32-unknown-unknown (using the wasm-bindgen-test hack) and wasm32-wasi (And wasm64-wasi if we add such support).

[target.'cfg(any(target_os = "dragonfly", target_os = "freebsd", target_os = "illumos", target_os = "netbsd", target_os = "openbsd", target_os = "redox", target_os = "solaris"))'.dependencies]
once_cell = { version = "1.8.0", default-features = false, features=["std"] }

[target.'cfg(all(target_arch = "wasm32", target_vendor = "unknown", target_os = "unknown", target_env = ""))'.dependencies]
[target.'cfg(all(target_family = "wasm", target_vendor = "unknown", target_os = "unknown", target_env = ""))'.dependencies]
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The subject of the PR says "wasm32-wasi" but the changes here seem to be trying to also add wasm64-* support. Let's split the changes that are only needed for wasm64 support into a separate PR. That is, let's not switch to target_family = "wasm" in this PR.

@@ -12,7 +12,7 @@
// WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION
// OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN
// CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.

#![cfg_attr(target_os = "wasi", allow(unused, dead_code))]
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not understanding why we suddenly have some code becoming dead code, but this doesn't seem right.

@@ -229,6 +229,22 @@ mod sysrand_chunk {
}
}

#[cfg(target_os = "wasi")]
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All the changes to rand should be subsumed by PR #1531.

(&[AARCH64, ARM, X86_64, X86], "crypto/fipsmodule/ec/ecp_nistz.c"),
(&[AARCH64, ARM, X86_64, X86], "crypto/fipsmodule/ec/gfp_p256.c"),
(&[AARCH64, ARM, X86_64, X86], "crypto/fipsmodule/ec/gfp_p384.c"),
(&[AARCH64, ARM, X86_64, X86], "crypto/fipsmodule/ec/p256.c"),

(&[X86_64, X86], "crypto/cpu-intel.c"),
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't make sense to compile these cpu-intel.c for wasm targets.

@@ -64,7 +66,7 @@ const RING_SRCS: &[(&[&str], &str)] = &[
(&[X86_64], "crypto/fipsmodule/ec/asm/p256-x86_64-asm.pl"),
(&[X86_64], "crypto/fipsmodule/modes/asm/aesni-gcm-x86_64.pl"),
(&[X86_64], "crypto/fipsmodule/modes/asm/ghash-x86_64.pl"),
(&[X86_64], "crypto/poly1305/poly1305_vec.c"),
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All this code is guarded by #if defined(BORINGSSL_HAS_UINT128) && defined(OPENSSL_X86_64) so adding it for WebAssembly targets doesn't make sense.

@@ -491,6 +493,17 @@ fn build_c_code(
);
}

fn cc_builder() -> cc::Build {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a more generic convention for --sysroot that isn't WASI-specific? Presumably TARGET_CFLAGS=--sysroot=${WASI_SDK_DIR} works fine?

@Xuanwo
Copy link

Xuanwo commented Feb 26, 2023

Hello, does this PR still go on?

@banocean
Copy link

Hello, does this PR still go on?

idk, looks quite abandoned 😕

@rjzak
Copy link

rjzak commented Jul 11, 2023

Hello, does this PR still go on?

idk, looks quite abandoned confused

I think it still works though. Try and see?

@briansmith
Copy link
Owner

briansmith commented Oct 14, 2023

Thanks for the PR. This has been superseded by PR #1745, so I'm closing this.

@briansmith briansmith closed this Oct 14, 2023
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.

6 participants