Merge "ART: Report array and proxy classes in ClassLoad & ClassPrepare" into oc-dev
diff --git a/runtime/mirror/class_ext.cc b/runtime/mirror/class_ext.cc
index 94e4b88..32d49bb 100644
--- a/runtime/mirror/class_ext.cc
+++ b/runtime/mirror/class_ext.cc
@@ -40,8 +40,6 @@
 
 void ClassExt::SetObsoleteArrays(ObjPtr<PointerArray> methods,
                                  ObjPtr<ObjectArray<DexCache>> dex_caches) {
-  DCHECK_EQ(GetLockOwnerThreadId(), Thread::Current()->GetThreadId())
-      << "Obsolete arrays are set without synchronization!";
   CHECK_EQ(methods.IsNull(), dex_caches.IsNull());
   auto obsolete_dex_cache_off = OFFSET_OF_OBJECT_MEMBER(ClassExt, obsolete_dex_caches_);
   auto obsolete_methods_off = OFFSET_OF_OBJECT_MEMBER(ClassExt, obsolete_methods_);
@@ -54,8 +52,7 @@
 // these arrays are written into without all threads being suspended we have a race condition! This
 // race could cause obsolete methods to be missed.
 bool ClassExt::ExtendObsoleteArrays(Thread* self, uint32_t increase) {
-  DCHECK_EQ(GetLockOwnerThreadId(), Thread::Current()->GetThreadId())
-      << "Obsolete arrays are set without synchronization!";
+  // TODO It would be good to check that we have locked the class associated with this ClassExt.
   StackHandleScope<5> hs(self);
   Handle<ClassExt> h_this(hs.NewHandle(this));
   Handle<PointerArray> old_methods(hs.NewHandle(h_this->GetObsoleteMethods()));
diff --git a/runtime/openjdkjvmti/ti_redefine.cc b/runtime/openjdkjvmti/ti_redefine.cc
index 7d95de8..0655079 100644
--- a/runtime/openjdkjvmti/ti_redefine.cc
+++ b/runtime/openjdkjvmti/ti_redefine.cc
@@ -216,7 +216,6 @@
 jvmtiError Redefiner::IsModifiableClass(jvmtiEnv* env ATTRIBUTE_UNUSED,
                                         jclass klass,
                                         jboolean* is_redefinable) {
-  // TODO Check for the appropriate feature flags once we have enabled them.
   art::Thread* self = art::Thread::Current();
   art::ScopedObjectAccess soa(self);
   art::StackHandleScope<1> hs(self);
@@ -790,9 +789,11 @@
     kSlotNewDexCache = 3,
     kSlotMirrorClass = 4,
     kSlotOrigDexFile = 5,
+    kSlotOldObsoleteMethods = 6,
+    kSlotOldDexCaches = 7,
 
     // Must be last one.
-    kNumSlots = 6,
+    kNumSlots = 8,
   };
 
   // This needs to have a HandleScope passed in that is capable of creating a new Handle without
@@ -815,7 +816,6 @@
     return arr_.IsNull();
   }
 
-  // TODO Maybe make an iterable view type to simplify using this.
   art::mirror::ClassLoader* GetSourceClassLoader(jint klass_index) const
       REQUIRES_SHARED(art::Locks::mutator_lock_) {
     return art::down_cast<art::mirror::ClassLoader*>(GetSlot(klass_index, kSlotSourceClassLoader));
@@ -842,6 +842,18 @@
     return art::down_cast<art::mirror::Object*>(GetSlot(klass_index, kSlotOrigDexFile));
   }
 
+  art::mirror::PointerArray* GetOldObsoleteMethods(jint klass_index) const
+      REQUIRES_SHARED(art::Locks::mutator_lock_) {
+    return art::down_cast<art::mirror::PointerArray*>(
+        GetSlot(klass_index, kSlotOldObsoleteMethods));
+  }
+
+  art::mirror::ObjectArray<art::mirror::DexCache>* GetOldDexCaches(jint klass_index) const
+      REQUIRES_SHARED(art::Locks::mutator_lock_) {
+    return art::down_cast<art::mirror::ObjectArray<art::mirror::DexCache>*>(
+        GetSlot(klass_index, kSlotOldDexCaches));
+  }
+
   void SetSourceClassLoader(jint klass_index, art::mirror::ClassLoader* loader)
       REQUIRES_SHARED(art::Locks::mutator_lock_) {
     SetSlot(klass_index, kSlotSourceClassLoader, loader);
@@ -866,6 +878,14 @@
       REQUIRES_SHARED(art::Locks::mutator_lock_) {
     SetSlot(klass_index, kSlotOrigDexFile, bytes);
   }
+  void SetOldObsoleteMethods(jint klass_index, art::mirror::PointerArray* methods)
+      REQUIRES_SHARED(art::Locks::mutator_lock_) {
+    SetSlot(klass_index, kSlotOldObsoleteMethods, methods);
+  }
+  void SetOldDexCaches(jint klass_index, art::mirror::ObjectArray<art::mirror::DexCache>* caches)
+      REQUIRES_SHARED(art::Locks::mutator_lock_) {
+    SetSlot(klass_index, kSlotOldDexCaches, caches);
+  }
 
   int32_t Length() const REQUIRES_SHARED(art::Locks::mutator_lock_) {
     return arr_->GetLength() / kNumSlots;
@@ -979,6 +999,15 @@
       REQUIRES_SHARED(art::Locks::mutator_lock_) {
     return holder_.GetOriginalDexFile(idx_);
   }
+  art::mirror::PointerArray* GetOldObsoleteMethods() const
+      REQUIRES_SHARED(art::Locks::mutator_lock_) {
+    return holder_.GetOldObsoleteMethods(idx_);
+  }
+  art::mirror::ObjectArray<art::mirror::DexCache>* GetOldDexCaches() const
+      REQUIRES_SHARED(art::Locks::mutator_lock_) {
+    return holder_.GetOldDexCaches(idx_);
+  }
+
   int32_t GetIndex() const {
     return idx_;
   }
@@ -1004,6 +1033,14 @@
       REQUIRES_SHARED(art::Locks::mutator_lock_) {
     holder_.SetOriginalDexFile(idx_, bytes);
   }
+  void SetOldObsoleteMethods(art::mirror::PointerArray* methods)
+      REQUIRES_SHARED(art::Locks::mutator_lock_) {
+    holder_.SetOldObsoleteMethods(idx_, methods);
+  }
+  void SetOldDexCaches(art::mirror::ObjectArray<art::mirror::DexCache>* caches)
+      REQUIRES_SHARED(art::Locks::mutator_lock_) {
+    holder_.SetOldDexCaches(idx_, caches);
+  }
 
  private:
   int32_t idx_;
@@ -1164,9 +1201,15 @@
   return true;
 }
 
-bool Redefiner::EnsureAllClassAllocationsFinished() {
-  for (Redefiner::ClassRedefinition& redef : redefinitions_) {
-    if (!redef.EnsureClassAllocationsFinished()) {
+void Redefiner::RestoreObsoleteMethodMapsIfUnneeded(RedefinitionDataHolder& holder) {
+  for (RedefinitionDataIter data = holder.begin(); data != holder.end(); ++data) {
+    data.GetRedefinition().RestoreObsoleteMethodMapsIfUnneeded(&data);
+  }
+}
+
+bool Redefiner::EnsureAllClassAllocationsFinished(RedefinitionDataHolder& holder) {
+  for (RedefinitionDataIter data = holder.begin(); data != holder.end(); ++data) {
+    if (!data.GetRedefinition().EnsureClassAllocationsFinished(&data)) {
       return false;
     }
   }
@@ -1239,13 +1282,9 @@
   // between allocating them and pausing all threads before we can update them so we need to do a
   // try loop.
   if (!CheckAllRedefinitionAreValid() ||
-      !EnsureAllClassAllocationsFinished() ||
+      !EnsureAllClassAllocationsFinished(holder) ||
       !FinishAllRemainingAllocations(holder) ||
       !CheckAllClassesAreVerified(holder)) {
-    // TODO Null out the ClassExt fields we allocated (if possible, might be racing with another
-    // redefineclass call which made it even bigger. Leak shouldn't be huge (2x array of size
-    // declared_methods_.length) but would be good to get rid of. All other allocations should be
-    // cleaned up by the GC eventually.
     return result_;
   }
 
@@ -1277,11 +1316,11 @@
     redef.FindAndAllocateObsoleteMethods(klass);
     redef.UpdateClass(klass, data.GetNewDexCache(), data.GetOriginalDexFile());
   }
+  RestoreObsoleteMethodMapsIfUnneeded(holder);
   // TODO We should check for if any of the redefined methods are intrinsic methods here and, if any
   // are, force a full-world deoptimization before finishing redefinition. If we don't do this then
   // methods that have been jitted prior to the current redefinition being applied might continue
   // to use the old versions of the intrinsics!
-  // TODO Shrink the obsolete method maps if possible?
   // TODO Do the dex_file release at a more reasonable place. This works but it muddles who really
   // owns the DexFile and when ownership is transferred.
   ReleaseAllDexFiles();
@@ -1372,9 +1411,35 @@
   ext->SetOriginalDexFile(original_dex_file);
 }
 
+// Restores the old obsolete methods maps if it turns out they weren't needed (ie there were no new
+// obsolete methods).
+void Redefiner::ClassRedefinition::RestoreObsoleteMethodMapsIfUnneeded(
+    const RedefinitionDataIter* cur_data) {
+  art::mirror::Class* klass = GetMirrorClass();
+  art::mirror::ClassExt* ext = klass->GetExtData();
+  art::mirror::PointerArray* methods = ext->GetObsoleteMethods();
+  int32_t old_length =
+      cur_data->GetOldDexCaches() == nullptr ? 0 : cur_data->GetOldDexCaches()->GetLength();
+  int32_t expected_length =
+      old_length + klass->NumDirectMethods() + klass->NumDeclaredVirtualMethods();
+  // Check to make sure we are only undoing this one.
+  if (expected_length == methods->GetLength()) {
+    for (int32_t i = old_length; i < expected_length; i++) {
+      if (methods->GetElementPtrSize<art::ArtMethod*>(i, art::kRuntimePointerSize) != nullptr) {
+        // We actually have some new obsolete methods. Just abort since we cannot safely shrink the
+        // obsolete methods array.
+        return;
+      }
+    }
+    // No new obsolete methods! We can get rid of the maps.
+    ext->SetObsoleteArrays(cur_data->GetOldObsoleteMethods(), cur_data->GetOldDexCaches());
+  }
+}
+
 // This function does all (java) allocations we need to do for the Class being redefined.
 // TODO Change this name maybe?
-bool Redefiner::ClassRedefinition::EnsureClassAllocationsFinished() {
+bool Redefiner::ClassRedefinition::EnsureClassAllocationsFinished(
+    /*out*/RedefinitionDataIter* cur_data) {
   art::StackHandleScope<2> hs(driver_->self_);
   art::Handle<art::mirror::Class> klass(hs.NewHandle(
       driver_->self_->DecodeJObject(klass_)->AsClass()));
@@ -1391,22 +1456,20 @@
     RecordFailure(ERR(OUT_OF_MEMORY), "Could not allocate ClassExt");
     return false;
   }
-  // Allocate the 2 arrays that make up the obsolete methods map.  Since the contents of the arrays
+  // First save the old values of the 2 arrays that make up the obsolete methods maps.  Then
+  // allocate the 2 arrays that make up the obsolete methods map.  Since the contents of the arrays
   // are only modified when all threads (other than the modifying one) are suspended we don't need
   // to worry about missing the unsyncronized writes to the array. We do synchronize when setting it
   // however, since that can happen at any time.
-  // TODO Clear these after we walk the stacks in order to free them in the (likely?) event there
-  // are no obsolete methods.
-  {
-    art::ObjectLock<art::mirror::ClassExt> lock(driver_->self_, ext);
-    if (!ext->ExtendObsoleteArrays(
-          driver_->self_, klass->GetDeclaredMethodsSlice(art::kRuntimePointerSize).size())) {
-      // OOM. Clear exception and return error.
-      driver_->self_->AssertPendingOOMException();
-      driver_->self_->ClearException();
-      RecordFailure(ERR(OUT_OF_MEMORY), "Unable to allocate/extend obsolete methods map");
-      return false;
-    }
+  cur_data->SetOldObsoleteMethods(ext->GetObsoleteMethods());
+  cur_data->SetOldDexCaches(ext->GetObsoleteDexCaches());
+  if (!ext->ExtendObsoleteArrays(
+        driver_->self_, klass->GetDeclaredMethodsSlice(art::kRuntimePointerSize).size())) {
+    // OOM. Clear exception and return error.
+    driver_->self_->AssertPendingOOMException();
+    driver_->self_->ClearException();
+    RecordFailure(ERR(OUT_OF_MEMORY), "Unable to allocate/extend obsolete methods map");
+    return false;
   }
   return true;
 }
diff --git a/runtime/openjdkjvmti/ti_redefine.h b/runtime/openjdkjvmti/ti_redefine.h
index 809a681..586259a 100644
--- a/runtime/openjdkjvmti/ti_redefine.h
+++ b/runtime/openjdkjvmti/ti_redefine.h
@@ -166,7 +166,8 @@
     // Preallocates all needed allocations in klass so that we can pause execution safely.
     // TODO We should be able to free the arrays if they end up not being used. Investigate doing
     // this in the future. For now we will just take the memory hit.
-    bool EnsureClassAllocationsFinished() REQUIRES_SHARED(art::Locks::mutator_lock_);
+    bool EnsureClassAllocationsFinished(/*out*/RedefinitionDataIter* data)
+        REQUIRES_SHARED(art::Locks::mutator_lock_);
 
     // This will check that no constraints are violated (more than 1 class in dex file, any changes
     // in number/declaration of methods & fields, changes in access flags, etc.)
@@ -198,6 +199,9 @@
                      art::ObjPtr<art::mirror::Object> original_dex_file)
         REQUIRES(art::Locks::mutator_lock_);
 
+    void RestoreObsoleteMethodMapsIfUnneeded(const RedefinitionDataIter* cur_data)
+        REQUIRES(art::Locks::mutator_lock_);
+
     void ReleaseDexFile() REQUIRES_SHARED(art::Locks::mutator_lock_);
 
     void UnregisterBreakpoints() REQUIRES_SHARED(art::Locks::mutator_lock_);
@@ -241,11 +245,16 @@
   bool CheckAllRedefinitionAreValid() REQUIRES_SHARED(art::Locks::mutator_lock_);
   bool CheckAllClassesAreVerified(RedefinitionDataHolder& holder)
       REQUIRES_SHARED(art::Locks::mutator_lock_);
-  bool EnsureAllClassAllocationsFinished() REQUIRES_SHARED(art::Locks::mutator_lock_);
+  bool EnsureAllClassAllocationsFinished(RedefinitionDataHolder& holder)
+      REQUIRES_SHARED(art::Locks::mutator_lock_);
   bool FinishAllRemainingAllocations(RedefinitionDataHolder& holder)
       REQUIRES_SHARED(art::Locks::mutator_lock_);
   void ReleaseAllDexFiles() REQUIRES_SHARED(art::Locks::mutator_lock_);
   void UnregisterAllBreakpoints() REQUIRES_SHARED(art::Locks::mutator_lock_);
+  // Restores the old obsolete methods maps if it turns out they weren't needed (ie there were no
+  // new obsolete methods).
+  void RestoreObsoleteMethodMapsIfUnneeded(RedefinitionDataHolder& holder)
+      REQUIRES(art::Locks::mutator_lock_);
 
   void RecordFailure(jvmtiError result, const std::string& class_sig, const std::string& error_msg);
   void RecordFailure(jvmtiError result, const std::string& error_msg) {