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.

Sign up for GitHub

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

Library sandboxing changes - support for per instance, cross platform, wasi, some debugging #1833

Closed
wants to merge 2 commits into from
Closed

Library sandboxing changes - support for per instance, cross platform, wasi, some debugging #1833

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f62d8f0
Library sandboxing changes
shravanrn Feb 17, 2022
4e151c1
Part 1 : FIx test suite
shravanrn Mar 5, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
/bin
/build
/build*
/out
/fuzz-out
/emscripten
*.pyc
*.o
Copy link
Member

Choose a reason for hiding this comment

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

Can you revert this file and submit this as a separate PR if you think there is a need for it. I for one am not a fan of .gitignore files that match a broad set of files like this, but we can have that discussion outside of this PR.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Will fix

.idea/
.vscode/
/cmake-build-debug/
21 changes: 16 additions & 5 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -351,11 +351,9 @@ set(WABT_LIBRARY_SRC

add_library(wabt STATIC ${WABT_LIBRARY_SRC})

IF (NOT WIN32)
add_library(wasm-rt-impl STATIC wasm2c/wasm-rt-impl.c wasm2c/wasm-rt-impl.h)
install(TARGETS wasm-rt-impl DESTINATION ${CMAKE_INSTALL_LIBDIR})
install(FILES wasm2c/wasm-rt.h wasm2c/wasm-rt-impl.h DESTINATION ${CMAKE_INSTALL_INCLUDEDIR})
endif ()
add_library(wasm-rt-impl STATIC SHARED wasm2c/wasm-rt-impl.c wasm2c/wasm-rt-os-unix.c wasm2c/wasm-rt-os-win.c wasm2c/wasm-rt-wasi.c)
install(TARGETS wasm-rt-impl DESTINATION ${CMAKE_INSTALL_LIBDIR})
install(FILES wasm2c/wasm-rt.h DESTINATION ${CMAKE_INSTALL_INCLUDEDIR})

if (BUILD_FUZZ_TOOLS)
set(FUZZ_FLAGS "-fsanitize=fuzzer,address")
Expand Down Expand Up @@ -472,6 +470,19 @@ if (BUILD_TOOLS)
INSTALL
)

if (NOT WIN32)
set(LDL_LIB "-ldl")
else()
set(LDL_LIB "")
endif()

wabt_executable(
NAME wasm2c-runner
SOURCES wasm2c/wasm-rt-runner.c
Copy link
Member

Choose a reason for hiding this comment

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

Can you explain the utility of this helper? Why compile the the wasm2c code to a dll and then load it dyamically? Why not just compile it with a main.c and run its like that? Maybe there is some use case there its useful to have the wasm2c output be built as a dll that I'm not getting.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is a pattern that is common in many sandboxing toolchains. Lucet is probably the easiest to look at https://github.com/bytecodealliance/lucet/blob/main/lucet-wasi/src/main.rs

The goal of the runner is to give developers an easy way to test simple applications without changing their code. The runner uses a default Wasm configuration (instantiates a heap and the runtime etc) and would execute any wasm2c compiled module that includes a main. The expected workflow is that developers can simply compile their existing application to a wasm module, generate C with wasm2c, compile the result to a dll and run it with no modification.

Copy link
Member

Choose a reason for hiding this comment

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

I guess the separation of runtime from compiled module is the key benefit here.. since it allows the compilation of the module and runtime to be separated. But it seems that benefit is largely proportional to size of the runtime itself and how likely it is that you would want to iterate on the runtime without recompiling the wasm2c code.

In practice, have you found this useful with wasm2c?

Copy link
Collaborator Author

@shravanrn shravanrn Mar 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Yup! I think there are couple of positives here

  • You are absolutely right in that the main use case is to avoid compiling wasm2c multiple times for each app. In particular, I was compiling SPEC from wasm at some point and this was a useful way to not have to compile wasm2c repeatedly for each of the SPEC benchmarks.
  • One additional (minor) benefit is that having a runner is nice as it simplifies tutorials --- hello_world need not explain what files are needed to compile the wasm2c runtime etc.

That said, compiling wasm2c doesn't take that long and is not that complicated, so its not the end of the world to have to compile it multiple times. Other Wasm compilers like Lucet really needed a runner as recompiling Lucet (which is written in rust) takes a while.

Given this, I think a runner would be nice to have but not a strict necessity.

LIBS ${LDL_LIB}
INSTALL
)

# wasm-opcodecnt
wabt_executable(
NAME wasm-opcodecnt
Expand Down
Loading