Merge "Remove disable card marks, fix SetPatchLocation." into dalvik-dev

6 files changed, 87 insertions, 62 deletions

diff --git a/src/class_linker.cc b/src/class_linker.cc
index 75f0f38158..abb6dcf8f5 100644
--- a/src/class_linker.cc
+++ b/src/class_linker.cc
@@ -1461,6 +1461,42 @@ const OatFile::OatClass* ClassLinker::GetOatClass(const DexFile& dex_file, const
   return oat_class;
 }
 
+static uint32_t GetOatMethodIndexFromMethodIndex(const DexFile& dex_file, uint32_t method_idx) {
+  const DexFile::MethodId& method_id = dex_file.GetMethodId(method_idx);
+  const DexFile::TypeId& type_id = dex_file.GetTypeId(method_id.class_idx_);
+  const DexFile::ClassDef* class_def = dex_file.FindClassDef(dex_file.GetTypeDescriptor(type_id));
+  CHECK(class_def != NULL);
+  const byte* class_data = dex_file.GetClassData(*class_def);
+  CHECK(class_data != NULL);
+  ClassDataItemIterator it(dex_file, class_data);
+  // Skip fields
+  while (it.HasNextStaticField()) {
+    it.Next();
+  }
+  while (it.HasNextInstanceField()) {
+    it.Next();
+  }
+  // Process methods
+  size_t class_def_method_index = 0;
+  while (it.HasNextDirectMethod()) {
+    if (it.GetMemberIndex() == method_idx) {
+      return class_def_method_index;
+    }
+    class_def_method_index++;
+    it.Next();
+  }
+  while (it.HasNextVirtualMethod()) {
+    if (it.GetMemberIndex() == method_idx) {
+      return class_def_method_index;
+    }
+    class_def_method_index++;
+    it.Next();
+  }
+  DCHECK(!it.HasNext());
+  LOG(FATAL) << "Failed to find method index " << method_idx << " in " << dex_file.GetLocation();
+  return 0;
+}
+
 const OatFile::OatMethod ClassLinker::GetOatMethodFor(const AbstractMethod* method) {
   // Although we overwrite the trampoline of non-static methods, we may get here via the resolution
   // method for direct methods (or virtual methods made direct).
@@ -1487,6 +1523,10 @@ const OatFile::OatMethod ClassLinker::GetOatMethodFor(const AbstractMethod* meth
   ClassHelper kh(declaring_class);
   UniquePtr<const OatFile::OatClass> oat_class(GetOatClass(kh.GetDexFile(), kh.GetDescriptor()));
   CHECK(oat_class.get() != NULL);
+  DCHECK_EQ(oat_method_index,
+            GetOatMethodIndexFromMethodIndex(*declaring_class->GetDexCache()->GetDexFile(),
+                                             method->GetDexMethodIndex()));
+
   return oat_class->GetOatMethod(oat_method_index);
 }
 
@@ -1496,6 +1536,15 @@ const void* ClassLinker::GetOatCodeFor(const AbstractMethod* method) {
   return GetOatMethodFor(method).GetCode();
 }
 
+const void* ClassLinker::GetOatCodeFor(const DexFile& dex_file, uint32_t method_idx) {
+  const DexFile::MethodId& method_id = dex_file.GetMethodId(method_idx);
+  const char* descriptor = dex_file.GetTypeDescriptor(dex_file.GetTypeId(method_id.class_idx_));
+  uint32_t oat_method_idx = GetOatMethodIndexFromMethodIndex(dex_file, method_idx);
+  UniquePtr<const OatFile::OatClass> oat_class(GetOatClass(dex_file, descriptor));
+  CHECK(oat_class.get() != NULL);
+  return oat_class->GetOatMethod(oat_method_idx).GetCode();
+}
+
 void ClassLinker::FixupStaticTrampolines(Class* klass) {
   ClassHelper kh(klass);
   const DexFile::ClassDef* dex_class_def = kh.GetClassDef();
diff --git a/src/class_linker.h b/src/class_linker.h
index f1530f3ec8..14c719e8fe 100644
--- a/src/class_linker.h
+++ b/src/class_linker.h
@@ -372,6 +372,9 @@ class ClassLinker {
   const void* GetOatCodeFor(const AbstractMethod* method)
       SHARED_LOCKS_REQUIRED(Locks::mutator_lock_);
 
+  // Get the oat code for a method from a method index.
+  const void* GetOatCodeFor(const DexFile& dex_file, uint32_t method_idx);
+
   // Relocate the OatFiles (ELF images)
   void RelocateExecutable() LOCKS_EXCLUDED(dex_lock_);
 
diff --git a/src/heap.cc b/src/heap.cc
index 2ed467f2d4..cf13eaec9a 100644
--- a/src/heap.cc
+++ b/src/heap.cc
@@ -137,7 +137,6 @@ Heap::Heap(size_t initial_size, size_t growth_limit, size_t min_free, size_t max
       card_table_(NULL),
       concurrent_gc_(concurrent_gc),
       have_zygote_space_(false),
-      card_marking_disabled_(false),
       is_gc_running_(false),
       last_gc_type_(kGcTypeNone),
       enforce_heap_growth_rate_(false),
diff --git a/src/heap.h b/src/heap.h
index 984a3296b2..8ed5881624 100644
--- a/src/heap.h
+++ b/src/heap.h
@@ -230,28 +230,19 @@ class Heap {
   // Must be called if a field of an Object in the heap changes, and before any GC safe-point.
   // The call is not needed if NULL is stored in the field.
   void WriteBarrierField(const Object* dst, MemberOffset /*offset*/, const Object* /*new_value*/) {
-    if (!card_marking_disabled_) {
-      card_table_->MarkCard(dst);
-    }
+    card_table_->MarkCard(dst);
   }
 
   // Write barrier for array operations that update many field positions
   void WriteBarrierArray(const Object* dst, int /*start_offset*/,
                          size_t /*length TODO: element_count or byte_count?*/) {
-    if (UNLIKELY(!card_marking_disabled_)) {
-      card_table_->MarkCard(dst);
-    }
+    card_table_->MarkCard(dst);
   }
 
   CardTable* GetCardTable() {
     return card_table_.get();
   }
 
-  void DisableCardMarking() {
-    // TODO: we shouldn't need to disable card marking, this is here to help the image_writer
-    card_marking_disabled_ = true;
-  }
-
   void AddFinalizerReference(Thread* self, Object* object);
 
   size_t GetBytesAllocated() const;
@@ -412,10 +403,6 @@ class Heap {
   // If we have a zygote space.
   bool have_zygote_space_;
 
-  // Used by the image writer to disable card marking on copied objects
-  // TODO: remove
-  bool card_marking_disabled_;
-
   // Guards access to the state of GC, associated conditional variable is used to signal when a GC
   // completes.
   Mutex* gc_complete_lock_ DEFAULT_MUTEX_ACQUIRED_AFTER;
diff --git a/src/image_writer.cc b/src/image_writer.cc
index 497ca2200e..642cee6a63 100644
--- a/src/image_writer.cc
+++ b/src/image_writer.cc
@@ -99,14 +99,11 @@ bool ImageWriter::Write(const std::string& image_filename,
     CheckNonImageClassesRemoved();
   }
 #endif
-  heap->DisableCardMarking();
-  {
-    Thread::Current()->TransitionFromSuspendedToRunnable();
-    CalculateNewObjectOffsets();
-    CopyAndFixupObjects();
-    PatchOatCodeAndMethods(compiler);
-    Thread::Current()->TransitionFromRunnableToSuspended(kNative);
-  }
+  Thread::Current()->TransitionFromSuspendedToRunnable();
+  CalculateNewObjectOffsets();
+  CopyAndFixupObjects();
+  PatchOatCodeAndMethods(compiler);
+  Thread::Current()->TransitionFromRunnableToSuspended(kNative);
 
   UniquePtr<File> file(OS::OpenFile(image_filename.c_str(), true));
   if (file.get() == NULL) {
@@ -192,7 +189,7 @@ void ImageWriter::ComputeEagerResolvedStrings()
     SHARED_LOCKS_REQUIRED(Locks::mutator_lock_) {
   // TODO: Check image spaces only?
   Heap* heap = Runtime::Current()->GetHeap();
-  ReaderMutexLock mu(Thread::Current(), *Locks::heap_bitmap_lock_);
+  WriterMutexLock mu(Thread::Current(), *Locks::heap_bitmap_lock_);
   heap->FlushAllocStack();
   heap->GetLiveBitmap()->Walk(ComputeEagerResolvedStringsCallback, this);
 }
@@ -397,11 +394,8 @@ void ImageWriter::CalculateNewObjectOffsets() {
   image_end_ += RoundUp(sizeof(ImageHeader), 8); // 64-bit-alignment
 
   {
-    ReaderMutexLock mu(self, *Locks::heap_bitmap_lock_);
+    WriterMutexLock mu(self, *Locks::heap_bitmap_lock_);
     heap->FlushAllocStack();
-  }
-
-  {
     // TODO: Image spaces only?
     // TODO: Add InOrderWalk to heap bitmap.
     const char* old = self->StartAssertNoThreadSuspension("ImageWriter");
@@ -434,7 +428,7 @@ void ImageWriter::CopyAndFixupObjects()
   // TODO: heap validation can't handle this fix up pass
   heap->DisableObjectValidation();
   // TODO: Image spaces only?
-  ReaderMutexLock mu(self, *Locks::heap_bitmap_lock_);
+  WriterMutexLock mu(self, *Locks::heap_bitmap_lock_);
   heap->FlushAllocStack();
   heap->GetLiveBitmap()->Walk(CopyAndFixupObjectsCallback, this);
   self->EndAssertNoThreadSuspension(old_cause);
@@ -553,7 +547,7 @@ void ImageWriter::FixupMethod(const AbstractMethod* orig, AbstractMethod* copy)
 void ImageWriter::FixupObjectArray(const ObjectArray<Object>* orig, ObjectArray<Object>* copy) {
   for (int32_t i = 0; i < orig->GetLength(); ++i) {
     const Object* element = orig->Get(i);
-    copy->SetWithoutChecks(i, GetImageAddress(element));
+    copy->SetPtrWithoutChecks(i, GetImageAddress(element));
   }
 }
 
@@ -587,7 +581,8 @@ void ImageWriter::FixupFields(const Object* orig,
       size_t right_shift = CLZ(ref_offsets);
       MemberOffset byte_offset = CLASS_OFFSET_FROM_CLZ(right_shift);
       const Object* ref = orig->GetFieldObject<const Object*>(byte_offset, false);
-      copy->SetFieldObject(byte_offset, GetImageAddress(ref), false);
+      // Use SetFieldPtr to avoid card marking since we are writing to the image.
+      copy->SetFieldPtr(byte_offset, GetImageAddress(ref), false);
       ref_offsets &= ~(CLASS_HIGH_BIT >> right_shift);
     }
   } else {
@@ -607,34 +602,13 @@ void ImageWriter::FixupFields(const Object* orig,
                         : klass->GetInstanceField(i));
         MemberOffset field_offset = field->GetOffset();
         const Object* ref = orig->GetFieldObject<const Object*>(field_offset, false);
-        copy->SetFieldObject(field_offset, GetImageAddress(ref), false);
+        // Use SetFieldPtr to avoid card marking since we are writing to the image.
+        copy->SetFieldPtr(field_offset, GetImageAddress(ref), false);
       }
     }
   }
 }
 
-static AbstractMethod* GetReferrerMethod(const Compiler::PatchInformation* patch)
-    SHARED_LOCKS_REQUIRED(Locks::mutator_lock_) {
-  ScopedObjectAccessUnchecked soa(Thread::Current());
-  ClassLinker* class_linker = Runtime::Current()->GetClassLinker();
-  DexCache* dex_cache = class_linker->FindDexCache(patch->GetDexFile());
-  AbstractMethod* method = class_linker->ResolveMethod(patch->GetDexFile(),
-                                               patch->GetReferrerMethodIdx(),
-                                               dex_cache,
-                                               NULL,
-                                               NULL,
-                                               patch->GetReferrerInvokeType());
-  CHECK(method != NULL)
-    << patch->GetDexFile().GetLocation() << " " << patch->GetReferrerMethodIdx();
-  CHECK(!method->IsRuntimeMethod())
-    << patch->GetDexFile().GetLocation() << " " << patch->GetReferrerMethodIdx();
-  CHECK(dex_cache->GetResolvedMethods()->Get(patch->GetReferrerMethodIdx()) == method)
-    << patch->GetDexFile().GetLocation() << " " << patch->GetReferrerMethodIdx() << " "
-    << PrettyMethod(dex_cache->GetResolvedMethods()->Get(patch->GetReferrerMethodIdx())) << " "
-    << PrettyMethod(method);
-  return method;
-}
-
 static AbstractMethod* GetTargetMethod(const Compiler::PatchInformation* patch)
     SHARED_LOCKS_REQUIRED(Locks::mutator_lock_) {
   ClassLinker* class_linker = Runtime::Current()->GetClassLinker();
@@ -657,9 +631,12 @@ static AbstractMethod* GetTargetMethod(const Compiler::PatchInformation* patch)
 }
 
 void ImageWriter::PatchOatCodeAndMethods(const Compiler& compiler) {
+  Thread* self = Thread::Current();
   ClassLinker* class_linker = Runtime::Current()->GetClassLinker();
+  const char* old_cause = self->StartAssertNoThreadSuspension("ImageWriter");
 
-  const std::vector<const Compiler::PatchInformation*>& code_to_patch = compiler.GetCodeToPatch();
+  typedef std::vector<const Compiler::PatchInformation*> Patches;
+  const Patches& code_to_patch = compiler.GetCodeToPatch();
   for (size_t i = 0; i < code_to_patch.size(); i++) {
     const Compiler::PatchInformation* patch = code_to_patch[i];
     AbstractMethod* target = GetTargetMethod(patch);
@@ -669,8 +646,7 @@ void ImageWriter::PatchOatCodeAndMethods(const Compiler& compiler) {
     SetPatchLocation(patch, reinterpret_cast<uint32_t>(GetOatAddress(code_offset)));
   }
 
-  const std::vector<const Compiler::PatchInformation*>& methods_to_patch
-      = compiler.GetMethodsToPatch();
+  const Patches& methods_to_patch = compiler.GetMethodsToPatch();
   for (size_t i = 0; i < methods_to_patch.size(); i++) {
     const Compiler::PatchInformation* patch = methods_to_patch[i];
     AbstractMethod* target = GetTargetMethod(patch);
@@ -680,16 +656,16 @@ void ImageWriter::PatchOatCodeAndMethods(const Compiler& compiler) {
   // Update the image header with the new checksum after patching
   ImageHeader* image_header = reinterpret_cast<ImageHeader*>(image_->Begin());
   image_header->SetOatChecksum(oat_file_->GetOatHeader().GetChecksum());
+  self->EndAssertNoThreadSuspension(old_cause);
 }
 
 void ImageWriter::SetPatchLocation(const Compiler::PatchInformation* patch, uint32_t value) {
   ClassLinker* class_linker = Runtime::Current()->GetClassLinker();
-  AbstractMethod* method = GetReferrerMethod(patch);
-  // Goodbye const, we are about to modify some code.
-  void* code = const_cast<void*>(class_linker->GetOatCodeFor(method));
+  const void* oat_code = class_linker->GetOatCodeFor(patch->GetDexFile(),
+                                                     patch->GetReferrerMethodIdx());
   OatHeader& oat_header = const_cast<OatHeader&>(oat_file_->GetOatHeader());
   // TODO: make this Thumb2 specific
-  uint8_t* base = reinterpret_cast<uint8_t*>(reinterpret_cast<uint32_t>(code) & ~0x1);
+  uint8_t* base = reinterpret_cast<uint8_t*>(reinterpret_cast<uint32_t>(oat_code) & ~0x1);
   uint32_t* patch_location = reinterpret_cast<uint32_t*>(base + patch->GetLiteralOffset());
 #ifndef NDEBUG
   const DexFile::MethodId& id = patch->GetDexFile().GetMethodId(patch->GetTargetMethodIdx());
diff --git a/src/object.h b/src/object.h
index 61fa3357dd..87f132e458 100644
--- a/src/object.h
+++ b/src/object.h
@@ -1142,6 +1142,10 @@ class MANAGED ObjectArray : public Array {
   // circumstances, such as during boot image writing
   void SetWithoutChecks(int32_t i, T* object) SHARED_LOCKS_REQUIRED(Locks::mutator_lock_);
 
+  // Set element without bound and element type checks, to be used in limited circumstances, such
+  // as during boot image writing. Does not do write barrier.
+  void SetPtrWithoutChecks(int32_t i, T* object) SHARED_LOCKS_REQUIRED(Locks::mutator_lock_);
+
   T* GetWithoutChecks(int32_t i) const SHARED_LOCKS_REQUIRED(Locks::mutator_lock_);
 
   static void Copy(const ObjectArray<T>* src, int src_pos,
@@ -2262,6 +2266,13 @@ void ObjectArray<T>::SetWithoutChecks(int32_t i, T* object) {
 }
 
 template<class T>
+void ObjectArray<T>::SetPtrWithoutChecks(int32_t i, T* object) {
+  DCHECK(IsValidIndex(i));
+  MemberOffset data_offset(DataOffset(sizeof(Object*)).Int32Value() + i * sizeof(Object*));
+  SetFieldPtr(data_offset, object, false);
+}
+
+template<class T>
 T* ObjectArray<T>::GetWithoutChecks(int32_t i) const {
   DCHECK(IsValidIndex(i));
   MemberOffset data_offset(DataOffset(sizeof(Object*)).Int32Value() + i * sizeof(Object*));