Skip to content

Commit e2ae16a

Browse files
author
Meghana Gupta
committed
[MERGE #5278 @meg-gupta] Don't reset hasBailedOutBitPtr, instead use ctor/dtor
Merge pull request #5278 from meg-gupta:hasbailoutbit We can loose hasBailedOut information due to the reset in nested try catches and this can result in incorrectly executing catch blocks that happen to be on the callstack try { // Set hasbailoutptr try {} catch{} // reset hasbailoutbitptr // inlinee code // bailout // exception } catch(){ // hasbailedout info lost} Fixes OS#17791189
2 parents 76531f9 + 23f775f commit e2ae16a

File tree

5 files changed

+135
-24
lines changed

5 files changed

+135
-24
lines changed

‎lib/Backend/BailOut.cpp

Lines changed: 11 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1044,10 +1044,12 @@ BailOutRecord::BailOutInlinedCommon(Js::JavascriptCallStackLayout * layout, Bail
10441044
Js::ScriptFunction * innerMostInlinee = nullptr;
10451045
BailOutInlinedHelper(layout, currentBailOutRecord, bailOutOffset, returnAddress, bailOutKind, registerSaves, &bailOutReturnValue, &innerMostInlinee, false, branchValue);
10461046

1047-
bool * hasBailOutBit = layout->functionObject->GetScriptContext()->GetThreadContext()->GetHasBailedOutBitPtr();
1048-
if (hasBailOutBit != nullptr && bailOutRecord->ehBailoutData)
1047+
bool * hasBailedOutBitPtr = layout->functionObject->GetScriptContext()->GetThreadContext()->GetHasBailedOutBitPtr();
1048+
Assert(!bailOutRecord->ehBailoutData || hasBailedOutBitPtr ||
1049+
bailOutRecord->ehBailoutData->ht == Js::HandlerType::HT_Finally /* When we bailout from inlinee in non exception finally, we maynot see hasBailedOutBitPtr*/);
1050+
if (hasBailedOutBitPtr && bailOutRecord->ehBailoutData)
10491051
{
1050-
*hasBailOutBit = true;
1052+
*hasBailedOutBitPtr = true;
10511053
}
10521054
Js::Var result = BailOutCommonNoCodeGen(layout, currentBailOutRecord, currentBailOutRecord->bailOutOffset, returnAddress, bailOutKind, branchValue,
10531055
registerSaves, &bailOutReturnValue);
@@ -1087,11 +1089,14 @@ BailOutRecord::BailOutFromLoopBodyInlinedCommon(Js::JavascriptCallStackLayout *
10871089
BailOutReturnValue bailOutReturnValue;
10881090
Js::ScriptFunction * innerMostInlinee = nullptr;
10891091
BailOutInlinedHelper(layout, currentBailOutRecord, bailOutOffset, returnAddress, bailOutKind, registerSaves, &bailOutReturnValue, &innerMostInlinee, true, branchValue);
1090-
bool * hasBailOutBit = layout->functionObject->GetScriptContext()->GetThreadContext()->GetHasBailedOutBitPtr();
1091-
if (hasBailOutBit != nullptr && bailOutRecord->ehBailoutData)
1092+
bool * hasBailedOutBitPtr = layout->functionObject->GetScriptContext()->GetThreadContext()->GetHasBailedOutBitPtr();
1093+
Assert(!bailOutRecord->ehBailoutData || hasBailedOutBitPtr ||
1094+
bailOutRecord->ehBailoutData->ht == Js::HandlerType::HT_Finally /* When we bailout from inlinee in non exception finally, we maynot see hasBailedOutBitPtr*/);
1095+
if (hasBailedOutBitPtr && bailOutRecord->ehBailoutData)
10921096
{
1093-
*hasBailOutBit = true;
1097+
*hasBailedOutBitPtr = true;
10941098
}
1099+
10951100
uint32 result = BailOutFromLoopBodyHelper(layout, currentBailOutRecord, currentBailOutRecord->bailOutOffset,
10961101
bailOutKind, nullptr, registerSaves, &bailOutReturnValue);
10971102
ScheduleLoopBodyCodeGen(Js::ScriptFunction::FromVar(layout->functionObject), innerMostInlinee, currentBailOutRecord, bailOutKind);

‎lib/Runtime/Language/JavascriptExceptionOperators.cpp

Lines changed: 21 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -72,6 +72,18 @@ namespace Js
7272
m_threadContext->SetTryCatchFrameAddr(m_prevTryCatchFrameAddr);
7373
}
7474

75+
JavascriptExceptionOperators::HasBailedOutPtrStack::HasBailedOutPtrStack(ScriptContext* scriptContext, bool *hasBailedOutPtr)
76+
{
77+
m_threadContext = scriptContext->GetThreadContext();
78+
m_prevHasBailedOutPtr = m_threadContext->GetHasBailedOutBitPtr();
79+
scriptContext->GetThreadContext()->SetHasBailedOutBitPtr(hasBailedOutPtr);
80+
}
81+
82+
JavascriptExceptionOperators::HasBailedOutPtrStack::~HasBailedOutPtrStack()
83+
{
84+
m_threadContext->SetHasBailedOutBitPtr(m_prevHasBailedOutPtr);
85+
}
86+
7587
JavascriptExceptionOperators::PendingFinallyExceptionStack::PendingFinallyExceptionStack(ScriptContext* scriptContext, Js::JavascriptExceptionObject *exceptionObj)
7688
{
7789
m_threadContext = scriptContext->GetThreadContext();
@@ -105,7 +117,8 @@ namespace Js
105117
void *continuation = nullptr;
106118
JavascriptExceptionObject *exception = nullptr;
107119
void *tryCatchFrameAddr = nullptr;
108-
scriptContext->GetThreadContext()->SetHasBailedOutBitPtr((bool*)((char*)frame + hasBailedOutOffset));
120+
121+
Js::JavascriptExceptionOperators::HasBailedOutPtrStack hasBailedOutPtrStack(scriptContext, (bool*)((char*)frame + hasBailedOutOffset));
109122

110123
PROBE_STACK(scriptContext, Constants::MinStackJitEHBailout + spillSize + argsSize);
111124
{
@@ -148,15 +161,13 @@ namespace Js
148161
// If we have bailed out, this exception is coming from the interpreter. It should not have been caught;
149162
// it so happens that this catch was on the stack and caught the exception.
150163
// Re-throw!
151-
scriptContext->GetThreadContext()->SetHasBailedOutBitPtr(nullptr);
152164
JavascriptExceptionOperators::DoThrow(exception, scriptContext);
153165
}
154166

155167
Var exceptionObject = exception->GetThrownObject(scriptContext);
156168
AssertMsg(exceptionObject, "Caught object is null.");
157169
continuation = amd64_CallWithFakeFrame(catchAddr, frame, spillSize, argsSize, exceptionObject);
158170
}
159-
scriptContext->GetThreadContext()->SetHasBailedOutBitPtr(nullptr);
160171
return continuation;
161172
}
162173

@@ -170,8 +181,8 @@ namespace Js
170181
{
171182
void *tryContinuation = nullptr;
172183
JavascriptExceptionObject *exception = nullptr;
173-
scriptContext->GetThreadContext()->SetHasBailedOutBitPtr((bool*)((char*)frame + hasBailedOutOffset));
174184

185+
Js::JavascriptExceptionOperators::HasBailedOutPtrStack hasBailedOutPtrStack(scriptContext, (bool*)((char*)frame + hasBailedOutOffset));
175186
PROBE_STACK(scriptContext, Constants::MinStackJitEHBailout + spillSize + argsSize);
176187

177188
try
@@ -213,7 +224,6 @@ namespace Js
213224
// If we have bailed out, this exception is coming from the interpreter. It should not have been caught;
214225
// it so happens that this catch was on the stack and caught the exception.
215226
// Re-throw!
216-
scriptContext->GetThreadContext()->SetHasBailedOutBitPtr(nullptr);
217227
JavascriptExceptionOperators::DoThrow(exception, scriptContext);
218228
}
219229

@@ -224,7 +234,6 @@ namespace Js
224234
}
225235
}
226236

