[webkit-reviews] review granted: [Bug 235382] [JSC] move function wrapping logic to a new Function type : [Attachment 449569] Patch

[bugzilla-daemon at webkit.org]

Yusuke Suzuki <ysuzuki at apple.com> has granted Caitlin Potter (:caitp)
<caitp at igalia.com>'s request for review:
Bug 235382: [JSC] move function wrapping logic to a new Function type
https://bugs.webkit.org/show_bug.cgi?id=235382

Attachment 449569: Patch

https://bugs.webkit.org/attachment.cgi?id=449569&action=review




--- Comment #8 from Yusuke Suzuki <ysuzuki at apple.com> ---
Comment on attachment 449569
  --> https://bugs.webkit.org/attachment.cgi?id=449569
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=449569&action=review

r=me with comments.

> Source/JavaScriptCore/builtins/ShadowRealmPrototype.js:37
>	   delete wrapped['name'];
>	   delete wrapped['length'];

Is it possible to remove them and integrate these changes into C++
JSRemoteFunction?

> Source/JavaScriptCore/jit/JITOperations.cpp:150
> +    JSValue result = getWrappedValue(globalObject, targetGlobalObject,
JSValue::decode(encodedValue));
> +    RETURN_IF_EXCEPTION(scope, JSValue::encode(result));
> +
> +    scope.release();
> +    return JSValue::encode(result);

Let's use

RELEASE_AND_RETURN(scope, JSValue::encode(getWrappedValue(globalObject,
targetGlobalObject, JSValue::decode(encodedValue))));

> Source/JavaScriptCore/jit/JITOperations.cpp:167
> +    JSValue result = getWrappedValue(globalObject, globalObject,
JSValue::decode(encodedValue));
> +    RETURN_IF_EXCEPTION(scope, JSValue::encode(result));
> +    scope.release();
> +    return JSValue::encode(result);

Let's use

RELEASE_AND_RETURN(scope, JSValue::encode(getWrappedValue(globalObject,
globalObject, JSValue::decode(encodedValue))));

> Source/JavaScriptCore/jit/JITOperations.cpp:183
> +    JSValue exceptionValue = scope.exception()->value();
> +    scope.clearException();

Let's return if it is terminated exception.
So,

Exception* exception = scope.exception();
if (UNLIKELY(vm.isTerminationException(exception))) {
    scope.release();
    return { };
}
JSValue exceptionValue = exception->value()

> Source/JavaScriptCore/jit/JITOperations.cpp:186
> +    scope.clearException();

Ditto.

Exception* toStringException = scope.exception();
if (ULIKELY(..._))
    ...

> Source/JavaScriptCore/jit/JITOperations.cpp:189
> +	   throwTypeError(globalObject, scope, exceptionString);

You can use

return throwVMTypeError(...);

> Source/JavaScriptCore/jit/JITOperations.cpp:191
> +	   throwTypeError(globalObject, scope);

Ditto.

> Source/JavaScriptCore/jit/JITOperations.cpp:193
> +    return encodedJSValue();

So, this line is not necessary.

> Source/JavaScriptCore/jit/JITOperations.h:98
> +    Jrf: JSRemoteFunction*

We do not need this since CCallHelpers can automatically handle them. So this
is not necessary :)

> Source/JavaScriptCore/jit/JITOperations.h:151
> +using J_JITOperation_JrfJ = EncodedJSValue(JIT_OPERATION_ATTRIBUTES
*)(JSRemoteFunction*, EncodedJSValue);

Ditto.

> Source/JavaScriptCore/jit/ThunkGenerators.cpp:1515
> +    // For each argument (order should not be observable):
> +    //     if the value is a Primitive, copy it into the new call frame
arguments, otherwise
> +    //     perform wrapping logic. If the wrapping logic results in a new
JSRemoteFunction,
> +    //     copy it into the new call frame's arguments, otherwise it must
have thrown a TypeError.

In the current code, we are copying right-to-left.
Is it possible that operationGetWrappedValueForTarget's exception reveals this
ordering?

> Source/JavaScriptCore/jit/ThunkGenerators.cpp:1517
> +    jit.sub32(CCallHelpers::TrustedImm32(1), GPRInfo::regT1);
> +    CCallHelpers::Jump done = jit.branchTest32(CCallHelpers::Zero,
GPRInfo::regT1);

We can use branchSub32 here.

