Bug 245468 – A live node becomes undefined in OSR Exit.

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

[EntryHi]

function main() {
  const v75 = {};
  v75.__proto__ = Object;
  Object.defineProperty(Object, 117982949, {});
  let v119 = 0;
  while (v119 < 10) {
    function v121() {
      for (let v130 = 0; v130 < 100; v130++) {}
      const v142 = [2.1,  NaN];
      print(v142[0]) // wrongly print undefined 
    }
    v121()
    v119++;
  }
}
noDFG(main);
noFTL(main);
main();

With the above script as input to JSC, run JSC with the following parameters:
./jsc test.js --useConcurrentJIT=0 --jitPolicyScale=0.001

v142[0] should be 2.1. But after OSRExit, v142[0] becomes undefined. The problem may be in CompileNewArray in DFGSpeculativeJIT. 2.1 is used (operand.use()) and NaN causes bailout. The use count of 2.1 is 0. So OSRExit wrongly thinks 2.1 is dead and put an undefined into the call frame.

[Radar WebKit Bug Importer]

<rdar://problem/100497797>

[David Degazio]

Created attachment 462916 [details]
Patch

[David Degazio]

Elaborating on the explanation: in SpeculativeJIT::compileNewArray, in specifically the slow path (reached when we have invalidated the isHavingABadTime watchpoint), we explicitly use() each operand after we write it to the buffer:

>   case ALL_ARRAY_STORAGE_INDEXING_TYPES: {
>       JSValueOperand operand(this, use, ManualOperandSpeculation);
>       JSValueRegs operandRegs = operand.jsValueRegs();
>       if (hasInt32(node->indexingType())) {
>           DFG_TYPE_CHECK(
>               operandRegs, use, SpecInt32Only,
>               m_jit.branchIfNotInt32(operandRegs));
>       }
>       m_jit.storeValue(operandRegs, buffer + operandIdx);
>       operand.use();
>       break;
>   }

This is an optimization to avoid spilling operands unnecessarily when we call the new-array operation further down, by manually managing the node lifetimes. If a later operand fails the DFG_TYPE_CHECK, however, we may OSRExit. At that point, the exit information will be collected after earlier operands were used, potentially leaving it dead, resulting in the undefined value we see in this bug.

This patch resolves the issue by doing two separate passes - first, we traverse the operands and emit the necessary type checks; second, we traverse the operands again and write them to the buffer. In this scheme, we generate the exit information prior to using any of the operands, so they should all remain live during this period.

[Yusuke Suzuki]

Comment on attachment 462916 [details]
Patch

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

r=me

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:9694
> +            for (unsigned operandIdx = 0; operandIdx < node->numChildren(); ++operandIdx) {
> +                Edge use = m_graph.m_varArgChildren[node->firstChild() + operandIdx];
> +                JSValueOperand operand(this, use, ManualOperandSpeculation);
> +                JSValueRegs operandRegs = operand.jsValueRegs();
> +                if (hasInt32(node->indexingType())) {
> +                    DFG_TYPE_CHECK(
> +                        operandRegs, use, SpecInt32Only,
> +                        m_jit.branchIfNotInt32(operandRegs));
> +                }
> +            }

Let's put it under `if (hasInt32(node->indexingType()))` And remove inner `if (hasInt32(node->indexingType()))`.

[David Degazio]

Created attachment 462917 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 462917 [details]
Patch

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

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:9657
>          case ALL_DOUBLE_INDEXING_TYPES: {

In WebKit coding style, `case` is aligned to `switch`.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:9684
>          case ALL_INT32_INDEXING_TYPES:
>          case ALL_CONTIGUOUS_INDEXING_TYPES:
>          case ALL_ARRAY_STORAGE_INDEXING_TYPES: {

Ditto.

[Yusuke Suzuki]

Comment on attachment 462917 [details]
Patch

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

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:9704
>          default:

Ditto.

[Mark Lam]

[e-mail]:
Pull request: https://github.com/WebKit/WebKit/pull/5211

[Mark Lam]

*** Bug 246307 has been marked as a duplicate of this bug. ***

[Mark Lam]

Comment on attachment 462917 [details]
Patch

Let's land this via the github PR.

[EWS]

Committed 256611@main (104b2a1546ed): <https://commits.webkit.org/256611@main>

Reviewed commits have been landed. Closing PR #5211 and removing active labels.