227-
scriptContext->GetThreadContext()->SetHasBailedOutBitPtr(nullptr);
228237
return tryContinuation;
229238
}
230239

@@ -278,7 +287,7 @@ namespace Js
278287
void *continuation = nullptr;
279288
JavascriptExceptionObject *exception = nullptr;
280289
void * tryCatchFrameAddr = nullptr;
281-
scriptContext->GetThreadContext()->SetHasBailedOutBitPtr((bool*)((char*)localsPtr + hasBailedOutOffset));
290+
Js::JavascriptExceptionOperators::HasBailedOutPtrStack hasBailedOutPtrStack(scriptContext, (bool*)((char*)framePtr + hasBailedOutOffset));
282291

283292
PROBE_STACK(scriptContext, Constants::MinStackJitEHBailout + argsSize);
284293
{
@@ -324,7 +333,6 @@ namespace Js
324333
// If we have bailed out, this exception is coming from the interpreter. It should not have been caught;
325334
// it so happens that this catch was on the stack and caught the exception.
326335
// Re-throw!
327-
scriptContext->GetThreadContext()->SetHasBailedOutBitPtr(nullptr);
328336
JavascriptExceptionOperators::DoThrow(exception, scriptContext);
329337
}
330338

@@ -337,7 +345,6 @@ namespace Js
337345
#endif
338346
}
339347

