WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
211543
Fix ArrayMode nodes after
r261260
https://bugs.webkit.org/show_bug.cgi?id=211543
Summary
Fix ArrayMode nodes after r261260
Keith Miller
Reported
2020-05-06 18:58:33 PDT
Fix ArrayMode nodes after
r261260
Attachments
Patch
(2.73 KB, patch)
2020-05-06 19:00 PDT
,
Keith Miller
ysuzuki
: review+
Details
Formatted Diff
Diff
View All
Add attachment
proposed patch, testcase, etc.
Keith Miller
Comment 1
2020-05-06 19:00:47 PDT
Created
attachment 398695
[details]
Patch
Yusuke Suzuki
Comment 2
2020-05-06 19:37:43 PDT
Comment on
attachment 398695
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=398695&action=review
r=me with nits
> Source/JavaScriptCore/dfg/DFGClobberize.h:162 > + // all nodes with an ArrayMode can clobber top. We make an exception for CheckArray since it has no effects. > + if (graph.m_planStage < PlanStage::AfterFixup && node->hasArrayMode() && node->op() != CheckArray) > return clobberTop();
Can we list up all ArrayMode nodes here not to forget adding some nodes? Like this. if (graph.m_planStage < PlanStage::AfterFixup && node->hasArrayMode()) { switch (node->op()) { case GetIndexedPropertyStorage: ... break; case CheckArray: return clobberTop(); default: DFG_ASSERT(...); } } Do we need to include CheckArrayOrEmpty too?
Mark Lam
Comment 3
2020-05-06 20:29:56 PDT
Comment on
attachment 398695
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=398695&action=review
>> Source/JavaScriptCore/dfg/DFGClobberize.h:162 >> return clobberTop(); > > Can we list up all ArrayMode nodes here not to forget adding some nodes? > > Like this. > > if (graph.m_planStage < PlanStage::AfterFixup && node->hasArrayMode()) { > switch (node->op()) { > case GetIndexedPropertyStorage: > ... > break; > case CheckArray: > return clobberTop(); > default: > DFG_ASSERT(...); > } > } > > Do we need to include CheckArrayOrEmpty too?
Yusuke, I think you meant the opposite: if (graph.m_planStage < PlanStage::AfterFixup && node->hasArrayMode()) { switch (node->op()) { case GetIndexedPropertyStorage: ... return clobberTop(); case CheckArray: break; default: DFG_ASSERT(...); } }
Yusuke Suzuki
Comment 4
2020-05-06 22:18:52 PDT
Comment on
attachment 398695
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=398695&action=review
>>> Source/JavaScriptCore/dfg/DFGClobberize.h:162 >>> return clobberTop(); >> >> Can we list up all ArrayMode nodes here not to forget adding some nodes? >> >> Like this. >> >> if (graph.m_planStage < PlanStage::AfterFixup && node->hasArrayMode()) { >> switch (node->op()) { >> case GetIndexedPropertyStorage: >> ... >> break; >> case CheckArray: >> return clobberTop(); >> default: >> DFG_ASSERT(...); >> } >> } >> >> Do we need to include CheckArrayOrEmpty too? > > Yusuke, I think you meant the opposite: > > if (graph.m_planStage < PlanStage::AfterFixup && node->hasArrayMode()) { > switch (node->op()) { > case GetIndexedPropertyStorage: > ... > return clobberTop(); > case CheckArray: > break; > default: > DFG_ASSERT(...); > } > }
Oops, right.
Ryan Haddad
Comment 5
2020-05-07 09:20:42 PDT
I reverted
r261260
in
https://trac.webkit.org/changeset/261293
to address test failures. This should be combined with the original patch and landed at once.
Keith Miller
Comment 6
2020-05-07 11:00:25 PDT
Committed
r261313
: <
https://trac.webkit.org/changeset/261313
>
Radar WebKit Bug Importer
Comment 7
2020-05-07 11:01:30 PDT
<
rdar://problem/62983681
>
Mark Lam
Comment 8
2020-05-07 11:14:42 PDT
<
rdar://problem/62838095
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug