Skip to content

Commit 468ab1f

Browse files
author
Meghana Gupta
committed
Add try finally test cases and fix asserts
1 parent 0f03165 commit 468ab1f

6 files changed

Lines changed: 91 additions & 7 deletions

File tree

‎lib/Backend/BailOut.cpp‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1095,6 +1095,9 @@ Js::Var BailOutRecord::BailOut(BailOutRecord const * bailOutRecord)
10951095
Js::JavascriptCallStackLayout *const layout = bailOutRecord->GetStackLayout();
10961096
Js::ScriptFunction * function = (Js::ScriptFunction *)layout->functionObject;
10971097

1098+
Assert(function->GetScriptContext()->GetThreadContext()->GetPendingFinallyException() == nullptr ||
1099+
bailOutRecord->bailOutKind == IR::BailOutOnException);
1100+
10981101
if (bailOutRecord->bailOutKind == IR::BailOutOnImplicitCalls)
10991102
{
11001103
function->GetScriptContext()->GetThreadContext()->CheckAndResetImplicitCallAccessorFlag();

‎lib/Backend/FlowGraph.cpp‎

Lines changed: 15 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -933,6 +933,7 @@ Loop::InsertLandingPad(FlowGraph *fg)
933933

934934
landingPadLabel->SetBasicBlock(landingPad);
935935
landingPadLabel->SetRegion(headBlock->GetFirstInstr()->AsLabelInstr()->GetRegion());
936+
landingPadLabel->m_hasNonBranchRef = headBlock->GetFirstInstr()->AsLabelInstr()->m_hasNonBranchRef;
936937
landingPad->SetBlockNum(fg->blockCount++);
937938
landingPad->SetFirstInstr(landingPadLabel);
938939
landingPad->SetLastInstr(landingPadLabel);
@@ -1768,6 +1769,10 @@ FlowGraph::Destroy(void)
17681769
break;
17691770
case Js::OpCode::BrOnNoException:
17701771
Assert(region->GetType() == RegionTypeTry || region->GetType() == RegionTypeCatch || region->GetType() == RegionTypeFinally ||
1772+
// A BrOnException from finally to early exit can be converted to BrOnNoException and Br
1773+
// The Br block maybe a common successor block for early exit along with the BrOnNoException block
1774+
// Region from Br block will be picked up from a predecessor which is not BrOnNoException due to early exit
1775+
// See test0() in test/EH/tryfinallytests.js
17711776
(predRegion->GetType() == RegionTypeFinally && predBlock->GetLastInstr()->AsBranchInstr()->m_brFinallyToEarlyExit));
17721777
break;
17731778
case Js::OpCode::Br:
@@ -1782,11 +1787,13 @@ FlowGraph::Destroy(void)
17821787
}
17831788
else if (region->GetType() == RegionTypeFinally && region != predRegion)
17841789
{
1785-
AssertMsg(predRegion->GetType() == RegionTypeTry, "Bad region type for the try");
1790+
// We may be left with edges from finally region to early exit
1791+
AssertMsg(predRegion->IsNonExceptingFinally() || predRegion->GetType() == RegionTypeTry, "Bad region type for the try");
17861792
}
17871793
else
17881794
{
1789-
AssertMsg(region == predRegion, "Bad region propagation through interior block");
1795+
// We may be left with edges from finally region to early exit
1796+
AssertMsg(predRegion->IsNonExceptingFinally() || region == predRegion, "Bad region propagation through interior block");
17901797
}
17911798
break;
17921799
default:
@@ -1895,7 +1902,6 @@ FlowGraph::UpdateRegionForBlock(BasicBlock * block)
18951902
// If a LeaveNull block had only BrOnException edges from predecessor
18961903
// We will end up in inaccurate region propagation if we propagate from the predecessor edges
18971904
// So pick up the finally region from the map
1898-
Assert(this->leaveNullLabelToFinallyLabelMap->ContainsKey(block->GetFirstInstr()->AsLabelInstr()));
18991905
IR::LabelInstr * finallyLabel = this->leaveNullLabelToFinallyLabelMap->Item(block->GetFirstInstr()->AsLabelInstr());
19001906
Assert(finallyLabel);
19011907
region = finallyLabel->GetRegion();
@@ -1939,7 +1945,11 @@ FlowGraph::UpdateRegionForBlock(BasicBlock * block)
19391945
labelInstr->m_hasNonBranchRef = true;
19401946
}
19411947