340-
scriptContext->GetThreadContext()->SetHasBailedOutBitPtr(nullptr);
341348
return continuation;
342349
}
343350

@@ -352,7 +359,7 @@ namespace Js
352359
{
353360
void *tryContinuation = nullptr;
354361
JavascriptExceptionObject *exception = nullptr;
355-
scriptContext->GetThreadContext()->SetHasBailedOutBitPtr((bool*)((char*)localsPtr + hasBailedOutOffset));
362+
Js::JavascriptExceptionOperators::HasBailedOutPtrStack hasBailedOutPtrStack(scriptContext, (bool*)((char*)framePtr + hasBailedOutOffset));
356363

357364
PROBE_STACK(scriptContext, Constants::MinStackJitEHBailout + argsSize);
358365
try
@@ -394,7 +401,6 @@ namespace Js
394401
// If we have bailed out, this exception is coming from the interpreter. It should not have been caught;
395402
// it so happens that this catch was on the stack and caught the exception.
396403
// Re-throw!
397-
scriptContext->GetThreadContext()->SetHasBailedOutBitPtr(nullptr);
398404
JavascriptExceptionOperators::DoThrow(exception, scriptContext);
399405
}
400406

@@ -409,7 +415,6 @@ namespace Js
409415
}
410416
}
411417

412-
scriptContext->GetThreadContext()->SetHasBailedOutBitPtr(nullptr);
413418
return tryContinuation;
414419
}
415420

@@ -473,7 +478,8 @@ namespace Js
473478
void* continuationAddr = NULL;
474479
Js::JavascriptExceptionObject* pExceptionObject = NULL;
475480
void *tryCatchFrameAddr = nullptr;
476-
scriptContext->GetThreadContext()->SetHasBailedOutBitPtr((bool*)((char*)framePtr + hasBailedOutOffset));
481+
482+
Js::JavascriptExceptionOperators::HasBailedOutPtrStack hasBailedOutPtrStack(scriptContext, (bool*)((char*)framePtr + hasBailedOutOffset));
477483

478484
PROBE_STACK(scriptContext, Constants::MinStackJitEHBailout);
479485
{
@@ -571,7 +577,6 @@ namespace Js
571577
// If we have bailed out, this exception is coming from the interpreter. It should not have been caught;
572578
// it so happens that this catch was on the stack and caught the exception.
573579
// Re-throw!
574-
scriptContext->GetThreadContext()->SetHasBailedOutBitPtr(nullptr);
575580
JavascriptExceptionOperators::DoThrow(pExceptionObject, scriptContext);
576581
}
577582

@@ -628,15 +633,15 @@ namespace Js
628633
#endif
629634
}
630635

631-
scriptContext->GetThreadContext()->SetHasBailedOutBitPtr(nullptr);
632636
return continuationAddr;
633637
}
634638

