Fix Errors in Generator Jit for SlotArray and CopyProp Restores #6700

rhuanjl · 2021-04-14T07:19:08Z

Issue 1 - Slot Array

The SlotArray is used to store variables that belong to local scopes (let and const), on an InterpreterStackFrame this is stored in a different location to other registers.

When resuming a jitted generator/async function after a yield/await point these local scopes must be restored BUT there is no logic to handle this restoration.

Instead immediately before the function is resumed in CallGenerator() copy the local scopes into "normal" reg slots that the jit can read from.

Notes:

this is only necessary if the function has already been partially executed (hence only do it if there is an Interpreter Stack Frame)
all Async functions, generator functions and AsyncGenerator functions go through CallGenerator - hence this is the right place for the fix

The jit is already trying to load these values from the location I'm now moving them to, see logic here:

ChakraCore/lib/Backend/LinearScan.cpp

Lines 5225 to 5233 in c7f192a

    
           else 
        
           { 
        
               srcOpnd = IR::IndirOpnd::New( 
        
                   this->interpreterFrameRegOpnd, 
        
                   this->GetOffsetFromInterpreterStackFrame(regSlot), 
        
                   copyPropStackSym->GetType(), 
        
                   this->func 
        
               ); 
        
           }

AND here:
https://github.com/chakra-core/ChakraCore/blob/master/lib/Backend/LinearScan.cpp#L5371
BUT was finding nothing - if the function begun in the interpreter it doesn't to write to this location, if it begun in the jit it's 0'd out when the function yields/awaits here:

ChakraCore/lib/Backend/BailOut.cpp

Lines 1742 to 1748 in c7f192a

    
           uint32 innerScopeCount = executeFunction->GetInnerScopeCount(); 
        
           for (uint32 i = 0; i < innerScopeCount; i++) 
        
           { 
        
               Js::RegSlot reg = executeFunction->GetFirstInnerScopeRegister() + i; 
        
               newInstance->SetInnerScopeFromIndex(i, newInstance->GetNonVarReg(reg)); 
        
               newInstance->SetNonVarReg(reg, nullptr); 
        
           }

Fix: #6678
Fix: #6679
Fix: #6692
Fix: #6690
Fix: #6697
Fix: #6698
Fix: #6699
Fix: #6705

Issue 2 - Copy prop'd syms

A book keeping error was resulting in hitting a FailFast unnecessarily when setting up a restore for a copy prop'd sym after a yield.

Also, whether or not a copy prop sym should be restored was being checked using its Key; correct if restoring to Key BUT when restoring to Value need to check based on Value.

Note, I've added the full POC from issue 6686 as a new test file because reducing it proved difficult.

Fix: #6685
Fix: #6686

@bin2415 thanks for the report
@xuelanxu thanks for the report

rhuanjl · 2021-04-14T08:18:31Z

@nhat-nguyen this was an interesting bug - curious if you’d come across it at all?

rhuanjl · 2021-04-14T15:00:06Z

lib/Runtime/ByteCode/ByteCodeEmitter.cpp

@@ -1792,7 +1792,7 @@ void ByteCodeGenerator::FinalizeRegisters(FuncInfo* funcInfo, Js::FunctionBody*
        }
    }

-    // NOTE: The FB expects the yield reg to be the final non-temp.
+    // NOTE: The FunctionBody expects the yield reg to be the final non-temp.


Ride-along comment improvement as I was tracking through where different registers go it took me a while to work out what an FB was

rhuanjl · 2021-04-14T15:02:23Z

lib/Runtime/Library/JavascriptGenerator.cpp

+        uint32 innerScopeCount = this->scriptFunction->GetFunctionBody()->GetInnerScopeCount();
+        for (uint32 i = 0; i < innerScopeCount; ++i)
+        {
+            Js::RegSlot reg = this->scriptFunction->GetFunctionBody()->GetFirstInnerScopeRegister() + i;
+            this->frame->SetNonVarReg(reg, this->frame->InnerScopeFromIndex(i));
+        }


This is effectively an inverse of lines 1742-1748 of BailOut.cpp.

NOTE - getting rid of the logic in BailOut.cpp would not be a sufficient fix as it's possible for a generator to start in the interpreter and transition to the Jit at a yield point

rhuanjl · 2021-04-14T15:04:14Z

test/es6/generator-jit-bugs.js

@@ -90,4 +90,51 @@ check(gen4.next().value, 1);
 check(gen4.next().value, 2);
 check(gen4.next().value, 3);

+title("Load Scope Slots in presence of for-in");


Note all these cases could be done as async functions (swap yield to await) - testing only as generators as internally the important code pathways are identical and doing it with generators means the test case doesn't have to wait for promises to resolve.

rhuanjl · 2021-04-14T23:41:55Z

lib/Backend/LinearScan.cpp

