From 8994a04162a92759f8ec531d18ee8901145dfda0 Mon Sep 17 00:00:00 2001 From: Andreas Gampe Date: Wed, 30 Dec 2015 19:03:17 +0000 Subject: [PATCH] Revert "Revert "ART: Fix up some multi-image cases"" This reverts commit de38b797c3e5ba3ee44c480db7093386975c51eb. Fix up imgdiag for std::string and multi-image. Bug: 26317072 Bug: 26320300 Change-Id: I94ce9528e9fea6fb3231a70c32db02d567143db9 --- dex2oat/dex2oat.cc | 82 ++++++++++++++++++++++++++++++++--------- imgdiag/imgdiag.cc | 16 ++------ runtime/gc/heap.cc | 66 ++++++--------------------------- runtime/gc/space/image_space.cc | 67 +++++++++++++++++++++++++++++++++ runtime/gc/space/image_space.h | 6 +++ runtime/oat_file_assistant.cc | 7 +++- runtime/runtime.cc | 10 ++--- test/Android.run-test.mk | 5 --- tools/run-jdwp-tests.sh | 3 +- tools/run-libcore-tests.sh | 6 ++- 10 files changed, 169 insertions(+), 99 deletions(-) diff --git a/dex2oat/dex2oat.cc b/dex2oat/dex2oat.cc index bb4224b7c..50480d904 100644 --- a/dex2oat/dex2oat.cc +++ b/dex2oat/dex2oat.cc @@ -854,6 +854,11 @@ class Dex2Oat FINAL { if (last_oat_slash == std::string::npos) { Usage("--multi-image used with unusable oat filename %s", base_oat.c_str()); } + // We also need to honor path components that were encoded through '@'. Otherwise the loading + // code won't be able to find the images. + if (base_oat.find('@', last_oat_slash) != std::string::npos) { + last_oat_slash = base_oat.rfind('@'); + } base_oat = base_oat.substr(0, last_oat_slash + 1); std::string base_img = image_filenames_[0]; @@ -861,9 +866,24 @@ class Dex2Oat FINAL { if (last_img_slash == std::string::npos) { Usage("--multi-image used with unusable image filename %s", base_img.c_str()); } + // We also need to honor path components that were encoded through '@'. Otherwise the loading + // code won't be able to find the images. + if (base_img.find('@', last_img_slash) != std::string::npos) { + last_img_slash = base_img.rfind('@'); + } + + // Get the prefix, which is the primary image name (without path components). Strip the + // extension. + std::string prefix = base_img.substr(last_img_slash + 1); + if (prefix.rfind('.') != std::string::npos) { + prefix = prefix.substr(0, prefix.rfind('.')); + } + if (!prefix.empty()) { + prefix = prefix + "-"; + } + base_img = base_img.substr(0, last_img_slash + 1); - // Now create the other names. // Note: we have some special case here for our testing. We have to inject the differentiating // parts for the different core images. std::string infix; // Empty infix by default. @@ -882,14 +902,15 @@ class Dex2Oat FINAL { infix = dex_file.substr(strlen("core")); } } - // Use a counted loop to skip the first one. + + // Now create the other names. Use a counted loop to skip the first one. for (size_t i = 1; i < dex_locations_.size(); ++i) { // TODO: Make everything properly std::string. - std::string image_name = CreateMultiImageName(dex_locations_[i], infix, ".art"); + std::string image_name = CreateMultiImageName(dex_locations_[i], prefix, infix, ".art"); char_backing_storage_.push_back(base_img + image_name); image_filenames_.push_back((char_backing_storage_.end() - 1)->c_str()); - std::string oat_name = CreateMultiImageName(dex_locations_[i], infix, ".oat"); + std::string oat_name = CreateMultiImageName(dex_locations_[i], prefix, infix, ".oat"); char_backing_storage_.push_back(base_oat + oat_name); oat_filenames_.push_back((char_backing_storage_.end() - 1)->c_str()); } @@ -904,13 +925,23 @@ class Dex2Oat FINAL { key_value_store_.reset(new SafeMap()); } + // Modify the input string in the following way: + // 0) Assume input is /a/b/c.d + // 1) Strip the path -> c.d + // 2) Inject prefix p -> pc.d + // 3) Inject infix i -> pci.d + // 4) Replace suffix with s if it's "jar" -> d == "jar" -> pci.s static std::string CreateMultiImageName(std::string in, + const std::string& prefix, const std::string& infix, - const char* suffix) { + const char* replace_suffix) { size_t last_dex_slash = in.rfind('/'); if (last_dex_slash != std::string::npos) { in = in.substr(last_dex_slash + 1); } + if (!prefix.empty()) { + in = prefix + in; + } if (!infix.empty()) { // Inject infix. size_t last_dot = in.rfind('.'); @@ -919,7 +950,8 @@ class Dex2Oat FINAL { } } if (EndsWith(in, ".jar")) { - in = in.substr(0, in.length() - strlen(".jar")) + (suffix != nullptr ? suffix : ""); + in = in.substr(0, in.length() - strlen(".jar")) + + (replace_suffix != nullptr ? replace_suffix : ""); } return in; } @@ -1284,16 +1316,21 @@ class Dex2Oat FINAL { void CreateDexOatMappings() { if (oat_files_.size() > 1) { + // TODO: This needs to change, as it is not a stable mapping. If a dex file is missing, + // the images will be out of whack. b/26317072 size_t index = 0; for (size_t i = 0; i < oat_files_.size(); ++i) { - std::vector dex_files = { 1, dex_files_[index] }; - dex_file_oat_filename_map_.emplace(dex_files_[index], oat_filenames_[i]); - index++; - while (index < dex_files_.size() && - (dex_files_[index]->GetBaseLocation() == dex_files_[index - 1]->GetBaseLocation())) { - dex_file_oat_filename_map_.emplace(dex_files_[index], oat_filenames_[i]); + std::vector dex_files; + if (index < dex_files_.size()) { dex_files.push_back(dex_files_[index]); + dex_file_oat_filename_map_.emplace(dex_files_[index], oat_filenames_[i]); index++; + while (index < dex_files_.size() && + (dex_files_[index]->GetBaseLocation() == dex_files_[index - 1]->GetBaseLocation())) { + dex_file_oat_filename_map_.emplace(dex_files_[index], oat_filenames_[i]); + dex_files.push_back(dex_files_[index]); + index++; + } } dex_files_per_oat_file_.push_back(std::move(dex_files)); } @@ -1384,8 +1421,9 @@ class Dex2Oat FINAL { if (IsBootImage() && image_filenames_.size() > 1) { // If we're compiling the boot image, store the boot classpath into the Key-Value store. If - // the image filename was adapted (e.g., for our tests), we need to change this here, too. We - // need this for the multi-image case. + // the image filename was adapted (e.g., for our tests), we need to change this here, too, but + // need to strip all path components (they will be re-established when loading). + // We need this for the multi-image case. std::ostringstream bootcp_oss; bool first_bootcp = true; for (size_t i = 0; i < dex_locations_.size(); ++i) { @@ -1396,14 +1434,24 @@ class Dex2Oat FINAL { std::string dex_loc = dex_locations_[i]; std::string image_filename = image_filenames_[i]; - // Use the dex_loc path, but the image_filename name. + // Use the dex_loc path, but the image_filename name (without path elements). size_t dex_last_slash = dex_loc.rfind('/'); + + // npos is max(size_t). That makes this a bit ugly. size_t image_last_slash = image_filename.rfind('/'); + size_t image_last_at = image_filename.rfind('@'); + size_t image_last_sep = (image_last_slash == std::string::npos) + ? image_last_at + : (image_last_at == std::string::npos) + ? std::string::npos + : std::max(image_last_slash, image_last_at); + // Note: whenever image_last_sep == npos, +1 overflow means using the full string. + if (dex_last_slash == std::string::npos) { - dex_loc = image_filename.substr(image_last_slash + 1); + dex_loc = image_filename.substr(image_last_sep + 1); } else { dex_loc = dex_loc.substr(0, dex_last_slash + 1) + - image_filename.substr(image_last_slash + 1); + image_filename.substr(image_last_sep + 1); } // Image filenames already end with .art, no need to replace. diff --git a/imgdiag/imgdiag.cc b/imgdiag/imgdiag.cc index b8a72afb6..93a0974fb 100644 --- a/imgdiag/imgdiag.cc +++ b/imgdiag/imgdiag.cc @@ -49,7 +49,7 @@ class ImgDiagDumper { public: explicit ImgDiagDumper(std::ostream* os, const ImageHeader& image_header, - const char* image_location, + const std::string& image_location, pid_t image_diff_pid) : os_(os), image_header_(image_header), @@ -163,7 +163,7 @@ class ImgDiagDumper { std::string error_msg; // Walk the bytes and diff against our boot image - const ImageHeader& boot_image_header = GetBootImageHeader(); + const ImageHeader& boot_image_header = image_header_; os << "\nObserving boot image header at address " << reinterpret_cast(&boot_image_header) @@ -812,14 +812,6 @@ class ImgDiagDumper { return page_frame_number != page_frame_number_clean; } - static const ImageHeader& GetBootImageHeader() { - gc::Heap* heap = Runtime::Current()->GetHeap(); - std::vector image_spaces = heap->GetBootImageSpaces(); - CHECK(!image_spaces.empty()); - const ImageHeader& image_header = image_spaces[0]->GetImageHeader(); - return image_header; - } - private: // Return the image location, stripped of any directories, e.g. "boot.art" or "core.art" std::string GetImageLocationBaseName() const { @@ -828,7 +820,7 @@ class ImgDiagDumper { std::ostream* os_; const ImageHeader& image_header_; - const char* image_location_; + const std::string image_location_; pid_t image_diff_pid_; // Dump image diff against boot.art if pid is non-negative DISALLOW_COPY_AND_ASSIGN(ImgDiagDumper); @@ -847,7 +839,7 @@ static int DumpImage(Runtime* runtime, std::ostream* os, pid_t image_diff_pid) { } ImgDiagDumper img_diag_dumper( - os, image_header, image_space->GetImageLocation().c_str(), image_diff_pid); + os, image_header, image_space->GetImageLocation(), image_diff_pid); if (!img_diag_dumper.Dump()) { return EXIT_FAILURE; } diff --git a/runtime/gc/heap.cc b/runtime/gc/heap.cc index 3e432c7c9..7f67ae4f0 100644 --- a/runtime/gc/heap.cc +++ b/runtime/gc/heap.cc @@ -265,6 +265,9 @@ Heap::Heap(size_t initial_size, // For code reuse, handle this like a work queue. std::vector image_file_names; image_file_names.push_back(image_file_name); + // The loaded spaces. Secondary images may fail to load, in which case we need to remove + // already added spaces. + std::vector added_image_spaces; for (size_t index = 0; index < image_file_names.size(); ++index) { std::string& image_name = image_file_names[index]; @@ -277,6 +280,7 @@ Heap::Heap(size_t initial_size, ATRACE_END(); if (boot_image_space != nullptr) { AddSpace(boot_image_space); + added_image_spaces.push_back(boot_image_space); // Oat files referenced by image files immediately follow them in memory, ensure alloc space // isn't going to get in the middle uint8_t* oat_file_end_addr = boot_image_space->GetImageHeader().GetOatFileEnd(); @@ -298,66 +302,18 @@ Heap::Heap(size_t initial_size, continue; } - std::vector images; - Split(boot_classpath, ':', &images); - - // Add the rest into the list. We have to adjust locations, possibly: - // - // For example, image_file_name is /a/b/c/d/e.art - // images[0] is f/c/d/e.art - // ---------------------------------------------- - // images[1] is g/h/i/j.art -> /a/b/h/i/j.art - - // Derive pattern. - std::vector left; - Split(image_file_name, '/', &left); - std::vector right; - Split(images[0], '/', &right); - - size_t common = 1; - while (common < left.size() && common < right.size()) { - if (left[left.size() - common - 1] != right[right.size() - common - 1]) { - break; - } - common++; - } - - std::vector prefix_vector(left.begin(), left.end() - common); - std::string common_prefix = Join(prefix_vector, '/'); - if (!common_prefix.empty() && common_prefix[0] != '/' && image_file_name[0] == '/') { - common_prefix = "/" + common_prefix; - } - - // Apply pattern to images[1] .. images[n]. - for (size_t i = 1; i < images.size(); ++i) { - std::string image = images[i]; - - size_t rslash = std::string::npos; - for (size_t j = 0; j < common; ++j) { - if (rslash != std::string::npos) { - rslash--; - } - - rslash = image.rfind('/', rslash); - if (rslash == std::string::npos) { - rslash = 0; - } - if (rslash == 0) { - break; - } - } - std::string image_part = image.substr(rslash); - - std::string new_image = common_prefix + (StartsWith(image_part, "/") ? "" : "/") + - image_part; - image_file_names.push_back(new_image); - } + space::ImageSpace::CreateMultiImageLocations(image_file_name, + boot_classpath, + &image_file_names); } } else { LOG(ERROR) << "Could not create image space with image file '" << image_file_name << "'. " << "Attempting to fall back to imageless running. Error was: " << error_msg << "\nAttempted image: " << image_name; - // TODO: Remove already loaded spaces. + // Remove already loaded spaces. + for (space::Space* loaded_space : added_image_spaces) { + RemoveSpace(loaded_space); + } break; } } diff --git a/runtime/gc/space/image_space.cc b/runtime/gc/space/image_space.cc index 952759ca7..dfdbd0442 100644 --- a/runtime/gc/space/image_space.cc +++ b/runtime/gc/space/image_space.cc @@ -523,6 +523,9 @@ ImageSpace* ImageSpace::Create(const char* image_location, } else if (!ImageCreationAllowed(is_global_cache, &reason)) { // Whether we can write to the cache. success = false; + } else if (secondary_image) { + reason = "Should not have to patch secondary image."; + success = false; } else { // Try to relocate. success = RelocateImage(image_location, cache_filename.c_str(), image_isa, &reason); @@ -615,6 +618,9 @@ ImageSpace* ImageSpace::Create(const char* image_location, return nullptr; } else if (!ImageCreationAllowed(is_global_cache, error_msg)) { return nullptr; + } else if (secondary_image) { + *error_msg = "Cannot compile a secondary image."; + return nullptr; } else if (!GenerateImage(cache_filename, image_isa, error_msg)) { *error_msg = StringPrintf("Failed to generate image '%s': %s", cache_filename.c_str(), error_msg->c_str()); @@ -969,6 +975,67 @@ void ImageSpace::Dump(std::ostream& os) const { << ",name=\"" << GetName() << "\"]"; } +void ImageSpace::CreateMultiImageLocations(const std::string& input_image_file_name, + const std::string& boot_classpath, + std::vector* image_file_names) { + DCHECK(image_file_names != nullptr); + + std::vector images; + Split(boot_classpath, ':', &images); + + // Add the rest into the list. We have to adjust locations, possibly: + // + // For example, image_file_name is /a/b/c/d/e.art + // images[0] is f/c/d/e.art + // ---------------------------------------------- + // images[1] is g/h/i/j.art -> /a/b/h/i/j.art + + // Derive pattern. + std::vector left; + Split(input_image_file_name, '/', &left); + std::vector right; + Split(images[0], '/', &right); + + size_t common = 1; + while (common < left.size() && common < right.size()) { + if (left[left.size() - common - 1] != right[right.size() - common - 1]) { + break; + } + common++; + } + + std::vector prefix_vector(left.begin(), left.end() - common); + std::string common_prefix = Join(prefix_vector, '/'); + if (!common_prefix.empty() && common_prefix[0] != '/' && input_image_file_name[0] == '/') { + common_prefix = "/" + common_prefix; + } + + // Apply pattern to images[1] .. images[n]. + for (size_t i = 1; i < images.size(); ++i) { + std::string image = images[i]; + + size_t rslash = std::string::npos; + for (size_t j = 0; j < common; ++j) { + if (rslash != std::string::npos) { + rslash--; + } + + rslash = image.rfind('/', rslash); + if (rslash == std::string::npos) { + rslash = 0; + } + if (rslash == 0) { + break; + } + } + std::string image_part = image.substr(rslash); + + std::string new_image = common_prefix + (StartsWith(image_part, "/") ? "" : "/") + + image_part; + image_file_names->push_back(new_image); + } +} + } // namespace space } // namespace gc } // namespace art diff --git a/runtime/gc/space/image_space.h b/runtime/gc/space/image_space.h index a54358a7e..b8ae4a033 100644 --- a/runtime/gc/space/image_space.h +++ b/runtime/gc/space/image_space.h @@ -122,6 +122,12 @@ class ImageSpace : public MemMapSpace { bool* has_data, bool *is_global_cache); + // Use the input image filename to adapt the names in the given boot classpath to establish + // complete locations for secondary images. + static void CreateMultiImageLocations(const std::string& input_image_file_name, + const std::string& boot_classpath, + std::vector* image_filenames); + // Return the end of the image which includes non-heap objects such as ArtMethods and ArtFields. uint8_t* GetImageEnd() const { return Begin() + GetImageHeader().GetImageSize(); diff --git a/runtime/oat_file_assistant.cc b/runtime/oat_file_assistant.cc index cb80cc91a..8543ff4ef 100644 --- a/runtime/oat_file_assistant.cc +++ b/runtime/oat_file_assistant.cc @@ -846,7 +846,12 @@ std::string OatFileAssistant::OldProfileFileName() { std::string OatFileAssistant::ImageLocation() { Runtime* runtime = Runtime::Current(); - return runtime->GetHeap()->GetBootImageSpaces()[0]->GetImageLocation(); + const std::vector& image_spaces = + runtime->GetHeap()->GetBootImageSpaces(); + if (image_spaces.empty()) { + return ""; + } + return image_spaces[0]->GetImageLocation(); } const uint32_t* OatFileAssistant::GetRequiredDexChecksum() { diff --git a/runtime/runtime.cc b/runtime/runtime.cc index 98aa8d8c0..5c7262932 100644 --- a/runtime/runtime.cc +++ b/runtime/runtime.cc @@ -827,13 +827,9 @@ static bool OpenDexFilesFromImage(const std::string& image_location, const OatHeader& boot_oat_header = oat_file->GetOatHeader(); const char* boot_cp = boot_oat_header.GetStoreValueByKey(OatHeader::kBootClassPath); if (boot_cp != nullptr) { - std::vector cp; - Split(boot_cp, ':', &cp); - - if (cp.size() > 1) { - // More images, enqueue (skipping the first). - image_locations.insert(image_locations.end(), cp.begin() + 1, cp.end()); - } + gc::space::ImageSpace::CreateMultiImageLocations(image_locations[0], + boot_cp, + &image_locations); } } diff --git a/test/Android.run-test.mk b/test/Android.run-test.mk index a3aeb6d9d..81cfb7003 100644 --- a/test/Android.run-test.mk +++ b/test/Android.run-test.mk @@ -337,11 +337,6 @@ endif TEST_ART_BROKEN_GCSTRESS_RUN_TESTS := -# b/26320300: multi image is broken and 119-noimage-patchoat fails because of it. -ART_TEST_KNOWN_BROKEN += $(call all-run-test-names,target,$(RUN_TYPES),$(PREBUILD_TYPES),$(COMPILER_TYPES), \ - $(RELOCATE_TYPES),$(TRACE_TYPES),$(GC_TYPES),$(JNI_TYPES),$(IMAGE_TYPES),$(PICTEST_TYPES),$(DEBUGGABLE_TYPES), 119-noimage-patchoat, \ - $(ALL_ADDRESS_SIZES)) - # 115-native-bridge setup is complicated. Need to implement it correctly for the target. ART_TEST_KNOWN_BROKEN += $(call all-run-test-names,target,$(RUN_TYPES),$(PREBUILD_TYPES),$(COMPILER_TYPES), \ $(RELOCATE_TYPES),$(TRACE_TYPES),$(GC_TYPES),$(JNI_TYPES),$(IMAGE_TYPES),$(PICTEST_TYPES),$(DEBUGGABLE_TYPES), 115-native-bridge, \ diff --git a/tools/run-jdwp-tests.sh b/tools/run-jdwp-tests.sh index 840fffbd2..f29e51f04 100755 --- a/tools/run-jdwp-tests.sh +++ b/tools/run-jdwp-tests.sh @@ -64,7 +64,8 @@ while true; do # with mksh. art="bash ${OUT_DIR-out}/host/linux-x86/bin/art" art_debugee="bash ${OUT_DIR-out}/host/linux-x86/bin/art" - image="-Ximage:${ANDROID_BUILD_TOP}/${OUT_DIR-out}/host/linux-x86/framework/core-jit.art" + # We force generation of a new image to avoid build-time and run-time classpath differences. + image="-Ximage:/system/non/existent/vogar.art" # We do not need a device directory on host. device_dir="" # Vogar knows which VM to use on host. diff --git a/tools/run-libcore-tests.sh b/tools/run-libcore-tests.sh index 3c93f196d..11ed8b94b 100755 --- a/tools/run-libcore-tests.sh +++ b/tools/run-libcore-tests.sh @@ -79,7 +79,11 @@ while true; do vogar_args="$vogar_args --vm-arg -Ximage:/data/art-test/core-optimizing.art" shift elif [[ "$1" == "--mode=host" ]]; then - vogar_args="$vogar_args --vm-arg -Ximage:${ANDROID_BUILD_TOP}/${OUT_DIR-out}/host/linux-x86/framework/core-jit.art" + # We explicitly give a wrong path for the image, to ensure vogar + # will create a boot image with the default compiler. Note that + # giving an existing image on host does not work because of + # classpath/resources differences when compiling the boot image. + vogar_args="$vogar_args --vm-arg -Ximage:/non/existent/vogar.art" shift elif [[ "$1" == "--debug" ]]; then # Remove the --debug from the arguments. -- 2.11.0