Add WASM API and Embedder Builtins to V8 #1
Conversation
deps/v8/include/v8-wasm.h
Outdated
| namespace v8 { | ||
| namespace wasm { | ||
|
|
||
| #define PAGE_SIZE 0x10000 |
There was a problem hiding this comment.
Suggstion: use a different name for this macro, or better, make this a const static field of Memory. This avoids potential confusion with the PAGE_SIZE of host memory pages. (it is possible that you're following this pattern from elsewhere in V8 source code, in which case, you can ignore this suggestion)
| } | ||
| } | ||
|
|
||
| // TODO(ohadrau): Clean up so that we're not maintaining 2 copies of this |
There was a problem hiding this comment.
nit: it is a good idea to include a reference to where the other copy is.
deps/v8/src/api/api-wasm.cc
Outdated
| uint8_t* Memory::data() { return data_; } | ||
|
|
||
| Table::Table(void* tableObject) { | ||
| tableObject_ = (void*) new i::WasmTableObject( |
There was a problem hiding this comment.
IIUC, this doing a copy construction of a new i::WasmTableObject. Was that the intention, or was the intention to keep a reference to the i::WasmTableObject passed in?
Either way, it would be more readable to extract the first cast to an assignment of its own:
auto otherTableObject = (i::WasmTableObject*) tableObject;
tableObject_ = new i::WasmTableObject(*otherTableObject);The cast to void* should be unnecessary.
There was a problem hiding this comment.
Quick explanation for the reason this is void* here: since I need to define this type in the V8 headers, I can't depend on anything in the v8::internal namespace. Thus I store it as a void* and cast it when necessary. I'm a little unsure of whether the copy here is correct, but IIRC the passed in value is just a reference to a stack-allocated table object that needs to be copied so that we can store it for later. The complication here is that the Context object may get used more than once or stored by the callee, so we have to be careful just creating a Context and then deleting it. Would like to discuss this in depth a little bit and find a better solution
deps/v8/src/api/api-wasm.cc
Outdated
| tableObject_ = (void*) new i::WasmTableObject( | ||
| *(i::WasmTableObject*) table.tableObject_); | ||
| } | ||
| Table::~Table() { delete tableObject_; } |
There was a problem hiding this comment.
I am not sure this delete works correctly (without relying on UB). The actual object you allocated was via new i::WasmTableObject, but the type of tableObject_ is void *.
There was a problem hiding this comment.
Great point, totally forgot about that
deps/v8/src/wasm/wasm-js.cc
Outdated
| Eternal<i::JSObject> imports = i_isolate->wasm_native_imports(); | ||
| Local<i::JSObject> result = imports.Get(isolate); | ||
| return_value.Set(Utils::ToLocal(result));*/ |
This commit adds a WebAssembly API to the V8 API. This enables the use and export of WASM values and functions from V8 API consumers. Specifically, this enables the use of many of the WASM C-API constructs alongside a normal V8 JavaScript engine. Additionally, this adds
WebAssembly.embedderBuiltins(), a JS function that can return a set of builtin modules (for WASM programs) provided by the V8 embedder.Design Docs
Main files are:
deps/v8/api/api-wasm.cc-- Primary implementation of API additionsdeps/v8/include/v8-wasm.h-- Headers for new APIChecklist
make -j4 test(UNIX), orvcbuild test(Windows) passes