Improve the stack trace output to be more readable.

vm/object.cc

 // Copyright (c) 2012, 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/object.h"
 
 #include "include/dart_api.h"
 #include "platform/assert.h"
 #include "vm/assembler.h"
 #include "vm/bigint_operations.h"
@@ skipping 83 matching lines @@
 RawClass* Object::icdata_class_ = reinterpret_cast<RawClass*>(RAW_NULL);
 RawClass* Object::subtypetestcache_class_ =
     reinterpret_cast<RawClass*>(RAW_NULL);
 RawClass* Object::api_error_class_ = reinterpret_cast<RawClass*>(RAW_NULL);
 RawClass* Object::language_error_class_ = reinterpret_cast<RawClass*>(RAW_NULL);
 RawClass* Object::unhandled_exception_class_ =
     reinterpret_cast<RawClass*>(RAW_NULL);
 RawClass* Object::unwind_error_class_ = reinterpret_cast<RawClass*>(RAW_NULL);
 #undef RAW_NULL
 
+
+// Takes a vm internal name and makes it suitable for external user.
+//
+// Examples:
+//
+// Internal getter and setter prefices are removed:

[siva, 2012/08/07 21:41:37]

prefixes

[turnidge, 2012/08/07 23:32:09]

Done.

+//
+//   get:foo -> foo
+//   set:foo -> foo

[siva, 2012/08/07 21:41:37]

I am wondering get:foo should become 'get foo' and
set:foo converted to 'set foo' in order to match the source code and have some
way of differentiating setters from getters. What do you think?

[turnidge, 2012/08/07 23:32:09]

Discussed offline.  For now this lets me share the pretty name with the
embedding api, mirrors, and stack traces.  From talking with Gilad, I believe
that foo= will be the official name for the setter from a spec perspective.  For
now we will commit is as is and change it if it is confusing.

On 2012/08/07 21:41:37, asiva wrote:

+//
+// Private name mangling is removed, possibly twice:
+//
+//   _ReceivePortImpl@6be832b -> _ReceivePortImpl
+//   _ReceivePortImpl@6be832b._internal@6be832b -> +ReceivePortImpl._internal
+//
+// The trailing . on the default constructor name is dropped:
+//
+//   List. -> List
+//
+// And so forth:
+//
+//   get:foo@6be832b -> foo
+//   _MyClass@6b3832b. -> _MyClass
+//   _MyClass@6b3832b.named -> _MyClass.named
+//
+static RawString* IdentifierPrettyName(const String& name) {
+  intptr_t len = name.Length();
+  intptr_t start = 0;
+  intptr_t at_pos = len;   // Position of '@' in the name.
+  intptr_t dot_pos = len;  // Position of '.' in the name.
+
+  for (int i = 0; i < name.Length(); i++) {
+    if (name.CharAt(i) == ':') {
+      ASSERT(start == 0);
+      start = i + 1;
+    } else if (name.CharAt(i) == '@') {
+      ASSERT(at_pos == len);
+      at_pos = i;
+    } else if (name.CharAt(i) == '.') {
+      dot_pos = i;
+      break;
+    }
+  }
+  intptr_t limit = (at_pos < dot_pos ? at_pos : dot_pos);
+  if (start == 0 && limit == len) {
+    // This name is fine as it is.
+    return name.raw();
+  }
+
+  String& result = String::Handle();
+  result = String::SubString(name, start, (limit - start));

[siva, 2012/08/07 21:41:37]

const String& result = String::Handle(String::Sub.......)
like the suffix setting below?

[turnidge, 2012/08/07 23:32:09]

Done.

+
+  // Look for a second '@' now to correctly handle names like
+  // "_ReceivePortImpl@6be832b._internal@6be832b".
+  at_pos = len;
+  for (int i = dot_pos; i < name.Length(); i++) {
+    if (name.CharAt(i) == '@') {
+      ASSERT(at_pos == len);
+      at_pos = i;
+    }
+  }
+
+  intptr_t suffix_len = at_pos - dot_pos;
+  if (suffix_len <= 1) {
+    // The constructor name is of length 0 or 1.  That means that
+    // either this isn't a constructor or that this is an unnamed
+    // constructor.  In either case, we're done.
+    return result.raw();
+  }
+
+  const String& suffix =
+      String::Handle(String::SubString(name, dot_pos, suffix_len));
+  return String::Concat(result, suffix);
+}
+
+
 int Object::GetSingletonClassIndex(const RawClass* raw_class) {
   ASSERT(raw_class->IsHeapObject());
   if (raw_class == class_class()) {
     return kClassClass;
   } else if (raw_class == null_class()) {
     return kNullClass;
   } else if (raw_class == dynamic_class()) {
     return kDynamicClass;
   } else if (raw_class == void_class()) {
     return kVoidClass;
@@ skipping 1010 matching lines @@
     case kUint64Array:
     case kExternalUint64Array:
       return Symbols::New("Uint64List");
     case kFloat32Array:
     case kExternalFloat32Array:
       return Symbols::New("Float32List");
     case kFloat64Array:
     case kExternalFloat64Array:
       return Symbols::New("Float64List");
     default:
-      return Name();
+      const String& name = String::Handle(Name());
+      return IdentifierPrettyName(name);
   }
   UNREACHABLE();
 }
 
 
 RawType* Class::SignatureType() const {
   ASSERT(IsSignatureClass());
   const Function& function = Function::Handle(signature_function());
   ASSERT(!function.IsNull());
   if (function.signature_class() != raw()) {
@@ skipping 3214 matching lines @@
   }
   return true;
 }
 
 
 bool Function::HasOptimizedCode() const {
   return HasCode() && Code::Handle(raw_ptr()->code_).is_optimized();
 }
 
 
