Refactor VM service IDs

runtime/vm/service.cc

Index: runtime/vm/service.cc
diff --git a/runtime/vm/service.cc b/runtime/vm/service.cc
index 087c933d0c3b0832dd98a4b8a9ba76dcb1c51a37..52078cc06dc65b46ed968a66212a11d2ac73a886 100644
--- a/runtime/vm/service.cc
+++ b/runtime/vm/service.cc
@@ -153,6 +153,26 @@ static void PrintArgumentsAndOptions(const JSONObject& obj, JSONStream* js) {
 }
 
 
+static void PrintCollectionErrorResponse(const char* collection_name,
+                                         JSONStream* js) {
+  JSONObject jsobj(js);
+  jsobj.AddProperty("type", "Error");
+  jsobj.AddPropertyF("text", "Must specify collection object id: %s/id",
+                     collection_name);
+}
+
+
+static void PrintCollectionRangeErrorResponse(const char* collection_name,
+                                              JSONStream* js,
+                                              intptr_t id,
+                                              intptr_t length) {
+  JSONObject jsobj(js);
+  jsobj.AddProperty("type", "Error");
+  jsobj.AddPropertyF("text", "%s id (%" Pd ") must be in [0, %" Pd ").",
+                     collection_name, id, length);
+}
+
+
 static void PrintGenericError(JSONStream* js) {
   JSONObject jsobj(js);
   jsobj.AddProperty("type", "Error");
@@ -238,6 +258,78 @@ static void HandleEcho(Isolate* isolate, JSONStream* js) {
     return;                                                                   \
   }
 
+#define CHECK_COLLECTION_ID_BOUNDS(collection, length, arg, id, js)            \
+  if (!GetIntegerId(arg, &id)) {                                               \
+    PrintCollectionErrorResponse(collection, js);                              \

[turnidge, 2013/12/19 20:39:29]

Why not just use PrintError (accepts format string) here instead?  I can see a
helper if it is used a lot, but only uses are here in the macro.

[Cutch, 2013/12/19 21:53:58]

Done.

+    return;                                                                    \
+  }                                                                            \
+  if ((id < 0) || (id >= length)) {                                            \
+    PrintCollectionRangeErrorResponse(collection, js, id, length);             \
+  }
+
+
+static bool GetIntegerId(const char* s, intptr_t* id) {
+  if ((s == NULL) || (*s == '\0')) {
+    // Empty string.
+    return false;
+  }
+  if (id == NULL) {
+    // No id pointer.
+    return false;
+  }
+  intptr_t r = 0;
+  char* end_ptr = NULL;
+  r = strtol(s, &end_ptr, 10);
+  if (end_ptr == s) {
+    // String was not advanced at all, cannot be valid.
+    return false;
+  }
+  *id = r;
+  return true;
+}
+
+
+static void HandleClassesClosures(Isolate* isolate, const Class& cls,
+                                  JSONStream* js) {
+  const GrowableObjectArray& closures =
+      GrowableObjectArray::Handle(cls.closures());
+  intptr_t id;
+  CHECK_COLLECTION_ID_BOUNDS("closures", closures.Length(), js->GetArgument(3),
+                             id, js);
+  Function& function = Function::Handle();
+  function ^= closures.At(id);
+  ASSERT(!function.IsNull());
+  function.PrintToJSONStream(js, false);
+}
+
+
+static void HandleClassesFunctions(Isolate* isolate, const Class& cls,
+                                   JSONStream* js) {
+  const Array& functions =
+      Array::Handle(cls.functions());
+  intptr_t id;
+  CHECK_COLLECTION_ID_BOUNDS("functions", functions.Length(),
+                             js->GetArgument(3), id, js);
+  Function& function = Function::Handle();
+  function ^= functions.At(id);
+  ASSERT(!function.IsNull());
+  function.PrintToJSONStream(js, false);
+}
+
+
+static void HandleClassesFields(Isolate* isolate, const Class& cls,
+                                JSONStream* js) {
+  const Array& fields =
+      Array::Handle(cls.fields());
+  intptr_t id;
+  CHECK_COLLECTION_ID_BOUNDS("fields", fields.Length(), js->GetArgument(3),
+                             id, js);
+  Field& field = Field::Handle();
+  field ^= fields.At(id);
+  ASSERT(!field.IsNull());
+  field.PrintToJSONStream(js, false);
+}
+
 
 static void HandleClasses(Isolate* isolate, JSONStream* js) {
   if (js->num_arguments() == 1) {
@@ -246,13 +338,33 @@ static void HandleClasses(Isolate* isolate, JSONStream* js) {
     return;
   }
   ASSERT(js->num_arguments() >= 2);
-  intptr_t id = atoi(js->GetArgument(1));
+  intptr_t id;
+  if (!GetIntegerId(js->GetArgument(1), &id)) {
+    PrintGenericError(js);

[turnidge, 2013/12/19 20:39:29]

Use PrintError and give a nice message here.

[Cutch, 2013/12/19 21:53:58]

Done.

+    return;
+  }
   ClassTable* table = isolate->class_table();
   if (!table->IsValidIndex(id)) {
     Object::null_object().PrintToJSONStream(js, false);

[turnidge, 2013/12/19 20:39:29]

So null is what we give for an invalid reference?  Seems ambiguous, would prefer
Error.

[Cutch, 2013/12/19 21:53:58]

Legacy code, returning Error now.

-  } else {
-    Class& cls = Class::Handle(table->At(id));
+    return;
+  }
+  Class& cls = Class::Handle(table->At(id));
+  if (js->num_arguments() == 2) {
     cls.PrintToJSONStream(js, false);
+    return;
+  } else if (js->num_arguments() == 4) {

[turnidge, 2013/12/19 20:39:29]

Maybe just check that num_arguments is >= 3 and let the helper function (e.g.
HandleClassesClosures) handle the Error case.  This will open the door for more
interesting things in the future like:

class/55/function/33/deoptimize

[Cutch, 2013/12/19 21:53:58]

Done.

+    const char* second = js->GetArgument(2);
+    if (!strcmp(second, "closures")) {
+      HandleClassesClosures(isolate, cls, js);
+    } else if (!strcmp(second, "fields")) {
+      HandleClassesFields(isolate, cls, js);
+    } else if (!strcmp(second, "functions")) {
+      HandleClassesFunctions(isolate, cls, js);
+    } else {
+      PrintError(js, "Invalid sub collection %s", second);
+    }
+  } else {
+    PrintGenericError(js);

[turnidge, 2013/12/19 20:39:29]

This goes away if you let helpers handle sub-errors.

I tend to think we should avoid PrintGenericError.

[Cutch, 2013/12/19 21:53:58]

Done.

   }
 }

[turnidge, 2013/12/19 20:39:29]

Here and below...

You should have my changes now which support C++ unit tests for service
handlers.  I think we should write C++ tests for these handlers which cover the
success and error cases.

[Cutch, 2013/12/19 21:53:58]

Done.

 
@@ -268,17 +380,108 @@ static void HandleLibrary(Isolate* isolate, JSONStream* js) {
 }
 
 
+static void HandleLibraries(Isolate* isolate, JSONStream* js) {
+  REQUIRE_COLLECTION_ID("libraries");
+  const GrowableObjectArray& libs =
+      GrowableObjectArray::Handle(isolate->object_store()->libraries());
+  ASSERT(!libs.IsNull());
+  intptr_t id = 0;
+  CHECK_COLLECTION_ID_BOUNDS("libraries", libs.Length(), js->GetArgument(1),
+                             id, js);
+  Library& lib = Library::Handle();
+  lib ^= libs.At(id);
+  ASSERT(!lib.IsNull());
+  lib.PrintToJSONStream(js, false);

[turnidge, 2013/12/19 20:39:29]

So do we not yet support Fields/Functions for libraries?  Maybe a TODO?

[Cutch, 2013/12/19 21:53:58]

Good point! TODO added.

+}
+
+
 static void HandleObjects(Isolate* isolate, JSONStream* js) {
   REQUIRE_COLLECTION_ID("objects");
   ASSERT(js->num_arguments() >= 2);
   ObjectIdRing* ring = isolate->object_id_ring();
   ASSERT(ring != NULL);
-  intptr_t id = atoi(js->GetArgument(1));
+  intptr_t id = -1;
+  if (!GetIntegerId(js->GetArgument(1), &id)) {
+    Object::null_object().PrintToJSONStream(js, false);

[turnidge, 2013/12/19 20:39:29]

Again, null object seems ambiguous.  I might prefer Error.

+    return;
+  }
   Object& obj = Object::Handle(ring->GetObjectForId(id));
   obj.PrintToJSONStream(js, false);
 }
 
 
+
+static void HandleScriptsEnumerate(Isolate* isolate, JSONStream* js) {
+  JSONObject jsobj(js);
+  jsobj.AddProperty("type", "ScriptList");
+  {
+    JSONArray members(&jsobj, "members");
+    const GrowableObjectArray& libs =
+      GrowableObjectArray::Handle(isolate->object_store()->libraries());
+    int num_libs = libs.Length();
+    Library &lib = Library::Handle();
+    Script& script = Script::Handle();
+    for (intptr_t i = 0; i < num_libs; i++) {
+      lib ^= libs.At(i);
+      ASSERT(!lib.IsNull());
+      ASSERT(Smi::IsValid(lib.index()));
+      const Array& loaded_scripts = Array::Handle(lib.LoadedScripts());
+      ASSERT(!loaded_scripts.IsNull());
+      intptr_t num_scripts = loaded_scripts.Length();
+      for (intptr_t i = 0; i < num_scripts; i++) {
+        script ^= loaded_scripts.At(i);
+        members.AddValue(script);
+      }
+    }
+  }
+}
+
+
+static void HandleScriptsFetch(Isolate* isolate, JSONStream* js) {
+  const GrowableObjectArray& libs =
+    GrowableObjectArray::Handle(isolate->object_store()->libraries());
+  int num_libs = libs.Length();
+  Library &lib = Library::Handle();
+  Script& script = Script::Handle();
+  String& url = String::Handle();
+  const String& id = String::Handle(String::New(js->GetArgument(1)));
+  ASSERT(!id.IsNull());
+  // The id is the url of the script % encoded, decode it.
+  String& requested_url = String::Handle(String::DecodeURI(id));
+  for (intptr_t i = 0; i < num_libs; i++) {
+    lib ^= libs.At(i);
+    ASSERT(!lib.IsNull());
+    ASSERT(Smi::IsValid(lib.index()));
+    const Array& loaded_scripts = Array::Handle(lib.LoadedScripts());
+    ASSERT(!loaded_scripts.IsNull());
+    intptr_t num_scripts = loaded_scripts.Length();
+    for (intptr_t i = 0; i < num_scripts; i++) {
+      script ^= loaded_scripts.At(i);
+      ASSERT(!script.IsNull());
+      url ^= script.url();
+      if (url.Equals(requested_url)) {
+        script.PrintToJSONStream(js, false);
+        return;
+      }
+    }
+  }
+  PrintError(js, "Cannot find script %s\n", requested_url.ToCString());
+}
+
+
+static void HandleScripts(Isolate* isolate, JSONStream* js) {
+  if (js->num_arguments() == 1) {
+    // Enumerate all scripts.
+    HandleScriptsEnumerate(isolate, js);
+  } else if (js->num_arguments() == 2) {
+    // Fetch specific script.
+    HandleScriptsFetch(isolate, js);
+  } else {
+    PrintError(js, "Command too long");
+  }
+}
+
+
 static void HandleDebug(Isolate* isolate, JSONStream* js) {
   if (js->num_arguments() == 1) {
     PrintError(js, "Must specify a subcommand");
@@ -324,10 +527,12 @@ static ServiceMessageHandlerEntry __message_handlers[] = {
   { "classes", HandleClasses },
   { "cpu", HandleCpu },
   { "debug", HandleDebug },
+  { "libraries", HandleLibraries },
   { "library", HandleLibrary },

[turnidge, 2013/12/19 20:39:29]

Is the plan to have both library and libraries?  I thought we were talking about
one or the other.

[Cutch, 2013/12/19 21:53:58]

library is how you get to the root library. libraries is the collection. We
could maybe rename it to "root_library" or something. Or will the root library
always be library id 0?

   { "name", HandleName },
   { "objecthistogram", HandleObjectHistogram},
   { "objects", HandleObjects },
+  { "scripts", HandleScripts },
   { "stacktrace", HandleStackTrace },
 };