[vm] Add source positions to stack traces of bool checks
If a condition has a source position in kernel, this source position is
now used in stack traces for bool checks.
Issue: https://github.com/dart-lang/sdk/issues/33584
Change-Id: Idb833abbaeaa32e6c5deb0d383cc4215642d3f2a
Reviewed-on: https://dart-review.googlesource.com/62360
Reviewed-by: Samir Jindel <sjindel@google.com>
Commit-Queue: Alexander Markov <alexmarkov@google.com>
diff --git a/runtime/vm/compiler/frontend/kernel_binary_flowgraph.cc b/runtime/vm/compiler/frontend/kernel_binary_flowgraph.cc
index 23b2002..86b3e06 100644
--- a/runtime/vm/compiler/frontend/kernel_binary_flowgraph.cc
+++ b/runtime/vm/compiler/frontend/kernel_binary_flowgraph.cc
@@ -8315,8 +8315,8 @@
return flow_graph_builder_->BuildImplicitClosureCreation(target);
}
-Fragment StreamingFlowGraphBuilder::CheckBoolean() {
- return flow_graph_builder_->CheckBoolean();
+Fragment StreamingFlowGraphBuilder::CheckBoolean(TokenPosition position) {
+ return flow_graph_builder_->CheckBoolean(position);
}
Fragment StreamingFlowGraphBuilder::CheckAssignableInCheckedMode(
@@ -8375,8 +8375,9 @@
if (*negate) {
SkipBytes(1); // Skip Not tag, thus go directly to the inner expression.
}
- Fragment instructions = BuildExpression(); // read expression.
- instructions += CheckBoolean();
+ TokenPosition position = TokenPosition::kNoSource;
+ Fragment instructions = BuildExpression(&position); // read expression.
+ instructions += CheckBoolean(position);
return instructions;
}
@@ -9621,8 +9622,10 @@
Fragment StreamingFlowGraphBuilder::BuildNot(TokenPosition* position) {
if (position != NULL) *position = TokenPosition::kNoSource;
- Fragment instructions = BuildExpression(); // read expression.
- instructions += CheckBoolean();
+ TokenPosition operand_position = TokenPosition::kNoSource;
+ Fragment instructions =
+ BuildExpression(&operand_position); // read expression.
+ instructions += CheckBoolean(operand_position);
instructions += BooleanNegate();
return instructions;
}
@@ -10291,17 +10294,18 @@
// The call to `_AssertionError._evaluateAssertion()` will take care of both
// and returns a boolean.
instructions += BuildExpression(); // read condition.
+
+ const TokenPosition condition_start_offset =
+ ReadPosition(); // read condition start offset.
+ const TokenPosition condition_end_offset =
+ ReadPosition(); // read condition end offset.
+
instructions += PushArgument();
instructions += EvaluateAssertion();
- instructions += CheckBoolean();
+ instructions += CheckBoolean(condition_start_offset);
instructions += Constant(Bool::True());
instructions += BranchIfEqual(&then, &otherwise, false);
- TokenPosition condition_start_offset =
- ReadPosition(); // read condition start offset.
- TokenPosition condition_end_offset =
- ReadPosition(); // read condition end offset.
-
const Class& klass =
Class::ZoneHandle(Z, Library::LookupCoreClass(Symbols::AssertionError()));
ASSERT(!klass.IsNull());
diff --git a/runtime/vm/compiler/frontend/kernel_binary_flowgraph.h b/runtime/vm/compiler/frontend/kernel_binary_flowgraph.h
index 5b1c7dd..2636f59 100644
--- a/runtime/vm/compiler/frontend/kernel_binary_flowgraph.h
+++ b/runtime/vm/compiler/frontend/kernel_binary_flowgraph.h
@@ -1492,7 +1492,7 @@
JoinEntryInstr* BuildJoinEntry(intptr_t try_index);
Fragment Goto(JoinEntryInstr* destination);
Fragment BuildImplicitClosureCreation(const Function& target);
- Fragment CheckBoolean();
+ Fragment CheckBoolean(TokenPosition position);
Fragment CheckAssignableInCheckedMode(const AbstractType& dst_type,
const String& dst_name);
Fragment CheckArgumentType(LocalVariable* variable, const AbstractType& type);
diff --git a/runtime/vm/compiler/frontend/kernel_to_il.cc b/runtime/vm/compiler/frontend/kernel_to_il.cc
index 1bb584e..7d677b5 100644
--- a/runtime/vm/compiler/frontend/kernel_to_il.cc
+++ b/runtime/vm/compiler/frontend/kernel_to_il.cc
@@ -2422,12 +2422,12 @@
ICData::kStatic);
}
-Fragment FlowGraphBuilder::CheckBoolean() {
+Fragment FlowGraphBuilder::CheckBoolean(TokenPosition position) {
Fragment instructions;
if (I->strong() || I->type_checks() || I->asserts()) {
LocalVariable* top_of_stack = MakeTemporary();
instructions += LoadLocal(top_of_stack);
- instructions += AssertBool();
+ instructions += AssertBool(position);
instructions += Drop();
}
return instructions;
@@ -2451,10 +2451,10 @@
return instructions;
}
-Fragment FlowGraphBuilder::AssertBool() {
+Fragment FlowGraphBuilder::AssertBool(TokenPosition position) {
Value* value = Pop();
- AssertBooleanInstr* instr = new (Z)
- AssertBooleanInstr(TokenPosition::kNoSource, value, GetNextDeoptId());
+ AssertBooleanInstr* instr =
+ new (Z) AssertBooleanInstr(position, value, GetNextDeoptId());
Push(instr);
return Fragment(instr);
}
diff --git a/runtime/vm/compiler/frontend/kernel_to_il.h b/runtime/vm/compiler/frontend/kernel_to_il.h
index 9bb30e5..3ef59b2 100644
--- a/runtime/vm/compiler/frontend/kernel_to_il.h
+++ b/runtime/vm/compiler/frontend/kernel_to_il.h
@@ -824,13 +824,13 @@
Fragment EvaluateAssertion();
Fragment CheckVariableTypeInCheckedMode(const AbstractType& dst_type,
const String& name_symbol);
- Fragment CheckBoolean();
+ Fragment CheckBoolean(TokenPosition position);
Fragment CheckAssignable(
const AbstractType& dst_type,
const String& dst_name,
AssertAssignableInstr::Kind kind = AssertAssignableInstr::kUnknown);
- Fragment AssertBool();
+ Fragment AssertBool(TokenPosition position);
Fragment AssertAssignable(
TokenPosition position,
const AbstractType& dst_type,
diff --git a/tests/language_2/language_2_kernel.status b/tests/language_2/language_2_kernel.status
index c456c91..b54747c 100644
--- a/tests/language_2/language_2_kernel.status
+++ b/tests/language_2/language_2_kernel.status
@@ -832,6 +832,7 @@
type_promotion_functions_test/none: CompileTimeError # Issue 31537
type_promotion_logical_and_test/01: MissingCompileTimeError
type_promotion_more_specific_test/04: CompileTimeError # Issue 31533
+vm/bool_check_stack_traces_test/02: RuntimeError # Issue 33584
vm/closure_memory_retention_test: Skip # KernelVM bug: Hits OOM
vm/regress_29145_test: Skip # Issue 29145
vm/type_cast_vm_test: RuntimeError
@@ -900,6 +901,8 @@
type_alias_equality_test/02: RuntimeError # Issue 31359
type_alias_equality_test/03: RuntimeError # Issue 31359
type_alias_equality_test/04: RuntimeError # Issue 31359
+vm/bool_check_stack_traces_test/01: RuntimeError # Issue 33584
+vm/bool_check_stack_traces_test/02: RuntimeError # Issue 33584
[ $compiler == dartkp && $runtime == dart_precompiled && $checked && $strong ]
assert_initializer_test/31: MissingCompileTimeError # KernelVM bug: Constant evaluation.
diff --git a/tests/language_2/vm/bool_check_stack_traces_test.dart b/tests/language_2/vm/bool_check_stack_traces_test.dart
new file mode 100644
index 0000000..cd80936
--- /dev/null
+++ b/tests/language_2/vm/bool_check_stack_traces_test.dart
@@ -0,0 +1,103 @@
+// Copyright (c) 2018, the Dart project authors. Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+
+import "package:expect/expect.dart";
+
+class Test1 {
+ void bar(bool condition) {
+ if (condition) {
+ Expect.fail('Should not reach here');
+ }
+ Expect.fail('Should throw earlier');
+ }
+}
+
+void test1(bool condition) {
+ Test1 obj = new Test1();
+ obj.bar(condition);
+}
+
+class Test2 {
+ bool condition;
+ Test2(this.condition);
+
+ void bar() {
+ if (!condition) {
+ Expect.fail('Should not reach here');
+ }
+ Expect.fail('Should throw earlier');
+ }
+}
+
+void test2(bool condition) {
+ Test2 obj = new Test2(condition);
+ obj.bar();
+}
+
+class Test3 {
+ bool condition;
+ Test3(this.condition);
+
+ bool bazz() => condition;
+
+ void bar() {
+ while (bazz() || bazz()) {
+ Expect.fail('Should not reach here');
+ }
+ Expect.fail('Should throw earlier');
+ }
+}
+
+void test3(bool condition) {
+ Test3 obj = new Test3(condition);
+ obj.bar();
+}
+
+const bool test4Condition = null;
+
+void test4(bool condition) {
+ if (test4Condition) {
+ Expect.fail('Should not reach here');
+ }
+ Expect.fail('Should throw earlier');
+}
+
+void test5(bool condition) {
+ if (null) {
+ Expect.fail('Should not reach here');
+ }
+ Expect.fail('Should throw earlier');
+}
+
+void testStackTrace(void testCase(bool condition), List<int> lineNumbers) {
+ try {
+ testCase(null);
+ Expect.fail("Using null in a bool condition should throw TypeError");
+ } catch (e, stacktrace) {
+ print('--------- exception ---------');
+ print(e);
+ print('-------- stack trace --------');
+ print(stacktrace);
+ print('-----------------------------');
+
+ Expect.isTrue(e is AssertionError);
+ Expect.equals(
+ 'Failed assertion: boolean expression must not be null', e.toString());
+
+ final String st = stacktrace.toString();
+ for (int lineNum in lineNumbers) {
+ String item = '.dart:$lineNum';
+ Expect.isTrue(st.contains(item), "Stack trace doesn't contain $item");
+ }
+ print('OK');
+ }
+}
+
+main() {
+ testStackTrace(test1, [9, 18]);
+ testStackTrace(test2, [26, 35]);
+ testStackTrace(test3, [45, 54]);
+ testStackTrace(test4, [60]); //# 01: ok
+ testStackTrace(test5, [67]); //# 02: ok
+}
