Skip to content

Commit cf2be31

Browse files
pleathpleath
committed
Precise closure-capture of local variables in the presence of deferred
nested functions. This is a good thing to do anyway (improves CQ and solves some odd memory leak cases) and is especially important for redeferral and it allows consistency of byte code regardless of deferral. (Byte code will not necessarily be identical, but it will be functionally the same.) The parser is now responsible for detecting and marking non-local references to user variables, and we no longer force closure-capture of all locals in the presence of a deferred child function.
1 parent 718a351 commit cf2be31

41 files changed

Lines changed: 467 additions & 641 deletions

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

‎lib/Backend/IRBuilder.cpp‎

Lines changed: 49 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -541,6 +541,13 @@ IRBuilder::Build()
541541
}
542542

543543
Js::RegSlot funcExprScopeReg = funcBody->GetFuncExprScopeRegister();
544+
if (funcExprScopeReg != Js::Constants::NoRegister)
545+
{
546+
IR::RegOpnd *funcExprScopeOpnd = BuildDstOpnd(funcExprScopeReg);
547+
instr = IR::Instr::New(Js::OpCode::NewPseudoScope, funcExprScopeOpnd, m_func);
548+
this->AddInstr(instr, (uint)-1);
549+
}
550+
544551
Js::RegSlot closureReg = funcBody->GetLocalClosureRegister();
545552
IR::RegOpnd *closureOpnd = nullptr;
546553
if (closureReg != Js::Constants::NoRegister)
@@ -556,13 +563,6 @@ IRBuilder::Build()
556563
}
557564
if (funcBody->HasScopeObject())
558565
{
559-
if (funcExprScopeReg != Js::Constants::NoRegister)
560-
{
561-
IR::RegOpnd *funcExprScopeOpnd = BuildDstOpnd(funcExprScopeReg);
562-
instr = IR::Instr::New(Js::OpCode::NewPseudoScope, funcExprScopeOpnd, m_func);
563-
this->AddInstr(instr, (uint)-1);
564-
}
565-
566566
if (funcBody->HasCachedScopePropIds())
567567
{
568568
this->BuildInitCachedScope(0, offset);
@@ -607,7 +607,7 @@ IRBuilder::Build()
607607
}
608608

609609
Js::RegSlot frameDisplayReg = funcBody->GetLocalFrameDisplayRegister();
610-
if (frameDisplayReg != Js::Constants::NoRegister && closureReg != Js::Constants::NoRegister)
610+
if (frameDisplayReg != Js::Constants::NoRegister)
611611
{
612612
Assert(!this->RegIsConstant(frameDisplayReg));
613613

@@ -617,7 +617,7 @@ IRBuilder::Build()
617617
{
618618
// Insert the function expression scope ahead of any enclosing scopes.
619619
IR::RegOpnd * funcExprScopeOpnd = BuildSrcOpnd(funcExprScopeReg);
620-
frameDisplayOpnd = IR::RegOpnd::New(TyVar, m_func);
620+
frameDisplayOpnd = closureReg != Js::Constants::NoRegister ? IR::RegOpnd::New(TyVar, m_func) : BuildDstOpnd(frameDisplayReg);
621621
instr = IR::Instr::New(Js::OpCode::LdFrameDisplay, frameDisplayOpnd, funcExprScopeOpnd, m_func);
622622
if (envReg != Js::Constants::NoRegister)
623623
{
@@ -626,46 +626,49 @@ IRBuilder::Build()
626626
this->AddInstr(instr, (uint)-1);
627627
}
628628

629-
IR::RegOpnd *dstOpnd;
630-
if (m_func->DoStackScopeSlots() && m_func->IsTopFunc())
631-
{
632-
dstOpnd = IR::RegOpnd::New(TyVar, m_func);
633-
}
634-
else
635-
{
636-
dstOpnd = this->BuildDstOpnd(frameDisplayReg);
637-
}
638-
instr = IR::Instr::New(op, dstOpnd, closureOpnd, m_func);
639-
if (frameDisplayOpnd != nullptr)
629+
if (closureReg != Js::Constants::NoRegister)
640630
{
641-
// We're building on an intermediate LdFrameDisplay result.
642-
instr->SetSrc2(frameDisplayOpnd);
643-
}
644-
else if (envReg != Js::Constants::NoRegister)
645-
{
646-
// We're building on the environment created by the enclosing function.
647-
instr->SetSrc2(this->BuildSrcOpnd(envReg));
648-
}
649-
this->AddInstr(instr, offset);
650-
if (dstOpnd->m_sym->m_isSingleDef)
651-
{
652-
dstOpnd->m_sym->m_isNotInt = true;
653-
}
631+
IR::RegOpnd *dstOpnd;
632+
if (m_func->DoStackScopeSlots() && m_func->IsTopFunc())
633+
{
634+
dstOpnd = IR::RegOpnd::New(TyVar, m_func);
635+
}
636+
else
637+
{
638+
dstOpnd = this->BuildDstOpnd(frameDisplayReg);
639+
}
640+
instr = IR::Instr::New(op, dstOpnd, closureOpnd, m_func);
641+
if (frameDisplayOpnd != nullptr)
642+
{
643+
// We're building on an intermediate LdFrameDisplay result.
644+
instr->SetSrc2(frameDisplayOpnd);
645+
}
646+
else if (envReg != Js::Constants::NoRegister)
647+
{
648+
// We're building on the environment created by the enclosing function.
649+
instr->SetSrc2(this->BuildSrcOpnd(envReg));
650+
}
651+
this->AddInstr(instr, offset);
652+
if (dstOpnd->m_sym->m_isSingleDef)
653+
{
654+
dstOpnd->m_sym->m_isNotInt = true;
655+
}
654656

655-
if (m_func->DoStackFrameDisplay())
656-
{
657-
// Use the stack closure sym to save the frame display pointer.
658-
this->AddInstr(
659-
IR::Instr::New(
660-
Js::OpCode::InitLocalClosure, this->BuildDstOpnd(m_func->GetLocalFrameDisplaySym()->m_id), m_func),
661-
(uint32)-1);
657+
if (m_func->DoStackFrameDisplay())
658+
{
659+
// Use the stack closure sym to save the frame display pointer.
660+
this->AddInstr(
661+
IR::Instr::New(
662+
Js::OpCode::InitLocalClosure, this->BuildDstOpnd(m_func->GetLocalFrameDisplaySym()->m_id), m_func),
663+
(uint32)-1);
662664

663-
this->AddInstr(
664-
IR::Instr::New(
665-
Js::OpCode::StSlot,
666-
this->BuildFieldOpnd(Js::OpCode::StSlot, m_func->GetLocalFrameDisplaySym()->m_id, 0, (Js::PropertyIdIndexType)-1, PropertyKindSlots),
667-
dstOpnd, m_func),
668-
(uint32)-1);
665+
this->AddInstr(
666+
IR::Instr::New(
667+
Js::OpCode::StSlot,
668+
this->BuildFieldOpnd(Js::OpCode::StSlot, m_func->GetLocalFrameDisplaySym()->m_id, 0, (Js::PropertyIdIndexType)-1, PropertyKindSlots),
669+
dstOpnd, m_func),
670+
(uint32)-1);
671+
}
669672
}
670673
}
671674
}

‎lib/Backend/amd64/LowererMDArch.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -430,7 +430,7 @@ LowererMDArch::LoadFuncExpression(IR::Instr *instrFuncExpr)
430430
paramOpnd = IR::SymOpnd::New(paramSym, TyMachReg, this->m_func);
431431
}
432432

