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

Keith Miller
Reported 2020-03-23 17:38:10 PDT
HasIndexedProperty should know about sane chain
Attachments
Patch (19.18 KB, patch)
2020-03-23 17:42 PDT, Keith Miller 		no flags
DetailsFormatted DiffDiff
Patch (19.14 KB, patch)
2020-03-23 17:47 PDT, Keith Miller 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Keith Miller
Comment 1 2020-03-23 17:42:46 PDT
Created attachment 394333 [details] Patch
Keith Miller
Comment 2 2020-03-23 17:47:52 PDT
Created attachment 394334 [details] Patch
Saam Barati
Comment 3 2020-03-23 18:10:15 PDT
Comment on attachment 394334 [details] Patch r=me
Keith Miller
Comment 4 2020-03-23 19:51:43 PDT
rdar://problem/59821190
EWS
Comment 5 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].
Caio Lima
Comment 6 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.
Saam Barati
Comment 7 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
Aakash Jain
Comment 8 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.
Keith Miller
Comment 9 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
Note You need to log in before you can comment on or make changes to this bug.