Ensure the GC visits Obsolete Methods

We were previously not visiting obsolete methods during GCs. This
could lead to the use of stale pointers.

Bug: 36335999
Test: ./test/testrunner/testrunner.py --host --interp-ac --gcstress -j40

Change-Id: I2b5c7c75b29f9037204a860501fcdb78104b5e7a

diff --git a/runtime/mirror/class-inl.h b/runtime/mirror/class-inl.h
index 2cff47e..6812026 100644 (file)
--- a/runtime/mirror/class-inl.h
+++ b/runtime/mirror/class-inl.h
@@ -29,6 +29,7 @@
 #include "dex_file.h"
 #include "gc/heap-inl.h"
 #include "iftable.h"
+#include "class_ext-inl.h"
 #include "object_array-inl.h"
 #include "read_barrier-inl.h"
 #include "reference-inl.h"
@@ -83,6 +84,12 @@ inline ClassLoader* Class::GetClassLoader() {
 }
 
 template<VerifyObjectFlags kVerifyFlags, ReadBarrierOption kReadBarrierOption>
+inline ClassExt* Class::GetExtData() {
+  return GetFieldObject<ClassExt, kVerifyFlags, kReadBarrierOption>(
+      OFFSET_OF_OBJECT_MEMBER(Class, ext_data_));
+}
+
+template<VerifyObjectFlags kVerifyFlags, ReadBarrierOption kReadBarrierOption>
 inline DexCache* Class::GetDexCache() {
   return GetFieldObject<DexCache, kVerifyFlags, kReadBarrierOption>(
       OFFSET_OF_OBJECT_MEMBER(Class, dex_cache_));
@@ -951,6 +958,10 @@ void Class::VisitNativeRoots(Visitor& visitor, PointerSize pointer_size) {
   for (ArtMethod& method : GetMethods(pointer_size)) {
     method.VisitRoots<kReadBarrierOption>(visitor, pointer_size);
   }
+  ObjPtr<ClassExt> ext(GetExtData<kDefaultVerifyFlags, kReadBarrierOption>());
+  if (!ext.IsNull()) {
+    ext->VisitNativeRoots<kReadBarrierOption, Visitor>(visitor, pointer_size);
+  }
 }
 
 inline IterationRange<StrideIterator<ArtMethod>> Class::GetDirectMethods(PointerSize pointer_size) {

diff --git a/runtime/mirror/class.cc b/runtime/mirror/class.cc
index eb2ec9b..c8ca7b6 100644 (file)
--- a/runtime/mirror/class.cc
+++ b/runtime/mirror/class.cc
@@ -64,10 +64,6 @@ void Class::VisitRoots(RootVisitor* visitor) {
   java_lang_Class_.VisitRootIfNonNull(visitor, RootInfo(kRootStickyClass));
 }
 
-ClassExt* Class::GetExtData() {
-  return GetFieldObject<ClassExt>(OFFSET_OF_OBJECT_MEMBER(Class, ext_data_));
-}
-
 ClassExt* Class::EnsureExtDataPresent(Thread* self) {
   ObjPtr<ClassExt> existing(GetExtData());
   if (!existing.IsNull()) {

diff --git a/runtime/mirror/class.h b/runtime/mirror/class.h
index c52b66a..c294aa1 100644 (file)
--- a/runtime/mirror/class.h
+++ b/runtime/mirror/class.h
@@ -1162,6 +1162,8 @@ class MANAGED Class FINAL : public Object {
 
   void SetClinitThreadId(pid_t new_clinit_thread_id) REQUIRES_SHARED(Locks::mutator_lock_);
 
+  template<VerifyObjectFlags kVerifyFlags = kDefaultVerifyFlags,
+           ReadBarrierOption kReadBarrierOption = kWithReadBarrier>
   ClassExt* GetExtData() REQUIRES_SHARED(Locks::mutator_lock_);
 
   // Returns the ExtData for this class, allocating one if necessary. This should be the only way

diff --git a/runtime/mirror/class_ext-inl.h b/runtime/mirror/class_ext-inl.h
new file mode 100644 (file)
index 0000000..feaac85
--- /dev/null
+++ b/runtime/mirror/class_ext-inl.h
@@ -0,0 +1,47 @@
+/*
+ * Copyright (C) 2017 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#ifndef ART_RUNTIME_MIRROR_CLASS_EXT_INL_H_
+#define ART_RUNTIME_MIRROR_CLASS_EXT_INL_H_
+
+#include "class_ext.h"
+
+#include "art_method-inl.h"
+
+namespace art {
+namespace mirror {
+
+template<ReadBarrierOption kReadBarrierOption, class Visitor>
+void ClassExt::VisitNativeRoots(Visitor& visitor, PointerSize pointer_size) {
+  ObjPtr<PointerArray> arr(GetObsoleteMethods<kDefaultVerifyFlags, kReadBarrierOption>());
+  if (arr.IsNull()) {
+    return;
+  }
+  int32_t len = arr->GetLength();
+  for (int32_t i = 0; i < len; i++) {
+    ArtMethod* method = arr->GetElementPtrSize<ArtMethod*,
+                                               kDefaultVerifyFlags,
+                                               kReadBarrierOption>(i, pointer_size);
+    if (method != nullptr) {
+      method->VisitRoots<kReadBarrierOption>(visitor, pointer_size);
+    }
+  }
+}
+
+}  // namespace mirror
+}  // namespace art
+
+#endif  // ART_RUNTIME_MIRROR_CLASS_EXT_INL_H_

diff --git a/runtime/mirror/class_ext.cc b/runtime/mirror/class_ext.cc
index 7270079..5dc3aca 100644 (file)
--- a/runtime/mirror/class_ext.cc
+++ b/runtime/mirror/class_ext.cc
@@ -14,7 +14,7 @@
  * limitations under the License.
  */
 
-#include "class_ext.h"
+#include "class_ext-inl.h"
 
 #include "art_method-inl.h"
 #include "base/casts.h"
@@ -24,7 +24,6 @@
 #include "gc/accounting/card_table-inl.h"
 #include "object-inl.h"
 #include "object_array.h"
-#include "object_array-inl.h"
 #include "stack_trace_element.h"
 #include "utils.h"
 #include "well_known_classes.h"
@@ -34,6 +33,11 @@ namespace mirror {
 
 GcRoot<Class> ClassExt::dalvik_system_ClassExt_;
 
+uint32_t ClassExt::ClassSize(PointerSize pointer_size) {
+  uint32_t vtable_entries = Object::kVTableLength;
+  return Class::ComputeClassSize(true, vtable_entries, 0, 0, 0, 0, 0, pointer_size);
+}
+
 void ClassExt::SetObsoleteArrays(ObjPtr<PointerArray> methods,
                                  ObjPtr<ObjectArray<DexCache>> dex_caches) {
   DCHECK_EQ(GetLockOwnerThreadId(), Thread::Current()->GetThreadId())

diff --git a/runtime/mirror/class_ext.h b/runtime/mirror/class_ext.h
index ad8a61b..fac955a 100644 (file)
--- a/runtime/mirror/class_ext.h
+++ b/runtime/mirror/class_ext.h
@@ -17,9 +17,8 @@
 #ifndef ART_RUNTIME_MIRROR_CLASS_EXT_H_
 #define ART_RUNTIME_MIRROR_CLASS_EXT_H_
 
-#include "class-inl.h"
-
 #include "array.h"
+#include "class.h"
 #include "dex_cache.h"
 #include "gc_root.h"
 #include "object.h"
@@ -36,10 +35,7 @@ namespace mirror {
 // C++ mirror of dalvik.system.ClassExt
 class MANAGED ClassExt : public Object {
  public:
-  static uint32_t ClassSize(PointerSize pointer_size) {
-    uint32_t vtable_entries = Object::kVTableLength;
-    return Class::ComputeClassSize(true, vtable_entries, 0, 0, 0, 0, 0, pointer_size);
-  }
+  static uint32_t ClassSize(PointerSize pointer_size);
 
   // Size of an instance of dalvik.system.ClassExt.
   static constexpr uint32_t InstanceSize() {
@@ -57,8 +53,11 @@ class MANAGED ClassExt : public Object {
         OFFSET_OF_OBJECT_MEMBER(ClassExt, obsolete_dex_caches_));
   }
 
-  PointerArray* GetObsoleteMethods() REQUIRES_SHARED(Locks::mutator_lock_) {
-    return GetFieldObject<PointerArray>(OFFSET_OF_OBJECT_MEMBER(ClassExt, obsolete_methods_));
+  template<VerifyObjectFlags kVerifyFlags = kDefaultVerifyFlags,
+           ReadBarrierOption kReadBarrierOption = kWithReadBarrier>
+  inline PointerArray* GetObsoleteMethods() REQUIRES_SHARED(Locks::mutator_lock_) {
+    return GetFieldObject<PointerArray, kVerifyFlags, kReadBarrierOption>(
+        OFFSET_OF_OBJECT_MEMBER(ClassExt, obsolete_methods_));
   }
 
   ByteArray* GetOriginalDexFileBytes() REQUIRES_SHARED(Locks::mutator_lock_) {
@@ -78,6 +77,10 @@ class MANAGED ClassExt : public Object {
   static void ResetClass();
   static void VisitRoots(RootVisitor* visitor) REQUIRES_SHARED(Locks::mutator_lock_);
 
+  template<ReadBarrierOption kReadBarrierOption = kWithReadBarrier, class Visitor>
+  inline void VisitNativeRoots(Visitor& visitor, PointerSize pointer_size)
+      REQUIRES_SHARED(Locks::mutator_lock_);
+
   static ClassExt* Alloc(Thread* self) REQUIRES_SHARED(Locks::mutator_lock_);
 
  private:

diff --git a/test/Android.run-test.mk b/test/Android.run-test.mk
index 051ee58..5dae9cf 100644 (file)
--- a/test/Android.run-test.mk
+++ b/test/Android.run-test.mk
@@ -374,7 +374,6 @@ TEST_ART_BROKEN_INTERPRETER_ACCESS_CHECK_TESTS :=
 # * 961-default-iface-resolution-gen and 964-default-iface-init-genare very long tests that often
 #   will take more than the timeout to run when gcstress is enabled. This is because gcstress
 #   slows down allocations significantly which these tests do a lot.
-# * 946-obsolete-throw: b/36335999.
 TEST_ART_BROKEN_GCSTRESS_RUN_TESTS := \
   137-cfi \
   152-dead-large-object \
@@ -383,7 +382,6 @@ TEST_ART_BROKEN_GCSTRESS_RUN_TESTS := \
   913-heaps \
   961-default-iface-resolution-gen \
   964-default-iface-init-gen \
-  946-obsolete-throw
 
 ifneq (,$(filter gcstress,$(GC_TYPES)))
   ART_TEST_KNOWN_BROKEN += $(call all-run-test-names,$(TARGET_TYPES),$(RUN_TYPES),$(PREBUILD_TYPES), \

diff --git a/test/knownfailures.json b/test/knownfailures.json
index 5923ebd..7a7d0e2 100644 (file)
--- a/test/knownfailures.json
+++ b/test/knownfailures.json
@@ -343,11 +343,6 @@
         "variant": "interpreter | optimizing | regalloc_gc | jit"
     },
     {
-        "tests": "946-obsolete-throw",
-        "bug": "http://b/36335999",
-        "variant": "interp-ac & gcstress"
-    },
-    {
         "tests": ["912-classes",
                   "616-cha",
                   "616-cha-abstract"],