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

runtime/vm/exceptions.cc

 // Copyright (c) 2011, 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.
 
 #include "vm/exceptions.h"
 
 #include "vm/dart_api_impl.h"
 #include "vm/dart_entry.h"
 #include "vm/debugger.h"
 #include "vm/flags.h"
@@ skipping 170 matching lines @@
 
 
 static bool ShouldShowFunction(const Function& function) {
   if (FLAG_verbose_stacktrace) {
     return true;
   }
   return function.is_visible();
 }
 
 
-// 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) {
     while (!frame->IsEntryFrame()) {
       if (frame->IsDartFrame()) {
         code = frame->LookupDartCode();
         if (code.is_optimized()) {
           // For optimized frames, extract all the inlined functions if any
           // into the stack trace.
           for (InlinedFunctionsIterator it(frame); !it.Done(); it.Advance()) {
             func = it.function();
             code = it.code();
             uword pc = it.pc();
             ASSERT(pc != 0);
             ASSERT(code.EntryPoint() <= pc);
             ASSERT(pc < (code.EntryPoint() + code.Size()));
             if (ShouldShowFunction(func)) {
               offset = Smi::New(pc - code.EntryPoint());
               builder->AddFrame(func, code, offset, dart_handler_found);
             }
           }
         } else {
           offset = Smi::New(frame->pc() - code.EntryPoint());
           func = code.function();
           if (ShouldShowFunction(func)) {
             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;
           }
         }
       }
       frame = frames.NextFrame();
       ASSERT(frame != NULL);
     }
     ASSERT(frame->IsEntryFrame());
     if (!handler_pc_set) {
       handler_pc_set = true;
+      handler_pc = frame->pc();
+      if (!builder->FullStacktrace()) {
+        return;
+      }
+    }
+    frame = frames.NextFrame();
+  }
+}
+
+
+// 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();
-      if (!builder->FullStacktrace()) {
-        return dart_handler_found;
-      }
+      *handler_needs_stacktrace = true;
     }
     frame = frames.NextFrame();
-  }
-  return dart_handler_found;
+  }  // 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) {
   // TODO(turnidge): Is there a faster way to get the next entry frame?
   StackFrameIterator frames(StackFrameIterator::kDontValidateFrames);
   StackFrame* frame = frames.NextFrame();
   ASSERT(frame != NULL);
@@ skipping 89 matching lines @@
                                     Object::empty_array());
   } else if (exception.raw() == isolate->object_store()->out_of_memory() ||
              exception.raw() == isolate->object_store()->stack_overflow()) {
     use_preallocated_stacktrace = true;
   }
   uword handler_pc = 0;
   uword handler_sp = 0;
   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()));
+                                          &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.
+    }
+    if (existing_stacktrace.IsNull()) {
       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);
-      }
-    }
-    // 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);
     } else {
       stacktrace ^= existing_stacktrace.raw();
       if (pc_offset_array.Length() != 0) {
         stacktrace.Append(func_array, code_array, pc_offset_array);
       }
       // Since we are re throwing and appending to the existing stack trace
       // we clear out the catch trace collected in the existing stack trace
       // as that trace will not be valid anymore.
       stacktrace.SetCatchStacktrace(Object::empty_array(),
                                     Object::empty_array(),
@@ skipping 275 matching lines @@
       break;
   }
 
   return DartLibraryCalls::InstanceCreate(library,
                                           *class_name,
                                           *constructor_name,
                                           arguments);
 }
 
 }  // namespace dart