Mark exception handlers if they have a stacktrace specified. Do not build a stacktrace if the handl…

runtime/vm/exceptions.cc

Index: runtime/vm/exceptions.cc
===================================================================
--- runtime/vm/exceptions.cc	(revision 26792)
+++ runtime/vm/exceptions.cc	(working copy)
@@ -188,19 +188,14 @@
 }
 
 
-// Iterate through the stack frames and try to find a frame with an
-// exception handler. Once found, set the pc, sp and fp so that execution
-// can continue in that frame.
-static bool FindExceptionHandler(uword* handler_pc,
-                                 uword* handler_sp,
-                                 uword* handler_fp,
-                                 StacktraceBuilder* builder) {
+static void BuildStackTrace(StacktraceBuilder* builder) {
   StackFrameIterator frames(StackFrameIterator::kDontValidateFrames);
   StackFrame* frame = frames.NextFrame();
   ASSERT(frame != NULL);  // We expect to find a dart invocation frame.
   Function& func = Function::Handle();
   Code& code = Code::Handle();
   Smi& offset = Smi::Handle();
+  uword handler_pc = NULL;

[hausner, 2013/08/28 16:08:20]

Should a uword variable not be initialized with 0? NULL is a pointer value.

[siva, 2013/08/28 17:00:09]

Maybe intialize it to kUwordMax.

[srdjan, 2013/08/28 20:38:29]

Initialized to kUWordMax and moved close to frame->FindExceptionHandler call.

[srdjan, 2013/08/28 20:38:29]

Using kUWordMax

   bool dart_handler_found = false;
   bool handler_pc_set = false;

[siva, 2013/08/28 17:00:09]

The variable dart_handler_found is not needed anymore.

I think the variable handler_pc_set is also not needed anymore, you could just
go with handler_pc == kUwordMax;

[srdjan, 2013/08/28 20:38:29]

Offline discussion: it is needed to note the catch frame.
Moved close to its only usage.

   while (frame != NULL) {
@@ -229,13 +224,13 @@
             builder->AddFrame(func, code, offset, dart_handler_found);
           }
         }
-        if (!handler_pc_set && frame->FindExceptionHandler(handler_pc)) {
+        bool needs_stacktrace = false;
+        if (!handler_pc_set &&
+            frame->FindExceptionHandler(&handler_pc, &needs_stacktrace)) {
           handler_pc_set = true;
-          *handler_sp = frame->sp();
-          *handler_fp = frame->fp();
           dart_handler_found = true;
           if (!builder->FullStacktrace()) {
-            return dart_handler_found;
+            return;
           }
         }
       }
@@ -245,19 +240,56 @@
     ASSERT(frame->IsEntryFrame());
     if (!handler_pc_set) {
       handler_pc_set = true;
-      *handler_pc = frame->pc();
-      *handler_sp = frame->sp();
-      *handler_fp = frame->fp();
+      handler_pc = frame->pc();
       if (!builder->FullStacktrace()) {
-        return dart_handler_found;
+        return;
       }
     }
     frame = frames.NextFrame();
   }
-  return dart_handler_found;
 }
 
 
