Skip to content

Commit

Permalink
Adjust the column number where a StackOverflow error is reported.
Browse files Browse the repository at this point in the history 
Before this change, the column number of the StackOverflow error was
reported at the opening '(' of the function parameter list:

  ReturnType function(...) { ... }
                     ^

This change moves it to the opening '{':

  ReturnType function(...) { ... }
                           ^

In case of single statement functions with '=>', the location of the '='
is reported as the column number of the StackOverflow error.

R=asiva@google.com

Review-Url: https://codereview.chromium.org/3002833003 .
  • Loading branch information
Beeravolu Siva Chandra Reddy committed Aug 21, 2017
1 parent 7e798f4 commit eeaeb6f
Show file tree
Hide file tree
Showing 4 changed files with 58 additions and 25 deletions.
4 changes: 3 additions & 1 deletion runtime/tests/vm/vm.status
View file
Expand Up @@ -292,7 +292,9 @@ cc/DartAPI_NegativeNativeFieldInIsolateMessage: Crash
cc/DartAPI_New: Crash
cc/DartAPI_ParsePatchLibrary: Crash
cc/DartAPI_PropagateError: Fail
cc/DartAPI_StackOverflowStackTraceInfo: Skip
cc/DartAPI_StackOverflowStackTraceInfoBraceFunction1: Fail
cc/DartAPI_StackOverflowStackTraceInfoBraceFunction2: Fail
cc/DartAPI_StackOverflowStackTraceInfoArrowFunction: Fail
cc/DartAPI_TestNativeFieldsAccess: Crash
cc/DartAPI_TypeGetParameterizedTypes: Crash
cc/DebuggerAPI_BreakpointStubPatching: Fail
Expand Down
51 changes: 39 additions & 12 deletions runtime/vm/dart_api_impl_test.cc
View file
Expand Up @@ -212,15 +212,13 @@ TEST_CASE(DartAPI_DeepStackTraceInfo) {
EXPECT(Dart_IsError(result));
}

