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

Jump to bottom

Implement WASI file functions and improve WASI structure #160

Merged
merged 1 commit into from
Nov 3, 2023
Merged

Implement WASI file functions and improve WASI structure #160

merged 1 commit into from
Nov 3, 2023

Conversation

kulcsaradam
Copy link
Contributor

Introduce class WasiFunction to enable WASI to acces Instance resources. Implement WASI types and path_open and fd_close functions.

@kulcsaradam
Copy link
Contributor Author

This pr tries to directly solve issue #133.

@kulcsaradam kulcsaradam force-pushed the wasi_acces branch 2 times, most recently from 841b5d0 to e0c4e25 Compare September 7, 2023 10:16
@kulcsaradam kulcsaradam marked this pull request as draft September 7, 2023 10:18
@kulcsaradam kulcsaradam force-pushed the wasi_acces branch 2 times, most recently from 4ce29c4 to 229aef0 Compare September 7, 2023 11:54
@kulcsaradam kulcsaradam marked this pull request as ready for review September 7, 2023 12:10
@kulcsaradam kulcsaradam force-pushed the wasi_acces branch 2 times, most recently from 98b6d82 to d4b9f79 Compare September 11, 2023 06:26
@kulcsaradam kulcsaradam changed the title Implement WASI file functions and improve WASI stucture Implement WASI file functions and improve WASI structure Sep 13, 2023
@kulcsaradam kulcsaradam force-pushed the wasi_acces branch 2 times, most recently from 4cf5bc7 to bf95b3e Compare September 13, 2023 09:06
@kulcsaradam
Copy link
Contributor Author

kulcsaradam commented Sep 13, 2023 •
edited
Loading

This patch introduces a new class WasiFunction which is only created for WASI functions. It is similar to ImportFunction, only differing in that it has a pointer to an instance instead of a void pointer. When an instance is created and a wasi functions is imported, this pointer will be set to the instance that imported it. Thus WASI functions could acces resources.

@kulcsaradam
Copy link
Contributor Author

@clover2123 @ksh8281
With one of the latest patches, this pr will not build.

Looks like the std c++17 libraries that I used are missing in c++11 but are present (in the experimental library) in c++ 14. I would like to ask you for help on how to fix this, I could think of two options:
1, Make a wasi build option that uses std c++14 like with msvc
2, Use boost/filesystem library, the base of std::filesystem (the license is already present in the project)

I am open to any other suggestions too.

@clover2123
Copy link
Collaborator

To support various platforms, we lowered the default version of language and build option as to c++11.
So we encourage to use c++11 version.
If boost/filesystem library is compatible with c++11, please use it if there is no license issue.

@kulcsaradam
Copy link
Contributor Author

kulcsaradam commented Sep 29, 2023 •
edited
Loading

Including Boost to work with Walrus proved to be highly difficult.

There are various problems, starting with that it is not recommended to build Boost with Cmake, instead it is prefered to build it's own engine, called b2.
Different packages (as I have seen) cannot be built without the b2 system.

Other solutions I can think of instead of Boost are:

  • Make Boost a dependency, like with CMake
  • Raise required stdc++ to version 14 for wasi builds
  • Build wabt with WASI ( there is an option, I am unsure if that is useable in practice)

Wabt wasi is highly experiemental and needs to be worked on.

Of course I am open to any other ideas.

@clover2123
Copy link
Collaborator

