-
Notifications
You must be signed in to change notification settings - Fork 0
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 WASM API and Embedder Builtins to V8 #1
base: canary
Are you sure you want to change the base?
Conversation
deps/v8/include/v8-wasm.h
Outdated
namespace v8 { | ||
namespace wasm { | ||
|
||
#define PAGE_SIZE 0x10000 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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));*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: remove commented code
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