[webkit-reviews] review denied: [Bug 213545] [JSC] add IC support for op_get_private_name : [Attachment 403323] Patch

[bugzilla-daemon at webkit.org]

Saam Barati <sbarati at apple.com> has denied Caitlin Potter (:caitp)
<caitp at igalia.com>'s request for review:
Bug 213545: [JSC] add IC support for op_get_private_name
https://bugs.webkit.org/show_bug.cgi?id=213545

Attachment 403323: Patch

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




--- Comment #4 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 403323
  --> https://bugs.webkit.org/attachment.cgi?id=403323
Patch

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

Mostly LGTM, I agree with the direction here. But I have a suggestion and I
also found a bug that we should have a test for.

> Source/JavaScriptCore/bytecode/AccessCase.cpp:268
> +    case LoadPrivate:

name nit: I'd call this "PrivateFieldLoad"

> Source/JavaScriptCore/bytecode/AccessCase.cpp:1437
> +	   if (viaProxy() && m_type != LoadPrivate) {

why not have this be an assert inside viaProxy()?

We could just bail on caching if via window proxy.

> Source/JavaScriptCore/bytecode/AccessCase.h:127
> +	   LoadPrivate,

Do we really need this to be a different type? It's going down the same path as
Load. Why can't all the logic for private fields and cacheablility be done
inside Repatch.cpp when deciding if it's ok to cache?

> Source/JavaScriptCore/jit/JITOperations.cpp:2229
> +    if (baseValue.isCell() && baseValue.asCell()->isObject()) {

nit: baseValue.isObject() instead

> Source/JavaScriptCore/jit/JITOperations.cpp:2231
> +	   RETURN_IF_EXCEPTION(scope, encodedJSValue());

can this throw in practice? Maybe this should be an EXCEPTION_ASSERT that no
exception happened? IIRC, throwing is related to OOM for resolving rope
strings.

> Source/JavaScriptCore/jit/JITPropertyAccess.cpp:112
> +	   auto notCell = branchIfNotCell(regT0);
> +	   notCell.link(this);

not needed

> Source/JavaScriptCore/jit/JITPropertyAccess32_64.cpp:302
> +	   auto notCell = branchIfNotCell(regT1);
> +	   notCell.link(this);

ditto

> Source/JavaScriptCore/jit/Repatch.cpp:181
> +	   return operationGetPrivateName;

this is wrong because the two functions here don't actually have the same
signature. This will lead to it being called with the wrong arguments when you
repatch. (For comparison, you can look at operationGetById and
operationGetByIdOptimize).

Please add a test for this, as it should lead to a crash.