From: Mathieu Chartier Date: Wed, 17 Feb 2016 01:16:01 +0000 (-0800) Subject: Only visit pointer arrays once during image relocation X-Git-Tag: android-x86-7.1-r1~395^2~2^2~42^2 X-Git-Url: http://git.osdn.net/view?a=commitdiff_plain;h=91edc62a9d8d4d8153b6b04140b50a3724cd3597;p=android-x86%2Fart.git Only visit pointer arrays once during image relocation Previously they could get visited twice, this caused corruption of the app image if a pointer was updated twice. Bug: 22858531 Change-Id: I1f1ba1ba5dc205be07dba51bc6ce7a825c82b33a --- diff --git a/runtime/gc/space/image_space.cc b/runtime/gc/space/image_space.cc index 0c06c386b..894ce9af7 100644 --- a/runtime/gc/space/image_space.cc +++ b/runtime/gc/space/image_space.cc @@ -676,13 +676,17 @@ class RelocationRange { dest_(dest), length_(length) {} - bool ContainsSource(uintptr_t address) const { + bool InSource(uintptr_t address) const { return address - source_ < length_; } + bool InDest(uintptr_t address) const { + return address - dest_ < length_; + } + // Translate a source address to the destination space. uintptr_t ToDest(uintptr_t address) const { - DCHECK(ContainsSource(address)); + DCHECK(InSource(address)); return address + Delta(); } @@ -724,24 +728,28 @@ class FixupVisitor : public ValueObject { template ALWAYS_INLINE T* ForwardObject(T* src) const { const uintptr_t uint_src = reinterpret_cast(src); - if (boot_image_.ContainsSource(uint_src)) { + if (boot_image_.InSource(uint_src)) { return reinterpret_cast(boot_image_.ToDest(uint_src)); } - if (app_image_.ContainsSource(uint_src)) { + if (app_image_.InSource(uint_src)) { return reinterpret_cast(app_image_.ToDest(uint_src)); } + // Since we are fixing up the app image, there should only be pointers to the app image and + // boot image. + DCHECK(src == nullptr) << reinterpret_cast(src); return src; } // Return the relocated address of a code pointer (contained by an oat file). ALWAYS_INLINE const void* ForwardCode(const void* src) const { const uintptr_t uint_src = reinterpret_cast(src); - if (boot_oat_.ContainsSource(uint_src)) { + if (boot_oat_.InSource(uint_src)) { return reinterpret_cast(boot_oat_.ToDest(uint_src)); } - if (app_oat_.ContainsSource(uint_src)) { + if (app_oat_.InSource(uint_src)) { return reinterpret_cast(app_oat_.ToDest(uint_src)); } + DCHECK(src == nullptr) << src; return src; } @@ -766,6 +774,11 @@ class FixupObjectAdapter : public FixupVisitor { template explicit FixupObjectAdapter(Args... args) : FixupVisitor(args...) {} + // Must be called on pointers that already have been relocated to the destination relocation. + ALWAYS_INLINE bool IsInAppImage(mirror::Object* object) const { + return app_image_.InDest(reinterpret_cast(object)); + } + template T* operator()(T* obj) const { return ForwardObject(obj); @@ -816,7 +829,10 @@ class FixupRootVisitor : public FixupVisitor { class FixupObjectVisitor : public FixupVisitor { public: template - explicit FixupObjectVisitor(Args... args) : FixupVisitor(args...) {} + explicit FixupObjectVisitor(gc::accounting::ContinuousSpaceBitmap* pointer_array_visited, + Args... args) + : FixupVisitor(args...), + pointer_array_visited_(pointer_array_visited) {} // Fix up separately since we also need to fix up method entrypoints. ALWAYS_INLINE void VisitRootIfNonNull( @@ -841,6 +857,19 @@ class FixupObjectVisitor : public FixupVisitor { } } + // Visit a pointer array and forward corresponding native data. Ignores pointer arrays in the + // boot image. Uses the bitmap to ensure the same array is not visited multiple times. + template + void VisitPointerArray(mirror::PointerArray* array, const Visitor& visitor) const + NO_THREAD_SAFETY_ANALYSIS { + if (array != nullptr && + visitor.IsInAppImage(array) && + !pointer_array_visited_->Test(array)) { + array->Fixup(array, sizeof(void*), visitor); + pointer_array_visited_->Set(array); + } + } + // java.lang.ref.Reference visitor. void operator()(mirror::Class* klass ATTRIBUTE_UNUSED, mirror::Reference* ref) const SHARED_REQUIRES(Locks::mutator_lock_) REQUIRES(Locks::heap_bitmap_lock_) { @@ -859,11 +888,9 @@ class FixupObjectVisitor : public FixupVisitor { mirror::Class* klass = obj->AsClass(); FixupObjectAdapter visitor(boot_image_, boot_oat_, app_image_, app_oat_); klass->FixupNativePointers(klass, sizeof(void*), visitor); - // Deal with the arrays. - mirror::PointerArray* vtable = klass->GetVTable(); - if (vtable != nullptr) { - vtable->Fixup(vtable, sizeof(void*), visitor); - } + // Deal with the pointer arrays. Use the helper function since multiple classes can reference + // the same arrays. + VisitPointerArray(klass->GetVTable(), visitor); mirror::IfTable* iftable = klass->GetIfTable(); if (iftable != nullptr) { for (int32_t i = 0, count = iftable->Count(); i < count; ++i) { @@ -871,12 +898,15 @@ class FixupObjectVisitor : public FixupVisitor { mirror::PointerArray* methods = iftable->GetMethodArray(i); DCHECK(methods != nullptr); - methods->Fixup(methods, sizeof(void*), visitor); + VisitPointerArray(methods, visitor); } } } } } + + private: + gc::accounting::ContinuousSpaceBitmap* const pointer_array_visited_; }; class ForwardObjectAdapter { @@ -1010,9 +1040,18 @@ static bool RelocateInPlace(ImageHeader& image_header, const ImageSection& objects_section = image_header.GetImageSection(ImageHeader::kSectionObjects); uintptr_t objects_begin = reinterpret_cast(target_base + objects_section.Offset()); uintptr_t objects_end = reinterpret_cast(target_base + objects_section.End()); - // Two pass approach, fix up all classes first, then fix up non class-objects. - FixupObjectVisitor fixup_object_visitor(boot_image, boot_oat, app_image, app_oat); if (fixup_image) { + // Two pass approach, fix up all classes first, then fix up non class-objects. + // The visited bitmap is used to ensure that pointer arrays are not forwarded twice. + std::unique_ptr visited_bitmap( + gc::accounting::ContinuousSpaceBitmap::Create("Pointer array bitmap", + target_base, + image_header.GetImageSize())); + FixupObjectVisitor fixup_object_visitor(visited_bitmap.get(), + boot_image, + boot_oat, + app_image, + app_oat); TimingLogger::ScopedTiming timing("Fixup classes", &logger); // Fixup class only touches app image classes, don't need the mutator lock since the space is // not yet visible to the GC. @@ -1025,7 +1064,7 @@ static bool RelocateInPlace(ImageHeader& image_header, bitmap->VisitMarkedRange(objects_begin, objects_end, fixup_object_visitor); FixupObjectAdapter fixup_adapter(boot_image, boot_oat, app_image, app_oat); // Fixup image roots. - CHECK(app_image.ContainsSource(reinterpret_cast( + CHECK(app_image.InSource(reinterpret_cast( image_header.GetImageRoots()))); image_header.RelocateImageObjects(app_image.Delta()); CHECK_EQ(image_header.GetImageBegin(), target_base);