+// Iterate through the stack frames and try to find a frame with an
+// exception handler. Once found, set the pc, sp and fp so that execution
+// can continue in that frame.
+static bool FindExceptionHandler(uword* handler_pc,
+                                 uword* handler_sp,
+                                 uword* handler_fp,
+                                 bool* handler_needs_stacktrace) {
+  StackFrameIterator frames(StackFrameIterator::kDontValidateFrames);
+  StackFrame* frame = frames.NextFrame();
+  ASSERT(frame != NULL);  // We expect to find a dart invocation frame.
+  bool handler_pc_set = false;
+  *handler_needs_stacktrace = false;
+  while (frame != NULL) {
+    while (!frame->IsEntryFrame()) {
+      if (frame->IsDartFrame()) {
+        if (!handler_pc_set &&
+            frame->FindExceptionHandler(handler_pc, handler_needs_stacktrace)) {
+          handler_pc_set = true;
+          *handler_sp = frame->sp();
+          *handler_fp = frame->fp();
+          return true;

[siva, 2013/08/28 17:00:09]

Don't you have to keep going till you hit the entry frame to see
if a stacktrace is needed?
Consider this case:

try {
  ....
  ....
  try {
     ...
     throw TypeAException_obj;
  } on TypeBException catch (e) {
     ...
  }
  ....
} on TypeAException catch (e, stacktrace) {
  ....
}

[srdjan, 2013/08/28 20:38:29]

Good 'catch'!

+        }
+      }
+      frame = frames.NextFrame();
+      ASSERT(frame != NULL);
+    }  // while !frame->IsEntryFrame.
+    ASSERT(frame->IsEntryFrame());
+    if (!handler_pc_set) {
+      handler_pc_set = true;
+      *handler_pc = frame->pc();
+      *handler_sp = frame->sp();
+      *handler_fp = frame->fp();
+      *handler_needs_stacktrace = true;
+    }
+    frame = frames.NextFrame();
+  }  // while frame != NULL.
+  return false;

[siva, 2013/08/28 17:00:09]

Since you are not building a full stack trace anymore here it is not
necessary to have the outer while loop. you could just do:

while (!frame->IsEntryFrame()) {
  ....
  ....
}
ASSERT(frame->IsEntryFrame());
...
*handler_fp - frame->fp();
return false;

(The variable handler_pc_set is also not needed.)

[srdjan, 2013/08/28 20:38:29]

Done.

+}
+
+
 static void FindErrorHandler(uword* handler_pc,
                              uword* handler_sp,
                              uword* handler_fp) {
@@ -364,56 +396,59 @@
   uword handler_fp = 0;
   Stacktrace& stacktrace = Stacktrace::Handle(isolate);
   bool handler_exists = false;
+  bool handler_needs_stacktrace = false;
   if (use_preallocated_stacktrace) {
     stacktrace ^= isolate->object_store()->preallocated_stack_trace();
     PreallocatedStacktraceBuilder frame_builder(stacktrace);
     handler_exists = FindExceptionHandler(&handler_pc,
                                           &handler_sp,
                                           &handler_fp,
-                                          &frame_builder);
+                                          &handler_needs_stacktrace);
+    if (handler_needs_stacktrace) {
+      BuildStackTrace(&frame_builder);
+    }
   } else {
+    // Get stacktrace field of class Error.
     const Field& stacktrace_field =
-        Field::Handle(LookupStacktraceField(exception));
+        Field::Handle(isolate, LookupStacktraceField(exception));
     bool full_stacktrace = !stacktrace_field.IsNull();
-    RegularStacktraceBuilder frame_builder(full_stacktrace);
     handler_exists = FindExceptionHandler(&handler_pc,
                                           &handler_sp,
                                           &handler_fp,
-                                          &frame_builder);
-    // Create arrays for function, code and pc_offset triplet of each frame.
-    const Array& func_array =
-        Array::Handle(isolate, Array::MakeArray(frame_builder.func_list()));
-    const Array& code_array =
-        Array::Handle(isolate, Array::MakeArray(frame_builder.code_list()));
-    const Array& pc_offset_array =
-        Array::Handle(isolate,
-                      Array::MakeArray(frame_builder.pc_offset_list()));
-    if (!stacktrace_field.IsNull()) {
-      // This is an error object and we need to capture the full stack trace
-      // here implicitly, so we set up the stack trace. The stack trace field
-      // is set only once, it is not overriden.
-      const Array& catch_func_array =
-          Array::Handle(isolate,
-                        Array::MakeArray(frame_builder.catch_func_list()));
-      const Array& catch_code_array =
-          Array::Handle(isolate,
-                        Array::MakeArray(frame_builder.catch_code_list()));
-      const Array& catch_pc_offset_array =
-          Array::Handle(isolate,
-                        Array::MakeArray(frame_builder.catch_pc_offset_list()));
-      stacktrace = Stacktrace::New(func_array, code_array, pc_offset_array);
-      stacktrace.SetCatchStacktrace(catch_func_array,
-                                    catch_code_array,
-                                    catch_pc_offset_array);
-      if (exception.GetField(stacktrace_field) == Object::null()) {
-        exception.SetField(stacktrace_field, stacktrace);
-      }
+                                          &handler_needs_stacktrace);
+    Array& func_array = Array::Handle(isolate, Object::empty_array().raw());
+    Array& code_array = Array::Handle(isolate, Object::empty_array().raw());
+    Array& pc_offset_array =
+        Array::Handle(isolate, Object::empty_array().raw());
+    if (handler_needs_stacktrace || full_stacktrace) {
+      RegularStacktraceBuilder frame_builder(full_stacktrace);
+      BuildStackTrace(&frame_builder);
+
+      // Create arrays for function, code and pc_offset triplet of each frame.
+      func_array = Array::MakeArray(frame_builder.func_list());
+      code_array = Array::MakeArray(frame_builder.code_list());
+      pc_offset_array = Array::MakeArray(frame_builder.pc_offset_list());
+      if (!stacktrace_field.IsNull()) {
+        // This is an error object and we need to capture the full stack trace
+        // here implicitly, so we set up the stack trace. The stack trace field
+        // is set only once, it is not overriden.
+        const Array& catch_func_array = Array::Handle(isolate,
+            Array::MakeArray(frame_builder.catch_func_list()));
+        const Array& catch_code_array = Array::Handle(isolate,
+            Array::MakeArray(frame_builder.catch_code_list()));
+        const Array& catch_pc_offset_array = Array::Handle(isolate,
+            Array::MakeArray(frame_builder.catch_pc_offset_list()));
+        stacktrace = Stacktrace::New(func_array, code_array, pc_offset_array);
+        stacktrace.SetCatchStacktrace(catch_func_array,
+                                      catch_code_array,
+                                      catch_pc_offset_array);
+        if (exception.GetField(stacktrace_field) == Object::null()) {
+          exception.SetField(stacktrace_field, stacktrace);
+        }
+      }  // if stacktrace needed.
     }
-    // TODO(5411263): At some point we can optimize by figuring out if a
-    // stack trace is needed based on whether the catch code specifies a
-    // stack trace object or there is a rethrow in the catch clause.
     if (existing_stacktrace.IsNull()) {
-        stacktrace = Stacktrace::New(func_array, code_array, pc_offset_array);
+      stacktrace = Stacktrace::New(func_array, code_array, pc_offset_array);
     } else {
       stacktrace ^= existing_stacktrace.raw();
       if (pc_offset_array.Length() != 0) {