TEST_CASE(DartAPI_StackOverflowStackTraceInfo) {
const char* kScriptChars =
"class C {\n"
" static foo() => foo();\n"
"}\n"
"testMain() => C.foo();\n";

Dart_Handle lib = TestCase::LoadTestScript(kScriptChars, NULL);
Dart_Handle error = Dart_Invoke(lib, NewString("testMain"), 0, NULL);
void VerifyStackOverflowStackTraceInfo(const char* script,
const char* top_frame_func_name,
const char* entry_func_name,
int expected_line_number,
int expected_column_number) {
Dart_Handle lib = TestCase::LoadTestScript(script, NULL);
Dart_Handle error = Dart_Invoke(lib, NewString(entry_func_name), 0, NULL);

EXPECT(Dart_IsError(error));

Expand All @@ -247,11 +245,11 @@ TEST_CASE(DartAPI_StackOverflowStackTraceInfo) {
&line_number, &column_number);
EXPECT_VALID(result);
Dart_StringToCString(function_name, &cstr);
EXPECT_STREQ("C.foo", cstr);
EXPECT_STREQ(top_frame_func_name, cstr);
Dart_StringToCString(script_url, &cstr);
EXPECT_STREQ("test-lib", cstr);
EXPECT_EQ(2, line_number);
EXPECT_EQ(13, column_number);
EXPECT_EQ(expected_line_number, line_number);
EXPECT_EQ(expected_column_number, column_number);

// Out-of-bounds frames.
result = Dart_GetActivationFrame(stacktrace, frame_count, &frame);
Expand All @@ -260,6 +258,35 @@ TEST_CASE(DartAPI_StackOverflowStackTraceInfo) {
EXPECT(Dart_IsError(result));
}

TEST_CASE(DartAPI_StackOverflowStackTraceInfoBraceFunction1) {
VerifyStackOverflowStackTraceInfo(
"class C {\n"
" static foo(int i) { foo(i); }\n"
"}\n"
"testMain() => C.foo(10);\n",
"C.foo", "testMain", 2, 21);
}

TEST_CASE(DartAPI_StackOverflowStackTraceInfoBraceFunction2) {
VerifyStackOverflowStackTraceInfo(
"class C {\n"
" static foo(int i, int j) {\n"
" foo(i, j);\n"
" }\n"
"}\n"
"testMain() => C.foo(10, 11);\n",
"C.foo", "testMain", 2, 28);
}

TEST_CASE(DartAPI_StackOverflowStackTraceInfoArrowFunction) {
VerifyStackOverflowStackTraceInfo(
"class C {\n"
" static foo(int i) => foo(i);\n"
"}\n"
"testMain() => C.foo(10);\n",
"C.foo", "testMain", 2, 21);
}

TEST_CASE(DartAPI_OutOfMemoryStackTraceInfo) {
const char* kScriptChars =
"var number_of_ints = 134000000;\n"
Expand Down
6 changes: 5 additions & 1 deletion runtime/vm/flow_graph_builder.cc
View file
Expand Up @@ -3874,8 +3874,12 @@ void EffectGraphVisitor::VisitSequenceNode(SequenceNode* node) {
// if we inline or not.
if (!function.IsImplicitGetterFunction() &&
!function.IsImplicitSetterFunction()) {
// We want the stack overlow error to be reported at the opening '{' or
// at the '=>' location. So, we get the sequence node corresponding to the
// body inside |node| and use its token position.
ASSERT(node->length() > 0);
CheckStackOverflowInstr* check = new (Z) CheckStackOverflowInstr(
node->token_pos(), 0, owner()->GetNextDeoptId());
node->NodeAt(0)->token_pos(), 0, owner()->GetNextDeoptId());
// If we are inlining don't actually attach the stack check. We must still
// create the stack check in order to allocate a deopt id.
if (!owner()->IsInlining()) {
Expand Down
22 changes: 11 additions & 11 deletions runtime/vm/flow_graph_builder_test.cc
View file
Expand Up @@ -292,7 +292,7 @@ TEST_CASE(SourcePosition_InstanceCalls) {
SourcePositionTest spt(thread, kScript);
spt.BuildGraphFor("main");
spt.FuzzyInstructionMatchAt("DebugStepCheck", 3, 5);
spt.FuzzyInstructionMatchAt("CheckStackOverflow", 3, 5);
spt.FuzzyInstructionMatchAt("CheckStackOverflow", 3, 8);
spt.InstanceCallAt(4, 13, Token::kADD);
spt.FuzzyInstructionMatchAt("DebugStepCheck", 5, 3);
spt.FuzzyInstructionMatchAt("Return", 5, 3);
Expand All @@ -314,7 +314,7 @@ TEST_CASE(SourcePosition_If) {
SourcePositionTest spt(thread, kScript);
spt.BuildGraphFor("main");
spt.FuzzyInstructionMatchAt("DebugStepCheck", 3, 5);
spt.FuzzyInstructionMatchAt("CheckStackOverflow", 3, 5);
spt.FuzzyInstructionMatchAt("CheckStackOverflow", 3, 8);
spt.FuzzyInstructionMatchAt("LoadStaticField", 4, 7);
spt.InstanceCallAt(4, 9, Token::kEQ);
spt.FuzzyInstructionMatchAt("Branch if StrictCompare", 4, 9);
Expand Down Expand Up @@ -342,7 +342,7 @@ TEST_CASE(SourcePosition_ForLoop) {
SourcePositionTest spt(thread, kScript);
spt.BuildGraphFor("main");
spt.FuzzyInstructionMatchAt("DebugStepCheck", 3, 5);
spt.FuzzyInstructionMatchAt("CheckStackOverflow", 3, 5);
spt.FuzzyInstructionMatchAt("CheckStackOverflow", 3, 8);
spt.FuzzyInstructionMatchAt("StoreLocal", 4, 14);
spt.FuzzyInstructionMatchAt("LoadLocal", 4, 19);
spt.InstanceCallAt(4, 21, Token::kLT);
Expand Down Expand Up @@ -375,7 +375,7 @@ TEST_CASE(SourcePosition_While) {
SourcePositionTest spt(thread, kScript);
spt.BuildGraphFor("main");
spt.FuzzyInstructionMatchAt("DebugStepCheck", 3, 5);
spt.FuzzyInstructionMatchAt("CheckStackOverflow", 3, 5);
spt.FuzzyInstructionMatchAt("CheckStackOverflow", 3, 8);

spt.FuzzyInstructionMatchAt("CheckStackOverflow", 4, 3);
spt.FuzzyInstructionMatchAt("Constant", 4, 10);
Expand Down Expand Up @@ -423,7 +423,7 @@ TEST_CASE(SourcePosition_WhileContinueBreak) {
SourcePositionTest spt(thread, kScript);
spt.BuildGraphFor("main");
spt.FuzzyInstructionMatchAt("DebugStepCheck", 3, 5);
spt.FuzzyInstructionMatchAt("CheckStackOverflow", 3, 5);
spt.FuzzyInstructionMatchAt("CheckStackOverflow", 3, 8);

spt.FuzzyInstructionMatchAt("CheckStackOverflow", 4, 3);
spt.FuzzyInstructionMatchAt("Constant(#Field", 4, 10);
Expand Down Expand Up @@ -458,7 +458,7 @@ TEST_CASE(SourcePosition_LoadIndexed) {
spt.BuildGraphFor("main");

spt.FuzzyInstructionMatchAt("DebugStepCheck", 3, 5);
spt.FuzzyInstructionMatchAt("CheckStackOverflow", 3, 5);
spt.FuzzyInstructionMatchAt("CheckStackOverflow", 3, 8);
spt.StaticCallAt("get:z", 4, 3);
spt.FuzzyInstructionMatchAt("Constant(#0)", 4, 5);
spt.InstanceCallAt(4, 4, Token::kINDEX);
Expand Down Expand Up @@ -496,7 +496,7 @@ TEST_CASE(SourcePosition_StoreIndexed) {
spt.BuildGraphFor("main");

spt.FuzzyInstructionMatchAt("DebugStepCheck", 3, 5);
spt.FuzzyInstructionMatchAt("CheckStackOverflow", 3, 5);
spt.FuzzyInstructionMatchAt("CheckStackOverflow", 3, 8);
spt.StaticCallAt("get:z", 4, 3);
spt.FuzzyInstructionMatchAt("Constant(#0)", 4, 5);
spt.InstanceCallAt(4, 4, Token::kINDEX);
Expand Down Expand Up @@ -543,7 +543,7 @@ TEST_CASE(SourcePosition_BitwiseOperations) {
spt.BuildGraphFor("main");

spt.FuzzyInstructionMatchAt("DebugStepCheck", 3, 5);
spt.FuzzyInstructionMatchAt("CheckStackOverflow", 3, 5);
spt.FuzzyInstructionMatchAt("CheckStackOverflow", 3, 8);

spt.FuzzyInstructionMatchAt("DebugStepCheck", 4, 7);
spt.FuzzyInstructionMatchAt("Constant(#null", 4, 7);
Expand Down Expand Up @@ -590,7 +590,7 @@ TEST_CASE(SourcePosition_IfElse) {
SourcePositionTest spt(thread, kScript);
spt.BuildGraphFor("main");
spt.FuzzyInstructionMatchAt("DebugStepCheck", 3, 5);
spt.FuzzyInstructionMatchAt("CheckStackOverflow", 3, 5);
spt.FuzzyInstructionMatchAt("CheckStackOverflow", 3, 8);
spt.FuzzyInstructionMatchAt("LoadStaticField", 4, 7);
spt.InstanceCallAt(4, 9, Token::kEQ);
spt.FuzzyInstructionMatchAt("Branch if StrictCompare", 4, 9);
Expand Down Expand Up @@ -620,7 +620,7 @@ TEST_CASE(SourcePosition_Switch) {
spt.BuildGraphFor("main");

spt.FuzzyInstructionMatchAt("DebugStepCheck", 3, 5);
spt.FuzzyInstructionMatchAt("CheckStackOverflow", 3, 5);
spt.FuzzyInstructionMatchAt("CheckStackOverflow", 3, 8);
spt.FuzzyInstructionMatchAt("Constant(#Field", 4, 11);
spt.FuzzyInstructionMatchAt("LoadStaticField", 4, 11);
spt.FuzzyInstructionMatchAt("StoreLocal(:switch_expr", 4, 11);
Expand Down Expand Up @@ -667,7 +667,7 @@ TEST_CASE(SourcePosition_TryCatchFinally) {
spt.BuildGraphFor("main");

spt.FuzzyInstructionMatchAt("DebugStepCheck", 3, 5);
spt.FuzzyInstructionMatchAt("CheckStackOverflow", 3, 5);
spt.FuzzyInstructionMatchAt("CheckStackOverflow", 3, 8);

spt.FuzzyInstructionMatchAt("LoadLocal(:current_context", 4, 3); // 't'
spt.FuzzyInstructionMatchAt("StoreLocal(:saved_try_context", 4, 3);
Expand Down

0 comments on commit eeaeb6f

Please sign in to comment.