635639
void* JavascriptExceptionOperators::OP_TryFinally(void* tryAddr, void* handlerAddr, void* framePtr, int hasBailedOutOffset, ScriptContext *scriptContext)
636640
{
637641
Js::JavascriptExceptionObject* pExceptionObject = NULL;
638642
void* continuationAddr = NULL;
639-
scriptContext->GetThreadContext()->SetHasBailedOutBitPtr((bool*)((char*)framePtr + hasBailedOutOffset));
643+
644+
Js::JavascriptExceptionOperators::HasBailedOutPtrStack hasBailedOutPtrStack(scriptContext, (bool*)((char*)framePtr + hasBailedOutOffset));
640645
PROBE_STACK(scriptContext, Constants::MinStackJitEHBailout);
641646

642647
try
@@ -731,7 +736,6 @@ namespace Js
731736
// If we have bailed out, this exception is coming from the interpreter. It should not have been caught;
732737
// it so happens that this catch was on the stack and caught the exception.
733738
// Re-throw!
734-
scriptContext->GetThreadContext()->SetHasBailedOutBitPtr(nullptr);
735739
JavascriptExceptionOperators::DoThrow(pExceptionObject, scriptContext);
736740
}
737741

@@ -799,7 +803,6 @@ namespace Js
799803
}
800804
}
801805

802-
scriptContext->GetThreadContext()->SetHasBailedOutBitPtr(nullptr);
803806
return continuationAddr;
804807
}
805808

‎lib/Runtime/Language/JavascriptExceptionOperators.h

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,17 @@ namespace Js
5454
~TryCatchFrameAddrStack();
5555
};
5656

57+
class HasBailedOutPtrStack
58+
{
59+
private:
60+
bool * m_prevHasBailedOutPtr;
61+
ThreadContext* m_threadContext;
62+
63+
public:
64+
HasBailedOutPtrStack(ScriptContext* scriptContext, bool *hasBailedOutPtr);
65+
~HasBailedOutPtrStack();
66+
};
67+
5768
class PendingFinallyExceptionStack
5869
{
5970
private:

‎test/EH/hasBailedOutBug2.js

Lines changed: 86 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,86 @@
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+
function func80() {
8+
}
9+
var uniqobj22 = new func80();
10+
try {
11+
(function () {
12+
try {
13+
try {
14+
} catch (ex) {
15+
}
16+
function func104() {
17+
uniqobj22 >>>= 1;
18+
}
19+
func104();
20+
} catch (ex) {
21+
WScript.Echo("FAILED");
22+
} finally {
23+
protoObj0();
24+
}
25+
}());
26+
} catch (ex) {
27+
}
28+
}
29+
test0();
30+
test0();
31+
32+
function test1() {
33+
var obj1 = {};
34+
var func2 = function () {
35+
try {
36+
} catch (ex) {
37+
}
38+
};
39+
obj1.method1 = func2;
40+
var IntArr0 = new Array();
41+
function v0() {
42+
function v2() {
43+
try {
44+
obj1.method1();
45+
function func7() {
46+
IntArr0[1];
47+
}
48+
func7();
49+
} catch (ex) {
50+
WScript.Echo("FAILED");
51+
}
52+
var v3 = runtime_error;
53+
}
54+
try {
55+
v2();
56+
} catch (ex) {
57+
}
58+
}
59+
v0();
60+
}
61+
test1();
62+
test1();
63+
test1();
64+
65+
function test2() {
66+
function makeArrayLength(x) {
67+
if (x < 1) {
68+
}
69+
}
70+
var func2 = function () {
71+
try {
72+
} finally {
73+
makeArrayLength(393266900 * 1957286472);
74+
}
75+
};
76+
func2();
77+
try {
78+
func2();
79+
} finally {
80+
}
81+
}
82+
test2();
83+
test2();
84+
test2();
85+
86+
WScript.Echo("Passed");

‎test/EH/rlexe.xml

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -183,6 +183,12 @@
183183
<baseline>hasBailedOutBug.baseline</baseline>
184184
</default>
185185
</test>
186+
<test>
187+
<default>
188+
<files>hasBailedOutBug2.js</files>
189+
<tags>exclude_dynapogo</tags>
190+
</default>
191+
</test>
186192
<test>
187193
<default>
188194
<files>StackOverflow.js</files>

0 commit comments

Comments
 (0)