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

Regression when updating from version 1.1.31 to newer when compiling ring to wasm32-unknown-unknown #1275

Closed
matheus23 opened this issue Nov 6, 2024 · 2 comments · Fixed by #1284

Comments

@matheus23
Copy link

Hi, when updating from cc version 1.1.31 to cc version 1.1.32, I fail to build ring for wasm32-unknown-unknown in github actions (see this PR with me testing different cc versions).

Specifically, ring generates "env" imports in its Wasm file:

  (import "env" "ring_core_0_17_8_x25519_fe_neg" (func $ring_core_0_17_8_x25519_fe_neg (;31;) (type 16)))
  (import "env" "ring_core_0_17_8_x25519_ge_scalarmult_base" (func $ring_core_0_17_8_x25519_ge_scalarmult_base (;32;) (type 3)))
  (import "env" "ring_core_0_17_8_x25519_ge_frombytes_vartime" (func $ring_core_0_17_8_x25519_ge_frombytes_vartime (;33;) (type 4)))
  (import "env" "ring_core_0_17_8_x25519_fe_invert" (func $ring_core_0_17_8_x25519_fe_invert (;34;) (type 2)))
  (import "env" "ring_core_0_17_8_x25519_fe_mul_ttt" (func $ring_core_0_17_8_x25519_fe_mul_ttt (;35;) (type 3)))

This indicates that there's some extern "C" in ring that didn't get picked up by wasm-bindgen for some reason and made it to the final .wasm.

I'd expect there to be no such imports at all. Having these imports means it's essentially not executable by Wasm hosts such as nodejs or browsers (because they don't provide an "env" module).

Downgrading cc from 1.1.32 to 1.1.31 fixes these issues.

Of course, this sounds a lot like a ring, issue, but given that this occurs when updating ccs patch version, I would've expected that ring doesn't need to change anything to keep working, right?

I'm also out of my depth here. I'd love to help debug this, but it works on my machine, just not on github actions.

@NobodyXu
Copy link
Collaborator

NobodyXu commented Nov 6, 2024

cc @madsmtm could that due to the wasm32-unknown-unknown change in the recent PR?

The one where we have to replace target.os with arch
#1225 (comment)

@madsmtm
Copy link
Collaborator

madsmtm commented Nov 6, 2024

I don't know what's the correct thing to do, see #1126 (comment)

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 a pull request may close this issue.

3 participants