Bug 159603 - REGRESSION(201900): validation failure for GetByOffset/PutByOffset in VALIDATE((node), node->child1().node() == node->child2().node() || node->child1()->result() == NodeResultStorage)
Summary: REGRESSION(201900): validation failure for GetByOffset/PutByOffset in VALIDAT...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Filip Pizlo
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-07-09 11:51 PDT by Filip Pizlo
Modified: 2016-07-09 14:08 PDT (History)
11 users (show)

See Also:


Attachments
the patch (2.73 KB, patch)
2016-07-09 12:06 PDT, Filip Pizlo
keith_miller: review+
Details | Formatted Diff | Diff
the patch (3.41 KB, patch)
2016-07-09 13:36 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Filip Pizlo 2016-07-09 11:51:23 PDT
This validation rule is broken.  It should always be valid to take:

Foo(@x, @x)

and turn it into:

a: ValueRep(@x)
b: ValueRep(@x)
Foo(@a, @b)

or:

y: Identity(@x)
Foo(@x, @y)

That's because it should be possible to rewire any data flow edge something that produces an equivalent value.  This validation rule means that such rewirings are invalid on GetByOffset/PutByOffset.
Comment 1 Filip Pizlo 2016-07-09 11:59:40 PDT
This is what the IR actually looks like when we crash:

     2641:< 1:->	ValueRep(Check:DoubleRep:@858<Double>, JS|PureInt, Bytecodedouble, bc#9)
     889:<!0:->	CheckStructure(Check:Cell:@2641, MustGen, [%Ea:Object], R:JSCell_structureID, Exits, bc#9)
     2642:< 1:->	ValueRep(Check:DoubleRep:@858<Double>, JS|PureInt, Bytecodedouble, bc#9)
     2643:< 1:->	ValueRep(Check:DoubleRep:@858<Double>, JS|PureInt, Bytecodedouble, bc#9)
     890:< 1:->	GetByOffset(Check:KnownCell:@2642, Check:KnownCell:@2643, JS|PureInt|UseAsInt, Nonboolint32, id24{Ca}, 0, inferredType = Int32, R:NamedProperties(24), Exits, bc#9)  predicting Nonboolint32

We fail validation at the GetByOffset.  But the GetByOffset is dead anyway, and even if it wasn't, it would be OK to ignore the first child of the GetByOffset when doing analysis even if it was different from the second child.  Both children produce the same value!
Comment 2 Filip Pizlo 2016-07-09 12:06:15 PDT
Created attachment 283263 [details]
the patch
Comment 3 Keith Miller 2016-07-09 12:24:48 PDT
Comment on attachment 283263 [details]
the patch

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

> Source/JavaScriptCore/ChangeLog:9
> +        This removes an incorrect validation rule and replaces it with a FIXME about how to make this
> +        aspect of IR easier to validate soundly.

You should say why the validation rule is not valid. It's not really clear what this patch is actually fixing without that information.
Comment 4 Filip Pizlo 2016-07-09 13:33:32 PDT
(In reply to comment #3)
> Comment on attachment 283263 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=283263&action=review
> 
> > Source/JavaScriptCore/ChangeLog:9
> > +        This removes an incorrect validation rule and replaces it with a FIXME about how to make this
> > +        aspect of IR easier to validate soundly.
> 
> You should say why the validation rule is not valid. It's not really clear
> what this patch is actually fixing without that information.

I'm assuming you just want me to copy-paste the bug description into the ChangeLog.
Comment 5 Keith Miller 2016-07-09 13:35:26 PDT
(In reply to comment #4)
> (In reply to comment #3)
> > Comment on attachment 283263 [details]
> > the patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=283263&action=review
> > 
> > > Source/JavaScriptCore/ChangeLog:9
> > > +        This removes an incorrect validation rule and replaces it with a FIXME about how to make this
> > > +        aspect of IR easier to validate soundly.
> > 
> > You should say why the validation rule is not valid. It's not really clear
> > what this patch is actually fixing without that information.
> 
> I'm assuming you just want me to copy-paste the bug description into the
> ChangeLog.

Yeah, that would be fine.
Comment 6 Filip Pizlo 2016-07-09 13:36:05 PDT
Created attachment 283268 [details]
the patch
Comment 7 Keith Miller 2016-07-09 13:37:00 PDT
Comment on attachment 283263 [details]
the patch

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

r=me if you add the description.

>>>> Source/JavaScriptCore/ChangeLog:9
>>>> +        aspect of IR easier to validate soundly.
>>> 
>>> You should say why the validation rule is not valid. It's not really clear what this patch is actually fixing without that information.
>> 
>> I'm assuming you just want me to copy-paste the bug description into the ChangeLog.
> 
> Yeah, that would be fine.

I actually missed that there was a description. If you do that I think the patch is g2g.
Comment 8 Keith Miller 2016-07-09 13:37:22 PDT
Comment on attachment 283268 [details]
the patch

r=me
Comment 9 WebKit Commit Bot 2016-07-09 14:08:26 PDT
Comment on attachment 283268 [details]
the patch

Clearing flags on attachment: 283268

Committed r203034: <http://trac.webkit.org/changeset/203034>
Comment 10 WebKit Commit Bot 2016-07-09 14:08:31 PDT
All reviewed patches have been landed.  Closing bug.