Bug 209457 - HasIndexedProperty should know about sane chain
Summary: HasIndexedProperty should know about sane chain
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Keith Miller
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-03-23 17:38 PDT by Keith Miller
Modified: 2020-03-24 15:07 PDT (History)
10 users (show)

See Also:


Attachments
Patch (19.18 KB, patch)
2020-03-23 17:42 PDT, Keith Miller
no flags Details | Formatted Diff | Diff
Patch (19.14 KB, patch)
2020-03-23 17:47 PDT, Keith Miller
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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