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

alloc free creates, memory import support, and more #3

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

Conversation

leon-thomm
Copy link

@leon-thomm leon-thomm commented Jan 8, 2024

Hey 👋 at eth-easl we're working on a fork of rWasm which we're using in one of our research projects. I would like to upstream as much as possible, so I want to open the discussion on the changes. Feel free to provide feedback on any part.

This PR adds

  • minor refactoring of parser
  • some documentation
  • option --no-alloc
  • option --crate-name
  • memory import support

notes on the changes to the code:

  • indirect function returns
    • rWasm used to return a Vec from indirect function calls. If the optionno-alloc is given, it now uses a new enum allocating an upper bound of space needed for the returns on the stack by considering the largest number of returns from any function in the compiled module.
  • memory changes
    • When memory is declared imported, a reference to the applicable memory type is required on module creation. The reference type required me to add lifetime annotations.
    • I eliminated the use of Box to keep it "simple" with --no-alloc. A Vec already lives on the heap, and a &mut [u8] can too, but a memory of type mut [u8] (--fixed-mem-size + not imported memory) now necessarily lives on the stack.
    • see also the new mem_type() function
    • currently, --no-alloc requires --fixed-mem-size
    • the memory system could probably be simplified, e.g. through a trait MemoryType 1
  • I added some comments and subjective styling changes to parser.rs
  • possibly a bugfix in section parsing, see comment in code
  • I removed the code causing Error: Unused bytes in the custom name section #2 but didn't fix the underlying issue

I think parser.rs syntax could use some love, but I couldn't come up with a nice solution yet. Do you have ideas?

I did some basic sanity checks after the changes but I didn't test thoroughly. I added a very basic integration test.2

Footnotes

  1. notice that for a Vec<T>-like type, T should probably be Sized, which can't be dynamically dispatched (so no MemoryType trait objects)

  2. I haven't tested WASI, but I didn't make WASI-specific changes.

and I changed how the names section parser reads the type, while the original implementation seems correct under the current number of subsection types, run!(32) is confusing because it should really only read one byte
I saw no good reason why some parsers should be defined within the generate! macro, and others not. For simplicity, I moved them all outside.
not a cosmetic change, will redo on another branch
Eliminates all uses of alloc::Box and alloc::Vec when --no-alloc is chosen.

The main difficulty with removing Vec is the fact that wasm functions have multi-returns, so when calling indirect we don't know how many returns come back.

This commit solves it by allocating as much space on the stack as needed for the highest possible number of returns when calling indirect.

There are other non-stack-based solutions, especially the tinyvec crate allows static heapless vector functionality without usage of unsafe.
makes it a lot nicer to consume the rWasm output from another program
Turns out we only need a panic handler in no_std crates if they are dynamic libraries, which shouldn't be necessary for the crate generated by rWasm.
This makes WasmModule::new() const, which allows for static initialization, which is important in heap-less environments.

It also makes access functions to global constant exports const, so global export constants can also be statically accessed, which is often useful as well.
will need this for incoming changes
This option allows passing a bytes array slice to the wasm module which will be used for the wasm memory. Since this must have a fixed size, this is currently only possible in combination with --fixed-mem-size.

Such a reference to an external chunk of memory requires lifetime annotation, which is a bit messy in the printer, I might try to come up with a macro which relaxes syntax boilerplate for code generation.
the option is implicit if the WASM module imports its memory
@jaybosamiya
Copy link
Member

jaybosamiya commented Jan 11, 2024

Hey! Glad to hear you're using a fork of rWasm for some of your research! Indeed upstreaming things would be good, and I'm open to merging a lot of these changes in.

Unfortunately, a few things are going to keep me busy for the next few months to be able to do a proper review of the code just yet, but a few quick comments:

  • Parser: I do agree that some things could be improved in the parser. I did notice you'd opened Error: Unused bytes in the custom name section #2 some time ago, but unfortunately haven't had a chance to dive in any deeper yet. Rather than just removing the code, as a workaround, it might be good to make it into a warning.
  • Adding more documentation is a great idea!
  • --no-alloc seems quite helpful, will have to look into the exact details to confirm before merging, but your notes seem to make sense overall right now
  • --crate-name is likely an obvious improvement, not much to comment there
  • memory import support is helpful!
  • indirect function returns using enums: it might be good for this to be a possible CLI switch (that automatically is enabled for no-alloc)
  • Minor: it would be awesome if you could revert out the removal of the main Cargo.lock (I plan to keep it around), and also remove the .wat files that you added to examples/

@jaybosamiya jaybosamiya self-requested a review January 11, 2024 20:14
@leon-thomm
Copy link
Author

Thank you for the feedback

Rather than just removing the code, as a workaround, it might be good to make it into a warning.

done; I added a dependency for colored output

  • indirect function returns using enums: it might be good for this to be a possible CLI switch (that automatically is enabled for no-alloc)

done

  • Minor: it would be awesome if you could revert out the removal of the main Cargo.lock (I plan to keep it around), and also remove the .wat files that you added to examples/

done

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