[Webkit-unassigned] [Bug 162124] op_get_by_id_with_this should use inline caching

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=162124

--- Comment #43 from Caio Lima <ticaiolima at gmail.com> ---
Comment on attachment 293804
  --> https://bugs.webkit.org/attachment.cgi?id=293804
Patch

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

>> Source/JavaScriptCore/bytecode/PolymorphicAccess.cpp:833
>>      const Identifier& ident = *state.ident;
> 
> Have you tested this code when we have the result/thisGPR be aliased to each other? What about when an exception is thrown and they're aliased and you're in the DFG/FTL? You should prove to yourself that these work, and also write test cases that stress them (at least for now, since register allocation may change in the future)

I analyzed the code and noticed that it can cause problems when thisGPR and valueRegs are aliased and valueRegs is used during the IC code. valueRegs are used in the following cases:

1. When m_type is GetGetter or Load. m_type is one of these type if AccessKind == Pure, and for GetByIdWithThis, it is always AccessKind == WithThis.
2. When viaProxy is true. To satisfy this condition, base cell type on tryCacheGetByID need to be PureForwardingProxyType (in our case, JSGlobalObject). I created the following test case to reproduce the case:

```
class Base {
    get value() { return this._value; }
};

let o = { test() { return super.value; } };
this.value = o.test;

Object.setPrototypeOf(this, Base.prototype);

this._value = 3;

print(this.value()); 
```
However it results on: 
```
Exception: TypeError: Cannot set prototype of this object
setPrototypeOf@[native code]
global code at ../js-tests/global-super.js:8:22
```

To see if any problem happens on our current tests suite, I changed the code to always alias valueRegs and thisGPR. No problem where found and the code ruined correctly on all tiers (tested on x86_64 only).
After this analysis, I couldn't think in test cases to stress it. Did you think in some cases?

>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:4370
>> +        thisValue.use();
> 
> This is not correct, we may need these if we throw an exception. So we're not yet done using these inputs.
> (I know this is done in other places, and those places are also wrong).

Reveted.

>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:4338
>> +        notCellList.append(m_jit.branchIfNotCell(JSValueRegs(thisValueGPR)));
> 
> Can you add modes to this node where we have CellUse for child1 and child2? Seems like this will be the common case, and the DFG may have already proven that these are cells.

Done.

>> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:2855
>> +            isCell(base, provenType(m_node->child1())), unsure(cellCase), unsure(notCellCase));
> 
> What about child2() being a cell? Also, see above comment about having a form where we have CellUse for these things.

Nice catch! Done.

>> Source/JavaScriptCore/jit/Repatch.cpp:347
>> +void repatchGetByID(ExecState* exec, JSValue baseValue, const Identifier& propertyName, const PropertySlot& slot, StructureStubInfo& stubInfo, GetByIDKind kind, FunctionPtr repatchCallOperation)
> 
> I would add a new GetByIDKind and change the appropriateGenerecGetByIdFunction instead of changing this method to take an extra parameter.

You are right. It is much better readable.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20161204/04b991fc/attachment.html>