Fix compaction bugs related to IdentityHashCode

IdentityHashCode is a suspend point if monitor inflation occurs.

Change-Id: I114021aed8b3f3437109ef622298de05e13b4e34

4 files changed, 56 insertions, 50 deletions

diff --git a/runtime/debugger.cc b/runtime/debugger.cc
index c63e2d735..169aa9c71 100644
--- a/runtime/debugger.cc
+++ b/runtime/debugger.cc
@@ -105,18 +105,17 @@ class AllocRecordStackTraceElement {
 
 jobject Dbg::TypeCache::Add(mirror::Class* t) {
   ScopedObjectAccessUnchecked soa(Thread::Current());
-  int32_t hash_code = t->IdentityHashCode();
+  JNIEnv* const env = soa.Env();
+  ScopedLocalRef<jobject> local_ref(soa.Env(), soa.AddLocalReference<jobject>(t));
+  const int32_t hash_code = soa.Decode<mirror::Class*>(local_ref.get())->IdentityHashCode();
   auto range = objects_.equal_range(hash_code);
   for (auto it = range.first; it != range.second; ++it) {
-    if (soa.Decode<mirror::Class*>(it->second) == t) {
+    if (soa.Decode<mirror::Class*>(it->second) == soa.Decode<mirror::Class*>(local_ref.get())) {
       // Found a matching weak global, return it.
       return it->second;
     }
   }
-  JNIEnv* env = soa.Env();
-  const jobject local_ref = soa.AddLocalReference<jobject>(t);
-  const jobject weak_global = env->NewWeakGlobalRef(local_ref);
-  env->DeleteLocalRef(local_ref);
+  const jobject weak_global = env->NewWeakGlobalRef(local_ref.get());
   objects_.insert(std::make_pair(hash_code, weak_global));
   return weak_global;
 }
diff --git a/runtime/instrumentation.cc b/runtime/instrumentation.cc
index 42bfe21b2..90115c388 100644
--- a/runtime/instrumentation.cc
+++ b/runtime/instrumentation.cc
@@ -671,12 +671,14 @@ bool Instrumentation::AddDeoptimizedMethod(mirror::ArtMethod* method) {
     return false;
   }
   // Not found. Add it.
+  static_assert(!kMovingMethods, "Not safe if methods can move");
   int32_t hash_code = method->IdentityHashCode();
   deoptimized_methods_.insert(std::make_pair(hash_code, GcRoot<mirror::ArtMethod>(method)));
   return true;
 }
 
 bool Instrumentation::FindDeoptimizedMethod(mirror::ArtMethod* method) {
+  static_assert(!kMovingMethods, "Not safe if methods can move");
   int32_t hash_code = method->IdentityHashCode();
   auto range = deoptimized_methods_.equal_range(hash_code);
   for (auto it = range.first; it != range.second; ++it) {
@@ -700,6 +702,7 @@ mirror::ArtMethod* Instrumentation::BeginDeoptimizedMethod() {
 }
 
 bool Instrumentation::RemoveDeoptimizedMethod(mirror::ArtMethod* method) {
+  static_assert(!kMovingMethods, "Not safe if methods can move");
   int32_t hash_code = method->IdentityHashCode();
   auto range = deoptimized_methods_.equal_range(hash_code);
   for (auto it = range.first; it != range.second; ++it) {
diff --git a/runtime/jobject_comparator.cc b/runtime/jobject_comparator.cc
index 77f93ffa7..1f424b34c 100644
--- a/runtime/jobject_comparator.cc
+++ b/runtime/jobject_comparator.cc
@@ -25,33 +25,32 @@ namespace art {
 
 bool JobjectComparator::operator()(jobject jobj1, jobject jobj2) const {
   // Ensure null references and cleared jweaks appear at the end.
-  if (jobj1 == NULL) {
+  if (jobj1 == nullptr) {
     return true;
-  } else if (jobj2 == NULL) {
+  } else if (jobj2 == nullptr) {
     return false;
   }
   ScopedObjectAccess soa(Thread::Current());
-  mirror::Object* obj1 = soa.Decode<mirror::Object*>(jobj1);
-  mirror::Object* obj2 = soa.Decode<mirror::Object*>(jobj2);
-  if (obj1 == NULL) {
+  StackHandleScope<2> hs(soa.Self());
+  Handle<mirror::Object> obj1(hs.NewHandle(soa.Decode<mirror::Object*>(jobj1)));
+  Handle<mirror::Object> obj2(hs.NewHandle(soa.Decode<mirror::Object*>(jobj2)));
+  if (obj1.Get() == nullptr) {
     return true;
-  } else if (obj2 == NULL) {
+  } else if (obj2.Get() == nullptr) {
     return false;
   }
   // Sort by class...
   if (obj1->GetClass() != obj2->GetClass()) {
     return obj1->GetClass()->IdentityHashCode() < obj2->GetClass()->IdentityHashCode();
-  } else {
-    // ...then by size...
-    size_t count1 = obj1->SizeOf();
-    size_t count2 = obj2->SizeOf();
-    if (count1 != count2) {
-      return count1 < count2;
-    } else {
-      // ...and finally by identity hash code.
-      return obj1->IdentityHashCode() < obj2->IdentityHashCode();
-    }
   }
+  // ...then by size...
+  const size_t count1 = obj1->SizeOf();
+  const size_t count2 = obj2->SizeOf();
+  if (count1 != count2) {
+    return count1 < count2;
+  }
+  // ...and finally by identity hash code.
+  return obj1->IdentityHashCode() < obj2->IdentityHashCode();
 }
 
 }  // namespace art
diff --git a/runtime/reference_table.cc b/runtime/reference_table.cc
index e454b20d7..357d45468 100644
--- a/runtime/reference_table.cc
+++ b/runtime/reference_table.cc
@@ -71,33 +71,6 @@ static size_t GetElementCount(mirror::Object* obj) SHARED_LOCKS_REQUIRED(Locks::
   return obj->AsArray()->GetLength();
 }
 
-struct ObjectComparator {
-  bool operator()(GcRoot<mirror::Object> root1, GcRoot<mirror::Object> root2) const
-    // TODO: enable analysis when analysis can work with the STL.
-      NO_THREAD_SAFETY_ANALYSIS {
-    Locks::mutator_lock_->AssertSharedHeld(Thread::Current());
-    mirror::Object* obj1 = root1.Read<kWithoutReadBarrier>();
-    mirror::Object* obj2 = root2.Read<kWithoutReadBarrier>();
-    DCHECK(obj1 != nullptr);
-    DCHECK(obj2 != nullptr);
-    Runtime* runtime = Runtime::Current();
-    DCHECK(!runtime->IsClearedJniWeakGlobal(obj1));
-    DCHECK(!runtime->IsClearedJniWeakGlobal(obj2));
-    // Sort by class...
-    if (obj1->GetClass() != obj2->GetClass()) {
-      return obj1->GetClass()->IdentityHashCode() < obj2->GetClass()->IdentityHashCode();
-    }
-    // ...then by size...
-    const size_t size1 = obj1->SizeOf();
-    const size_t size2 = obj2->SizeOf();
-    if (size1 != size2) {
-      return size1 < size2;
-    }
-    // ...and finally by identity hash code.
-    return obj1->IdentityHashCode() < obj2->IdentityHashCode();
-  }
-};
-
 // Log an object with some additional info.
 //
 // Pass in the number of elements in the array (or 0 if this is not an
@@ -143,6 +116,38 @@ void ReferenceTable::Dump(std::ostream& os) {
 }
 
 void ReferenceTable::Dump(std::ostream& os, Table& entries) {
+  // Compare GC roots, first by class, then size, then address.
+  struct GcRootComparator {
+    bool operator()(GcRoot<mirror::Object> root1, GcRoot<mirror::Object> root2) const
+      // TODO: enable analysis when analysis can work with the STL.
+        NO_THREAD_SAFETY_ANALYSIS {
+      Locks::mutator_lock_->AssertSharedHeld(Thread::Current());
+      // These GC roots are already forwarded in ReferenceTable::Dump. We sort by class since there
+      // are no suspend points which can happen during the sorting process. This works since
+      // we are guaranteed that the addresses of obj1, obj2, obj1->GetClass, obj2->GetClass wont
+      // change during the sorting process. The classes are forwarded by ref->GetClass().
+      mirror::Object* obj1 = root1.Read<kWithoutReadBarrier>();
+      mirror::Object* obj2 = root2.Read<kWithoutReadBarrier>();
+      DCHECK(obj1 != nullptr);
+      DCHECK(obj2 != nullptr);
+      Runtime* runtime = Runtime::Current();
+      DCHECK(!runtime->IsClearedJniWeakGlobal(obj1));
+      DCHECK(!runtime->IsClearedJniWeakGlobal(obj2));
+      // Sort by class...
+      if (obj1->GetClass() != obj2->GetClass()) {
+        return obj1->GetClass() < obj2->GetClass();
+      }
+      // ...then by size...
+      const size_t size1 = obj1->SizeOf();
+      const size_t size2 = obj2->SizeOf();
+      if (size1 != size2) {
+        return size1 < size2;
+      }
+      // ...and finally by address.
+      return obj1 < obj2;
+    }
+  };
+
   if (entries.empty()) {
     os << "  (empty)\n";
     return;
@@ -201,7 +206,7 @@ void ReferenceTable::Dump(std::ostream& os, Table& entries) {
   if (sorted_entries.empty()) {
     return;
   }
-  std::sort(sorted_entries.begin(), sorted_entries.end(), ObjectComparator());
+  std::sort(sorted_entries.begin(), sorted_entries.end(), GcRootComparator());
 
   // Dump a summary of the whole table.
   os << "  Summary:\n";