Bug 144176 – Function allocation sinking works for wrong reasons

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

[Basile Clement]

It is by design that we don't support sinking of function allocation through any related operations.
Object allocation, however, does sink through field assignment (PutByOffset etc.), so we have a check that is supposed to prevent function allocations from sinking through these.

However that check is misguided and (a) do not prevent function allocation sinking through field assignment and (b) ensures that if a function allocation ever sinks through a PutByOffset, very bad things happen instead.
Fortunately, PutByOffset (and their relatives) require first allocating a custom structure for the JSFunction object, thus the PutByOffset will always be preceded by an AllocatePropertyStorage through which no allocation can sink, and materialization would always be triggered before the PutByOffset - so this works by chance.

Anyway, this should be done the proper way, and I will submit a patch soon to prevent properly have checks preventing function allocation sinking through PutByOffset et al.

[Basile Clement]

Created attachment 251602 [details]
Should do the right thing

Compiles, but untested - will check my understanding of the nodes is correct and does not prevent valid object allocation sinking and add a ChangeLog entry on Monday.

[Basile Clement]

Created attachment 251763 [details]
Seems to be doing the right thing

[Basile Clement]

Created attachment 251764 [details]
Seems to be doing the right thing

[Basile Clement]

Comment on attachment 251764 [details]
Seems to be doing the right thing

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

> Source/JavaScriptCore/dfg/DFGObjectAllocationSinkingPhase.cpp:812
> +            if (target->op() != NewObject)

Unsure if I should add a helper function in DFG::Node for that...

[Filip Pizlo]

You should rebase :-)

[Filip Pizlo]

Comment on attachment 251764 [details]
Seems to be doing the right thing

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

>> Source/JavaScriptCore/dfg/DFGObjectAllocationSinkingPhase.cpp:812
>> +            if (target->op() != NewObject)
> 
> Unsure if I should add a helper function in DFG::Node for that...

Seems wrong - MaterializeNewObject is also a sink candidate.

[Basile Clement]

(In reply to comment #6)
> Comment on attachment 251764 [details]
> Seems to be doing the right thing
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=251764&action=review
> 
> >> Source/JavaScriptCore/dfg/DFGObjectAllocationSinkingPhase.cpp:812
> >> +            if (target->op() != NewObject)
> > 
> > Unsure if I should add a helper function in DFG::Node for that...
> 
> Seems wrong - MaterializeNewObject is also a sink candidate.

Oh, duh, indeed. Fixing that, w/ a helper function then.

[Basile Clement]

Created attachment 251768 [details]
MaterializeNewObject is also sinkable...

[Basile Clement]

Sorry, should have tried a clean build.

[Basile Clement]

Created attachment 251771 [details]
Remove spurious MustGenerate

[WebKit Commit Bot]

Comment on attachment 251771 [details]
Remove spurious MustGenerate

Clearing flags on attachment: 251771

Committed r183419: <http://trac.webkit.org/changeset/183419>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.