Fix DCHECK failures from Class::VisitFieldRoots

We now use GetDeclaringClassUnchecked when marking roots to fix
flaky test failures. Fixed a race condition in root marking where
we could have non zero field array length with a null pointer.
Fixed a race condition where we could be marking roots before
FixupTemporaryDeclaringClass had finished. The solution is to
only do the declaring class CHECK if we are at least resolved.
Fixed JDWP tests by changing FieldId / MethodId to be 64 bits.

Also some cleanup.

Change-Id: Ibac09519860d93c3f68a5cc964bbc91dc10a279a

9 files changed, 36 insertions, 30 deletions

diff --git a/runtime/gc_root.h b/runtime/gc_root.h
index 0d3c93b83d..bdc7d5c8e6 100644
--- a/runtime/gc_root.h
+++ b/runtime/gc_root.h
@@ -50,7 +50,7 @@ enum RootType {
 };
 std::ostream& operator<<(std::ostream& os, const RootType& root_type);
 
-// Only used by hprof. tid and root_type are only used by hprof.
+// Only used by hprof. thread_id_ and type_ are only used by hprof.
 class RootInfo {
  public:
   // Thread id 0 is for non thread roots.
@@ -85,12 +85,13 @@ class RootVisitor {
  public:
   virtual ~RootVisitor() { }
 
-  // Single root versions, not overridable.
+  // Single root version, not overridable.
   ALWAYS_INLINE void VisitRoot(mirror::Object** roots, const RootInfo& info)
       SHARED_LOCKS_REQUIRED(Locks::mutator_lock_) {
     VisitRoots(&roots, 1, info);
   }
 
+  // Single root version, not overridable.
   ALWAYS_INLINE void VisitRootIfNonNull(mirror::Object** roots, const RootInfo& info)
       SHARED_LOCKS_REQUIRED(Locks::mutator_lock_) {
     if (*roots != nullptr) {
diff --git a/runtime/handle.h b/runtime/handle.h
index 3ebb2d5d30..d94d87552a 100644
--- a/runtime/handle.h
+++ b/runtime/handle.h
@@ -70,8 +70,8 @@ class Handle : public ValueObject {
     return reinterpret_cast<jobject>(reference_);
   }
 
-  StackReference<mirror::Object>* GetReference() SHARED_LOCKS_REQUIRED(Locks::mutator_lock_)
-      ALWAYS_INLINE {
+  ALWAYS_INLINE StackReference<mirror::Object>* GetReference()
+      SHARED_LOCKS_REQUIRED(Locks::mutator_lock_) {
     return reference_;
   }
 
diff --git a/runtime/jdwp/jdwp.h b/runtime/jdwp/jdwp.h
index a503b17e87..8dffee606c 100644
--- a/runtime/jdwp/jdwp.h
+++ b/runtime/jdwp/jdwp.h
@@ -51,22 +51,24 @@ namespace JDWP {
  * Fundamental types.
  *
  * ObjectId and RefTypeId must be the same size.
+ * Its OK to change MethodId and FieldId sizes as long as the size is <= 8 bytes.
+ * Note that ArtFields are 64 bit pointers on 64 bit targets. So this one must remain 8 bytes.
  */
-typedef uint32_t FieldId;     /* static or instance field */
-typedef uint32_t MethodId;    /* any kind of method, including constructors */
+typedef uint64_t FieldId;     /* static or instance field */
+typedef uint64_t MethodId;    /* any kind of method, including constructors */
 typedef uint64_t ObjectId;    /* any object (threadID, stringID, arrayID, etc) */
 typedef uint64_t RefTypeId;   /* like ObjectID, but unique for Class objects */
 typedef uint64_t FrameId;     /* short-lived stack frame ID */
 
 ObjectId ReadObjectId(const uint8_t** pBuf);
 
-static inline void SetFieldId(uint8_t* buf, FieldId val) { return Set4BE(buf, val); }
-static inline void SetMethodId(uint8_t* buf, MethodId val) { return Set4BE(buf, val); }
+static inline void SetFieldId(uint8_t* buf, FieldId val) { return Set8BE(buf, val); }
+static inline void SetMethodId(uint8_t* buf, MethodId val) { return Set8BE(buf, val); }
 static inline void SetObjectId(uint8_t* buf, ObjectId val) { return Set8BE(buf, val); }
 static inline void SetRefTypeId(uint8_t* buf, RefTypeId val) { return Set8BE(buf, val); }
 static inline void SetFrameId(uint8_t* buf, FrameId val) { return Set8BE(buf, val); }
-static inline void expandBufAddFieldId(ExpandBuf* pReply, FieldId id) { expandBufAdd4BE(pReply, id); }
-static inline void expandBufAddMethodId(ExpandBuf* pReply, MethodId id) { expandBufAdd4BE(pReply, id); }
+static inline void expandBufAddFieldId(ExpandBuf* pReply, FieldId id) { expandBufAdd8BE(pReply, id); }
+static inline void expandBufAddMethodId(ExpandBuf* pReply, MethodId id) { expandBufAdd8BE(pReply, id); }
 static inline void expandBufAddObjectId(ExpandBuf* pReply, ObjectId id) { expandBufAdd8BE(pReply, id); }
 static inline void expandBufAddRefTypeId(ExpandBuf* pReply, RefTypeId id) { expandBufAdd8BE(pReply, id); }
 static inline void expandBufAddFrameId(ExpandBuf* pReply, FrameId id) { expandBufAdd8BE(pReply, id); }
diff --git a/runtime/jdwp/jdwp_event.cc b/runtime/jdwp/jdwp_event.cc
index ccf8bffba3..1ec800fce6 100644
--- a/runtime/jdwp/jdwp_event.cc
+++ b/runtime/jdwp/jdwp_event.cc
@@ -957,7 +957,7 @@ void JdwpState::PostFieldEvent(const EventLocation* pLoc, ArtField* field,
     VLOG(jdwp) << StringPrintf("  this=%#" PRIx64, instance_id);
     VLOG(jdwp) << StringPrintf("  type=%#" PRIx64, field_type_id) << " "
         << Dbg::GetClassName(field_id);
-    VLOG(jdwp) << StringPrintf("  field=%#" PRIx32, field_id) << " "
+    VLOG(jdwp) << StringPrintf("  field=%#" PRIx64, field_id) << " "
         << Dbg::GetFieldName(field_id);
     VLOG(jdwp) << "  suspend_policy=" << suspend_policy;
   }
diff --git a/runtime/jdwp/jdwp_handler.cc b/runtime/jdwp/jdwp_handler.cc
index d0ca214ee4..2457f1452c 100644
--- a/runtime/jdwp/jdwp_handler.cc
+++ b/runtime/jdwp/jdwp_handler.cc
@@ -38,11 +38,11 @@ namespace art {
 namespace JDWP {
 
 std::string DescribeField(const FieldId& field_id) {
-  return StringPrintf("%#x (%s)", field_id, Dbg::GetFieldName(field_id).c_str());
+  return StringPrintf("%#" PRIx64 " (%s)", field_id, Dbg::GetFieldName(field_id).c_str());
 }
 
 std::string DescribeMethod(const MethodId& method_id) {
-  return StringPrintf("%#x (%s)", method_id, Dbg::GetMethodName(method_id).c_str());
+  return StringPrintf("%#" PRIx64 " (%s)", method_id, Dbg::GetMethodName(method_id).c_str());
 }
 
 std::string DescribeRefTypeId(const RefTypeId& ref_type_id) {
@@ -101,8 +101,8 @@ static JdwpError RequestInvoke(JdwpState*, Request* request, ExpandBuf* pReply,
 
   VLOG(jdwp) << StringPrintf("    --> thread_id=%#" PRIx64 " object_id=%#" PRIx64,
                              thread_id, object_id);
-  VLOG(jdwp) << StringPrintf("        class_id=%#" PRIx64 " method_id=%x %s.%s", class_id,
-                             method_id, Dbg::GetClassName(class_id).c_str(),
+  VLOG(jdwp) << StringPrintf("        class_id=%#" PRIx64 " method_id=%#" PRIx64 " %s.%s",
+                             class_id, method_id, Dbg::GetClassName(class_id).c_str(),
                              Dbg::GetMethodName(method_id).c_str());
   VLOG(jdwp) << StringPrintf("        %d args:", arg_count);
 
@@ -256,8 +256,6 @@ static JdwpError VM_TopLevelThreadGroups(JdwpState*, Request*, ExpandBuf* pReply
 
 /*
  * Respond with the sizes of the basic debugger types.
- *
- * All IDs are 8 bytes.
  */
 static JdwpError VM_IDSizes(JdwpState*, Request*, ExpandBuf* pReply)
     SHARED_LOCKS_REQUIRED(Locks::mutator_lock_) {
diff --git a/runtime/jdwp/jdwp_request.cc b/runtime/jdwp/jdwp_request.cc
index 7b15d6de11..18f40a143c 100644
--- a/runtime/jdwp/jdwp_request.cc
+++ b/runtime/jdwp/jdwp_request.cc
@@ -87,13 +87,13 @@ uint32_t Request::ReadUnsigned32(const char* what) {
 }
 
 FieldId Request::ReadFieldId() {
-  FieldId id = Read4BE();
+  FieldId id = Read8BE();
   VLOG(jdwp) << "    field id " << DescribeField(id);
   return id;
 }
 
 MethodId Request::ReadMethodId() {
-  MethodId id = Read4BE();
+  MethodId id = Read8BE();
   VLOG(jdwp) << "    method id " << DescribeMethod(id);
   return id;
 }
diff --git a/runtime/mirror/class-inl.h b/runtime/mirror/class-inl.h
index f4656ec78b..aaa66f9579 100644
--- a/runtime/mirror/class-inl.h
+++ b/runtime/mirror/class-inl.h
@@ -809,18 +809,24 @@ inline ObjectArray<String>* Class::GetDexCacheStrings() {
 template<class Visitor>
 void mirror::Class::VisitFieldRoots(Visitor& visitor) {
   ArtField* const sfields = GetSFieldsUnchecked();
-  for (size_t i = 0, count = NumStaticFields(); i < count; ++i) {
-    if (kIsDebugBuild && GetStatus() != kStatusRetired) {
-      CHECK_EQ(sfields[i].GetDeclaringClass(), this);
+  // Since we visit class roots while we may be writing these fields, check against null.
+  // TODO: Is this safe for concurrent compaction?
+  if (sfields != nullptr) {
+    for (size_t i = 0, count = NumStaticFields(); i < count; ++i) {
+      if (kIsDebugBuild && IsResolved()) {
+        CHECK_EQ(sfields[i].GetDeclaringClass(), this) << GetStatus();
+      }
+      visitor.VisitRoot(sfields[i].DeclaringClassRoot().AddressWithoutBarrier());
     }
-    visitor.VisitRoot(sfields[i].DeclaringClassRoot().AddressWithoutBarrier());
   }
   ArtField* const ifields = GetIFieldsUnchecked();
-  for (size_t i = 0, count = NumInstanceFields(); i < count; ++i) {
-    if (kIsDebugBuild && GetStatus() != kStatusRetired) {
-      CHECK_EQ(ifields[i].GetDeclaringClass(), this);
+  if (ifields != nullptr) {
+    for (size_t i = 0, count = NumInstanceFields(); i < count; ++i) {
+      if (kIsDebugBuild && IsResolved()) {
+        CHECK_EQ(ifields[i].GetDeclaringClass(), this) << GetStatus();
+      }
+      visitor.VisitRoot(ifields[i].DeclaringClassRoot().AddressWithoutBarrier());
     }
-    visitor.VisitRoot(ifields[i].DeclaringClassRoot().AddressWithoutBarrier());
   }
 }
 
diff --git a/runtime/mirror/object_reference.h b/runtime/mirror/object_reference.h
index 5edda8b346..055be8524c 100644
--- a/runtime/mirror/object_reference.h
+++ b/runtime/mirror/object_reference.h
@@ -91,7 +91,7 @@ class MANAGED HeapReference : public ObjectReference<kPoisonHeapReferences, Mirr
       : ObjectReference<kPoisonHeapReferences, MirrorType>(mirror_ptr) {}
 };
 
-// Standard compressed reference used in the runtime. Used for StackRefernce and GC roots.
+// Standard compressed reference used in the runtime. Used for StackReference and GC roots.
 template<class MirrorType>
 class MANAGED CompressedReference : public mirror::ObjectReference<false, MirrorType> {
  public:
diff --git a/runtime/native/dalvik_system_VMRuntime.cc b/runtime/native/dalvik_system_VMRuntime.cc
index 1a6adf8fc9..196a231892 100644
--- a/runtime/native/dalvik_system_VMRuntime.cc
+++ b/runtime/native/dalvik_system_VMRuntime.cc
@@ -250,8 +250,7 @@ typedef std::map<std::string, mirror::String*> StringTable;
 
 class PreloadDexCachesStringsVisitor : public SingleRootVisitor {
  public:
-  explicit PreloadDexCachesStringsVisitor(StringTable* table) : table_(table) {
-  }
+  explicit PreloadDexCachesStringsVisitor(StringTable* table) : table_(table) { }
 
   void VisitRoot(mirror::Object* root, const RootInfo& info ATTRIBUTE_UNUSED)
       OVERRIDE SHARED_LOCKS_REQUIRED(Locks::mutator_lock_) {