Start implementing internal V8 APIs #12821
Conversation
|
❌ @190n, your commit has failing tests :( 🪟💻 3 failing tests Windows x64 baseline
🪟💻 3 failing tests Windows x64
|
…ode native addons
| @@ -341,6 +342,7 @@ class NAPIFunction : public JSC::JSFunction { | |||
| NAPICallFrame frame(JSC::ArgList(args), function->m_dataPtr); | |||
|
|
|||
| auto scope = DECLARE_THROW_SCOPE(vm); | |||
| v8::HandleScope handleScope(v8::Isolate::fromGlobalObject(static_cast<Zig::GlobalObject*>(globalObject))); | |||
There was a problem hiding this comment.
I think this can technically be a node:vm global object but I don't think we handle this correctly elsewhere either
| return globalObject()->vm(); | ||
| } | ||
|
|
||
| const Zig::GlobalObject* globalObject() const |
There was a problem hiding this comment.
We should probably use Bun::GlobalScope for these, honestly
There was a problem hiding this comment.
but it's not a blocker
| void* raw_ptr = root.getPtr<void>(); | ||
| // check if this pointer is identical to the fixed locations where these primitive | ||
| // values are stored | ||
| if (raw_ptr == globalInternals->undefinedSlot()->getPtr<void>()) { |
There was a problem hiding this comment.
interesting that literally everything is a pointer
|
|
||
| namespace v8 { | ||
|
|
||
| Local<External> External::New(Isolate* isolate, void* value) |
There was a problem hiding this comment.
lol v8 external wraps napi external
backwards
| vm, | ||
| globalObject, | ||
| globalObject->functionPrototype(), | ||
| JSC::TypeInfo(JSC::InternalFunctionType, StructureFlags), |
There was a problem hiding this comment.
Is this meant to be a constructor? I think InternalFunctionType is fine. But if we can use JSFunction it is generally faster than InternalFunction (i am semi-mindlessly repeating something I saw in a PR review on webkit's github)
There was a problem hiding this comment.
I think it can be a constructor but I don't support that yet.
| } | ||
| }; | ||
|
|
||
| using FunctionCallback = void (*)(const FunctionCallbackInfo<Value>&); |
There was a problem hiding this comment.
Does this need a specified calling convention?
There was a problem hiding this comment.
I don't think so. It's only called by native code (inside FunctionTemplate::functionCall) not JS, and V8's definition doesn't specify a calling convention. On windows, the compiler should adapt from SysV to MSVC calling convention for us inside FunctionTemplate::functionCall.
|
|
||
| // An array used by HandleScope to store the items. Must keep pointer stability when resized, since | ||
| // v8::Locals point inside this array. | ||
| class HandleScopeBuffer : public JSC::JSNonFinalObject { |
There was a problem hiding this comment.
Can this be a JSC::JSCell instead?
| // the layout is correct. | ||
| struct Roots { | ||
| // v8-internal.h:775 | ||
| static const int kUndefinedValueRootIndex = 4; |
There was a problem hiding this comment.
Maybe we could use constexpr for these?
| } | ||
| // TODO(@190n) span.data() is latin1 not utf8, but this is okay as long as the only way to make | ||
| // a v8 string is NewFromUtf8. that's because NewFromUtf8 will use either all ASCII or all UTF-16. | ||
| memcpy(buffer, span.data(), to_copy); |
There was a problem hiding this comment.
Can to_copy be zero?
There was a problem hiding this comment.
Yes. I'm working on UTF-16 right now in this function so this path is probably gonna change a bit
| ASSERT_GC_OBJECT_INHERITS(thisObject, info()); | ||
| Base::visitChildren(thisObject, visitor); | ||
|
|
||
| for (int i = 0; i < thisObject->size; i++) { |
There was a problem hiding this comment.
this should be unsigned or size_t
There was a problem hiding this comment.
and we should copy the size into a local variable to avoid the extra pointer lookup
| Handle& HandleScopeBuffer::createUninitializedHandle() | ||
| { | ||
| RELEASE_ASSERT(size < capacity - 1); | ||
| int index = size; |
There was a problem hiding this comment.
| int index = size; | |
| unsigned index = size; |
|
|
||
| Handle& HandleScopeBuffer::createUninitializedHandle() | ||
| { | ||
| RELEASE_ASSERT(size < capacity - 1); |
There was a problem hiding this comment.
| RELEASE_ASSERT(size < capacity - 1); | |
| ASSERT(size <= capacity); |
| static constexpr int capacity = 64; | ||
|
|
||
| Handle storage[capacity]; | ||
| int size = 0; |
There was a problem hiding this comment.
| int size = 0; | |
| unsigned size = 0; |
What does this PR do?
Implements some of the V8 APIs that native modules written especially for Node.js use. Some have real implementations while some have stubs that just crash, but even those will let us see which APIs are missing because it will print which method failed instead of the generic
dyld: missing symbol calledon macOS.Most Node.js modules still won't fully load because
node_module_registerisn't fully implemented.TODOs before we can reasonably expect some modules to work (at least based on
cpu-features's usage):HandleScopeandEscapableHandleScopewithout abortingObjectTemplateandFunctionTemplateHow did you verify your code works?
I wrote automated tests