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

Fixing compilation for wasm32 targets #9

Open
wants to merge 2 commits into
base: master
from

Conversation

@luizirber
Copy link
Contributor

luizirber commented Aug 29, 2019

Trying to fix succinct-rs to run in wasm32 (since it's a transitive dependency for me, I'm using pdatastructs.rs). As the name implies, wasm32 is 32-bit and some implementations are missing (where checks for 64-bit pointer size were used).

This is a WIP just doing the bare minimum to make it compile and pass the current tests, but still need to write more tests for 32-bit cases.

@luizirber luizirber referenced this pull request Aug 29, 2019
@tov

This comment has been minimized.

Copy link
Owner

tov commented Nov 5, 2019

I’m wondering—I took WIP to mean don’t actually merge yet. Was that wrong?


#[cfg(target_pointer_width = "32")]
fn get(&self, t: usize) -> u64 {
debug_assert!(t < 8);

let t = t.wrapping_sub(1);
let shift = t.wrapping_add(t >> 28 & 8) * 9;
self.0 >> shift & 0x1FF
}

#[cfg(target_pointer_width = "32")]
fn set(&mut self, t: usize, value: u64) {
debug_assert!(t < 8);

let t = t.wrapping_sub(1);
let shift = t.wrapping_add(t >> 28 & 8) * 9;

let old_part = self.0 & !(0x1FF << shift);
let new_part = (value & 0x1FF) << shift;

self.0 = old_part | new_part;
}
Comment on lines +50 to +71

This comment has been minimized.

Copy link
@luizirber

luizirber Nov 6, 2019

Author Contributor

While the changes make compilation on 32-bit targets work, I'm not sure how to write tests to check if these methods are correct. Any pointers on how to get started?

This comment has been minimized.

Copy link
@luizirber

luizirber Nov 8, 2019

Author Contributor

OK, so I managed to run the tests in a 32-bit platform: wasm32-wasi (thanks to cargo-wasi.

Tests are still working after $ cargo wasi test, which... is good? But I still don't know if the tests are hitting the corner cases for 32-bit pointers.

This comment has been minimized.

Copy link
@luizirber

luizirber Nov 12, 2019

Author Contributor

Upon further reading, these changes still work fine, but might not be the fastest (because u64 will need to be emulated by 32-bit targets, just like u128 would need to be emulated in 64-bit targets). Should I leave a note for the future?

@luizirber

This comment has been minimized.

Copy link
Contributor Author

luizirber commented Nov 8, 2019

Is it OK if I add two github actions workflows for compiling and testing under wasm32? With https://github.com/actions-rs/cargo it is easier to setup than on Travis, and I also don't want to extend the travis build times too much

@tov

This comment has been minimized.

Copy link
Owner

tov commented Nov 8, 2019

@luizirber luizirber force-pushed the luizirber:wasm_fix branch 6 times, most recently from a9e3ce8 to b496d82 Nov 8, 2019
@luizirber luizirber force-pushed the luizirber:wasm_fix branch from b496d82 to d869e3d Nov 8, 2019
@luizirber

This comment has been minimized.

Copy link
Contributor Author

luizirber commented Nov 8, 2019

Added! The check is not showing here yet (because only members of the repo can add new actions), but here is the output from it running in my fork: https://github.com/luizirber/succinct-rs/runs/295115774

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.