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

Add libc-backing support for WASI-SDK (v8) #19

Merged
merged 18 commits into from
Mar 24, 2021

Conversation

geky
Copy link
Collaborator

@geky geky commented Aug 9, 2020

The intent of this PR is to add initial support for WASI SDK. WASI SDK has a few benefits for Cortex-M development, such as allowing garbage collection of the libc-hooks. WASI SDK also provides prebuilt libc binaries, which is very nice for first time users. Also Wasmception has been deprecated in favor of WASI SDK, so would be a good idea to eventually migrate.

This PR has a few concessions:

  1. Stuck on WASI SDK v8. Moving beyond v8, the Wasm backend for LLVM starts to emit if instructions, which aWsm currently doesn't support, and I'm not confident enough with the inner-workings of aWsm to implement branching instructions :)
  2. Not an exhaustive implementation. Currently the WASI API is "best effort" similar to aWsm's level of instruction support. One part of this is that the WASI API is still a moving target, so it may be a waste of time to fully fill out the API (you may notice the API is prefixed with "wasi_unstable_"). The current API is plenty on Cortex-M, and a bit more than enough to get code_benches passing.

Changes:

  • Added memory.size and memory.grow instructions. WASI SDK uses these instead of a mmap hook.
  • Added copysign/ceil/nearest float instructions. The updated LLVM Wasm backend started emitting these. I tried to mirror the implementation of similar float instructions.
  • Added wasi_sdk_backing.c and renamed wasmception_backing.c. These provide the backing for both libcs and can be chosen similar to the memory/*.c options.
  • Added --wask-sdk and --wasmception options to code_benches/run.py. Also adopted argparse there as the argument handling seems to have gotten complex enough. By default run.py chooses the most recent one that exists.

Note for posterity:

Anyways, @Others, let me know how this looks

geky added 13 commits August 8, 2020 19:25
These simply call the externally linked instruction_memory_grow and
instruction_memory_size functions in the runtime.

Once connected, these runtime functions can use the existing expand_memory
logic that is specific to each runtime-memory implementation.
Note this is only needed when incrementally compiling, which we don't do
because of LTO.
- Removed wasmception integration for now
- Consistent readv/writev implementation
- Changed some magic numbers to enums
- Added predeclaration of (get|set)_(i8|16|32|64|f32|f64) stubs
- Consistent offset type in get/set stubs
- Moved env_(sin|cos) to runtime/libc/env.c
These aren't needed to pass code_benches, but may be used depending on
what functions the wasm binary uses (unlink -> wasi_unstable_path_unlink_file
for example).

Note this the WASI API backing is still not exhaustive.
These were emitted by wasi-sdk-8.0.
And some cleanup around wasm-libc selection
@geky geky changed the title Wasi sdk libc Add libc-backing support for WASI-SDK (v8) Aug 9, 2020
@geky
Copy link
Collaborator Author

geky commented Aug 9, 2020

Sorry about the delay on this. This PR has a number of different changes in it, let me know if you want me to split it into multiple PRs.

@geky geky mentioned this pull request Aug 9, 2020
Copy link
Collaborator

@gparmer gparmer left a comment

Choose a reason for hiding this comment

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

This looks like a clean, easy addition to me. @Others you agree?

This isn't solving all problems, but it is certainly adding some great support.

runtime/memory/64bit_nix.c Outdated Show resolved Hide resolved
runtime/libc/wasi_sdk_backing.c Show resolved Hide resolved
runtime/libc/wasi_sdk_backing.c Outdated Show resolved Hide resolved
runtime/memory/64bit_nix.c Outdated Show resolved Hide resolved
return memory_size / WASM_PAGE_SIZE;
}

i32 instruction_memory_grow(i32 count) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This feels pretty generic, but is replicated across backends. I wonder if there's an opportunity for more code sharing. Perhaps a future issue?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can address in this PR. Maybe @geky can lift these to runtime.c?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Assuming expand_memory, memory_size, and WASM_PAGE_SIZE are all available globally I can move these to runtime.c.

Though I wonder if memory_grow/memory_size would be a better interface than expand_memory/memory_size. They are isomorphic to expand_memory/memory_size, but are closer the Wasm specification.

code_benches/run.py Show resolved Hide resolved
Comment on lines +70 to +73
SILVERFISH_PATH, = bestpath([
(SILVERFISH_RELEASE_PATH, args.release),
(SILVERFISH_DEBUG_PATH, args.debug),
])
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if someone passes in both --release and --debug?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Dunno. Probably breaks.

I didn't think it was worth enforcing, but I can look into how to enforce exclusivity if you think it is worth it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Do you know of an argparse option that would help?

Copy link
Contributor

Choose a reason for hiding this comment

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

Still don't want to merge with out an explicit check for this edge case.

Also can you clean up the name and documentation of the "best path" function? It's a little more specific than the name let's on

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this documentation should help with this: https://docs.python.org/3/library/argparse.html#mutual-exclusion

code_benches/run.py Show resolved Hide resolved
code_benches/run.py Show resolved Hide resolved
runtime/libc/env.c Show resolved Hide resolved
runtime/memory/64bit_nix.c Outdated Show resolved Hide resolved
runtime/memory/cortex_m.c Outdated Show resolved Hide resolved
return memory_size / WASM_PAGE_SIZE;
}

i32 instruction_memory_grow(i32 count) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can address in this PR. Maybe @geky can lift these to runtime.c?

runtime/runtime.h Show resolved Hide resolved
src/wasm.rs Show resolved Hide resolved
geky added 4 commits August 19, 2020 03:25
Currently:
- code_benches/run.py --polybench
- code_benches/run.py --app
- code_benches/run.py --custom
The memory instruction implementation is just a thing wrapper around
the memory_size/memory_expand API in aWsm.
@geky
Copy link
Collaborator Author

geky commented Aug 19, 2020

Ok, sorry about being slow to update this! I've just pushed some changes based on your guys feedback:

  • Moved memory instruction implementations into runtime.c
  • Added proper conversion from system errno -> WASI errno
  • Added asserts to check against incorrect argument combos (--debug+--release, --wasi-sdk+--wasmception)

Also added a small bit of test filtering so you can run a subset of the code_benches. This is useful for sanity checking minor changes without needing to run the whole bench-suite:

./code_benches/run.py --polybench

src/wasm.rs Show resolved Hide resolved
runtime/libc/wasi_sdk_backing.c Show resolved Hide resolved
@Others
Copy link
Contributor

Others commented Mar 3, 2021

Here is what needs done to merge this:

  • Regarding the incompatible flags issue, we may want to move the official solution Sean suggested.
  • Regarding the best path function:

Also can you clean up the name and documentation of the "best path" function? It's a little more specific than the name let's on

  • env_sin and env_cos back to wasmception specific code, as they are a hack.
  • rebase off master
  • add an issue to track work on non-native page sizes/the weird behavior of memory.size in the presence of non-native page sizes, and fixing up the mmap wasmception hack we currently use for non-native page sizes
  • add an issue to track fixing the broken programs for wasi-sdk
  • add an issue to track implementing multiple memory support (right now this may cause weirdness if we aren't asserting that we're always loading from memory 0)

@Others
Copy link
Contributor

Others commented Mar 3, 2021

  • add an issue to track wasi CPU version changes

@Others
Copy link
Contributor

Others commented Mar 7, 2021

Had a go trying to rebase this, and very quickly ran into problems.

/Users/peachg/Projects/aWsm_rebase/wasi-sdk/bin/clang -Wl,--allow-undefined,-z,stack-size=16384,--no-threads,--stack-first,--no-entry,--export-all,--export=main,--export=dummy --target=wasm32-wasi -mcpu=mvp -nostartfiles -O3 -flto --sysroot=/Users/peachg/Projects/aWsm_rebase/wasi-sdk/share/wasi-sysroot  -O3 -flto ../dummy.c binarytrees.c -o bin/custom_binarytrees.wasm
/bin/sh: /Users/peachg/Projects/aWsm_rebase/wasi-sdk/bin/clang: No such file or directory

And there is no wasi-sdk module around. Is this missing a necessary submodule?

@geky
Copy link
Collaborator Author

geky commented Mar 9, 2021

Ah, it looks like I didn't update the instructions. You need to download wasi-sdk outside of git:

WASI_SDK_URL=https://github.com/WebAssembly/wasi-sdk/releases/download/wasi-sdk-8/wasi-sdk-8.0-linux.tar.gz
wget $WASI_SDK_URL -O wasi-sdk.tar.gz
mkdir -p wasi-sdk
tar xvfz wasi-sdk.tar.gz --strip-components=1 -C wasi-sdk

(I have this in the CI script, but I just realized it's nowhere else.)

This raises an interesting question of if the wasi-sdk should be available as a submodule. One of the benefits of moving to wasi-sdk is you don't need to compile LLVM thanks to the prebuilt binaries, but these aren't available as a git repo.

It may be valuable to also include wasi-sdk as a submodule, in which case this is the release/hash I was using for wasi-sdk:
https://github.com/WebAssembly/wasi-sdk/releases/wasi-sdk-8

Though not you would need to compile this before it would be usable, you also may need to tweak the paths to point to the compiled binaries correctly since I think the binary paths in the source differ from the prebuilt-binaries.

Currently fixed as WASI-SDK v8, downloads and untars if --wasi-sdk is
specified and WASI_SDK_PATH does not exist.
@Others
Copy link
Contributor

Others commented Mar 21, 2021

We can merge this, but it is totally broken on Mac.

  1. Downloads a linux specific binary
  2. Produces a compilation error:
clang -lm  -O3 -flto -g bin/custom_binarytrees.bc /Users/peachg/Projects/aWsm_rebase/runtime/runtime.c /Users/peachg/Projects/aWsm_rebase/runtime/libc/wasi_sdk_backing.c /Users/peachg/Projects/aWsm_rebase/runtime/libc/env.c /Users/peachg/Projects/aWsm_rebase/runtime/memory/cortex_m.c -o bin/custom_binarytrees_cm
warning: overriding the module target triple with x86_64-apple-macosx10.15.0 [-Woverride-module]
1 warning generated.
/Users/peachg/Projects/aWsm_rebase/runtime/libc/wasi_sdk_backing.c:280:1: warning: non-void function does not return a value in all control paths [-Wreturn-type]
}
^
/Users/peachg/Projects/aWsm_rebase/runtime/libc/wasi_sdk_backing.c:305:49: error: use of undeclared identifier 'O_RSYNC'
            ((fdflags & WASI_FDFLAG_RSYNC   ) ? O_RSYNC    : 0) |
                                                ^
/Users/peachg/Projects/aWsm_rebase/runtime/libc/wasi_sdk_backing.c:361:20: error: use of undeclared identifier 'O_RSYNC'
            ((fl & O_RSYNC)    ? WASI_FDFLAG_RSYNC      : 0) |
                   ^
/Users/peachg/Projects/aWsm_rebase/runtime/libc/wasi_sdk_backing.c:374:43: error: use of undeclared identifier 'O_RSYNC'
        ((flags & WASI_FDFLAG_RSYNC   ) ? O_RSYNC    : 0) |
                                          ^
/Users/peachg/Projects/aWsm_rebase/runtime/libc/wasi_sdk_backing.c:395:15: warning: implicit declaration of function 'fdatasync' is invalid in C99 [-Wimplicit-function-declaration]
    int res = fdatasync(fd);

@Others
Copy link
Contributor

Others commented Mar 21, 2021

@geky @gparmer Should we merge this and fast follow with a fix for OS-X?

@Others
Copy link
Contributor

Others commented Mar 21, 2021

I got this working on my machine with the following diff: https://gist.github.com/Others/49f7201c707edbe84dd0898ae17a185a

I think we can either apply this diff + do something smarter to pick the wasi release to download, or ship this PR, and I can follow up with a patch to fix it on mac.

@Others Others merged commit fa6e003 into gwsystems:master Mar 24, 2021
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.

4 participants