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

Making scryer-prolog build successful with wasm32-wasi (discussion only) #1966

Closed
wants to merge 1 commit into from

Conversation

rujialiu
Copy link

Intended to make discussions in #615 easier.

@rujialiu rujialiu marked this pull request as draft August 19, 2023 07:23
@mthom
Copy link
Owner

mthom commented Aug 19, 2023

hi @rujialiu ,

There's one thing I need confirmation for @mthom because I can't compile the following codes on 32-bit system. 64-bit system > can compile because both HeapValueCell and arena pointer's are 64-bits. But it looks like in the following code, the actual
parameters are ptr's, not cells:

@@ -250,13 +253,17 @@ macro_rules! match_untyped_arena_ptr_pat_body {
         #[allow(unused_braces)]
         $code
     }};
-    ($cell:ident, OssifiedOpDir, $n:ident, $code:expr) => {{
-        let $n = cell_as_ossified_op_dir!($cell);
+    ($ptr:ident, OssifiedOpDir, $n:ident, $code:expr) => {{
+        //let $n = cell_as_ossified_op_dir!($cell);
+        let payload_ptr = unsafe { std::mem::transmute::<_, *mut OssifiedOpDir>($ptr.payload_offset()) };
+        let $n = TypedArenaPtr::new(payload_ptr);
         #[allow(unused_braces)]
         $code
     }};
-    ($cell:ident, LiveLoadState, $n:ident, $code:expr) => {{
-        let $n = cell_as_load_state_payload!($cell);
+    ($ptr:ident, LiveLoadState, $n:ident, $code:expr) => {{
+        //let $n = cell_as_load_state_payload!($cell);
+        let payload_ptr = unsafe { std::mem::transmute::<_, *mut LiveLoadState>($ptr.payload_offset()) };
+        let $n = TypedArenaPtr::new(payload_ptr);
         #[allow(unused_braces)]
         $code
     }};

Yes, the LiveLoadState and OssifiedOpDir objects are being extracted from an AllocSlab in the arena (ptr points to an AllocSlab which is tagged in its header as either LiveLoadState or OssifiedOpDir, and the slab contains an object of whichever type). The payload_offset() function skips past the arena header word. They should be transmuted as 32-bit pointers on 32-bit systems, no? I'm not sure I understand what you're asking here.

@rujialiu
Copy link
Author

Sorry for the confusion @mthom. I was simply asking whether my changes are correct :)
The old codes don't compile in 32-bit system, and my changes make them compile. However, compile doesn't mean "correct", so I'm asking. I think the old code "coincidentially" work in 64-bit system because both HeapValueCell and arena pointers are 64-bit.

@rujialiu
Copy link
Author

Now the code is updated on top of the merged #1972 @triska @lucksus

You can build it with

wasm-pack build --target web -- --no-default-features

But wasm-pack seems to ignore the -no-default-features flag (or maybe I'm using it incorrectly) so I had to disable the default features in Cargo.toml :(

Anyway, when wasm-pack is finished, there will be a pkg directory containing wasm and some js files. Run a local http server (like python -m http.server) and point your browser to index.html and you'll see the result of the sudoku solver :) You can change index.js to feed different codes.

The codes are very preliminary and not meant to be merged, but you can playaround it especially whether it works with your own branch.

TODO: Still can't make ring fully work. So I'm making a looooot of mocks. Some functions are available locally after enabling ring's wasm2_c feature (not pushed yet) but some functions are still missing (those implemented in asm?).

@triska
Copy link
Contributor

triska commented Aug 26, 2023

@rujialiu: Thank you so much!

Regarding ring: I think it would be ideal to file upstream issues in the ring repository if something you need does not work, so that the author of ring can look into these issue while you are working on other WASM changes. It is of course great if more features are available in the WASM port!

@rujialiu
Copy link
Author

Regarding ring: I think it would be ideal to file upstream issues in the ring repository if something you need does not work, so that the author of ring can look into these issue while you are working on other WASM changes. It is of course great if more features are available in the WASM port!

I have a feeling that it's all my fault, so I'll take a deeper look first :) I'm pushing the codes now because people can play around other things first.

@triska
Copy link
Contributor

triska commented Aug 29, 2023

For reference, I used the following instructions to compile to WASM, executed in the scryer-prolog source directory:

$ git remote add rujialiu https://github.com/rujialiu/scryer-prolog
$ git fetch rujialiu
$ git checkout -b wasm32-discussion rujialiu/wasm32-discussion
$ curl https://rustwasm.github.io/wasm-pack/installer/init.sh -sSf | sh
$ wasm-pack build --target web -- --no-default-features

Excepting a few warnings, compilation works smoothly:

    Finished release [optimized + debuginfo] target(s) in 4m 28s
[INFO]: ⬇️  Installing wasm-bindgen...
[INFO]: ✨   Done in 4m 52s
[INFO]: 📦   Your wasm pkg is ready to publish at ~/scryer-prolog/pkg.

Now I launch the http server:

$ python -m http.server

Then I browse to http://localhost:8000/, and I see (stemming from index.html):

result: [9,8,7,6,5,4,3,2,1]. [2,4,6,1,7,3,9,8,5]. [3,5,1,9,2,8,7,4,6]. [1,2,8,5,3,7,6,9,4]. [6,3,4,8,9,2,1,5,7]. [7,9,5,4,6,1,8,3,2]. [5,1,9,2,8,6,4,7,3]. [4,7,2,3,1,9,5,6,8]. [8,6,3,7,4,5,2,1,9]. 

Beautiful!

I also have the directory scryer-prolog/pkg/, containing the following files:

LICENSE                   
README.md                 
package.json              
scryer_prolog.d.ts        
scryer_prolog.js          
scryer_prolog_bg.wasm     
scryer_prolog_bg.wasm.d.ts

@rujialiu
Copy link
Author

Interestingly, yesterday I dreamed of someone using scryer-prolog to draw some animated ASCII-art in the browser (probably some game)! Anyway, I hope I can find sometime to finish ring issues this week.

And maybe some kind of minimalist top-level that's easier to port to wasm (rustyline+crossterm is no easy to port)? I don't know how hard to write another one, or just to use cfg to replace just important functions to interact with JS, but I do see another top-level in the PR list. what's that about? Is it possible to port that one to wasm instead? @mthom

Then you can embed the wasm with the minimal top-level in the pages of Power of Prolog @triska. That'll be cool! (and won't add pressure to your server besides serving a medium-sized wasm file)

@rujialiu
Copy link
Author

It looks like ring's wasm32 support is incomplete: briansmith/ring#918
For example, I can confirm that CHACHA20_POLY1305 won't work with wasm32 because it's only implemented in assembly (for several archs, but not wasm32).
What do you think? Should we cfg some functions, provide mocks like I'm doing now, or implement some functions from some other crates? @triska

@rujialiu
Copy link
Author

I took some time to learn a bit of crypto, and it seems that all things you got from ring are also implemented in other dedicated crates? If ring's is more reliable and preferred whenever possible, we can at least use other crates only in wasm, but maybe it's better to abstract away some crypto stuffs and put them to something like crypto.rs and do the dirty cfg things in it?

Another way is to first identify exactly which features are not supported in wasm and then only use other crates for those things. It's not easy to find the feature set by reading codes, though (but experiementing should be easy) @triska

@triska
Copy link
Contributor

triska commented Aug 30, 2023

The cryptographic functionality from library(crypto) is confined to 12 Rust functions, corresponding to 24 entries in src/machine/dispatch.rs (since we have Call... and Execute... variants, so duplicating each of these functions), in the order they currently occur in the file:

&Instruction::CallCryptoRandomByte => {
&Instruction::ExecuteCryptoRandomByte => {
&Instruction::CallCryptoDataHash => {
&Instruction::ExecuteCryptoDataHash => {
&Instruction::CallCryptoDataHKDF => {
&Instruction::ExecuteCryptoDataHKDF => {
&Instruction::CallCryptoPasswordHash => {
&Instruction::ExecuteCryptoPasswordHash => {
&Instruction::CallCryptoDataEncrypt => {
&Instruction::ExecuteCryptoDataEncrypt => {
&Instruction::CallCryptoDataDecrypt => {
&Instruction::ExecuteCryptoDataDecrypt => {
&Instruction::CallCryptoCurveScalarMult => {
&Instruction::ExecuteCryptoCurveScalarMult => {
&Instruction::CallEd25519Sign => {
&Instruction::ExecuteEd25519Sign => {
&Instruction::CallEd25519Verify => {
&Instruction::ExecuteEd25519Verify => {
&Instruction::CallEd25519NewKeyPair => {
&Instruction::ExecuteEd25519NewKeyPair => {
&Instruction::CallEd25519KeyPairPublicKey => {
&Instruction::ExecuteEd25519KeyPairPublicKey => {
&Instruction::CallCurve25519ScalarMult => {
&Instruction::ExecuteCurve25519ScalarMult => {

I have used strikeout text for those functions where I think the compilation to WASM should work (because they use pure Rust crates), and I think you therefore can simply retain these functions as they are. I have used bold for those functions that I think must currently (potentially) be treated specially for WASM. For these functions, defining a cfg (conditional compilation) is better than providing a "stub", because invoking a stub in cryptographic applications may not be safe (for instance, the application may assume that data is encrypted after calling a predicate, but it actually is not encrypted by the stub, or a signature may not be validated by the stub etc.). It is much better to yield a clean existence error if this functionality is not provided, instead of yielding wrong results! The dispatcher in dispatch.rs may be an easy place to throw such an error, or simply fail in such cases.

All these functions have corresponding definitions in src/machine/system_calls.rs. They appear one after the other, and it may be possible to conditionally compile or remove them as a whole block, especially if you consider reordering them so that everything that depends on ring appears in a continuous sequence.

For your WASM port, I think it would be best to simply (automatically) remove unsupported functionality at compilation time. As the need arises, I will try to find suitable crates for replacement. I did consider other crates too when I implemented the cryptographic functionality, but found them less suitable at the time I wrote these functions. The situation may well have changed now, and we may be able to more easily use other crates now that have since become available. A good example for this is the crrl crate by @pornin, which has since become available and which has allowed us to get rid of OpenSSL.

@rujialiu
Copy link
Author

Unfortunatelly, after commenting out the 6 functions you mentioned, the resulting wasm has still missing functions related to x25519, but maybe they are just referenced, not actually used. In that case, mocking these functions should be ok. So could you please write a few prolog tests that I can run in browsers (can easily check manually) to check whether each of these 12 functions are working or not? @triska

@triska
Copy link
Contributor

triska commented Aug 31, 2023

If any of these functions yields warnings or does not compile for the WASM port, please simply remove it by "cfg"-ing it away for the port in its entirety.

All cryptographic Rust functions appear in one contiguous block in system_calls.rs, starting here:

pub(crate) fn crypto_random_byte(&mut self) {

and ending here:

You can "cfg"-away the entire region, and then can also remove the ring and crrl dependencies from system_calls.rs, starting around here and the following lines:

use ring::rand::{SecureRandom, SystemRandom};

I will incrementally add these functions back as needed, when I find suitable crates that work with WASM.

@rujialiu
Copy link
Author

rujialiu commented Sep 1, 2023

I made some mistake. Actually exactly the set of striked out functions work in WASM :) Should I make a separate PR with just those changes (and some wasm32 related changes that won't break anything) because currently this PR is breaking non-wasm builds (defaults to no optional features) and contains some extra codes (eval_code for javascript, and even an index.html)? @triska

But one problem: I don't know how to throw existance error in dispatch.rs I looked around and didn't find any similar codes... Currently I'm only cfging the function call, but I don't think that's what I'm supposed to do

@triska
Copy link
Contributor

triska commented Sep 1, 2023

@rujialiu: If you (via "cfg"-directives) remove all functions and corresponding dispatch entries that currently do not work with WASM, and also remove (via "cfg") all corresponding entries in build/instructions_template.rs, such as in:

#[strum_discriminants(strum(props(Arity = "7", Name = "$crypto_data_encrypt")))]

and:

&Instruction::CallCryptoDataEncrypt |

then everything should work as expected: An existence error will be thrown automatically if these (internal) predicates are not defined. You do not have to add any code, only remove (via "cfg") those parts that do not compile with WASM.

The cryptographic functionality always occurs contiguously in one block everywhere it occurs in the Rust files, i.e., in src/machine/dispatch.rs, build/instructions_template.rs and in src/machine/system_calls.rs. I hope this helps to keep the necessary changes minimally invasive in that it may be possible to "remove" entire regions of code at once instead of each function individually. When these functions are all "removed", you can also remove (also via "cfg") the ring dependency.

Yes, please do file pull requests at any time for everything that can be merged immediately without causing regressions for the regular 64-bit target! In this way, other contributors who are now also working on the code can already incorporate the changes you know will be eventually necessary for the WASM port, and it will also simplify the remaining changes and make them easier to review and discuss. Thank you a lot!

@rujialiu
Copy link
Author

rujialiu commented Sep 2, 2023

PR issued. What next? :) @triska

@triska
Copy link
Contributor

triska commented Sep 2, 2023

Very impressive, thank you a lot!

Next, I hope you are already preparing an awesome announcement and explanation of this new feature (explaining what it is, how it can be enabled, instructions for compilation, screenshot of an example etc.) and post it to the Scryer discussions as soon as the PR is merged! This will attract more interested application programmers to your contribution!

@triska
Copy link
Contributor

triska commented Sep 9, 2023

@rujialiu: I think this is now most excellently resolved via #1986, and can be closed? Thank you a lot!

@rujialiu
Copy link
Author

rujialiu commented Sep 9, 2023 via email

@rujialiu rujialiu closed this Sep 9, 2023
@rujialiu rujialiu deleted the wasm32-discussion branch September 9, 2023 14:54
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.

3 participants