<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#c74">Comment # 74</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=300268&action=diff" name="attach_300268" title="Patch Updated">attachment 300268</a> <a href="attachment.cgi?id=300268&action=edit" title="Patch Updated">[details]</a></span>
Patch Updated
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=300268&action=review">https://bugs.webkit.org/attachment.cgi?id=300268&action=review</a>
<span class="quote">>> Source/JavaScriptCore/bytecode/PolymorphicAccess.cpp:1153
>> + GPRReg baseForCustomValue = m_type == CustomValueGetter || m_type == CustomValueSetter ? baseForAccessGPR : baseForCustomGetGPR;
>
> I think the above comment makes this code wrong. We should have tests for this. Do you already have tests?
> According to the above comment:
> - CustomValue should be passed the property holder, which might not necessarily be |this|, however, your code says that it is.
> - Custom accessors are indeed passed |this|.
>
> Please add tests for both of these.</span >
I'm confused now. When we have a customAccessor, thisValue is the receiver, in that case it shouldn't be the "thisGPR"? I created a test to cover the customAccessorGetter:
```
// CustomGetter case
let customGetter = createCustomGetterObject();
Object.setPrototypeOf(customGetter, Object.prototype);
let subObj = {
__proto__: customGetter,
get value () {
return super.customGetterAccessor;
}
}
for (let i = 0; i < 1000000; i++) {
let value = getterValue(subObj);
assert(value == 100);
}
subObj.shouldThrow = true;
for (let i = 0; i < 1000000; i++) {
try {
getterValue(subObj);
assert(false);
} catch(e) {
assert(e instanceof TypeError);
};
}
```
If I don't set baseForCustomValue with thisGPR, this test case will fail. Is that correct behavior?
<span class="quote">>> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:2869
>> + } else if (m_node->child1().useKind() == CellUse || m_node->child2().useKind() == CellUse) {
>
> This case should now be impossible, please remove.</span >
Nice catch. My bad.
<span class="quote">>> Source/JavaScriptCore/jit/JITPropertyAccess32_64.cpp:676
>> + emitJumpSlowCaseIfNotJSCell(thisVReg, regT4);
>
> This still has the problem I was talking about in my review of an earlier patch. The slow case does not handle regT3/regT4 not being populated.</span >
My bad =(. Just forgot do modify it before.</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>