1942-
if (region && this->func->HasFinally() && region->GetType() == RegionTypeRoot && !labelInstr->m_hasNonBranchRef)
1948+
// One of the pred blocks maybe an eh region, in that case it is important to mark this label's m_hasNonBranchRef
1949+
// If not later in codegen, this label can get deleted. And during SccLiveness, region is propagated to newly created labels in lowerer from the previous label's region
1950+
// We can end up assigning an eh region to a label in a non eh region. And if there is a bailout in such a region, bad things will happen in the interpreter :)
1951+
// See test2() in tryfinallytests.js
1952+
if (region && region->GetType() == RegionTypeRoot && !labelInstr->m_hasNonBranchRef)
19431953
{
19441954
FOREACH_PREDECESSOR_BLOCK(predBlock, block)
19451955
{
@@ -2440,7 +2450,7 @@ FlowGraph::InsertCompensationCodeForBlockMove(FlowEdge * edge, bool insertToLoo
24402450

24412451
bool assignRegionsBeforeGlobopt = this->func->HasTry() && (this->func->DoOptimizeTry() ||
24422452
(this->func->IsSimpleJit() && this->func->hasBailout));
2443-
// MGTODO : maybe we can just set the pred region instead of calling Propagate function ?
2453+
24442454
if (assignRegionsBeforeGlobopt)
24452455
{
24462456
UpdateRegionForBlockFromEHPred(compBlock);

‎lib/Backend/GlobOpt.cpp‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17617,6 +17617,11 @@ GlobOpt::RemoveFlowEdgeToFinallyOnExceptionBlock(IR::Instr * instr)
1761717617
{
1761817618
// We add edge from finally to early exit block
1761917619
// We should not remove this edge
17620+
// If a loop has continue, and we add edge in finally to continue
17621+
// Break block removal can move all continues inside the loop to branch to the continue added within finally
17622+
// If we get rid of this edge, then loop may loose all backedges
17623+
// Ideally, doing tail duplication before globopt would enable us to remove these edges, but since we do it after globopt, keep it this way for now
17624+
// See test1() in core/test/tryfinallytests.js
1762017625
return false;
1762117626
}
1762217627

@@ -17643,9 +17648,9 @@ GlobOpt::RemoveFlowEdgeToFinallyOnExceptionBlock(IR::Instr * instr)
1764317648
{
1764417649
if (!(nextLabel->m_next->IsBranchInstr() && nextLabel->m_next->AsBranchInstr()->IsUnconditional()))
1764517650
{
17646-
// Already processed in loop prepass
1764717651
return false;
1764817652
}
17653+
1764917654
BasicBlock * nextBlock = nextLabel->GetBasicBlock();
1765017655
IR::BranchInstr * branchTofinallyBlockOrEarlyExit = nextLabel->m_next->AsBranchInstr();
1765117656
IR::LabelInstr * finallyBlockLabelOrEarlyExitLabel = branchTofinallyBlockOrEarlyExit->GetTarget();

‎lib/Backend/IR.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -750,7 +750,7 @@ class BranchInstr : public Instr
750750
static BranchInstr * New(Js::OpCode opcode, Opnd* destOpnd, LabelInstr * branchTarget, Opnd *srcOpnd, Func *func);
751751
static BranchInstr * New(Js::OpCode opcode, LabelInstr * branchTarget, Opnd *src1Opnd, Opnd *src2Opnd, Func *func);
752752

753-
BranchInstr(bool hasBailOutInfo = false) : Instr(hasBailOutInfo), m_branchTarget(nullptr), m_isAirlock(false), m_isSwitchBr(false), m_isOrphanedLeave(false), m_areCmpRegisterFlagsUsedLater(false), m_brFinallyToEarlyExit(nullptr)
753+
BranchInstr(bool hasBailOutInfo = false) : Instr(hasBailOutInfo), m_branchTarget(nullptr), m_isAirlock(false), m_isSwitchBr(false), m_isOrphanedLeave(false), m_areCmpRegisterFlagsUsedLater(false), m_brFinallyToEarlyExit(false)
754754
{
755755
#if DBG
756756
m_isMultiBranch = false;

‎test/EH/rlexe.xml‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -113,4 +113,9 @@
113113
<baseline>tryfinallybug0.baseline</baseline>
114114
</default>
115115
</test>
116+
<test>
117+
<default>
118+
<files>tryfinallytests.js</files>
119+
</default>
120+
</test>
116121
</regress-exe>

‎test/EH/tryfinallytests.js‎

Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,61 @@
1+
//-------------------------------------------------------------------------------------------------------
2+
// Copyright (C) Microsoft. All rights reserved.
3+
// Licensed under the MIT license. See LICENSE.txt file in the project root for full license information.
4+
//-------------------------------------------------------------------------------------------------------
5+
6+
function test0() {
7+
var ui32 = new Uint32Array();
8+
for (var _strvar23 in ui32) {
9+
if (typeof _strvar23) {
10+
continue;
11+
}
12+
try {
13+
} catch (ex) {
14+
continue;
15+
} finally {
16+
}
17+
break;
18+
}
19+
}
20+
21+
function test1() {
22+
var arrObj0 = {};
23+
if (arrObj0.length) {
24+
for (var _strvar0 of IntArr0) {
25+
if (typeof _strvar0) {
26+
continue;
27+
}
28+
try {
29+
} catch (ex) {
30+
continue;
31+
} finally {
32+
}
33+
break;
34+
}
35+
}
36+
}
37+
38+
function test2() {
39+
var obj1 = {};
40+
var ary = Array();
41+
var i8 = new Int8Array();
42+
if (!(-530320868 >= ((obj1.method1) & 255))) {
43+
for (var _strvar1 of i8) {
44+
}
45+
}
46+
ary | 0;
47+
}
48+
49+
test0();
50+
test0();
51+
test0();
52+
53+
test1();
54+
test1();
55+
test1();
56+
57+
test2();
58+
test2();
59+
test2();
60+
61+
WScript.Echo("passed");

0 commit comments

Comments
 (0)