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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Sat Jan 28 15:26:56 PST 2017


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

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

>>> 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.


>> Source/JavaScriptCore/bytecode/PolymorphicAccess.cpp:1154
>> +            GPRReg baseForCustomValue = m_type == CustomValueGetter || m_type == CustomValueSetter ? baseForAccessGPR : baseForCustomGetGPR;
> Do you have tests for this?

Yes. They are in JSTests/stress/super-get-by-id.js#91.

>> 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.

Actually, this one isn't necessary. Just removing it. The version with JSValueRegs is in line 2070.

>> 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.

Thanks for the catch. Done.

>> 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.

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.

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/20170128/729066ff/attachment.html>

More information about the webkit-unassigned mailing list