+RawString* Function::UserVisibleName() const {
+  String& tmp = String::Handle();
+  tmp = name();
+  tmp = IdentifierPrettyName(tmp);
+  if (kind() == RawFunction::kSetterFunction) {
+    const String& suffix = String::Handle(Symbols::Equals());
+    tmp = String::Concat(tmp, suffix);

[siva, 2012/08/07 21:41:37]

See comment above regarding setter/gettter, does it make sense to match the
source code?

[turnidge, 2012/08/07 23:32:09]

See above.
 

+  }
+  return tmp.raw();
+}
+
+
+RawString* Function::QualifiedUserVisibleName() const {
+  String& tmp = String::Handle();
+  String& suffix = String::Handle();
+  const Class& cls = Class::Handle(owner());
+
+  if (IsClosureFunction()) {
+    if (IsLocalFunction()) {
+      const Function& parent = Function::Handle(parent_function());
+      tmp = parent.QualifiedUserVisibleName();
+    } else {
+      return UserVisibleName();
+    }
+  } else {
+    if (cls.IsTopLevel()) {
+      return UserVisibleName();
+    } else {
+      tmp = cls.UserVisibleName();
+    }
+  }
+  suffix = Symbols::Dot();
+  tmp = String::Concat(tmp, suffix);
+  suffix = UserVisibleName();
+  return String::Concat(tmp, suffix);
+}
+
+
 const char* Function::ToCString() const {
   const char* static_str = is_static() ? " static" : "";
   const char* abstract_str = is_abstract() ? " abstract" : "";
   const char* kind_str = NULL;
   const char* const_str = is_const() ? " const" : "";
   switch (kind()) {
     case RawFunction::kRegularFunction:
     case RawFunction::kClosureFunction:
     case RawFunction::kGetterFunction:
     case RawFunction::kSetterFunction:
@@ skipping 133 matching lines @@
   }
   result.set_is_final(is_final);
   result.set_is_const(is_const);
   result.set_owner(owner);
   result.set_token_pos(token_pos);
   result.set_has_initializer(false);
   return result.raw();
 }
 
 
+RawString* Field::UserVisibleName() const {
+  const String& str = String::Handle(name());
+  return IdentifierPrettyName(str);
+}
+
+
 const char* Field::ToCString() const {
   const char* kF0 = is_static() ? " static" : "";
   const char* kF1 = is_final() ? " final" : "";
   const char* kF2 = is_const() ? " const" : "";
   const char* kFormat = "Field <%s.%s>:%s%s%s";
   const char* field_name = String::Handle(name()).ToCString();
   const Class& cls = Class::Handle(owner());
   const char* cls_name = String::Handle(cls.Name()).ToCString();
   intptr_t len =
       OS::SNPrint(NULL, 0, kFormat, cls_name, field_name, kF0, kF1, kF2) + 1;
@@ skipping 6349 matching lines @@
     obj = func_list.At(j);
     function_array.SetAt(i, obj);
     obj = code_list.At(j);
     code_array.SetAt(i, obj);
     obj = pc_offset_list.At(j);
     pc_offset_array.SetAt(i, obj);
   }
 }
 
 
