Skip to content

Commit dd6f5ce

Browse files
author
Meghana Gupta
committed
[MERGE #5913 @meg-gupta] Add SlotArrayCheck in Lowerer instead of IRBuilder
Merge pull request #5913 from meg-gupta:fixslotarraycheck Fixes OS#19767482 We insert LdSlots at the top of the function in jitloopbody for all syms that are coming in live to the loop. These LdSlots should be restored correctly on BailOutFromSimpleJitToJitLoopBody. However we do unreachable code elimination in flowgraph in simplejit, which can dead code the uses of these syms, and so they will not be restored on bailout. This works if we run deadstore pass which will cleanup all the LdSlots inserted at the top of the function, after we run unreachable block elimination phase. But since we turn off deadstore for functions with try/catch, we end up having a nullptr AV when we do a SlotArrayCheck followed by LdSlot
2 parents 5d83d3e + c99cec2 commit dd6f5ce

File tree

8 files changed

+123
-136
lines changed

8 files changed

+123
-136
lines changed

‎lib/Backend/Func.cpp

Lines changed: 0 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -143,7 +143,6 @@ Func::Func(JitArenaAllocator *alloc, JITTimeWorkItem * workItem,
143143
, constantAddressRegOpnd(alloc)
144144
, lastConstantAddressRegLoadInstr(nullptr)
145145
, m_totalJumpTableSizeInBytesForSwitchStatements(0)
146-
, slotArrayCheckTable(nullptr)
147146
, frameDisplayCheckTable(nullptr)
148147
, stackArgWithFormalsTracker(nullptr)
149148
, m_forInLoopBaseDepth(0)
@@ -1031,29 +1030,6 @@ Func::GetLocalsPointer() const
10311030

10321031
#endif
10331032

1034-
void Func::AddSlotArrayCheck(IR::SymOpnd *fieldOpnd)
1035-
{
1036-
if (PHASE_OFF(Js::ClosureRangeCheckPhase, this))
1037-
{
1038-
return;
1039-
}
1040-
1041-
Assert(IsTopFunc());
1042-
if (this->slotArrayCheckTable == nullptr)
1043-
{
1044-
this->slotArrayCheckTable = SlotArrayCheckTable::New(m_alloc, 4);
1045-
}
1046-
1047-
PropertySym *propertySym = fieldOpnd->m_sym->AsPropertySym();
1048-
uint32 slot = propertySym->m_propertyId;
1049-
uint32 *pSlotId = this->slotArrayCheckTable->FindOrInsert(slot, propertySym->m_stackSym->m_id);
1050-
1051-
if (pSlotId && (*pSlotId == (uint32)-1 || *pSlotId < slot))
1052-
{
1053-
*pSlotId = propertySym->m_propertyId;
1054-
}
1055-
}
1056-
10571033
void Func::AddFrameDisplayCheck(IR::SymOpnd *fieldOpnd, uint32 slotId)
10581034
{
10591035
if (PHASE_OFF(Js::ClosureRangeCheckPhase, this))

‎lib/Backend/Func.h

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -665,7 +665,6 @@ static const unsigned __int64 c_debugFillPattern8 = 0xcececececececece;
665665
PropertyIdSet lazyBailoutProperties;
666666
bool anyPropertyMayBeWrittenTo;
667667

668-
SlotArrayCheckTable *slotArrayCheckTable;
669668
FrameDisplayCheckTable *frameDisplayCheckTable;
670669

671670
IR::Instr * m_headInstr;
@@ -995,7 +994,6 @@ static const unsigned __int64 c_debugFillPattern8 = 0xcececececececece;
995994
void MarkConstantAddressSyms(BVSparse<JitArenaAllocator> * bv);
996995
void DisableConstandAddressLoadHoist() { canHoistConstantAddressLoad = false; }
997996

998-
void AddSlotArrayCheck(IR::SymOpnd *fieldOpnd);
999997
void AddFrameDisplayCheck(IR::SymOpnd *fieldOpnd, uint32 slotId = (uint32)-1);
1000998

1001999
void EnsureStackArgWithFormalsTracker();

‎lib/Backend/IRBuilder.cpp

Lines changed: 0 additions & 108 deletions
Original file line numberDiff line numberDiff line change
@@ -314,56 +314,6 @@ IRBuilder::AddEnvOpndForInnerFrameDisplay(IR::Instr *instr, uint offset)
314314
}
315315
}
316316

317-
bool
318-
IRBuilder::DoSlotArrayCheck(IR::SymOpnd *fieldOpnd, bool doDynamicCheck)
319-
{
320-
if (PHASE_OFF(Js::ClosureRangeCheckPhase, m_func))
321-
{
322-
return true;
323-
}
324-
325-
PropertySym *propertySym = fieldOpnd->m_sym->AsPropertySym();
326-
IR::Instr *instrDef = propertySym->m_stackSym->m_instrDef;
327-
IR::Opnd *allocOpnd = nullptr;
328-
329-
if (instrDef == nullptr)
330-
{
331-
if (doDynamicCheck)
332-
{
333-
return false;
334-
}
335-
Js::Throw::FatalInternalError();
336-
}
337-
switch(instrDef->m_opcode)
338-
{
339-
case Js::OpCode::NewScopeSlots:
340-
case Js::OpCode::NewStackScopeSlots:
341-
case Js::OpCode::NewScopeSlotsWithoutPropIds:
342-
allocOpnd = instrDef->GetSrc1();
343-
break;
344-
345-
case Js::OpCode::LdSlot:
346-
case Js::OpCode::LdSlotArr:
347-
if (doDynamicCheck)
348-
{
349-
return false;
350-
}
351-
// fall through
352-
default:
353-
Js::Throw::FatalInternalError();
354-
}
355-
356-
uint32 allocCount = allocOpnd->AsIntConstOpnd()->AsUint32();
357-
uint32 slotId = (uint32)propertySym->m_propertyId;
358-
359-
if (slotId >= allocCount)
360-
{
361-
Js::Throw::FatalInternalError();
362-
}
363-
364-
return true;
365-
}
366-
367317
///----------------------------------------------------------------------------
368318
///
369319
/// IRBuilder::Build
@@ -904,40 +854,6 @@ IRBuilder::Build()
904854
void
905855
IRBuilder::EmitClosureRangeChecks()
906856
{
907-
// Emit closure range checks
908-
if (m_func->slotArrayCheckTable)
909-
{
910-
// Local slot array checks, should only be necessary in jitted loop bodies.
911-
FOREACH_HASHTABLE_ENTRY(uint32, bucket, m_func->slotArrayCheckTable)
912-
{
913-
uint32 slotId = bucket.element;
914-
Assert(slotId != (uint32)-1 && slotId >= Js::ScopeSlots::FirstSlotIndex);
915-
916-
if (slotId > Js::ScopeSlots::FirstSlotIndex)
917-
{
918-
// Emit a SlotArrayCheck instruction, chained to the instruction (LdSlot) that defines the pointer.
919-
StackSym *stackSym = m_func->m_symTable->FindStackSym(bucket.value);
920-
Assert(stackSym && stackSym->m_instrDef);
921-
922-
IR::Instr *instrDef = stackSym->m_instrDef;
923-
IR::Instr *insertInstr = instrDef->m_next;
924-
IR::RegOpnd *dstOpnd = instrDef->UnlinkDst()->AsRegOpnd();
925-
IR::Instr *instr = IR::Instr::New(Js::OpCode::SlotArrayCheck, dstOpnd, m_func);
926-
927-
dstOpnd = IR::RegOpnd::New(TyVar, m_func);
928-
instrDef->SetDst(dstOpnd);
929-
instr->SetSrc1(dstOpnd);
930-
931-
// Attach the slot ID to the check instruction.
932-
IR::IntConstOpnd *slotIdOpnd = IR::IntConstOpnd::New(bucket.element, TyUint32, m_func);
933-
instr->SetSrc2(slotIdOpnd);
934-
935-
insertInstr->InsertBefore(instr);
936-
}
937-
}
938-
NEXT_HASHTABLE_ENTRY;
939-
}
940-
941857
if (m_func->frameDisplayCheckTable)
942858
{
943859
// Frame display checks. Again, chain to the instruction (LdEnv/LdSlot).
@@ -3571,8 +3487,6 @@ IRBuilder::BuildElementSlotI1(Js::OpCode newOpcode, uint32 offset, Js::RegSlot r
35713487
if (IsLoopBody())
35723488
{
35733489
fieldOpnd = this->BuildFieldOpnd(Js::OpCode::LdSlotArr, closureSym->m_id, slotId, (Js::PropertyIdIndexType)-1, PropertyKindSlotArray);
3574-
// Need a dynamic check on the size of the local slot array.
3575-
m_func->GetTopFunc()->AddSlotArrayCheck(fieldOpnd);
35763490
}
35773491
}
35783492
else if (IsLoopBody())
@@ -3594,11 +3508,6 @@ IRBuilder::BuildElementSlotI1(Js::OpCode newOpcode, uint32 offset, Js::RegSlot r
35943508
}
35953509
this->AddInstr(instr, offset);
35963510

3597-
if (!m_func->DoStackFrameDisplay() && IsLoopBody())
3598-
{
3599-
// Need a dynamic check on the size of the local slot array.
3600-
m_func->GetTopFunc()->AddSlotArrayCheck(fieldOpnd);
3601-
}
36023511
break;
36033512

36043513
case Js::OpCode::LdParamObjSlot:
@@ -3676,8 +3585,6 @@ IRBuilder::BuildElementSlotI1(Js::OpCode newOpcode, uint32 offset, Js::RegSlot r
36763585
if (IsLoopBody())
36773586
{
36783587
fieldOpnd = this->BuildFieldOpnd(Js::OpCode::LdSlotArr, closureSym->m_id, slotId, (Js::PropertyIdIndexType)-1, PropertyKindSlotArray);
3679-
// Need a dynamic check on the size of the local slot array.
3680-
m_func->GetTopFunc()->AddSlotArrayCheck(fieldOpnd);
36813588
}
36823589
}
36833590
else
@@ -3697,11 +3604,6 @@ IRBuilder::BuildElementSlotI1(Js::OpCode newOpcode, uint32 offset, Js::RegSlot r
36973604
instr->SetSrc2(fieldOpnd);
36983605
}
36993606

3700-
if (!m_func->DoStackFrameDisplay() && IsLoopBody())
3701-
{
3702-
// Need a dynamic check on the size of the local slot array.
3703-
m_func->GetTopFunc()->AddSlotArrayCheck(fieldOpnd);
3704-
}
37053607
break;
37063608

37073609
case Js::OpCode::StParamObjSlot:
@@ -4013,11 +3915,6 @@ IRBuilder::BuildElementSlotI2(Js::OpCode newOpcode, uint32 offset, Js::RegSlot r
40133915
else
40143916
{
40153917
fieldOpnd = this->BuildFieldOpnd(Js::OpCode::StSlot, slotId1, slotId2, (Js::PropertyIdIndexType)-1, PropertyKindSlots);
4016-
if (!this->DoSlotArrayCheck(fieldOpnd, IsLoopBody()))
4017-
{
4018-
// Need a dynamic check on the size of the local slot array.
4019-
m_func->GetTopFunc()->AddSlotArrayCheck(fieldOpnd);
4020-
}
40213918
}
40223919
newOpcode =
40233920
newOpcode == Js::OpCode::StInnerObjSlot || newOpcode == Js::OpCode::StInnerSlot ?
@@ -4056,11 +3953,6 @@ IRBuilder::BuildElementSlotI2(Js::OpCode newOpcode, uint32 offset, Js::RegSlot r
40563953
else
40573954
{
40583955
fieldOpnd = this->BuildFieldOpnd(Js::OpCode::LdSlot, slotId1, slotId2, (Js::PropertyIdIndexType)-1, PropertyKindSlots);
4059-
if (!this->DoSlotArrayCheck(fieldOpnd, IsLoopBody()))
4060-
{
4061-
// Need a dynamic check on the size of the local slot array.
4062-
m_func->GetTopFunc()->AddSlotArrayCheck(fieldOpnd);
4063-
}
40643956
}
40653957
regOpnd = this->BuildDstOpnd(regSlot);
40663958
instr = IR::Instr::New(Js::OpCode::LdSlot, regOpnd, fieldOpnd, m_func);

‎lib/Backend/IRBuilder.h

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -292,7 +292,6 @@ class IRBuilder
292292
Js::RegSlot GetEnvRegForEvalCode() const;
293293
Js::RegSlot GetEnvRegForInnerFrameDisplay() const;
294294
void AddEnvOpndForInnerFrameDisplay(IR::Instr *instr, uint offset);
295-
bool DoSlotArrayCheck(IR::SymOpnd *fieldOpnd, bool doDynamicCheck);
296295
void EmitClosureRangeChecks();
297296
void DoClosureRegCheck(Js::RegSlot reg);
298297
void BuildInitCachedScope(int auxOffset, int offset);

‎lib/Backend/Lower.cpp

Lines changed: 84 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2251,13 +2251,20 @@ Lowerer::LowerRange(IR::Instr *instrStart, IR::Instr *instrEnd, bool defaultDoFa
22512251
break;
22522252

22532253
case Js::OpCode::StSlot:
2254+
{
2255+
PropertySym *propertySym = instr->GetDst()->AsSymOpnd()->m_sym->AsPropertySym();
2256+
instrPrev = AddSlotArrayCheck(propertySym, instr);
22542257
this->LowerStSlot(instr);
22552258
break;
2259+
}
22562260

22572261
case Js::OpCode::StSlotChkUndecl:
2262+
{
2263+
PropertySym *propertySym = instr->GetDst()->AsSymOpnd()->m_sym->AsPropertySym();
2264+
instrPrev = AddSlotArrayCheck(propertySym, instr);
22582265
this->LowerStSlotChkUndecl(instr);
22592266
break;
2260-
2267+
}
22612268
case Js::OpCode::ProfiledLoopStart:
22622269
{
22632270
Assert(m_func->DoSimpleJitDynamicProfile());
@@ -2432,6 +2439,10 @@ Lowerer::LowerRange(IR::Instr *instrStart, IR::Instr *instrEnd, bool defaultDoFa
24322439
#endif
24332440

24342441
case Js::OpCode::LdSlot:
2442+
{
2443+
PropertySym *propertySym = instr->GetSrc1()->AsSymOpnd()->m_sym->AsPropertySym();
2444+
instrPrev = AddSlotArrayCheck(propertySym, instr);
2445+
}
24352446
case Js::OpCode::LdSlotArr:
24362447
{
24372448
Js::ProfileId profileId;
@@ -10889,6 +10900,78 @@ Lowerer::CreateOpndForSlotAccess(IR::Opnd * opnd)
1088910900
return indirOpnd;
1089010901
}
1089110902

10903+
IR::Instr* Lowerer::AddSlotArrayCheck(PropertySym *propertySym, IR::Instr* instr)
10904+
{
10905+
if (propertySym->m_stackSym != m_func->GetLocalClosureSym() || PHASE_OFF(Js::ClosureRangeCheckPhase, m_func))
10906+
{
10907+
return instr->m_prev;
10908+
}
10909+
10910+
IR::Instr *instrDef = propertySym->m_stackSym->m_instrDef;
10911+
10912+
bool doDynamicCheck = this->m_func->IsLoopBody();
10913+
bool insertSlotArrayCheck = false;
10914+
uint32 slotId = (uint32)propertySym->m_propertyId;
10915+
10916+
if (instrDef)
10917+
{
10918+
switch (instrDef->m_opcode)
10919+
{
10920+
case Js::OpCode::NewScopeSlots:
10921+
case Js::OpCode::NewStackScopeSlots:
10922+
case Js::OpCode::NewScopeSlotsWithoutPropIds:
10923+
{
10924+
IR::Opnd *allocOpnd = allocOpnd = instrDef->GetSrc1();
10925+
uint32 allocCount = allocOpnd->AsIntConstOpnd()->AsUint32();
10926+
10927+
if (slotId >= allocCount)
10928+
{
10929+
Js::Throw::FatalInternalError();
10930+
}
10931+
break;
10932+
}
10933+
case Js::OpCode::ArgIn_A:
10934+
break;
10935+
case Js::OpCode::LdSlot:
10936+
case Js::OpCode::LdSlotArr:
10937+
{
10938+
if (doDynamicCheck && slotId > Js::ScopeSlots::FirstSlotIndex)
10939+
{
10940+
insertSlotArrayCheck = true;
10941+
}
10942+
break;
10943+
}
10944+
case Js::OpCode::SlotArrayCheck:
10945+
{
10946+
uint32 currentSlotId = instrDef->GetSrc2()->AsIntConstOpnd()->AsInt32();
10947+
if (slotId > currentSlotId)
10948+
{
10949+
instrDef->ReplaceSrc2(IR::IntConstOpnd::New(slotId, TyUint32, m_func));
10950+
}
10951+
break;
10952+
}
10953+
default:
10954+
Js::Throw::FatalInternalError();
10955+
}
10956+
}
10957+
if (insertSlotArrayCheck)
10958+
{
10959+
IR::Instr *insertInstr = instrDef->m_next;
10960+
IR::RegOpnd *dstOpnd = instrDef->UnlinkDst()->AsRegOpnd();
10961+
IR::Instr *checkInstr = IR::Instr::New(Js::OpCode::SlotArrayCheck, dstOpnd, m_func);
10962+
10963+
dstOpnd = IR::RegOpnd::New(TyVar, m_func);
10964+
instrDef->SetDst(dstOpnd);
10965+
checkInstr->SetSrc1(dstOpnd);
10966+
10967+
// Attach the slot ID to the check instruction.
10968+
IR::IntConstOpnd *slotIdOpnd = IR::IntConstOpnd::New(slotId, TyUint32, m_func);
10969+
checkInstr->SetSrc2(slotIdOpnd);
10970+
insertInstr->InsertBefore(checkInstr);
10971+
}
10972+
return instr->m_prev;
10973+
}
10974+
1089210975
IR::Instr *
1089310976
Lowerer::LowerStSlot(IR::Instr *instr)
1089410977
{

‎lib/Backend/Lower.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -229,6 +229,7 @@ class Lowerer
229229
IR::Instr * LowerDeleteElemI(IR::Instr *instr, bool strictMode);
230230
IR::Instr * LowerStElemC(IR::Instr *instr);
231231
void LowerLdArrHead(IR::Instr *instr);
232+
IR::Instr* AddSlotArrayCheck(PropertySym *propertySym, IR::Instr* instr);
232233
IR::Instr * LowerStSlot(IR::Instr *instr);
233234
IR::Instr * LowerStSlotChkUndecl(IR::Instr *instr);
234235
void LowerStLoopBodyCount(IR::Instr* instr);

‎test/Bugs/loopcrash.js

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
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+
function test0() {
6+
var i32 = new Int32Array(1);
7+
{
8+
class class0 {
9+
}
10+
class class8 {
11+
}
12+
class class17 {
13+
static func91(argMath135) {
14+
if (new class0() * h) {
15+
}
16+
}
17+
static func94() {
18+
return class8.func78;
19+
}
20+
}
21+
for (var _strvar2 in i32) {
22+
continue;
23+
try {
24+
} catch (ex) {
25+
class8;
26+
}
27+
}
28+
}
29+
}
30+
test0();
31+
test0();
32+
print("Passed");

‎test/Bugs/rlexe.xml

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -557,4 +557,10 @@
557557
<compile-flags>-esdynamicimport -mutehosterrormsg -args summary -endargs</compile-flags>
558558
</default>
559559
</test>
560+
<test>
561+
<default>
562+
<files>loopcrash.js</files>
563+
<compile-flags>-maxinterpretcount:1 -maxsimplejitruncount:1 -MinMemOpCount:0 -werexceptionsupport -bgjit- -loopinterpretcount:1</compile-flags>
564+
</default>
565+
</test>
560566
</regress-exe>

0 commit comments

Comments
 (0)