Bug 209457

Summary: HasIndexedProperty should know about sane chain
Product: WebKit Reporter: Keith Miller <keith_miller>
Component: New BugsAssignee: Keith Miller <keith_miller>
Status: RESOLVED FIXED    
Severity: Normal CC: aakash_jain, ap, ews-watchlist, mark.lam, msaboff, saam, ticaiolima, tzagallo, webkit-bot-watchers-bugzilla, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=209507
Attachments:
Description Flags
Patch
none
Patch none

Description Keith Miller 2020-03-23 17:38:10 PDT 
HasIndexedProperty should know about sane chain
Comment 1 Keith Miller 2020-03-23 17:42:46 PDT 
Created attachment 394333 [details]
Patch
Comment 2 Keith Miller 2020-03-23 17:47:52 PDT 
Created attachment 394334 [details]
Patch
Comment 3 Saam Barati 2020-03-23 18:10:15 PDT 
Comment on attachment 394334 [details]
Patch

r=me
Comment 4 Keith Miller 2020-03-23 19:51:43 PDT 
rdar://problem/59821190
Comment 5 EWS 2020-03-23 20:02:35 PDT 
Committed r258901: <https://trac.webkit.org/changeset/258901>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 394334 [details].
Comment 6 Caio Lima 2020-03-24 11:01:54 PDT 
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.
Comment 7 Saam Barati 2020-03-24 12:14:40 PDT 
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
Comment 8 Aakash Jain 2020-03-24 14:25:08 PDT 
(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.
Comment 9 Keith Miller 2020-03-24 15:07:10 PDT 
Uploaded a patch to address Caio and Saam's comments:
https://bugs.webkit.org/show_bug.cgi?id=209507