433-
if (this->m_func->GetJnFunction()->IsGenerator())
433+
if (instrFuncExpr->m_func->GetJnFunction()->IsGenerator())
434434
{
435435
// the function object for generator calls is a GeneratorVirtualScriptFunction object
436436
// and we need to return the real JavascriptGeneratorFunction object so grab it before

‎lib/Backend/i386/LowererMDArch.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -501,7 +501,7 @@ LowererMDArch::LoadFuncExpression(IR::Instr *instrFuncExpr)
501501
paramOpnd = IR::SymOpnd::New(paramSym, TyMachReg, func);
502502
}
503503

504-
if (this->m_func->GetJnFunction()->IsGenerator())
504+
if (instrFuncExpr->m_func->GetJnFunction()->IsGenerator())
505505
{
506506
// the function object for generator calls is a GeneratorVirtualScriptFunction object
507507
// and we need to return the real JavascriptGeneratorFunction object so grab it before

‎lib/Parser/Hash.h‎

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -74,10 +74,11 @@ class Span
7474

7575
struct PidRefStack
7676
{
77-
PidRefStack() : isAsg(false), isDynamic(false), id(0), sym(nullptr), prev(nullptr), isModuleExport(false) {}
78-
PidRefStack(int id) : isAsg(false), isDynamic(false), id(id), sym(nullptr), prev(nullptr), isModuleExport(false) {}
77+
PidRefStack() : isAsg(false), isDynamic(false), id(0), funcId(0), sym(nullptr), prev(nullptr), isModuleExport(false) {}
78+
PidRefStack(int id, Js::LocalFunctionId funcId) : isAsg(false), isDynamic(false), id(id), funcId(funcId), sym(nullptr), prev(nullptr), isModuleExport(false) {}
7979

8080
int GetScopeId() const { return id; }
81+
Js::LocalFunctionId GetFuncScopeId() const { return funcId; }
8182
Symbol *GetSym() const { return sym; }
8283
void SetSym(Symbol *sym) { this->sym = sym; }
8384
bool IsAssignment() const { return isAsg; }
@@ -95,6 +96,7 @@ struct PidRefStack
9596
bool isDynamic;
9697
bool isModuleExport;
9798
int id;
99+
Js::LocalFunctionId funcId;
98100
Symbol *sym;
99101
PidRefStack *prev;
100102
};
@@ -178,10 +180,11 @@ struct Ident
178180
return nullptr;
179181
}
180182

