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

Fix WASM32 compilation for Cairo1-run and cairo-vm #1792

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

wraitii
Copy link

@wraitii wraitii commented Jun 21, 2024

Fix WASM32 compilation for Cairo1-run and cairo-vm

Description

Running the following fails on main:
cargo build --bin cairo1-run --target wasm32-unknown-unknown --release --no-default-features

There are 4 issues:

  • zip is not wasm-compatible by default. This can be circumvented with appropriate feature flags.
  • cairo1-run/src/cairo_run.rs sets an initial gas to 9999999999999_usize which is bigger than usize on 32 bit platforms. I'm not sure it's actually safe to downgrade this to merely 999999999.
  • Two lines in vm/src/lib.rs and vm/src/vm/runners/cairo_pie.rs pessimistically assume wasm <=> no_std, when the former works just fine - we can be more precise.

I also remove a comment to check WASM compatibility with some libraries, as they are indeed compatible.

This PR fixes both cairo1-run and cairo-vm for WASM targets.

@@ -141,6 +141,10 @@ pub fn cairo_run_program(

let main_func = find_function(sierra_program, "::main")?;

#[cfg(target_pointer_width = "32")]
let initial_gas = 999999999_usize;
Copy link
Member

Choose a reason for hiding this comment

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

If this value works, why not just use this?
Otherwise, why not change initial_gas to a u64 instead of usize?

Copy link
Author

@wraitii wraitii Jun 24, 2024

Choose a reason for hiding this comment

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

If this value works, why not just use this?

It compiles, but that doesn't tell me it's 'working' - I haven't really checked what it's being used for, and this is a billion gas - perhaps an achievable amount (where 1 trillion wasn't). If it's used to initialise a "Max" value, it's perhaps unsafe - would need someone to know why 999999999999 was chosen in the first place.

Otherwise, why not change initial_gas to a u64 instead of usize?

Much smaller change / laziness, but I can update the PR to change this to u64 for sure
Edit: mh actually should probably just have tried that, it's just a few lines

Copy link
Member

Choose a reason for hiding this comment

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

It compiles, but that doesn't tell me it's 'working' - I haven't really checked what it's being used for, and this is a billion gas - perhaps an achievable amount (where 1 trillion wasn't). If it's used to initialise a "Max" value, it's perhaps unsafe - would need someone to know why 999999999999 was chosen in the first place.

IIUC that's just a "should be enough to not fail due to gas exhaustion" value, nothing else. I personally prefer to use the MAX constant for whatever type I'm using to make it more obvious that this is just a sloppy infinity value.

@Oppen
Copy link
Member

Oppen commented Jun 24, 2024

  • Two lines in vm/src/lib.rs and vm/src/vm/runners/cairo_pie.rs pessimistically assume wasm <=> no_std, when the former works just fine - we can be more precise.

Those really shouldn't be handled there in the first place. We have the stdlib module to abstract those differences away.

@wraitii
Copy link
Author

wraitii commented Jun 24, 2024

  • Two lines in vm/src/lib.rs and vm/src/vm/runners/cairo_pie.rs pessimistically assume wasm <=> no_std, when the former works just fine - we can be more precise.

Those really shouldn't be handled there in the first place. We have the stdlib module to abstract those differences away.

I'm not sure if this means I need to update the PR or if it's a general comment, could you clarify ?


Should I add this as a fix to the changelog? The PR template wasn't clear on this (this isn't a change that needs to be 'documented', per se).

@Oppen
Copy link
Member

Oppen commented Jun 24, 2024

I'm not sure if this means I need to update the PR or if it's a general comment, could you clarify ?

Just a general comment. Your fix for those is OK. We should clean up at some later point.

@Oppen
Copy link
Member

Oppen commented Jun 24, 2024

Should I add this as a fix to the changelog? The PR template wasn't clear on this (this isn't a change that needs to be 'documented', per se).

Just the gist, something like "compilation of cairo-1-run and cairo-vm on wasm32 has been fixed" if the latest release was already broken. If only main broke and you fixed it before it reached a release, then it shouldn't affect users reading the changelog.

The intention of the template (we may need to rephrase it) is to document functional changes mostly. We consider non-functional changes thing like refactors, cleanups, or improvements to the build system or the CI, same for simply adding tests, while bug fixes for reported issues, significant performance changes or added/broken interfaces need to be in the changelog.

@Oppen
Copy link
Member

Oppen commented Jun 24, 2024

Oh BTW, congrats! 🎉
I just noticed it's your first PR. It's on the right track for merging.

@Oppen
Copy link
Member

Oppen commented Jun 24, 2024

Also, I played a bit of 0AD like 10 years ago or so. I didn't do much because IIRC there were no campaigns I had nobody to play with. Nice to cross paths with a contributor.

@wraitii
Copy link
Author

wraitii commented Jun 25, 2024

Just the gist, [...]

Added a line, can't hurt much anyways.

Also, I played a bit of 0AD like 10 years ago or so. I didn't do much because IIRC there were no campaigns I had nobody to play with. Nice to cross paths with a contributor.

Ah that's fun 😄 we have support for campaigns nowadays, but IIRC still none of them. Though a small multiplayer user base exists.

@wraitii
Copy link
Author

wraitii commented Jul 23, 2024

Hey @Oppen , bumping this - from my perspective this is still ready to merge

@Oppen
Copy link
Member

Oppen commented Jul 23, 2024

Hey @Oppen , bumping this - from my perspective this is still ready to merge

I agree. We need a second reviewer to approve it. I'll remind the team we have this PR waiting.

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.

2 participants