Use image oat file instead of image header for immune spaces

The old immune spaces logic used the oat file information in the
image header instead of the actual oat file pointer. This was
incorrect for the app image case since the app image oat file is
not necessarily at the address specified in the header. This bug
could cause an incorrect immune region that caused large objects
to get freed if they were within this immune region.

Added test.

Bug: 22858531

(cherry picked from commit 5351da0225d027a19420153615634a1c78966bca)

Change-Id: Ibf41b0c0a9a7b0d093146311e2603a186033e339

diff --git a/runtime/gc/collector/immune_region.h b/runtime/gc/collector/immune_region.h
index b60426d..c9ac435 100644 (file)
--- a/runtime/gc/collector/immune_region.h
+++ b/runtime/gc/collector/immune_region.h
@@ -66,6 +66,10 @@ class ImmuneRegion {
     return end_;
   }
 
+  size_t Size() const {
+    return size_;
+  }
+
  private:
   void UpdateSize() {
     size_ = reinterpret_cast<uintptr_t>(end_) - reinterpret_cast<uintptr_t>(begin_);

diff --git a/runtime/gc/collector/immune_spaces.cc b/runtime/gc/collector/immune_spaces.cc
index 8f9a9e2..0862ff0 100644 (file)
--- a/runtime/gc/collector/immune_spaces.cc
+++ b/runtime/gc/collector/immune_spaces.cc
@@ -18,6 +18,7 @@
 
 #include "gc/space/space-inl.h"
 #include "mirror/object.h"
+#include "oat_file.h"
 
 namespace art {
 namespace gc {
@@ -45,11 +46,16 @@ void ImmuneSpaces::CreateLargestImmuneRegion() {
       space::ImageSpace* image_space = space->AsImageSpace();
       // Update the end to include the other non-heap sections.
       space_end = RoundUp(reinterpret_cast<uintptr_t>(image_space->GetImageEnd()), kPageSize);
-      uintptr_t oat_begin = reinterpret_cast<uintptr_t>(image_space->GetOatFileBegin());
-      uintptr_t oat_end = reinterpret_cast<uintptr_t>(image_space->GetOatFileEnd());
-      if (space_end == oat_begin) {
-        DCHECK_GE(oat_end, oat_begin);
-        space_end = oat_end;
+      // For the app image case, GetOatFileBegin is where the oat file was mapped during image
+      // creation, the actual oat file could be somewhere else.
+      const OatFile* const image_oat_file = image_space->GetOatFile();
+      if (image_oat_file != nullptr) {
+        uintptr_t oat_begin = reinterpret_cast<uintptr_t>(image_oat_file->Begin());
+        uintptr_t oat_end = reinterpret_cast<uintptr_t>(image_oat_file->End());
+        if (space_end == oat_begin) {
+          DCHECK_GE(oat_end, oat_begin);
+          space_end = oat_end;
+        }
       }
     }
     if (cur_begin == 0u) {
@@ -71,6 +77,8 @@ void ImmuneSpaces::CreateLargestImmuneRegion() {
   }
   largest_immune_region_.SetBegin(reinterpret_cast<mirror::Object*>(best_begin));
   largest_immune_region_.SetEnd(reinterpret_cast<mirror::Object*>(best_end));
+  VLOG(gc) << "Immune region " << largest_immune_region_.Begin() << "-"
+           << largest_immune_region_.End();
 }
 
 void ImmuneSpaces::AddSpace(space::ContinuousSpace* space) {

diff --git a/runtime/gc/collector/immune_spaces_test.cc b/runtime/gc/collector/immune_spaces_test.cc
index ea290dd..56838f5 100644 (file)
--- a/runtime/gc/collector/immune_spaces_test.cc
+++ b/runtime/gc/collector/immune_spaces_test.cc
@@ -72,17 +72,31 @@ TEST_F(ImmuneSpacesTest, AppendBasic) {
   EXPECT_EQ(reinterpret_cast<uint8_t*>(spaces.GetLargestImmuneRegion().End()), b.Limit());
 }
 
+class DummyOatFile : public OatFile {
+ public:
+  DummyOatFile(uint8_t* begin, uint8_t* end) : OatFile("Location", /*is_executable*/ false) {
+    begin_ = begin;
+    end_ = end;
+  }
+};
+
 class DummyImageSpace : public space::ImageSpace {
  public:
-  DummyImageSpace(MemMap* map, accounting::ContinuousSpaceBitmap* live_bitmap)
+  DummyImageSpace(MemMap* map,
+                  accounting::ContinuousSpaceBitmap* live_bitmap,
+                  std::unique_ptr<DummyOatFile>&& oat_file)
       : ImageSpace("DummyImageSpace",
                    /*image_location*/"",
                    map,
                    live_bitmap,
-                   map->End()) {}
+                   map->End()) {
+    oat_file_ = std::move(oat_file);
+    oat_file_non_owned_ = oat_file_.get();
+  }
 
-  // OatSize is how large the oat file is after the image.
-  static DummyImageSpace* Create(size_t size, size_t oat_size) {
+  // Size is the size of the image space, oat offset is where the oat file is located
+  // after the end of image space. oat_size is the size of the oat file.
+  static DummyImageSpace* Create(size_t size, size_t oat_offset, size_t oat_size) {
     std::string error_str;
     std::unique_ptr<MemMap> map(MemMap::MapAnonymous("DummyImageSpace",
                                                      nullptr,
@@ -100,6 +114,9 @@ class DummyImageSpace : public space::ImageSpace {
     if (live_bitmap == nullptr) {
       return nullptr;
     }
+    // The actual mapped oat file may not be directly after the image for the app image case.
+    std::unique_ptr<DummyOatFile> oat_file(new DummyOatFile(map->End() + oat_offset,
+                                                            map->End() + oat_offset + oat_size));
     // Create image header.
     ImageSection sections[ImageHeader::kSectionCount];
     new (map->Begin()) ImageHeader(
@@ -108,6 +125,7 @@ class DummyImageSpace : public space::ImageSpace {
         sections,
         /*image_roots*/PointerToLowMemUInt32(map->Begin()) + 1,
         /*oat_checksum*/0u,
+        // The oat file data in the header is always right after the image space.
         /*oat_file_begin*/PointerToLowMemUInt32(map->End()),
         /*oat_data_begin*/PointerToLowMemUInt32(map->End()),
         /*oat_data_end*/PointerToLowMemUInt32(map->End() + oat_size),
@@ -121,7 +139,7 @@ class DummyImageSpace : public space::ImageSpace {
         /*is_pic*/false,
         ImageHeader::kStorageModeUncompressed,
         /*storage_size*/0u);
-    return new DummyImageSpace(map.release(), live_bitmap.release());
+    return new DummyImageSpace(map.release(), live_bitmap.release(), std::move(oat_file));
   }
 };
 
@@ -129,7 +147,9 @@ TEST_F(ImmuneSpacesTest, AppendAfterImage) {
   ImmuneSpaces spaces;
   constexpr size_t image_size = 123 * kPageSize;
   constexpr size_t image_oat_size = 321 * kPageSize;
-  std::unique_ptr<DummyImageSpace> image_space(DummyImageSpace::Create(image_size, image_oat_size));
+  std::unique_ptr<DummyImageSpace> image_space(DummyImageSpace::Create(image_size,
+                                                                       0,
+                                                                       image_oat_size));
   ASSERT_TRUE(image_space != nullptr);
   const ImageHeader& image_header = image_space->GetImageHeader();
   EXPECT_EQ(image_header.GetImageSize(), image_size);
@@ -150,6 +170,18 @@ TEST_F(ImmuneSpacesTest, AppendAfterImage) {
   EXPECT_EQ(reinterpret_cast<uint8_t*>(spaces.GetLargestImmuneRegion().Begin()),
             image_space->Begin());
   EXPECT_EQ(reinterpret_cast<uint8_t*>(spaces.GetLargestImmuneRegion().End()), space.Limit());
+  // Check that appending with a gap between the map does not include the oat file.
+  image_space.reset(DummyImageSpace::Create(image_size, kPageSize, image_oat_size));
+  spaces.Reset();
+  {
+    WriterMutexLock mu(Thread::Current(), *Locks::heap_bitmap_lock_);
+    spaces.AddSpace(image_space.get());
+  }
+  EXPECT_EQ(reinterpret_cast<uint8_t*>(spaces.GetLargestImmuneRegion().Begin()),
+            image_space->Begin());
+  // Size should be equal, we should not add the oat file since it is not adjacent to the image
+  // space.
+  EXPECT_EQ(spaces.GetLargestImmuneRegion().Size(), image_size);
 }
 
 }  // namespace collector

diff --git a/runtime/image.h b/runtime/image.h
index c449e43..146ee00 100644 (file)
--- a/runtime/image.h
+++ b/runtime/image.h
@@ -143,6 +143,8 @@ class PACKED(4) ImageHeader {
     oat_checksum_ = oat_checksum;
   }
 
+  // The location that the oat file was expected to be when the image was created. The actual
+  // oat file may be at a different location for application images.
   uint8_t* GetOatFileBegin() const {
     return reinterpret_cast<uint8_t*>(oat_file_begin_);
   }

diff --git a/runtime/oat_file.h b/runtime/oat_file.h
index bcc2d33..910163c 100644 (file)
--- a/runtime/oat_file.h
+++ b/runtime/oat_file.h
@@ -40,6 +40,12 @@ class OatMethodOffsets;
 class OatHeader;
 class OatDexFile;
 
+namespace gc {
+namespace collector {
+class DummyOatFile;
+}  // namespace collector
+}  // namespace gc
+
 class OatFile {
  public:
   typedef art::OatDexFile OatDexFile;
@@ -312,6 +318,7 @@ class OatFile {
   // elements. std::list<> and std::deque<> satisfy this requirement, std::vector<> doesn't.
   mutable std::list<std::string> string_cache_ GUARDED_BY(secondary_lookup_lock_);
 
+  friend class gc::collector::DummyOatFile;  // For modifying begin_ and end_.
   friend class OatClass;
   friend class art::OatDexFile;
   friend class OatDumper;  // For GetBase and GetLimit