@@ -5135,8 +5135,10 @@ void LinearScan::GeneratorBailIn::BuildBailInSymbolList(

        if (unrestorableSymbols.TestAndClear(value->m_id))
        {
-            if (this->NeedsReloadingSymWhenBailingIn(copyPropSym.Key()))
+            if (this->NeedsReloadingSymWhenBailingIn(copyPropSym.Value()))


Compare with lines 5146-5150.

Important point is we must check if this Sym needs reloading based on its BackEndId (see line 5140 compared with line 5148.

rhuanjl · 2021-04-15T06:51:33Z

lib/Backend/LinearScan.cpp

            {
                BailInSymbol bailInSym(key->m_id /* fromByteCodeRegSlot */, value->m_id /* toBackendId */);
                bailInSymbols->PrependNode(this->func->m_alloc, bailInSym);
            }
        }
-        else if (unrestorableSymbols.TestAndClear(key->m_id))
+        if (unrestorableSymbols.TestAndClear(key->m_id))


Sometimes both the 'value' and 'key' appear in the unrestorableSymbols list; therefore always check both.

- copy prop'd syms can be referenced by key and value - both key and value can be in the unrestorableSymbols list - always check for both key and value - in each case check if the symbol is needed based on the BackEndId being checked 'key' or 'value'

rhuanjl force-pushed the gen_scope_slots branch from 561d343 to 54c61a5 Compare April 14, 2021 07:21

rhuanjl requested a review from ppenzin April 14, 2021 08:17

rhuanjl commented Apr 14, 2021

View reviewed changes

rhuanjl force-pushed the gen_scope_slots branch from 00942b6 to 4b19819 Compare April 14, 2021 23:38

rhuanjl commented Apr 14, 2021

View reviewed changes

rhuanjl changed the title ~~Handle SlotArray loads in jitted generators~~ Fix Errors in Generator Jit for SlotArray and CopyProp Restores Apr 14, 2021

rhuanjl force-pushed the gen_scope_slots branch from 4b19819 to be5693a Compare April 15, 2021 06:49

rhuanjl commented Apr 15, 2021

View reviewed changes

rhuanjl added 2 commits April 16, 2021 07:34

Handle SlotArray loads in jitted generators

d5c6f59

rhuanjl force-pushed the gen_scope_slots branch 2 times, most recently from ba708b5 to d9e818b Compare April 16, 2021 13:31

This was referenced Apr 17, 2021

Invalid Memory Access in OP_LdSlot #6705

Closed

Invalid Memory Access in GetScriptContext #6704

Open

rhuanjl force-pushed the gen_scope_slots branch from a73e08c to d9e818b Compare April 17, 2021 23:06

ppenzin approved these changes Apr 18, 2021

View reviewed changes

ppenzin merged commit 8917a7e into chakra-core:master Apr 18, 2021

This was referenced Apr 22, 2024

Segmentation fault in NativeCodeGenerator::CheckCodeGen #6965

Open

Assertion error: bad type object in conversion ToInteger #6964

Open

Segmentation fault in Js::RecyclableObject::GetType #6963

Open

ShortDevelopment mentioned this pull request Apr 23, 2024

[Epic]: Fix jitting of generator functions #6988

Open

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Errors in Generator Jit for SlotArray and CopyProp Restores #6700

Fix Errors in Generator Jit for SlotArray and CopyProp Restores #6700

rhuanjl commented Apr 14, 2021 •

edited

Loading

rhuanjl commented Apr 14, 2021

rhuanjl Apr 14, 2021

rhuanjl Apr 14, 2021

rhuanjl Apr 14, 2021

rhuanjl Apr 14, 2021 •

edited

Loading

rhuanjl Apr 15, 2021

	else
	{
	srcOpnd = IR::IndirOpnd::New(
	this->interpreterFrameRegOpnd,
	this->GetOffsetFromInterpreterStackFrame(regSlot),
	copyPropStackSym->GetType(),
	this->func
	);
	}

	uint32 innerScopeCount = executeFunction->GetInnerScopeCount();
	for (uint32 i = 0; i < innerScopeCount; i++)
	{
	Js::RegSlot reg = executeFunction->GetFirstInnerScopeRegister() + i;
	newInstance->SetInnerScopeFromIndex(i, newInstance->GetNonVarReg(reg));
	newInstance->SetNonVarReg(reg, nullptr);
	}

Fix Errors in Generator Jit for SlotArray and CopyProp Restores #6700

Fix Errors in Generator Jit for SlotArray and CopyProp Restores #6700

Conversation

rhuanjl commented Apr 14, 2021 • edited Loading

Issue 1 - Slot Array

Issue 2 - Copy prop'd syms

rhuanjl commented Apr 14, 2021

rhuanjl Apr 14, 2021

Choose a reason for hiding this comment

rhuanjl Apr 14, 2021

Choose a reason for hiding this comment

rhuanjl Apr 14, 2021

Choose a reason for hiding this comment

rhuanjl Apr 14, 2021 • edited Loading

Choose a reason for hiding this comment

rhuanjl Apr 15, 2021

Choose a reason for hiding this comment

rhuanjl commented Apr 14, 2021 •

edited

Loading

rhuanjl Apr 14, 2021 •

edited

Loading