Implement base for Walrus WASI #105
Conversation
|
Please give your insgihts on this if this is an accpetable approach @ksh8281 @clover2123 @zherczeg . |
|
It looks good to support Wasi as Wasi functions are widely used. @zherczeg I wonder your opinion and development plan as well |
|
It was closed by mistake. |
kulcsaradam
force-pushed
the
wasi
branch
2 times, most recently
from
86857d1
to
9a50432
Compare
|
@clover2123 please check out this version of a base for wasi. |
kulcsaradam
force-pushed
the
wasi
branch
2 times, most recently
from
255237a
to
94cfbf5
Compare
src/wasi/Wasi.h
Outdated
| FuncEnd, | ||
| }; | ||
|
|
||
| static FunctionType* determineFuncionType(std::string str) |
There was a problem hiding this comment.
Typo Funcion in this method name
There was a problem hiding this comment.
One more)
Instead of creating each FunctionType for WASI function
What about reusing SpecTestFunctionTypes defined in Shell.cpp?
There was a problem hiding this comment.
It can be done, I am just unsure if there are wasi functions where either the amount of arguments or results can be variable and we need to create new FunctionTypes, so just in case I created this function.
Shall I just change it to reuse SpecTestFunctionTypes ?
There was a problem hiding this comment.
Yes, please reuse SpecTestFunctionTypes as we already share a lot of it.
If we need new FunctionTypes in the future, then we can just add types on SpecTestFunctionTypes.
There was a problem hiding this comment.
I moved SpecTestFunctionTypes to a different file and also reworked the wasi functions to use those types. I am also thinking that renaming those to something like ConstantFunctionTypes could help to understand the code later down the line. What is your opinion on that?
kulcsaradam
force-pushed
the
wasi
branch
4 times, most recently
from
8763214
to
059cbc7
Compare
kulcsaradam
force-pushed
the
wasi
branch
2 times, most recently
from
6f0a750
to
abb1250
Compare
src/shell/Shell.cpp
Outdated
| Walrus::WASI wasi = Walrus::WASI(); | ||
| wasi.fillWasiFuncTable(); |
There was a problem hiding this comment.
WASI instance and its function table are created whenever executeWASM called.
But can we allocate WASI once and share it in each executeWASM call?
There was a problem hiding this comment.
It was a rookie mistake on my part, but yes, we can. I moved it into main after the creation of Store. If you'd like we could add a flag so that wasi is only allocated if it needs to be used.
kulcsaradam
force-pushed
the
wasi
branch
3 times, most recently
from
7aeb0c8
to
a1f7b88
Compare
kulcsaradam
force-pushed
the
wasi
branch
2 times, most recently
from
cbf3aa9
to
74a6342
Compare
src/shell/Shell.cpp
Outdated
| auto ft = functionTypes[SpecTestFunctionTypes::I32]; | ||
| Walrus::WASI::WasiFunc* wasiImportFunc = wasi->find(import->fieldName()); | ||
| if (wasiImportFunc == nullptr) { | ||
| break; |
There was a problem hiding this comment.
Is it okay to just stop (break) the whole importing process when an unimplemented wasi function is imported?
What if the remained import elements (importTypes) are valid?
There was a problem hiding this comment.
You are right, I honestly did not think about the unimplemented functions as I plan to implement all of preview1.
As far as I remember the documentation doesn't really mention this, I would believe it is left to the implementing environment. I would like your opinion on this matter. Which would you prefer to just communicate the error to the user and continue or should we leave it as it is now ( exit with error Insufficient import ) ? I am open to any other ideas too.
There was a problem hiding this comment.
How about handling unimplemented WASI features simply?
I mean that importing only supported WASI functions and just ignoring others.
if (wasiImportFunc != nullptr) {
// import wasi function
...
}
// continue importing of next elementThere was a problem hiding this comment.
I am okay with this and have updated the patch to implement it.
Implement basic structure of wasi functions and also update test runner to run wasi tests with the 'wasi' argument. Signed-off-by: Adam Laszlo Kulcsar <[email protected]>
I have started working on implemeting WASI in Walrus.
As I have seen WASI is in a change right now where they are abandoning their witx format for wit. They are in a constant change and it could be troublesome to import other WASI systems in Walrus, as all other WebAssembly runtimes implemented their own version too as I have seen (on the basis of the official c-api).
This base can help us start implementing our own version too. This is just a rough base though, and needs some working.
My basic thought were:
only import functions that are used by the module
have a constant list with our implemented functions (this needs work, but I am open to other structures too)
be easily expendable
The function list right now is just for example, I do not mean to actually implement it this way.