Bug 159603 – REGRESSION(201900): validation failure for GetByOffset/PutByOffset in VALIDATE((node), node->child1().node() == node->child2().node() || node->child1()->result() == NodeResultStorage)

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Filip Pizlo]

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.

[Filip Pizlo]

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!

[Filip Pizlo]

Created attachment 283263 [details]
the patch

[Keith Miller]

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.

[Filip Pizlo]

(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.

[Keith Miller]

(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.

[Filip Pizlo]

Created attachment 283268 [details]
the patch

[Keith Miller]

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.

[Keith Miller]

Comment on attachment 283268 [details]
the patch

r=me

[WebKit Commit Bot]

Comment on attachment 283268 [details]
the patch

Clearing flags on attachment: 283268

Committed r203034: <http://trac.webkit.org/changeset/203034>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.