Disable OpHelperReg Optimisation in Coroutins, also fix pre-BailIn sp…

…ill code
chakra-core · Apr 19, 2021 · af1b538 · af1b538
1 parent 8917a7e
commit af1b538
Show file tree

Hide file tree

Showing 4 changed files with 35 additions and 22 deletions.
diff --git a/lib/Backend/LinearScan.cpp b/lib/Backend/LinearScan.cpp
@@ -3339,7 +3339,7 @@ LinearScan::KillImplicitRegs(IR::Instr *instr)
         return;
     }
 
-    if (instr->m_opcode == Js::OpCode::Yield)
+    if (instr->m_opcode == Js::OpCode::GeneratorBailInLabel)
     {
         this->bailIn.SpillRegsForBailIn();
         return;
@@ -5041,6 +5041,7 @@ IR::Instr* LinearScan::GeneratorBailIn::GenerateBailIn(IR::GeneratorBailInInstr*
     // - We don't need to restore argObjSyms because StackArgs is currently not enabled
     //   Commented out here in case we do want to enable it in the future:
     // this->InsertRestoreSymbols(bailOutInfo->capturedValues->argObjSyms, insertionPoint, saveInitializedReg);
+    Assert(!this->func->IsStackArgsEnabled());
     // 
     // - We move all argout symbols right before the call so we don't need to restore argouts either
 
@@ -5050,13 +5051,7 @@ IR::Instr* LinearScan::GeneratorBailIn::GenerateBailIn(IR::GeneratorBailInInstr*
         bailInInstr->capturedValues
     );
 
-    this->InsertRestoreSymbols(
-        *bailOutInfo->byteCodeUpwardExposedUsed,
-        bailInInstr->upwardExposedUses,
-        bailInInstr->capturedValues,
-        insertionPoint
-    );
-    Assert(!this->func->IsStackArgsEnabled());
+    this->InsertRestoreSymbols(insertionPoint);
 
 #ifdef ENABLE_DEBUG_CONFIG_OPTIONS
     if (PHASE_TRACE(Js::Phase::BailInPhase, this->func))
@@ -5201,12 +5196,7 @@ void LinearScan::GeneratorBailIn::BuildBailInSymbolList(
     AssertOrFailFastMsg(unrestorableSymbols.IsEmpty(), "There are unrestorable backend-only symbols across yield points");
 }
 
-void LinearScan::GeneratorBailIn::InsertRestoreSymbols(
-    const BVSparse<JitArenaAllocator>& byteCodeUpwardExposedUses,
-    const BVSparse<JitArenaAllocator>& upwardExposedUses,
-    const CapturedValues& capturedValues,
-    BailInInsertionPoint& insertionPoint
-)
+void LinearScan::GeneratorBailIn::InsertRestoreSymbols(BailInInsertionPoint& insertionPoint)
 {
     FOREACH_SLISTBASE_ENTRY(BailInSymbol, bailInSymbol, this->bailInSymbols)
     {

diff --git a/lib/Backend/LinearScan.h b/lib/Backend/LinearScan.h
@@ -1,5 +1,6 @@
 //-------------------------------------------------------------------------------------------------------
 // Copyright (C) Microsoft. All rights reserved.
+// Copyright (c) 2021 ChakraCore Project Contributors. All rights reserved.
 // Licensed under the MIT license. See LICENSE.txt file in the project root for full license information.
 //-------------------------------------------------------------------------------------------------------
 #pragma once
@@ -296,12 +297,7 @@ class LinearScan
         IR::SymOpnd* CreateGeneratorObjectOpnd() const;
 
         // Insert instructions to restore symbols in the `bailInSymbols` list
-        void InsertRestoreSymbols(
-            const BVSparse<JitArenaAllocator>& bytecodeUpwardExposedUses,
-            const BVSparse<JitArenaAllocator>& upwardExposedUses,
-            const CapturedValues& capturedValues,
-            BailInInsertionPoint& insertionPoint
-        );
+        void InsertRestoreSymbols(BailInInsertionPoint& insertionPoint);
 
         // Fill `bailInSymbols` list with all of the symbols that need to be restored
         void BuildBailInSymbolList(

diff --git a/lib/Backend/SccLiveness.cpp b/lib/Backend/SccLiveness.cpp
@@ -1,5 +1,6 @@
 //-------------------------------------------------------------------------------------------------------
 // Copyright (C) Microsoft Corporation and contributors. All rights reserved.
+// Copyright (c) 2021 ChakraCore Project Contributors. All rights reserved.
 // Licensed under the MIT license. See LICENSE.txt file in the project root for full license information.
 //-------------------------------------------------------------------------------------------------------
 
@@ -260,9 +261,19 @@ SCCLiveness::Build()
             {
                 this->EndOpHelper(labelInstr);
             }
-            if (labelInstr->isOpHelper && !PHASE_OFF(Js::OpHelperRegOptPhase, this->func))
+            if (labelInstr->isOpHelper)
             {
-                this->lastOpHelperLabel = labelInstr;
+                if (!PHASE_OFF(Js::OpHelperRegOptPhase, this->func) &&
+                    !this->func->GetTopFunc()->GetJITFunctionBody()->IsCoroutine())
+                {
+                    this->lastOpHelperLabel = labelInstr;
+                }
+#ifdef DBG
+                else
+                {
+                    labelInstr->AsLabelInstr()->m_noHelperAssert = true;
+                }
+#endif
             }
         }
         else if (instr->IsBranchInstr() && !instr->AsBranchInstr()->IsMultiBranch())

diff --git a/test/es6/generator-jit-bugs.js b/test/es6/generator-jit-bugs.js
@@ -152,5 +152,21 @@ check(gf8().next().value.v8, 1.1);
 check(gf8().next().value.v8, 1.1);
 check(gf8().next().value.v8, 1.1);
 
+// Test 9 - Invalid OpHelperBlockReg spill
+title("Inner function access after for...in loop containing yield that is not hit")
+{ // Note this bug only occurred when generator was declared inside an outer scope block
+    let i = 0;
+    function* gf9() {
+        function test() {}
+        for (var unused in []) {
+            yield undefined;
+        }
+        if(++i < 4)
+            gf9().next()
+        test + 1;
+    }
+}
+check(gf9().next().done, true);
+
 
 print("pass");