Bug 209457 – HasIndexedProperty should know about sane chain

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Keith Miller]

HasIndexedProperty should know about sane chain

[Keith Miller]

Created attachment 394333 [details]
Patch

[Keith Miller]

Created attachment 394334 [details]
Patch

[Saam Barati]

Comment on attachment 394334 [details]
Patch

r=me

[Keith Miller]

rdar://problem/59821190

[EWS]

Committed r258901: <https://trac.webkit.org/changeset/258901>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 394334 [details].

[Caio Lima]

Comment on attachment 394334 [details]
Patch

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

I've found a bug on 32-bits code path and left some additional comments.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:13658
> +            m_jit.isEmpty(scratchGPR, resultGPR);

The name `isEmpty` looks strange to me. `HasIndexedProperty` returns true if index is present and false otherwise. Naming this function as `isEmpty` and placing result on `resultGPR` implies that the semantics of this operation is inverted, where it returns true if index is not present and false otherwise. Maybe it would be better name it as `isNotEmpty`.

> Source/JavaScriptCore/jit/AssemblyHelpers.h:964
> +    void isEmpty(GPRReg gpr, GPRReg dst)

This looks a bit strange. The JSVALUE64 version is testing if value is not empty, while 32-bits is checking if value is empty.

[Saam Barati]

Comment on attachment 394334 [details]
Patch

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

>> Source/JavaScriptCore/jit/AssemblyHelpers.h:964
>> +    void isEmpty(GPRReg gpr, GPRReg dst)
> 
> This looks a bit strange. The JSVALUE64 version is testing if value is not empty, while 32-bits is checking if value is empty.

agreed. Also, your 64-bit implementation can just test against itself for zero/nonzero

[Aakash Jain]

(In reply to EWS from comment #5)
> Committed r258901: <https://trac.webkit.org/changeset/258901>
This seems to have broken 186 jsc tests on jsc-armv7 as indicated by ews (https://ews-build.webkit.org/#/builders/26/builds/12356). Can you please have a look?

This is slowing down jsc-armv7 EWS.

[Keith Miller]

Uploaded a patch to address Caio and Saam's comments:
https://bugs.webkit.org/show_bug.cgi?id=209507