From e271fe1e2797205c57c052212c32139234f781ea Mon Sep 17 00:00:00 2001 From: Jeff Hao Date: Mon, 4 Jan 2016 17:38:06 -0800 Subject: [PATCH] Fix multi-image TODOs in patchoat. - Reinstated checks that were still valid (one check was removed). - Removed unused path that patches only images. Bug: 26317072 Change-Id: I0e140cb110abbfa469c097c805657ecfdc8552d6 --- patchoat/patchoat.cc | 111 ++++----------------------------------------------- patchoat/patchoat.h | 7 ++-- 2 files changed, 10 insertions(+), 108 deletions(-) diff --git a/patchoat/patchoat.cc b/patchoat/patchoat.cc index b403abd2b..3037652c4 100644 --- a/patchoat/patchoat.cc +++ b/patchoat/patchoat.cc @@ -150,100 +150,6 @@ static bool FinishFile(File* file, bool close) { } } -bool PatchOat::Patch(const std::string& image_location, off_t delta, - File* output_image, InstructionSet isa, - TimingLogger* timings) { - CHECK(Runtime::Current() == nullptr); - CHECK(output_image != nullptr); - CHECK_GE(output_image->Fd(), 0); - CHECK(!image_location.empty()) << "image file must have a filename."; - CHECK_NE(isa, kNone); - - TimingLogger::ScopedTiming t("Runtime Setup", timings); - const char *isa_name = GetInstructionSetString(isa); - std::string image_filename; - if (!LocationToFilename(image_location, isa, &image_filename)) { - LOG(ERROR) << "Unable to find image at location " << image_location; - return false; - } - std::unique_ptr input_image(OS::OpenFileForReading(image_filename.c_str())); - if (input_image.get() == nullptr) { - LOG(ERROR) << "unable to open input image file at " << image_filename - << " for location " << image_location; - return false; - } - - int64_t image_len = input_image->GetLength(); - if (image_len < 0) { - LOG(ERROR) << "Error while getting image length"; - return false; - } - ImageHeader image_header; - if (sizeof(image_header) != input_image->Read(reinterpret_cast(&image_header), - sizeof(image_header), 0)) { - LOG(ERROR) << "Unable to read image header from image file " << input_image->GetPath(); - return false; - } - - if (image_header.GetStorageMode() != ImageHeader::kStorageModeUncompressed) { - LOG(ERROR) << "Patchoat is not supported with compressed image files " - << input_image->GetPath(); - return false; - } - - /*bool is_image_pic = */IsImagePic(image_header, input_image->GetPath()); - // Nothing special to do right now since the image always needs to get patched. - // Perhaps in some far-off future we may have images with relative addresses that are true-PIC. - - // Set up the runtime - RuntimeOptions options; - NoopCompilerCallbacks callbacks; - options.push_back(std::make_pair("compilercallbacks", &callbacks)); - std::string img = "-Ximage:" + image_location; - options.push_back(std::make_pair(img.c_str(), nullptr)); - options.push_back(std::make_pair("imageinstructionset", reinterpret_cast(isa_name))); - options.push_back(std::make_pair("-Xno-sig-chain", nullptr)); - if (!Runtime::Create(options, false)) { - LOG(ERROR) << "Unable to initialize runtime"; - return false; - } - // Runtime::Create acquired the mutator_lock_ that is normally given away when we Runtime::Start, - // give it away now and then switch to a more manageable ScopedObjectAccess. - Thread::Current()->TransitionFromRunnableToSuspended(kNative); - ScopedObjectAccess soa(Thread::Current()); - - t.NewTiming("Image and oat Patching setup"); - // Create the map where we will write the image patches to. - std::string error_msg; - std::unique_ptr image(MemMap::MapFile(image_len, - PROT_READ | PROT_WRITE, - MAP_PRIVATE, - input_image->Fd(), - 0, - /*low_4gb*/false, - input_image->GetPath().c_str(), - &error_msg)); - if (image.get() == nullptr) { - LOG(ERROR) << "unable to map image file " << input_image->GetPath() << " : " << error_msg; - return false; - } - // TODO: Support multi-image when patchoat is only patching images. Ever used? b/26317072 - gc::space::ImageSpace* ispc = Runtime::Current()->GetHeap()->GetBootImageSpaces()[0]; - - PatchOat p(isa, image.release(), ispc->GetLiveBitmap(), ispc->GetMemMap(), delta, timings); - t.NewTiming("Patching files"); - if (!p.PatchImage(true)) { - LOG(ERROR) << "Failed to patch image file " << input_image->GetPath(); - return false; - } - - t.NewTiming("Writing files"); - if (!p.WriteImage(output_image)) { - return false; - } - return true; -} - bool PatchOat::Patch(File* input_oat, const std::string& image_location, off_t delta, File* output_oat, File* output_image, InstructionSet isa, TimingLogger* timings, @@ -765,8 +671,6 @@ bool PatchOat::InHeap(mirror::Object* o) { void PatchOat::PatchVisitor::operator() (mirror::Object* obj, MemberOffset off, bool is_static_unused ATTRIBUTE_UNUSED) const { mirror::Object* referent = obj->GetFieldObject(off); - // TODO: Modify check for multi-image support? b/26317072 - // DCHECK(patcher_->InHeap(referent)) << "Referent is not in the heap."; mirror::Object* moved_object = patcher_->RelocatedAddressOfPointer(referent); copy_->SetFieldObjectWithoutWriteBarrier(off, moved_object); } @@ -775,8 +679,7 @@ void PatchOat::PatchVisitor::operator() (mirror::Class* cls ATTRIBUTE_UNUSED, mirror::Reference* ref) const { MemberOffset off = mirror::Reference::ReferentOffset(); mirror::Object* referent = ref->GetReferent(); - // TODO: Modify check for multi-image support? b/26317072 - // DCHECK(patcher_->InHeap(referent)) << "Referent is not in the heap."; + DCHECK(patcher_->InHeap(referent)) << "Referent is not in the heap."; mirror::Object* moved_object = patcher_->RelocatedAddressOfPointer(referent); copy_->SetFieldObjectWithoutWriteBarrier(off, moved_object); } @@ -1271,8 +1174,12 @@ static int patchoat(int argc, char **argv) { bool have_image_files = have_output_image; bool have_oat_files = have_output_oat; - if (!have_oat_files && !have_image_files) { - Usage("Must be patching either an oat or an image file or both."); + if (!have_oat_files) { + if (have_image_files) { + Usage("Cannot patch an image file without an oat file"); + } else { + Usage("Must be patching either an oat file or an image file with an oat file."); + } } if (!have_oat_files && !isa_set) { @@ -1507,10 +1414,6 @@ static int patchoat(int argc, char **argv) { output_oat_fd >= 0, // was it opened from FD? new_oat_out); ret = FinishFile(output_oat.get(), ret); - } else if (have_image_files) { - TimingLogger::ScopedTiming pt("patch image", &timings); - ret = PatchOat::Patch(input_image_location, base_delta, output_image.get(), isa, &timings); - ret = FinishFile(output_image.get(), ret); } else { CHECK(false); ret = true; diff --git a/patchoat/patchoat.h b/patchoat/patchoat.h index cb0d14b2c..ceddc343b 100644 --- a/patchoat/patchoat.h +++ b/patchoat/patchoat.h @@ -133,12 +133,11 @@ class PatchOat { if (obj == nullptr) { return nullptr; } - // TODO: Fix these checks for multi-image. Some may still be valid. b/26317072 - // DCHECK_GT(reinterpret_cast(obj), reinterpret_cast(heap_->Begin())); - // DCHECK_LT(reinterpret_cast(obj), reinterpret_cast(heap_->End())); + DCHECK_GT(reinterpret_cast(obj), reinterpret_cast(heap_->Begin())); + DCHECK_LT(reinterpret_cast(obj), reinterpret_cast(heap_->End())); uintptr_t heap_off = reinterpret_cast(obj) - reinterpret_cast(heap_->Begin()); - // DCHECK_LT(heap_off, image_->Size()); + DCHECK_LT(heap_off, image_->Size()); return reinterpret_cast(image_->Begin() + heap_off); } -- 2.11.0