OSDN Git Service

Revert "runtime: Mmap uncompressed dex files (in zip) as clean memory"
authorNicolas Geoffray <ngeoffray@google.com>
Sun, 12 Feb 2017 15:48:07 +0000 (15:48 +0000)
committerNicolas Geoffray <ngeoffray@google.com>
Sun, 12 Feb 2017 15:48:07 +0000 (15:48 +0000)
Getting on the bots:
+ERROR: Memory mapping for /data/run-test/test-6248/071-dexfile-map-clean-ex.jar is unexpectedly dirty
+Private_Dirty:         4 kB

Bug: 27650033

This reverts commit 19e5f834501c5e69fbd731038b88c10332cc6bc7.

Change-Id: I833ef95401187b764d336ab4558936c63678061c

runtime/dex_file.cc
runtime/zip_archive.cc
runtime/zip_archive.h
test/071-dexfile-map-clean/build [deleted file]
test/071-dexfile-map-clean/expected.txt [deleted file]
test/071-dexfile-map-clean/info.txt [deleted file]
test/071-dexfile-map-clean/run [deleted file]
test/071-dexfile-map-clean/src-ex/Another.java [deleted file]
test/071-dexfile-map-clean/src/Main.java [deleted file]
test/etc/default-build
test/run-test

index 187af4b..f59420d 100644 (file)
@@ -333,32 +333,7 @@ std::unique_ptr<const DexFile> DexFile::OpenOneDexFileFromZip(const ZipArchive&
     *error_code = ZipOpenErrorCode::kDexFileError;
     return nullptr;
   }