181-
void PushPidRef(int blockId, PidRefStack *newRef)
183+
void PushPidRef(int blockId, Js::LocalFunctionId funcId, PidRefStack *newRef)
182184
{
183185
AssertMsg(blockId >= 0, "Block Id's should be greater than 0");
184186
newRef->id = blockId;
187+
newRef->funcId = funcId;
185188
newRef->prev = m_pidRefStack;
186189
m_pidRefStack = newRef;
187190
}
@@ -204,13 +207,13 @@ struct Ident
204207
return prevRef;
205208
}
206209

207-
PidRefStack * FindOrAddPidRef(ArenaAllocator *alloc, int scopeId)
210+
PidRefStack * FindOrAddPidRef(ArenaAllocator *alloc, int scopeId, Js::LocalFunctionId funcId)
208211
{
209212
// If the stack is empty, or we are pushing to the innermost scope already,
210213
// we can go ahead and push a new PidRef on the stack.
211214
if (m_pidRefStack == nullptr)
212215
{
213-
PidRefStack *newRef = Anew(alloc, PidRefStack, scopeId);
216+
PidRefStack *newRef = Anew(alloc, PidRefStack, scopeId, funcId);
214217
if (newRef == nullptr)
215218
{
216219
return nullptr;
@@ -234,7 +237,7 @@ struct Ident
234237
if (ref->prev == nullptr || ref->id < scopeId)
235238
{
236239
// No existing PidRef for this scopeId, so create and insert one at this position.
237-
PidRefStack *newRef = Anew(alloc, PidRefStack, scopeId);
240+
PidRefStack *newRef = Anew(alloc, PidRefStack, scopeId, funcId);
238241
if (newRef == nullptr)
239242
{
240243
return nullptr;

0 commit comments

Comments
 (0)