<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - op_get_by_id_with_this should use inline caching"
href="https://bugs.webkit.org/show_bug.cgi?id=162124#c62">Comment # 62</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - op_get_by_id_with_this should use inline caching"
href="https://bugs.webkit.org/show_bug.cgi?id=162124">bug 162124</a>
from <span class="vcard"><a class="email" href="mailto:ticaiolima@gmail.com" title="Caio Lima <ticaiolima@gmail.com>"> <span class="fn">Caio Lima</span></a>
</span></b>
<pre>Comment on <span class=""><a href="attachment.cgi?id=297746&action=diff" name="attach_297746" title="Patch Updated">attachment 297746</a> <a href="attachment.cgi?id=297746&action=edit" title="Patch Updated">[details]</a></span>
Patch Updated
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=297746&action=review">https://bugs.webkit.org/attachment.cgi?id=297746&action=review</a>
<span class="quote">>>> Source/JavaScriptCore/bytecode/PolymorphicAccess.cpp:971
>>> + GPRReg baseForCustomGetGPR = baseGPR != thisGPR ? thisGPR : baseForGetGPR;
>>
>> Is this correct? Does DOMJIT expect the property holder or the receiver? This is a good question for Yusuke.
>
> DOMJIT custom getter assumes that the base value should be the DOM object.
> So it is imcompatible with op_get_by_id_with_this.
> For now, you can drop the DOMJIT acceleration support for `baseGPR != thisGPR` case.</span >
Done.
<span class="quote">>> Source/JavaScriptCore/bytecode/PolymorphicAccess.cpp:1154
>> + GPRReg baseForCustomValue = m_type == CustomValueGetter || m_type == CustomValueSetter ? baseForAccessGPR : baseForCustomGetGPR;
>
> Do you have tests for this?</span >
Yes. They are in JSTests/stress/super-get-by-id.js#91.
<span class="quote">>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h:2080
>> + JITCompiler::Call callOperation(J_JITOperation_ESsiJJI operation, JSValueRegs result, StructureStubInfo* stubInfo, GPRReg arg1Tag, GPRReg arg1Payload, GPRReg arg2Tag, GPRReg arg2Payload, UniquedStringImpl* uid)
>
> These could be JSValueRegs.</span >
Actually, this one isn't necessary. Just removing it. The version with JSValueRegs is in line 2070.
<span class="quote">>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:4368
>> + JSValueOperand thisValue(this, node->child2());
>
> This is wrong. If either base or this here are CellUse, you're failing to speculate, which is incorrect. One way to solve this is to only allow to use kind states for this node:
> 1 && 2 are both CellUse
> 1 && 2 are both Untyped use.
>
> You could ensure this inside the FixupPhase. Otherwise, you need to handle all 4 possibilities.</span >
Thanks for the catch. Done.
<span class="quote">>> Source/JavaScriptCore/jit/JITPropertyAccess.cpp:695
>> + Call call = callOperation(WithProfile, operationGetByIdWithThisOptimize, resultVReg, gen.stubInfo(), regT0, regT1, ident->impl());
>
> It seems like given your fast path code, these registers aren't guaranteed to be materialized to the values you want. Maybe load all values on the fast path before emitting a branch? Please add a test for this as it should probably be an insta-crash.</span >
Nice catch around that. I didn't find cases that can clobber regT0 and regT1 here, maybe you can help me think on that. Look the IC code generate for the following example:
```
let obj = {
method() { return super.foo; }
};
```
Here is the IC Assembly:
Generated JIT code for Access stub for jaz#D4sAX5:[0x1053682e0->0x1053b6e40, BaselineFunctionCall, 36] bc#28 with return point CodePtr(0x3b99796019b6): Getter:(Generated, structure
= 0x105374720:[Object, {foo:0}, NonArray, Proto:0x1053ac0a0, Leaf], offset = 0, callLinkInfo = 0x1047eb7f8):
Code at [0x3b9979601e60, 0x3b9979601f00):
0x3b9979601e60: cmp $0x108, (%rax)
0x3b9979601e66: jnz 0x3b9979601b25
0x3b9979601e6c: mov 0x10(%rax), %rdx
0x3b9979601e70: mov $0x1c, 0x24(%rbp)
0x3b9979601e77: mov 0x10(%rdx), %rdx
0x3b9979601e7b: test %rdx, %rdx
0x3b9979601e7e: jz 0x3b9979601ece
0x3b9979601e84: sub $0x20, %rsp
0x3b9979601e88: mov $0x1, 0x10(%rsp)
0x3b9979601e90: mov %rdx, 0x8(%rsp)
0x3b9979601e95: mov %rsi, 0x18(%rsp)
0x3b9979601e9a: mov $0x0, %r11
0x3b9979601ea4: cmp %r11, %rdx
0x3b9979601ea7: jnz 0x3b9979601eb7
0x3b9979601ead: call 0x3b9979601eb2
0x3b9979601eb2: jmp 0x3b9979601ed8
0x3b9979601eb7: mov %rdx, %rax
0x3b9979601eba: mov $0x1047eb7f8, %rdx
0x3b9979601ec4: call 0x3b9979601da0
0x3b9979601ec9: jmp 0x3b9979601ed8
0x3b9979601ece: mov $0xa, %rax
0x3b9979601ed8: lea -0x50(%rbp), %rsp
0x3b9979601edc: jmp 0x3b99796019b6
0x3b9979601ee1: jmp 0x3b9979601b25
Generated JIT code for InlineAccess: linking constant jump:
Code at [0x3b997960199f, 0x3b997960199f):
0x3b997960199f: jmp 0x3b9979601e60
Operations that clobbers regT0 (in x86, it is %eax) are 0x3b9979601eb7 (slowPath when the function isn't compiled yet) and 0x3b9979601ece (return undefined case). Both cases doesn't go to slowPath_get_by_id_with_this.
Also, we can't emmit before the branch in JITGetByIDWithThis::generateFastPath, because the code from there is just a jump for slowPath and nops to reserve space to IC code in the first moment:
[ 28] get_by_id_with_this loc8, loc7, this, foo(@id2) predicting Stringident
0x3b997960198e: mov -0x40(%rbp), %rax
0x3b9979601992: mov 0x28(%rbp), %rsi
0x3b9979601996: test %rax, %r15
0x3b9979601999: jnz 0x3b9979601b25
0x3b997960199f: jmp 0x3b9979601b25
0x3b99796019a4: o16 nop %cs:0x200(%rax,%rax)
0x3b99796019b3: nop (%rax)
0x3b99796019b6: mov %rax, 0x1047873f0
0x3b99796019c0: mov %rax, -0x48(%rbp)
The jump is changed when IC code is compiled and updated, so if we want to assure that the registers are materialized, I can add emit emitGetVirtualRegister() before callOperation in JIT::emitSlow_op_get_by_id_with_this.
It is important to notice that JIT::emitSlow_op_get_by_id is also trusting that its reg is materialized, but in that case, it is just regT0. If we find a bug for op_get_by_id_with_this, probably it will also be reproducible in op_get_by_id.
I'm going to add 2 test cases to reproduce if the registers can be clobbered and break stuff in the next Patch.
.</pre>
</div>
</p>
<hr>
<span>You are receiving this mail because:</span>
<ul>
<li>You are the assignee for the bug.</li>
</ul>
</body>
</html>