> Source/JavaScriptCore/jit/ThunkGenerators.cpp:1544
> +	   jit.sub32(CCallHelpers::TrustedImm32(1), GPRInfo::regT1);
> +	   jit.branchTest32(CCallHelpers::NonZero, GPRInfo::regT1).linkTo(loop,
&jit);

We can use branchSub32 here.

> Source/JavaScriptCore/jit/ThunkGenerators.cpp:1563
> +#if ENABLE(ASSERT) && !CPU(ARM64E)

Use ASSERT_ENABLED

> Source/JavaScriptCore/jit/ThunkGenerators.cpp:1574
> +    // rethrow logic won't occur here. Need to set up an 

Looks like comment is trunced.

> Source/JavaScriptCore/jit/ThunkGenerators.cpp:1579
> +    JSValueRegs resultRegs(GPRInfo::regT0);

Let's use GPRInfo::returnValueGPR.

> Source/JavaScriptCore/jit/ThunkGenerators.cpp:1614
> +    targetFunctionException.link(&jit);
> +    jit.loadCell(CCallHelpers::addressFor(CallFrameSlot::callee),
GPRInfo::regT2);
> +   
jit.setupArguments<decltype(operationThrowRemoteFunctionException)>(GPRInfo::re
gT2);
> +    jit.prepareCallOperation(vm);
> +    jit.call(GPRInfo::nonArgGPR0, OperationPtrTag);
> +    // Fall through to jump to handler
> +
> +    exceptionChecks.link(&jit);
> +    jit.copyCalleeSavesToEntryFrameCalleeSavesBuffer(vm.topEntryFrame);
> +   
jit.setupArguments<decltype(operationLookupExceptionHandler)>(CCallHelpers::Tru
stedImmPtr(&vm));
> +    jit.prepareCallOperation(vm);
> +   
jit.move(CCallHelpers::TrustedImmPtr(tagCFunction<OperationPtrTag>(operationLoo
kupExceptionHandler)), GPRInfo::nonArgGPR0);
> +    emitPointerValidation(jit, GPRInfo::nonArgGPR0, OperationPtrTag);
> +    jit.call(GPRInfo::nonArgGPR0, OperationPtrTag);

Can we have a test using this path?

> Source/JavaScriptCore/runtime/JSFunctionInlines.h:84
> +    return static_cast<bool>(jsDynamicCast<const
JSRemoteFunction*>(globalObject()->vm(), this));

You can use,

return inherits<JSRemoteFunction>(vm)

> Source/JavaScriptCore/runtime/JSRemoteFunction.cpp:55
> +	   return JSValue(JSRemoteFunction::create(vm, targetGlobalObject,
target));

JSValue(...) is not necessary.

> Source/JavaScriptCore/runtime/JSRemoteFunction.cpp:66
> +    if (result.isEmpty())

We can use if (!result)

> Source/JavaScriptCore/runtime/JSRemoteFunction.cpp:76
> +    if (result.isEmpty())

Ditto.

> Source/JavaScriptCore/runtime/JSRemoteFunction.cpp:81
> +JSC_DEFINE_HOST_FUNCTION(remoteJSFunctionCall, (JSGlobalObject*
globalObject, CallFrame* callFrame))

Let's rename it to remoteFunctionCallForJSFunction to make it more explicit.

> Source/JavaScriptCore/runtime/JSRemoteFunction.cpp:87
> +    JSFunction* target = remoteFunction->targetFunction();

Let's do,

JSFunction* target = jsCast<JSFunction*>(remoteFunction->targetFunction());

> Source/JavaScriptCore/runtime/JSRemoteFunction.cpp:123
> +JSC_DEFINE_HOST_FUNCTION(remoteFunctionCall, (JSGlobalObject* globalObject,
CallFrame* callFrame))

Let's rename remoteFunctionCall to remoteFunctionCallGeneric to make it more
explicit.

> Source/JavaScriptCore/runtime/JSRemoteFunction.h:56
> +    JSCell* target() { return m_target.get(); }

Let's rename it to targetFunction().

> Source/JavaScriptCore/runtime/JSRemoteFunction.h:57
> +    JSFunction* targetFunction() { return
static_cast<JSFunction*>(getJSFunction(target())); }

Let's drop it.

> Source/JavaScriptCore/runtime/JSRemoteFunction.h:76
> +    WriteBarrier<JSCell> m_target;

Let's rename it to m_targetFunction to align it to JSBoundFunction.
And let's hold JSObject