-
-  std::unique_ptr<MemMap> map;
-  if (zip_entry->IsUncompressed()) {
-    if (!zip_entry->IsAlignedTo(alignof(Header))) {
-      // Do not mmap unaligned ZIP entries because
-      // doing so would fail dex verification which requires 4 byte alignment.
-      LOG(WARNING) << "Can't mmap dex file " << location << "!" << entry_name << " directly; "
-                   << "please zipalign to " << alignof(Header) << " bytes. "
-                   << "Falling back to extracting file.";
-    } else {
-      // Map uncompressed files within zip as file-backed to avoid a dirty copy.
-      map.reset(zip_entry->MapDirectlyFromFile(location.c_str(), /*out*/error_msg));
-      if (map == nullptr) {
-        LOG(WARNING) << "Can't mmap dex file " << location << "!" << entry_name << " directly; "
-                     << "is your ZIP file corrupted? Falling back to extraction.";
-        // Try again with Extraction which still has a chance of recovery.
-      }
-    }
-  }
-
-  if (map == nullptr) {
-    // Default path for compressed ZIP entries,
-    // and fallback for stored ZIP entries.
-    map.reset(zip_entry->ExtractToMemMap(location.c_str(), entry_name, error_msg));
-  }
-
+  std::unique_ptr<MemMap> map(zip_entry->ExtractToMemMap(location.c_str(), entry_name, error_msg));
   if (map == nullptr) {
     *error_msg = StringPrintf("Failed to extract '%s' from '%s': %s", entry_name, location.c_str(),
                               error_msg->c_str());
@@ -525,11 +500,6 @@ DexFile::DexFile(const uint8_t* base,
       oat_dex_file_(oat_dex_file) {
   CHECK(begin_ != nullptr) << GetLocation();
   CHECK_GT(size_, 0U) << GetLocation();
-
-  // Check base (=header) alignment.
-  // Must be 4-byte aligned to avoid undefined behavior when accessing
-  // any of the sections via a pointer.
-  CHECK_ALIGNED(begin_, alignof(Header));
 }
 
 DexFile::~DexFile() {
index 416873f..cd79bb6 100644 (file)
 #include <unistd.h>
 #include <vector>
 
-#include "android-base/stringprintf.h"
 #include "base/unix_file/fd_file.h"
 
 namespace art {
 
-// Log file contents and mmap info when mapping entries directly.
-static constexpr const bool kDebugZipMapDirectly = false;
-
-using android::base::StringPrintf;
-
 uint32_t ZipEntry::GetUncompressedLength() {
   return zip_entry_->uncompressed_length;
 }
@@ -41,15 +35,6 @@ uint32_t ZipEntry::GetCrc32() {
   return zip_entry_->crc32;
 }
 
-bool ZipEntry::IsUncompressed() {
-  return zip_entry_->method == kCompressStored;
-}
-
-bool ZipEntry::IsAlignedTo(size_t alignment) {
-  DCHECK(IsPowerOfTwo(alignment)) << alignment;
-  return IsAlignedParam(zip_entry_->offset, static_cast<int>(alignment));
-}
-
 ZipEntry::~ZipEntry() {
   delete zip_entry_;
 }
@@ -88,102 +73,6 @@ MemMap* ZipEntry::ExtractToMemMap(const char* zip_filename, const char* entry_fi
   return map.release();
 }
 
-MemMap* ZipEntry::MapDirectlyFromFile(const char* zip_filename, std::string* error_msg) {
-  const int zip_fd = GetFileDescriptor(handle_);
-  const char* entry_filename = entry_name_.c_str();
-
-  // Should not happen since we don't have a memory ZipArchive constructor.
-  // However the underlying ZipArchive isn't required to have an FD,
-  // so check to be sure.
-  CHECK_GE(zip_fd, 0) <<
-      StringPrintf("Cannot map '%s' (in zip '%s') directly because the zip archive "
-                   "is not file backed.",
-                   entry_filename,
-                   zip_filename);
-
-  if (!IsUncompressed()) {
-    *error_msg = StringPrintf("Cannot map '%s' (in zip '%s') directly because it is compressed.",
-                              entry_filename,
-                              zip_filename);
-    return nullptr;
-  } else if (zip_entry_->uncompressed_length != zip_entry_->compressed_length) {
-    *error_msg = StringPrintf("Cannot map '%s' (in zip '%s') directly because "
-                              "entry has bad size (%u != %u).",
-                              entry_filename,
-                              zip_filename,
-                              zip_entry_->uncompressed_length,
-                              zip_entry_->compressed_length);
-    return nullptr;
-  }
-
-  std::string name(entry_filename);
-  name += " mapped directly in memory from ";
-  name += zip_filename;
-
-  const off_t offset = zip_entry_->offset;
-
-  if (kDebugZipMapDirectly) {
-    LOG(INFO) << "zip_archive: " << "make mmap of " << name << " @ offset = " << offset;
-  }
-
-  std::unique_ptr<MemMap> map(
-      MemMap::MapFileAtAddress(nullptr,  // Expected pointer address
-                               GetUncompressedLength(),  // Byte count
-                               PROT_READ | PROT_WRITE,
-                               MAP_PRIVATE,
-                               zip_fd,
-                               offset,
-                               false,  // Don't restrict allocation to lower4GB
-                               false,  // Doesn't overlap existing map (reuse=false)
-                               name.c_str(),
-                               /*out*/error_msg));
-
-  if (map == nullptr) {
-    DCHECK(!error_msg->empty());
-  }
-
-  if (kDebugZipMapDirectly) {
-    // Dump contents of file, same format as using this shell command:
-    // $> od -j <offset> -t x1 <zip_filename>
-    static constexpr const int kMaxDumpChars = 15;
-    lseek(zip_fd, 0, SEEK_SET);
-
-    int count = offset + kMaxDumpChars;
-
-    std::string tmp;
-    char buf;
-
-    // Dump file contents.
-    int i = 0;
-    while (read(zip_fd, &buf, 1) > 0 && i < count) {
-      tmp += StringPrintf("%3d ", (unsigned int)buf);
-      ++i;
-    }
-
-    LOG(INFO) << "map_fd raw bytes starting at 0";
-    LOG(INFO) << "" << tmp;
-    LOG(INFO) << "---------------------------";
-
-    // Dump map contents.
-    if (map != nullptr) {
-      tmp = "";
-
-      count = kMaxDumpChars;
-
-      uint8_t* begin = map->Begin();
-      for (i = 0; i < count; ++i) {
-        tmp += StringPrintf("%3d ", (unsigned int)begin[i]);
-      }
-
-      LOG(INFO) << "map address " << StringPrintf("%p", begin);
-      LOG(INFO) << "map first " << kMaxDumpChars << " chars:";
-      LOG(INFO) << tmp;
-    }
-  }
-
-  return map.release();
-}
-
 static void SetCloseOnExec(int fd) {
   // This dance is more portable than Linux's O_CLOEXEC open(2) flag.
   int flags = fcntl(fd, F_GETFD);
@@ -240,7 +129,7 @@ ZipEntry* ZipArchive::Find(const char* name, std::string* error_msg) const {
     return nullptr;
   }
 
-  return new ZipEntry(handle_, zip_entry.release(), name);
+  return new ZipEntry(handle_, zip_entry.release());
 }
 
 ZipArchive::~ZipArchive() {
index 1858444..42bf55c 100644 (file)
@@ -37,35 +37,19 @@ class MemMap;
 class ZipEntry {
  public:
   bool ExtractToFile(File& file, std::string* error_msg);
-  // Extract this entry to anonymous memory (R/W).
-  // Returns null on failure and sets error_msg.
   MemMap* ExtractToMemMap(const char* zip_filename, const char* entry_filename,
                           std::string* error_msg);
-  // Create a file-backed private (clean, R/W) memory mapping to this entry.
-  // 'zip_filename' is used for diagnostics only,
-  //   the original file that the ZipArchive was open with is used
-  //   for the mapping.
-  //
-  // Will only succeed if the entry is stored uncompressed.
-  // Returns null on failure and sets error_msg.
-  MemMap* MapDirectlyFromFile(const char* zip_filename, /*out*/std::string* error_msg);
   virtual ~ZipEntry();
 
   uint32_t GetUncompressedLength();
   uint32_t GetCrc32();
 
-  bool IsUncompressed();
-  bool IsAlignedTo(size_t alignment);
-
  private:
   ZipEntry(ZipArchiveHandle handle,
-           ::ZipEntry* zip_entry,
-           const std::string& entry_name)
-    : handle_(handle), zip_entry_(zip_entry), entry_name_(entry_name) {}
+           ::ZipEntry* zip_entry) : handle_(handle), zip_entry_(zip_entry) {}
 
   ZipArchiveHandle handle_;
   ::ZipEntry* const zip_entry_;
-  std::string const entry_name_;
 
   friend class ZipArchive;
   DISALLOW_COPY_AND_ASSIGN(ZipEntry);
diff --git a/test/071-dexfile-map-clean/build b/test/071-dexfile-map-clean/build
deleted file mode 100755 (executable)
index a171fc3..0000000
+++ /dev/null
@@ -1,21 +0,0 @@
-#!/bin/bash
-#
-# Copyright 2017 The Android Open Source Project
-#
-# Licensed under the Apache License, Version 2.0 (the "License");
-# you may not use this file except in compliance with the License.
-# You may obtain a copy of the License at
-#
-#      http://www.apache.org/licenses/LICENSE-2.0
-#
-# Unless required by applicable law or agreed to in writing, software
-# distributed under the License is distributed on an "AS IS" BASIS,
-# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-# See the License for the specific language governing permissions and
-# limitations under the License.
-
-# Any JAR files used by this test shall have their classes.dex be stored, NOT compressed.
-# This is needed for our test correctness which validates classes.dex are mapped file-backed.
-#
-# In addition, align to at least 4 bytes since that's the dex alignment requirement.
-./default-build "$@" --zip-compression-method store --zip-align 4
diff --git a/test/071-dexfile-map-clean/expected.txt b/test/071-dexfile-map-clean/expected.txt
deleted file mode 100644 (file)
index af7fb28..0000000
+++ /dev/null
@@ -1,3 +0,0 @@
-Another
-Secondary dexfile mmap is clean
-Another Instance
diff --git a/test/071-dexfile-map-clean/info.txt b/test/071-dexfile-map-clean/info.txt
deleted file mode 100644 (file)
index 7e45808..0000000
+++ /dev/null
@@ -1,11 +0,0 @@
-Exercise Dalvik-specific DEX file feature. Will not work on RI.
-
-If these conditions are met:
-* When we are loading in a secondary dex file
-* and when dex2oat is not used
-* and the dex file is stored uncompressed in a ZIP file
-
-Then check:
-* The dex file is memory-mapped file-backed as clean memory
-(i.e. there is no extraction step)
-
diff --git a/test/071-dexfile-map-clean/run b/test/071-dexfile-map-clean/run
deleted file mode 100755 (executable)
index b045109..0000000
+++ /dev/null
@@ -1,22 +0,0 @@
-#!/bin/bash
-#
-# Copyright 2017 The Android Open Source Project
-#
-# Licensed under the Apache License, Version 2.0 (the "License");
-# you may not use this file except in compliance with the License.
-# You may obtain a copy of the License at
-#
-#      http://www.apache.org/licenses/LICENSE-2.0
-#
-# Unless required by applicable law or agreed to in writing, software
-# distributed under the License is distributed on an "AS IS" BASIS,
-# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-# See the License for the specific language governing permissions and
-# limitations under the License.
-
-# Run without dex2oat so that we don't create oat/vdex files
-# when trying to load the secondary dex file.
-
-# In this way, the secondary dex file will be forced to be
-# loaded directly.
-./default-run "$@" --no-dex2oat
diff --git a/test/071-dexfile-map-clean/src-ex/Another.java b/test/071-dexfile-map-clean/src-ex/Another.java
deleted file mode 100644 (file)
index 58464a6..0000000
+++ /dev/null
@@ -1,21 +0,0 @@
-/*
- * Copyright (C) 2017 The Android Open Source Project
- *
- * Licensed under the Apache License, Version 2.0 (the "License");
- * you may not use this file except in compliance with the License.
- * You may obtain a copy of the License at
- *
- *      http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing, software
- * distributed under the License is distributed on an "AS IS" BASIS,
- * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
- * See the License for the specific language governing permissions and
- * limitations under the License.
- */
-
-public class Another {
-    public Another() {
-        System.out.println("Another Instance");
-    }
-}
diff --git a/test/071-dexfile-map-clean/src/Main.java b/test/071-dexfile-map-clean/src/Main.java
deleted file mode 100644 (file)
index 8a196dd..0000000
+++ /dev/null
@@ -1,147 +0,0 @@
-/*
- * Copyright (C) 2017 The Android Open Source Project
- *
- * Licensed under the Apache License, Version 2.0 (the "License");
- * you may not use this file except in compliance with the License.
- * You may obtain a copy of the License at
- *
- *      http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing, software
- * distributed under the License is distributed on an "AS IS" BASIS,
- * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
- * See the License for the specific language governing permissions and
- * limitations under the License.
- */
-
-import java.lang.reflect.Method;
-import java.util.Enumeration;
-
-import java.nio.file.Files;
-import java.nio.file.Paths;
-
-/**
- * DexFile tests (Dalvik-specific).
- */
-public class Main {
-    private static final String CLASS_PATH =
-        System.getenv("DEX_LOCATION") + "/071-dexfile-map-clean-ex.jar";
-
-    /**
-     * Prep the environment then run the test.
-     */
-    public static void main(String[] args) throws Exception {
-        // Load the dex file, this is a pre-requisite to mmap-ing it in.
-        Class<?> AnotherClass = testDexFile();
-        // Check that the memory maps are clean.
-        testDexMemoryMaps();
-
-        // Prevent garbage collector from collecting our DexFile
-        // (and unmapping too early) by using it after we finish
-        // our verification.
-        AnotherClass.newInstance();
-    }
-
-    private static boolean checkSmapsEntry(String[] smapsLines, int offset) {
-      String nameDescription = smapsLines[offset];
-      String[] split = nameDescription.split(" ");
-
-      String permissions = split[1];
-      // Mapped as read-only + anonymous.
-      if (!permissions.startsWith("r--p")) {
-        return false;
-      }
-
-      boolean validated = false;
-
-      // We have the right entry, now make sure it's valid.
-      for (int i = offset; i < smapsLines.length; ++i) {
-        String line = smapsLines[i];
-
-        if (line.startsWith("Shared_Dirty") || line.startsWith("Private_Dirty")) {
-          String lineTrimmed = line.trim();
-          String[] lineSplit = lineTrimmed.split(" +");
-
-          String sizeUsuallyInKb = lineSplit[lineSplit.length - 2];
-
-          sizeUsuallyInKb = sizeUsuallyInKb.trim();
-
-          if (!sizeUsuallyInKb.equals("0")) {
-            System.out.println(
-                "ERROR: Memory mapping for " + CLASS_PATH + " is unexpectedly dirty");
-            System.out.println(line);
-          } else {
-            validated = true;
-          }
-        }
-
-        // VmFlags marks the "end" of an smaps entry.
-        if (line.startsWith("VmFlags")) {
-          break;
-        }
-      }
-
-      if (validated) {
-        System.out.println("Secondary dexfile mmap is clean");
-      } else {
-        System.out.println("ERROR: Memory mapping is missing Shared_Dirty/Private_Dirty entries");
-      }
-
-      return true;
-    }
-
-    // This test takes relies on dex2oat being skipped.
-    // (enforced in 'run' file by using '--no-dex2oat'
-    //
-    // This could happen in a non-test situation
-    // if a secondary dex file is loaded (but not yet maintenance-mode compiled)
-    // with JIT.
-    //
-    // Or it could also happen if a secondary dex file is loaded and forced
-    // into running into the interpreter (e.g. duplicate classes).
-    //
-    // Rather than relying on those weird fallbacks,
-    // we force the runtime not to dex2oat the dex file to ensure
-    // this test is repeatable and less brittle.
-    private static void testDexMemoryMaps() throws Exception {
-        // Ensure that the secondary dex file is mapped clean (directly from JAR file).
-        String smaps = new String(Files.readAllBytes(Paths.get("/proc/self/smaps")));
-
-        String[] smapsLines = smaps.split("\n");
-        boolean found = true;
-        for (int i = 0; i < smapsLines.length; ++i) {
-          if (smapsLines[i].contains(CLASS_PATH)) {
-            if (checkSmapsEntry(smapsLines, i)) {
-              return;
-            } // else we found the wrong one, keep going.
-          }
-        }
-
-        // Error case:
-        System.out.println("Could not find " + CLASS_PATH + " RO-anonymous smaps entry");
-        System.out.println(smaps);
-    }
-
-    private static Class<?> testDexFile() throws Exception {
-        ClassLoader classLoader = Main.class.getClassLoader();
-        Class<?> DexFile = classLoader.loadClass("dalvik.system.DexFile");
-        Method DexFile_loadDex = DexFile.getMethod("loadDex",
-                                                   String.class,
-                                                   String.class,
-                                                   Integer.TYPE);
-        Method DexFile_entries = DexFile.getMethod("entries");
-        Object dexFile = DexFile_loadDex.invoke(null, CLASS_PATH, null, 0);
-        Enumeration<String> e = (Enumeration<String>) DexFile_entries.invoke(dexFile);
-        while (e.hasMoreElements()) {
-            String className = e.nextElement();
-            System.out.println(className);
-        }
-
-        Method DexFile_loadClass = DexFile.getMethod("loadClass",
-                                                     String.class,
-                                                     ClassLoader.class);
-        Class<?> AnotherClass = (Class<?>)DexFile_loadClass.invoke(dexFile,
-            "Another", Main.class.getClassLoader());
-        return AnotherClass;
-    }
-}
index 330a32e..e9e3886 100755 (executable)
@@ -60,12 +60,6 @@ else
   HAS_SRC_DEX2OAT_UNRESOLVED=false
 fi
 
-# Allow overriding ZIP_COMPRESSION_METHOD with e.g. 'store'
-ZIP_COMPRESSION_METHOD="deflate"
-# Align every ZIP file made by calling $ZIPALIGN command?
-WITH_ZIP_ALIGN=false
-ZIP_ALIGN_BYTES="-1"
-
 DX_FLAGS=""
 SKIP_DX_MERGER="false"
 EXPERIMENTAL=""
@@ -124,17 +118,6 @@ while true; do
     DEFAULT_EXPERIMENT=""
     EXPERIMENTAL="${EXPERIMENTAL} $1"
     shift
-  elif [ "x$1" = "x--zip-compression-method" ]; then
-    # Allow using different zip compression method, e.g. 'store'
-    shift
-    ZIP_COMPRESSION_METHOD="$1"
-    shift
-  elif [ "x$1" = "x--zip-align" ]; then
-    # Align ZIP entries to some # of bytes.
-    shift
-    WITH_ZIP_ALIGN=true
-    ZIP_ALIGN_BYTES="$1"
-    shift
   elif expr "x$1" : "x--" >/dev/null 2>&1; then
     echo "unknown $0 option: $1" 1>&2
     exit 1
@@ -163,26 +146,6 @@ for experiment in ${EXPERIMENTAL}; do
   JAVAC_ARGS="${JAVAC_ARGS} ${JAVAC_EXPERIMENTAL_ARGS[${experiment}]}"
 done
 
-#########################################
-
-# Catch all commands to 'ZIP' and prepend extra flags.
-# Optionally, zipalign results to some alignment.
-function zip() {
-  local zip_target="$1"
-  local entry_src="$2"
-  shift 2
-
-  command zip --compression-method "$ZIP_COMPRESSION_METHOD" "$zip_target" "$entry_src" "$@"
-
-  if "$WITH_ZIP_ALIGN"; then
-    # zipalign does not operate in-place, so write results to a temp file.
-    local tmp_file="$(mktemp)"
-    "$ZIPALIGN" -f "$ZIP_ALIGN_BYTES" "$zip_target" "$tmp_file"
-    # replace original zip target with our temp file.
-    mv "$tmp_file" "$zip_target"
-  fi
-}
-
 if [ -e classes.dex ]; then
   zip $TEST_NAME.jar classes.dex
   exit 0
index 4936039..27c700e 100755 (executable)
@@ -90,22 +90,6 @@ fi
 
 export JACK="$JACK -g -cp $JACK_CLASSPATH"
 
-# Zipalign is not on the PATH in some configs, auto-detect it.
-if [ -z "$ZIPALIGN" ]; then
-  if which zipalign >/dev/null; then
-    ZIPALIGN="zipalign";
-  else
-    # TODO: Add a dependency for zipalign in Android.run-test.mk
-    # once it doesn't depend on libandroidfw (b/35246701)
-    case "$OSTYPE" in
-      darwin*)  ZIPALIGN="$ANDROID_BUILD_TOP/prebuilts/sdk/tools/darwin/bin/zipalign" ;;
-      linux*)   ZIPALIGN="$ANDROID_BUILD_TOP/prebuilts/sdk/tools/linux/bin/zipalign" ;;
-      *)        echo "Can't find zipalign: unknown: $OSTYPE" >&2;;
-    esac
-  fi
-fi
-export ZIPALIGN
-
 info="info.txt"
 build="build"
 run="run"