First debugger API unit test

runtime/vm/debugger_api_impl.cc

Index: runtime/vm/debugger_api_impl.cc
===================================================================
--- runtime/vm/debugger_api_impl.cc	(revision 2291)
+++ runtime/vm/debugger_api_impl.cc	(working copy)
@@ -28,6 +28,85 @@
   } while (0);
 
 
+#define CHECK_AND_CAST(type, var, param)                                      \
+  if (param == NULL) {                                                        \
+    return Api::Error("%s expects argument '%s' to be non-null.",             \
+                      CURRENT_FUNC, #param);                                  \
+  }                                                                           \
+  type* var = reinterpret_cast<type*>(param);

[siva, 2011/12/09 02:20:04]

We are handing out these C++ VM object pointers and are assuming that the
embedder will pass back a pointer of the appropriate type here. Not sure if this
is going to be a problem. I am wondering if we should have a base
Debugger_Object class with virtual methods IsBreakpoint, IsActivationFrame,
IsStackTrace etc. and then have the individual classes Breakpoint, Stacktrace,
ActivationFrame etc. derive from this base class and return true for the
appropriate method. That way you can check here if it is the appropriate type
before the cast and report an error if it is some random object.

[hausner, 2011/12/13 00:14:59]

As we discussed in person, we have type safety through the typedefs that make
Dart_Breakpoint, Dart_StackTrace, Dart_Isolate etc incompatible. I agree that
longer-term we want a much more robust API that can detect whether the pointers
point to valid objects of the correct type. I put that on the long term TODO
list.

+
+
+#define CHECK_NOT_NULL(param)                                                 \
+  if (param == NULL) {                                                        \
+    return Api::Error("%s expects argument '%s' to be non-null.",             \
+                      CURRENT_FUNC, #param);                                  \
+  }
+
+
+DART_EXPORT Dart_Handle Dart_StackTraceLength(
+                            Dart_StackTrace trace,
+                            intptr_t* length) {
+  Isolate* isolate = Isolate::Current();
+  DARTSCOPE(isolate);
+  CHECK_NOT_NULL(length);
+  CHECK_AND_CAST(StackTrace, stack_trace, trace);
+  *length = stack_trace->Length();
+  return Api::True();
+}
+
+
+DART_EXPORT Dart_Handle Dart_GetActivationFrame(
+                            Dart_StackTrace trace,
+                            int frame_index,
+                            Dart_ActivationFrame* frame) {
+  Isolate* isolate = Isolate::Current();
+  DARTSCOPE(isolate);
+  CHECK_NOT_NULL(frame);
+  CHECK_AND_CAST(StackTrace, stack_trace, trace);
+  if ((frame_index < 0) || (frame_index >= stack_trace->Length())) {
+    return Api::Error("argument 'frame_index' is out of range for %s",
+                      CURRENT_FUNC);
+  }
+  *frame = reinterpret_cast<Dart_ActivationFrame>(
+       stack_trace->ActivationFrameAt(frame_index));
+  return Api::True();
+}
+
+
+DART_EXPORT void Dart_SetBreakpointHandler(
+                     Dart_BreakpointHandler bp_handler) {
+  Isolate* isolate = Isolate::Current();
+  DARTSCOPE(isolate);
+  BreakpointHandler* handler =
+      reinterpret_cast<BreakpointHandler*>(bp_handler);

[siva, 2011/12/09 02:20:04]

can bp_handler be NULL? Do we need a non NULL check here?

[hausner, 2011/12/13 00:14:59]

It can be NULL. Cf. other comment.

+
+  isolate->debugger()->SetBreakpointHandler(handler);
+}
+
+
+DART_EXPORT Dart_Handle Dart_ActivationFrameInfo(
+                            Dart_ActivationFrame activation_frame,
+                            Dart_Handle* function_name,
+                            Dart_Handle* script_url,
+                            intptr_t* line_number) {
+  Isolate* isolate = Isolate::Current();
+  DARTSCOPE(isolate);
+  CHECK_AND_CAST(ActivationFrame, frame, activation_frame);
+  if (function_name != NULL) {
+    const String& name = String::Handle(frame->QualifiedFunctionName());
+    *function_name = Api::NewLocalHandle(name);
+  }
+  if (script_url != NULL) {
+    const String& url = String::Handle(frame->SourceUrl());
+    *script_url = Api::NewLocalHandle(url);
+  }
+  if (line_number != NULL) {
+    *line_number = frame->LineNumber();
+  }
+  return Api::True();
+}
+
+
 DART_EXPORT Dart_Handle Dart_SetBreakpointAtEntry(
                             Dart_Handle library_in,
                             Dart_Handle class_name_in,
@@ -42,33 +121,39 @@
   UNWRAP_AND_CHECK_PARAM(Library, library, library_in);
   UNWRAP_AND_CHECK_PARAM(String, class_name, class_name_in);
   UNWRAP_AND_CHECK_PARAM(String, function_name, function_name_in);
+  CHECK_NOT_NULL(breakpoint);
 
   const char* msg = CheckIsolateState(isolate);
   if (msg != NULL) {
     return Api::Error(msg);
   }
 
-  if (breakpoint != NULL) {
-    *breakpoint = NULL;
-  }
-
   // Resolve the breakpoint target function.
   Debugger* debugger = isolate->debugger();
   const Function& bp_target = Function::Handle(
       debugger->ResolveFunction(library, class_name, function_name));
   if (bp_target.IsNull()) {
-    return Api::Error("Breakpoint target function does not exist");
+    const bool toplevel = class_name.Length() == 0;
+    return Api::Error("%s: could not find function '%s%s%s'",
+                      CURRENT_FUNC,

[siva, 2011/12/09 02:20:04]

I presume the library URL is also needed to fully qualify the name?

[hausner, 2011/12/13 00:14:59]

I think the names would become very unreadable if we added the library url. It's
technically necessary to clearly identify the function, though.

+                      toplevel ? "" : class_name.ToCString(),
+                      toplevel ? "" : ".",
+                      function_name.ToCString());
   }
 
   LongJump* base = isolate->long_jump_base();
   LongJump jump;
   isolate->set_long_jump_base(&jump);
   Dart_Handle result = Api::True();
+  *breakpoint = NULL;
   if (setjmp(*jump.Set()) == 0) {
     Breakpoint* bpt = debugger->SetBreakpointAtEntry(bp_target);
-    if (breakpoint != NULL) {
-      *breakpoint = reinterpret_cast<Dart_Breakpoint>(bpt);
+    if (bpt == NULL) {
+      const char* target_name = Debugger::QualifiedFunctionName(bp_target);
+      result = Api::Error("%s: no breakpoint location found in '%s'",
+                          CURRENT_FUNC, target_name);
     }
+    *breakpoint = reinterpret_cast<Dart_Breakpoint>(bpt);

[siva, 2011/12/09 02:20:04]

Should the setting of *breakpoint be under an else { .... } as *breakpoint has
already been initialized to NULL so there is no need to again assign NULL to it
when bpt is NULL.

[hausner, 2011/12/13 00:14:59]

Yes, that's cleaner. Done.

   } else {
     SetupErrorResult(&result);
   }