-const char* Stacktrace::ToCStringInternal(bool verbose) const {
+const char* Stacktrace::ToCString() const {
   Function& function = Function::Handle();
   Code& code = Code::Handle();
-  Class& function_class = Class::Handle();
+  Class& owner = Class::Handle();
   Script& script = Script::Handle();
   String& function_name = String::Handle();
-  String& class_name = String::Handle();
   String& url = String::Handle();
 
   // Iterate through the stack frames and create C string description
   // for each frame.
   intptr_t total_len = 0;
-  const char* kFormat = verbose ?
-      " %d. Function: '%s%s%s' url: '%s' line:%d col:%d code-entry: 0x%x\n" :
-      " %d. Function: '%s%s%s' url: '%s' line:%d col:%d\n";
+  const char* kFormat = "    at %s (%s:%d:%d)\n";

[siva, 2012/08/07 21:41:37]

Looking at the sample output you have attached I am not sure adding the 'at'
string to the stack trace adds any value.

Also I was wondering if we have recursive calls then it might be useful to have
the frame number in the trace as it provides a reference number.

[turnidge, 2012/08/07 23:32:09]

Replaced "at" with a frame number.

On 2012/08/07 21:41:37, asiva wrote:

   GrowableArray<char*> frame_strings;
   for (intptr_t i = 0; i < Length(); i++) {
     function = FunctionAtFrame(i);
     code = CodeAtFrame(i);
     uword pc = code.EntryPoint() + Smi::Value(PcOffsetAtFrame(i));
     intptr_t token_pos = code.GetTokenIndexOfPC(pc);
-    function_class = function.owner();
-    script = function_class.script();
-    function_name = function.name();
-    class_name = function_class.Name();
+    owner = function.owner();
+    script = owner.script();
+    function_name = function.QualifiedUserVisibleName();
     url = script.url();
     intptr_t line = -1;
     intptr_t column = -1;
     if (token_pos >= 0) {
       script.GetTokenLocation(token_pos, &line, &column);
     }
     intptr_t len = OS::SNPrint(NULL, 0, kFormat,
-                               i,
-                               class_name.ToCString(),
-                               function_class.IsTopLevel() ? "" : ".",
                                function_name.ToCString(),
                                url.ToCString(),
-                               line, column,
-                               code.EntryPoint());
+                               line, column);
     total_len += len;
     char* chars = Isolate::Current()->current_zone()->Alloc<char>(len + 1);
     OS::SNPrint(chars, (len + 1), kFormat,
-                i,
-                class_name.ToCString(),
-                function_class.IsTopLevel() ? "" : ".",
                 function_name.ToCString(),
                 url.ToCString(),
-                line, column,
-                code.EntryPoint());
+                line, column);
     frame_strings.Add(chars);
   }
 
   // Now concatentate the frame descriptions into a single C string.
   char* chars = Isolate::Current()->current_zone()->Alloc<char>(total_len + 1);
   intptr_t index = 0;
   for (intptr_t i = 0; i < frame_strings.length(); i++) {
     index += OS::SNPrint((chars + index),
                          (total_len + 1 - index),
                          "%s",
                          frame_strings[i]);
   }
   return chars;
 }
 
 
-const char* Stacktrace::ToCString() const {
-  return ToCStringInternal(false);
-}
-
-
 void JSRegExp::set_pattern(const String& pattern) const {
   StorePointer(&raw_ptr()->pattern_, pattern.raw());
 }
 
 
 void JSRegExp::set_num_bracket_expressions(intptr_t value) const {
   raw_ptr()->num_bracket_expressions_ = Smi::New(value);
 }
 
 
@@ skipping 78 matching lines @@
 const char* JSRegExp::ToCString() const {
   const String& str = String::Handle(pattern());
   const char* format = "JSRegExp: pattern=%s flags=%s";
   intptr_t len = OS::SNPrint(NULL, 0, format, str.ToCString(), Flags());
   char* chars = Isolate::Current()->current_zone()->Alloc<char>(len + 1);
   OS::SNPrint(chars, (len + 1), format, str.ToCString(), Flags());
   return chars;
 }
 
 }  // namespace dart