Don't add the catch frame at a rethrow to the stacktrace.

R=asiva@google.com

Review URL: https://codereview.chromium.org//342473006

git-svn-id: https://dart.googlecode.com/svn/branches/bleeding_edge/dart@37712 260f80e4-7a28-3924-810f-c04153c831b5

runtime/vm/exceptions.cc

@@ -375,7 +375,8 @@ RawStacktrace* Exceptions::CurrentStacktrace() {
 
 static void ThrowExceptionHelper(Isolate* isolate,
                                  const Instance& incoming_exception,
-                                 const Instance& existing_stacktrace) {
+                                 const Instance& existing_stacktrace,
+                                 const bool is_rethrow) {
   bool use_preallocated_stacktrace = false;
   Instance& exception = Instance::Handle(isolate, incoming_exception.raw());
   if (exception.IsNull()) {
@@ -437,9 +438,13 @@ static void ThrowExceptionHelper(Isolate* isolate,
     if (existing_stacktrace.IsNull()) {
       stacktrace = Stacktrace::New(code_array, pc_offset_array);
     } else {
+      ASSERT(is_rethrow);
       stacktrace ^= existing_stacktrace.raw();
       if (pc_offset_array.Length() != 0) {
-        stacktrace.Append(code_array, pc_offset_array);
+        // Skip the first frame during a rethrow. This is the catch clause with
+        // the rethrow statement, which is not part of the original trace a
+        // rethrow is supposed to preserve.
+        stacktrace.Append(code_array, pc_offset_array, 1);
       }
       // Since we are re throwing and appending to the existing stack trace
       // we clear out the catch trace collected in the existing stack trace
@@ -574,7 +579,7 @@ void Exceptions::CreateAndThrowTypeError(intptr_t location,
 void Exceptions::Throw(Isolate* isolate, const Instance& exception) {
   isolate->debugger()->SignalExceptionThrown(exception);
   // Null object is a valid exception object.
-  ThrowExceptionHelper(isolate, exception, Instance::Handle(isolate));
+  ThrowExceptionHelper(isolate, exception, Instance::Handle(isolate), false);
 }
 
 
@@ -582,7 +587,7 @@ void Exceptions::ReThrow(Isolate* isolate,
                          const Instance& exception,
                          const Instance& stacktrace) {
   // Null object is a valid exception object.
-  ThrowExceptionHelper(isolate, exception, stacktrace);
+  ThrowExceptionHelper(isolate, exception, stacktrace, true);
 }
 
 

runtime/vm/object.cc

@@ -18596,13 +18596,20 @@ RawStacktrace* Stacktrace::New(const Array& code_array,
 
 
 void Stacktrace::Append(const Array& code_list,
-                        const Array& pc_offset_list) const {
-  intptr_t old_length = Length();
-  intptr_t new_length = old_length + pc_offset_list.Length();
+                        const Array& pc_offset_list,
+                        const intptr_t start_index) const {
+  ASSERT(start_index <= code_list.Length());
   ASSERT(pc_offset_list.Length() == code_list.Length());
+  intptr_t old_length = Length();
+  intptr_t new_length = old_length + pc_offset_list.Length() - start_index;
+  if (new_length == old_length) {
+    // Nothing to append. Avoid work and an assert that growing arrays always
+    // increases their size.
+    return;
+  }
 
-  // Grow the arrays for function, code and pc_offset triplet to accommodate
-  // the new stack frames.
+  // Grow the arrays for code, pc_offset pairs to accommodate the new stack
+  // frames.
   Array& code_array = Array::Handle(raw_ptr()->code_array_);
   Array& pc_offset_array = Array::Handle(raw_ptr()->pc_offset_array_);
   code_array = Array::Grow(code_array, new_length);
@@ -18610,7 +18617,7 @@ void Stacktrace::Append(const Array& code_list,
   set_code_array(code_array);
   set_pc_offset_array(pc_offset_array);
   // Now append the new function and code list to the existing arrays.
-  intptr_t j = 0;
+  intptr_t j = start_index;
   Object& obj = Object::Handle();
   for (intptr_t i = old_length; i < new_length; i++, j++) {
     obj = code_list.At(j);

runtime/vm/object.h

@@ -6781,7 +6781,9 @@ class Stacktrace : public Instance {
                           const Array& pc_offset_array) const;
   void set_expand_inlined(bool value) const;
 
-  void Append(const Array& code_list, const Array& pc_offset_list) const;
+  void Append(const Array& code_list,
+              const Array& pc_offset_list,
+              const intptr_t start_index) const;
 
   static intptr_t InstanceSize() {
     return RoundedAllocationSize(sizeof(RawStacktrace));

tests/language/language_dart2js.status

@@ -10,6 +10,8 @@ full_stacktrace1_test: Pass, RuntimeError # Issue 12698
 full_stacktrace2_test: Pass, RuntimeError # Issue 12698
 full_stacktrace3_test: Pass, RuntimeError # Issue 12698
 stacktrace_test: Pass, RuntimeError # # Issue 12698
+stacktrace_rethrow_nonerror_test: Pass, RuntimeError # Issue 12698
+stacktrace_rethrow_error_test: Pass, RuntimeError # Issue 12698
 illegal_invocation_test/01: CompileTimeError # Issue 13630
 instantiate_type_variable_test/01: CompileTimeError # Issue 13631
 library_ambiguous_test/00: CompileTimeError # Issue 13632

tests/language/stacktrace_rethrow_error_test.dart

@@ -0,0 +1,155 @@
+// Copyright (c) 2014, 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.
+
+class SubclassOfError extends Error {}
+
+fail() => throw "Fail";
+
+// == Rethrow, skipping through typed handlers. ==
+
+aa1() {
+  try {
+    bb1();
+    fail();
+  } catch(error
+          , stacktrace  /// withtraceparameter: ok
+          ) {
+    expectTrace(['gg1', 'ff1', 'ee1', 'dd1', 'cc1', 'bb1', 'aa1'], error.stackTrace);
+    expectTrace(['gg1', 'ff1', 'ee1', 'dd1', 'cc1', 'bb1', 'aa1'], stacktrace);  /// withtraceparameter: continued
+  }
+}
+
+bb1() => cc1();
+
+cc1() {
+  try {
+    dd1();
+  } on String catch(e) {
+    fail();
+  } on int catch(e) {
+    fail();
+  }
+}
+
+dd1() => ee1();
+
+ee1() {
+  try {
+    ff1();
+  } catch(e) {
+    rethrow;
+  }
+}
+
+ff1() => gg1();
+
+gg1() => throw new SubclassOfError();
+
+// == Rethrow, rethrow again in typed handler. ==
+
+aa2() {
+  try {
+    bb2();
+    fail();
+  } catch(error
+          , stacktrace  /// withtraceparameter: continued
+          ) {
+    expectTrace(['gg2', 'ff2', 'ee2', 'dd2', 'cc2', 'bb2', 'aa2'], error.stackTrace);
+    expectTrace(['gg2', 'ff2', 'ee2', 'dd2', 'cc2', 'bb2', 'aa2'], stacktrace);  /// withtraceparameter: continued
+  }
+}
+
+bb2() => cc2();
+
+cc2() {
+  try {
+    dd2();
+  } on SubclassOfError catch(e) {
+    rethrow;
+  } on int catch(e) {
+    fail();
+  }
+}
+
+dd2() => ee2();
+
+ee2() {
+  try {
+    ff2();
+  } catch(e) {
+    rethrow;
+  }
+}
+
+ff2() => gg2();
+
+gg2() => throw new SubclassOfError();
+
+// == Rethrow, with intervening catch without a trace parameter.
+
+aa3() {
+  try {
+    bb3();
+    fail();
+  } catch(error
+          , stacktrace  /// withtraceparameter: continued
+         ) {
+    expectTrace(['gg3', 'ff3', 'ee3', 'dd3', 'cc3', 'bb3', 'aa3'], error.stackTrace);
+    expectTrace(['cc3', 'bb3', 'aa3'], stacktrace);  /// withtraceparameter: continued
+  }
+}
+
+bb3() => cc3();
+
+cc3() {
+  try {
+    dd3();
+  } catch(e) {
+    throw e;
+  }
+}
+
+dd3() => ee3();
+
+ee3() {
+  try {
+    ff3();
+  } catch(e) {
+    rethrow;
+  }
+}
+
+ff3() => gg3();
+
+gg3() => throw new SubclassOfError();
+
+expectTrace(functionNames, stacktrace) {
+  // Note we don't expect functionNames to cover the whole trace, only the
+  // top portion, because the frames below main are an implementation detail.
+  var traceLines = stacktrace.toString().split('\n');
+  var expectedIndex = 0;
+  var actualIndex = 0;
+  print(stacktrace);
+  print(functionNames);
+  while (expectedIndex < functionNames.length) {
+    var expected = functionNames[expectedIndex];
+    var actual = traceLines[actualIndex];
+    if (actual.indexOf(expected) == -1) {
+      if (expectedIndex == 0) {
+        actualIndex++; // Skip over some helper frames at the top
+      } else {
+        throw "Expected: $expected actual: $actual";
+      }
+    } else {
+      actualIndex++;
+      expectedIndex++;
+    }
+  }
+}
+
+main() {
+  aa1();
+  aa2();
+  aa3();
+}

tests/language/stacktrace_rethrow_nonerror_test.dart

@@ -0,0 +1,148 @@
+// Copyright (c) 2014, 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.
+
+class NotASubclassOfError {}
+
+fail() => throw "Fail";
+
+// == Rethrow, skipping through typed handlers. ==
+
+aa1() {
+  try {
+    bb1();
+    fail();
+  } catch(exception, stacktrace) {
+    expectTrace(['gg1', 'ff1', 'ee1', 'dd1', 'cc1', 'bb1', 'aa1'],
+                stacktrace);
+  }
+}
+
+bb1() => cc1();
+
+cc1() {
+  try {
+    dd1();
+  } on String catch(e) {
+    fail();
+  } on int catch(e) {
+    fail();
+  }
+}
+
+dd1() => ee1();
+
+ee1() {
+  try {
+    ff1();
+  } catch(e) {
+    rethrow;
+  }
+}
+
+ff1() => gg1();
+
+gg1() => throw new NotASubclassOfError();
+
+// == Rethrow, rethrow again in typed handler. ==
+
+aa2() {
+  try {
+    bb2();
+    fail();
+  } catch(exception, stacktrace) {
+    expectTrace(['gg2', 'ff2', 'ee2', 'dd2', 'cc2', 'bb2', 'aa2'],
+                stacktrace);
+  }
+}
+
+bb2() => cc2();
+
+cc2() {
+  try {
+    dd2();
+  } on NotASubclassOfError catch(e) {
+    rethrow;
+  } on int catch(e) {
+    fail();
+  }
+}
+
+dd2() => ee2();
+
+ee2() {
+  try {
+    ff2();
+  } catch(e) {
+    rethrow;
+  }
+}
+
+ff2() => gg2();
+
+gg2() => throw new NotASubclassOfError();
+
+// == Rethrow, with intervening catch without a trace parameter.
+
+aa3() {
+  try {
+    bb3();
+    fail();
+  } catch(exception, stacktrace) {
+    expectTrace(['cc3', 'bb3', 'aa3'], stacktrace);
+  }
+}
+
+bb3() => cc3();
+
+cc3() {
+  try {
+    dd3();
+  } catch(e) {
+    throw e;
+  }
+}
+
+dd3() => ee3();
+
+ee3() {
+  try {
+    ff3();
+  } catch(e) {
+    rethrow;
+  }
+}
+
+ff3() => gg3();
+
+gg3() => throw new NotASubclassOfError();
+
+expectTrace(functionNames, stacktrace) {
+  // Note we don't expect functionNames to cover the whole trace, only the
+  // top portion, because the frames below main are an implementation detail.
+  var traceLines = stacktrace.toString().split('\n');
+  var expectedIndex = 0;
+  var actualIndex = 0;
+  print(stacktrace);
+  print(functionNames);
+  while (expectedIndex < functionNames.length) {
+    var expected = functionNames[expectedIndex];
+    var actual = traceLines[actualIndex];
+    if (actual.indexOf(expected) == -1) {
+      if (expectedIndex == 0) {
+        actualIndex++; // Skip over some helper frames at the top
+      } else {
+        throw "Expected: $expected actual: $actual";
+      }
+    } else {
+      actualIndex++;
+      expectedIndex++;
+    }
+  }
+}
+
+main() {
+  aa1();
+  aa2();
+  aa3();
+}