wabt engine currently uses uvwasi (https://github.com/nodejs/uvwasi) for its WASI implementation.
Basically, uvwasi is implemented in C language, so I think that it's fine with our build system (CMake)
How about considering uvwasi as an alternative of WASI implementation?

@kulcsaradam
Copy link
Contributor Author

I think it is okay. I will see if I can implement it in Walrus and I will keep you updated on the process!

@kulcsaradam kulcsaradam force-pushed the wasi_acces branch 2 times, most recently from d222b2d to 633532d Compare October 11, 2023 10:56
@kulcsaradam
Copy link
Contributor Author

I have implemented uvwasi into Walrus.

Some tests were failling, especially on armv7 and aarch64, because uvwasi uses libuv, which is downloaded via git, so I added it to those builds (others did not fail because of this).

I will look into fixing the test problem on windows-x86-64 and update the patch once its done.

@clover2123
Copy link
Collaborator

What about including uvwasi as a submodule instead of copying the whole source files?

@kulcsaradam
Copy link
Contributor Author

It is not an issue, I will be working on that then.

@kulcsaradam kulcsaradam force-pushed the wasi_acces branch 2 times, most recently from 187a895 to ff49b21 Compare October 16, 2023 13:13
@kulcsaradam
Copy link
Contributor Author

kulcsaradam commented Oct 16, 2023 •
edited
Loading

After addig uvwasi as a submodule I am having some build problems with macos and windows, which I am unsure how to fix yet, so I am open to any insight I can get.

@kulcsaradam
Copy link
Contributor Author

Windows problems are fixed, it was a problem with msvc and templates. I will start fixing macos problems.

@kulcsaradam kulcsaradam force-pushed the wasi_acces branch 4 times, most recently from a6fb8fc to eb42799 Compare October 20, 2023 14:20
@kulcsaradam
Copy link
Contributor Author

Macos problems were caused by cmake not fetching uvwasi's dependency libuv. All build problems should be fixed now.

@kulcsaradam kulcsaradam force-pushed the wasi_acces branch 4 times, most recently from 697191a to 2a68017 Compare October 27, 2023 12:50
clover2123
clover2123 reviewed Oct 31, 2023
View reviewed changes
build/wasi.cmake Outdated Show resolved Hide resolved
src/runtime/Function.h Show resolved Hide resolved
src/runtime/Function.h Outdated Show resolved Hide resolved
src/runtime/Function.cpp Outdated Show resolved Hide resolved
src/wasi/Wasi.h Outdated Show resolved Hide resolved
@kulcsaradam kulcsaradam changed the base branch from interp to main October 31, 2023 10:02
@kulcsaradam
Copy link
Contributor Author

I have rebased the patch to main branch and updated it with the suggested changes.

clover2123
clover2123 reviewed Nov 2, 2023
View reviewed changes
build/wasi.cmake Outdated
add_library(coverage_config INTERFACE)

option(CODE_COVERAGE "Enable coverage reporting" OFF)
if(CODE_COVERAGE AND CMAKE_C_COMPILER_ID MATCHES "AppleClang|GNU|Clang")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would you simplify this wasi.cmake file?
For example, we currently don't need any test or analysis about wasi, so we can remove coverage, asan and test build etc.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Other looks fine to me :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have simplified wasi.cmake significantly, but left in the debug logging option, beacuse it may come usefull during development, though set it OFF initially. Anything other than that that is not crucial is removed.

@kulcsaradam
Implement uvwasi library and improve WASI structure
d88d4e7 
Introduce uvwasi into the build system.
Fix build issues with libuv integration.
Introduce class WasiFunction to enable WASI to acces Instance resources.
Implement further WASI types and fd_write function.

Signed-off-by: Adam Laszlo Kulcsar <[email protected]>
clover2123
clover2123 approved these changes Nov 3, 2023
View reviewed changes
Copy link
Collaborator

@clover2123 clover2123 left a comment

Choose a reason for hiding this comment

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

LGTM

@clover2123 clover2123 merged commit 96ad876 into Samsung:main Nov 3, 2023
12 checks passed
@kulcsaradam kulcsaradam mentioned this pull request Nov 3, 2023
Closed
@kulcsaradam kulcsaradam deleted the wasi_acces branch November 3, 2023 11:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@clover2123 clover2123 clover2123 approved these changes

@ksh8281 ksh8281 Awaiting requested review from ksh8281 ksh8281 is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@kulcsaradam @clover2123