From 288b1e9a0dddfb91e85067fe81de55174f313c7c Mon Sep 17 00:00:00 2001 From: Andreas Gampe Date: Mon, 28 Dec 2015 10:41:49 -0800 Subject: [PATCH] ART: Fix up some multi-image cases Change the auto-generated multi-image names to include the path components from the first image, as well as prefix them with the first image's name to disambiguate. This fixes vogar-style usage. Fix an out-of-bounds issue in dex2oat when dex files are missing. Forbid generating or patching multi-image parts when loading images. Instead just fail loading them. Remember ImageSpace instances that have been added while trying to load a multi-image set. Remove all loaded instances when the overall loading process fails. Refactor the dex location adaptation into ImageSpace. Reuse the code in the Runtime path for fallback, so that all dex files can be found correctly. Fix an out-of-bounds access in OatFileAssistant in fallback mode. Partially reverts d895961d07a1d320b29f2045a48bc5a1944a4d3c. Push an actual image name, that is, something with an art extension, to the vogar scripts. Partially reverts c525604b313bb77a2077e1fec43dfab76cb1b9b1. Test 119-noimage-patchoat works again. Bug: 26317072 Bug: 26320300 Change-Id: I3f05fa77f22a2b9ca54c3105ffc53646c1928604 --- dex2oat/dex2oat.cc | 82 ++++++++++++++++++++++++++++++++--------- 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 ++- 9 files changed, 165 insertions(+), 87 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/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