Bug 112477 – Potentially unsafe register allocations in DFG code generation

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Michael Saboff]

A register that is allocated via GPRTemporary within one side or the other of an if () {} else {} is unsafe.  The register allocation can cause a spill on the one path that doesn't happen on the other path.  A subsequent use of the spilled register will load from the spilled location.

This was the root of https://bugs.webkit.org/show_bug.cgi?id=111777 - "Crash when updating predictions below JSC::arrayProtoFuncForEach on tuaw.com article"

I looked at the uses of GPRResult and GPRTemporary objects and found the following possibly similar issues:
   GPRTemporary structure(this) in SpeculativeJIT::compileObjectOrOtherLogicalNot for both DFGSpeculativeJIT32_64.cpp (~1598) and DFGSpeculativeJIT64.cpp (~1612)
   GPRTemporary structure(this) in SpeculativeJIT::compileObjectToObjectOrOtherEquality for both DFGSpeculativeJIT32_64.cpp (~1387) and DFGSpeculativeJIT64.cpp (~1411)
   GPRTemporary structure(this) in SpeculativeJIT::compilePeepHoleObjectToObjectOrOtherEquality for both DFGSpeculativeJIT32_64.cpp (~1488) and DFGSpeculativeJIT64.cpp (~1509)

Tracked also in <rdar://problem/13406093>

[Michael Saboff]

Created attachment 193397 [details]
Patch

[Early Warning System Bot]

Comment on attachment 193397 [details]
Patch

Attachment 193397 [details] did not pass qt-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/17060952

[Michael Saboff]

Created attachment 193402 [details]
Speculative patch for Qt build failure

Added initialization to InvalidGPRReg for structureGPR in three places.

[Filip Pizlo]

Comment on attachment 193402 [details]
Speculative patch for Qt build failure

Why was it wrong before?

[Michael Saboff]

(In reply to comment #4)
> (From update of attachment 193402 [details])
> Why was it wrong before?

Compiler error for uninitialized variable.

[Geoffrey Garen]

Comment on attachment 193402 [details]
Speculative patch for Qt build failure

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

r=me with one change suggested below:

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:1609
> +    GPRTemporary structure;
> +    GPRReg structureGPR = InvalidGPRReg;
> +
> +    if (!m_jit.graph().globalObjectFor(m_currentNode->codeOrigin)->masqueradesAsUndefinedWatchpoint()->isStillValid()) {
> +        GPRTemporary realStructure(this); // Only allocate on this path, before the branch
> +        structure.adopt(realStructure);
> +        structureGPR = structure.gpr();
> +    }

Let's use this model everywhere -- that way, we're always allocating our registers at the top of the function, and it's clearer that we got this right.

Let's update this comment so it's clearer why we're doing this. Something like, "The masquerades as undefined case will use the structure register, so allocate it here. Do this at the top of the function to avoid branching around a register allocation."

[Geoffrey Garen]

> +    if (!m_jit.graph().globalObjectFor(m_currentNode->codeOrigin)->masqueradesAsUndefinedWatchpoint()->isStillValid()) {

You mentioned that you were worried about this cost of this line of code. Let's put the result in a local variable and reuse it. That will resolve the cost, and read better too.

[Michael Saboff]

Committed r146100: <http://trac.webkit.org/changeset/146100>

[Ryosuke Niwa]

Is it possible that this caused 20% memory regression on Mac?

https://perf.webkit.org/#mode=charts&chartList=%5B%5B%22Mac%20MountainLion%22%2C%22Parser%2Fhtml5-full-render%3AJSHeap%22%5D%2C%5B%22Mac%20Lion%22%2C%22Parser%2Fhtml5-full-render%3AJSHeap%22%5D%5D&zoom=%5B1363572327935.7915%2C1363623676769.9104%5D

[Geoffrey Garen]

(In reply to comment #9)
> Is it possible that this caused 20% memory regression on Mac?

I would put the probability of that below 1%.

[Ryosuke Niwa]

(In reply to comment #10)
> (In reply to comment #9)
> > Is it possible that this caused 20% memory regression on Mac?
> 
> I would put the probability of that below 1%.

Okay, it's probably http://trac.webkit.org/changeset/146089 then.

[Michael Saboff]

(In reply to comment #9)
> Is it possible that this caused 20% memory regression on Mac?
> 
> https://perf.webkit.org/#mode=charts&chartList=%5B%5B%22Mac%20MountainLion%22%2C%22Parser%2Fhtml5-full-render%3AJSHeap%22%5D%2C%5B%22Mac%20Lion%22%2C%22Parser%2Fhtml5-full-render%3AJSHeap%22%5D%5D&zoom=%5B1363572327935.7915%2C1363623676769.9104%5D

Have you checked for leaks or does it appear to be just more usage?

As Geoff says, not likely this change.  I only moved around register allocations in the DFG JIT.