diff --git a/src/test/java/com/google/devtools/build/android/dexer/DexLimitTrackerTest.java b/src/test/java/com/google/devtools/build/android/dexer/DexLimitTrackerTest.java index f88ca36ab64709..9e93a0d9a67591 100644 --- a/src/test/java/com/google/devtools/build/android/dexer/DexLimitTrackerTest.java +++ b/src/test/java/com/google/devtools/build/android/dexer/DexLimitTrackerTest.java @@ -42,46 +42,61 @@ public void setUp() throws Exception { public void testUnderLimit() { DexLimitTracker tracker = new DexLimitTracker(Math.max(dex.methodIds().size(), dex.fieldIds().size())); - assertThat(tracker.track(dex)).isFalse(); + tracker.track(dex); + assertThat(tracker.outsideLimits()).isFalse(); } @Test public void testOverLimit() throws Exception { DexLimitTracker tracker = new DexLimitTracker(Math.max(dex.methodIds().size(), dex.fieldIds().size()) - 1); - assertThat(tracker.track(dex)).isTrue(); - assertThat(tracker.track(dex)).isTrue(); - assertThat(tracker.track(DexFiles.toDex(convertClass(DexLimitTracker.class)))).isTrue(); + tracker.track(dex); + assertThat(tracker.outsideLimits()).isTrue(); + tracker.track(dex); + assertThat(tracker.outsideLimits()).isTrue(); + tracker.track(DexFiles.toDex(convertClass(DexLimitTracker.class))); + assertThat(tracker.outsideLimits()).isTrue(); } @Test public void testRepeatedReferencesDeduped() throws Exception { DexLimitTracker tracker = new DexLimitTracker(Math.max(dex.methodIds().size(), dex.fieldIds().size())); - assertThat(tracker.track(dex)).isFalse(); - assertThat(tracker.track(dex)).isFalse(); - assertThat(tracker.track(dex)).isFalse(); - assertThat(tracker.track(dex)).isFalse(); - assertThat(tracker.track(DexFiles.toDex(convertClass(DexLimitTracker.class)))).isTrue(); - assertThat(tracker.track(dex)).isTrue(); + tracker.track(dex); + assertThat(tracker.outsideLimits()).isFalse(); + tracker.track(dex); + assertThat(tracker.outsideLimits()).isFalse(); + tracker.track(dex); + assertThat(tracker.outsideLimits()).isFalse(); + tracker.track(dex); + assertThat(tracker.outsideLimits()).isFalse(); + tracker.track(DexFiles.toDex(convertClass(DexLimitTracker.class))); + assertThat(tracker.outsideLimits()).isTrue(); + tracker.track(dex); + assertThat(tracker.outsideLimits()).isTrue(); } @Test public void testGoOverLimit() throws Exception { DexLimitTracker tracker = new DexLimitTracker(Math.max(dex.methodIds().size(), dex.fieldIds().size())); - assertThat(tracker.track(dex)).isFalse(); - assertThat(tracker.track(DexFiles.toDex(convertClass(DexLimitTracker.class)))).isTrue(); + tracker.track(dex); + assertThat(tracker.outsideLimits()).isFalse(); + tracker.track(DexFiles.toDex(convertClass(DexLimitTracker.class))); + assertThat(tracker.outsideLimits()).isTrue(); } @Test public void testClear() throws Exception { DexLimitTracker tracker = new DexLimitTracker(Math.max(dex.methodIds().size(), dex.fieldIds().size())); - assertThat(tracker.track(dex)).isFalse(); - assertThat(tracker.track(DexFiles.toDex(convertClass(DexLimitTracker.class)))).isTrue(); + tracker.track(dex); + assertThat(tracker.outsideLimits()).isFalse(); + tracker.track(DexFiles.toDex(convertClass(DexLimitTracker.class))); + assertThat(tracker.outsideLimits()).isTrue(); tracker.clear(); - assertThat(tracker.track(dex)).isFalse(); + tracker.track(dex); + assertThat(tracker.outsideLimits()).isFalse(); } private static DexFile convertClass(Class clazz) throws Exception { diff --git a/src/test/shell/bazel/android/BUILD b/src/test/shell/bazel/android/BUILD index f0b63e53795237..4a4d14d1f4672b 100644 --- a/src/test/shell/bazel/android/BUILD +++ b/src/test/shell/bazel/android/BUILD @@ -233,3 +233,17 @@ android_sh_test( # See https://github.com/bazelbuild/bazel/issues/8235 tags = ["no-remote"], ) + +android_sh_test( + name = "DexFileSplitter_synthetic_classes_test", + size = "medium", + srcs = ["DexFileSplitter_synthetic_classes_test.sh"], + data = [ + ":android_helper", + "//external:android_sdk_for_testing", + "//src/test/shell/bazel:test-deps", + ], + tags = [ + "no_windows", + ], +) diff --git a/src/test/shell/bazel/android/DexFileSplitter_synthetic_classes_test.sh b/src/test/shell/bazel/android/DexFileSplitter_synthetic_classes_test.sh new file mode 100755 index 00000000000000..bd70e9f4533437 --- /dev/null +++ b/src/test/shell/bazel/android/DexFileSplitter_synthetic_classes_test.sh @@ -0,0 +1,142 @@ +#!/bin/bash +# +# Copyright 2023 The Bazel Authors. All rights reserved. +# +# 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. + +# For these tests to run do the following: +# +# 1. Install an Android SDK from https://developer.android.com +# 2. Set the $ANDROID_HOME environment variable +# 3. Uncomment the line in WORKSPACE containing android_sdk_repository +# +# Note that if the environment is not set up as above android_integration_test +# will silently be ignored and will be shown as passing. + +# Load the test setup defined in the parent directory +CURRENT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" + +source "${CURRENT_DIR}/android_helper.sh" \ + || { echo "android_helper.sh not found!" >&2; exit 1; } +fail_if_no_android_sdk + +source "${CURRENT_DIR}/../../integration_test_setup.sh" \ + || { echo "integration_test_setup.sh not found!" >&2; exit 1; } + +resolve_android_toolchains "$1" + +function test_DexFileSplitter_synthetic_classes_crossing_dexfiles() { + create_new_workspace + setup_android_sdk_support + + mkdir -p java/com/testapp + + cat > java/com/testapp/AndroidManifest.xml < + + + + + + + + + + + + +EOF + + cat > java/com/testapp/MainActivity.java < java/com/testapp/BigLib.java + + cat > java/com/testapp/BUILD < $i," + done + + echo " };" + echo " }" + echo " }" + echo "}" +} + +run_suite "Tests for DexFileSplitter with synthetic classes crossing dexfiles" \ No newline at end of file diff --git a/src/tools/android/java/com/google/devtools/build/android/dexer/DexFileAggregator.java b/src/tools/android/java/com/google/devtools/build/android/dexer/DexFileAggregator.java index e3fed0a1d1e3e9..c6f88b73d79d3f 100644 --- a/src/tools/android/java/com/google/devtools/build/android/dexer/DexFileAggregator.java +++ b/src/tools/android/java/com/google/devtools/build/android/dexer/DexFileAggregator.java @@ -82,7 +82,8 @@ public DexFileAggregator add(Dex dexFile) { if (multidex.isMultidexAllowed()) { // To determine whether currentShard is "full" we track unique field and method signatures, // which predicts precisely the number of field and method indices. - if (tracker.track(dexFile) && !currentShard.isEmpty()) { + tracker.track(dexFile); + if (tracker.outsideLimits() && !currentShard.isEmpty()) { // For simplicity just start a new shard to fit the given file. // Don't bother with waiting for a later file that might fit the old shard as in the extreme // we'd have to wait until the end to write all shards. diff --git a/src/tools/android/java/com/google/devtools/build/android/dexer/DexFileSplitter.java b/src/tools/android/java/com/google/devtools/build/android/dexer/DexFileSplitter.java index 4e684c87543b3d..3dd844d30ba485 100644 --- a/src/tools/android/java/com/google/devtools/build/android/dexer/DexFileSplitter.java +++ b/src/tools/android/java/com/google/devtools/build/android/dexer/DexFileSplitter.java @@ -14,6 +14,7 @@ package com.google.devtools.build.android.dexer; import static com.google.common.base.Preconditions.checkArgument; +import static com.google.common.base.Preconditions.checkNotNull; import static com.google.common.base.Preconditions.checkState; import static java.nio.charset.StandardCharsets.UTF_8; @@ -21,8 +22,8 @@ import com.android.dex.DexFormat; import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Predicates; -import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; +import com.google.common.collect.TreeMultimap; import com.google.common.io.ByteStreams; import com.google.common.io.Closer; import com.google.devtools.build.android.Converters.ExistingPathConverter; @@ -40,13 +41,17 @@ import java.nio.file.Files; import java.nio.file.Path; import java.nio.file.StandardOpenOption; -import java.util.Comparator; -import java.util.LinkedHashMap; +import java.util.ArrayList; +import java.util.HashSet; import java.util.List; import java.util.Map; +import java.util.Scanner; +import java.util.Set; +import java.util.TreeMap; import java.util.function.Predicate; import java.util.zip.ZipEntry; import java.util.zip.ZipFile; +import javax.annotation.Nullable; /** * Shuffles .class.dex files from input archives into 1 or more archives each to be merged into a @@ -159,32 +164,43 @@ static void splitIntoShards(Options options) throws IOException { try (Closer closer = Closer.create(); DexFileSplitter out = new DexFileSplitter(options.outputDirectory, options.maxNumberOfIdxPerDex)) { + // 1. Scan inputs in order and keep first occurrence of each class, keeping all zips open. // We don't process anything yet so we can shard in sorted order, which is what dx would do // if presented with a single jar containing all the given inputs. // TODO(kmb): Abandon alphabetic sorting to process each input fully before moving on (still // requires scanning inputs twice for main dex list). + Predicate inclusionFilter = ZipEntryPredicates.suffixes(".dex", ".class"); if (expected != null) { inclusionFilter = inclusionFilter.and(e -> expected.contains(e.getName())); } - LinkedHashMap deduped = new LinkedHashMap<>(); + + // Maps a dex file name to the zip file containing that dex file. + TreeMap dexFilesAndContainingZip = + new TreeMap<>(ZipEntryComparator::compareClassNames); + // Maps a class to its synthetic classes, if any. + TreeMultimap contextClassesToSyntheticClasses = TreeMultimap.create(); + for (Path inputArchive : options.inputArchives) { ZipFile zip = closer.register(new ZipFile(inputArchive.toFile())); + + // synthetic-contexts.map is generated by CompatDexBuilder. + ZipEntry syntheticContextsZipEntry = zip.getEntry("META-INF/synthetic-contexts.map"); + if (syntheticContextsZipEntry != null) { + parseSyntheticContextsMap( + zip.getInputStream(syntheticContextsZipEntry), contextClassesToSyntheticClasses); + } + zip.stream() .filter(inclusionFilter) - .forEach(e -> deduped.putIfAbsent(e.getName(), zip)); + .forEach(e -> dexFilesAndContainingZip.putIfAbsent(e.getName(), zip)); } - ImmutableList> files = - deduped - .entrySet() - .stream() - .sorted(Comparator.comparing(e -> e.getKey(), ZipEntryComparator::compareClassNames)) - .collect(ImmutableList.toImmutableList()); // 2. Process each class in desired order, rolling from shard to shard as needed. if (classesInMainDex == null || classesInMainDex.isEmpty()) { - out.processDexFiles(files, Predicates.alwaysTrue()); + out.processDexes( + dexFilesAndContainingZip, contextClassesToSyntheticClasses, Predicates.alwaysTrue()); } else { checkArgument(classesInMainDex.stream().noneMatch(s -> s.startsWith("j$/")), "%s lists classes in package 'j$', which can't be included in classes.dex and can " @@ -194,14 +210,15 @@ static void splitIntoShards(Options options) throws IOException { // 1. process only the classes listed in the given file // 2. process the remaining files Predicate mainDexFilter = ZipEntryPredicates.classFileNameFilter(classesInMainDex); - out.processDexFiles(files, mainDexFilter); + out.processDexes(dexFilesAndContainingZip, contextClassesToSyntheticClasses, mainDexFilter); // Fail if main_dex_list is too big, following dx's example checkState(out.shardsWritten() == 0, "Too many classes listed in main dex list file " + "%s, main dex capacity exceeded", options.mainDexListFile); if (options.minimalMainDex) { out.nextShard(); // Start new .dex file if requested } - out.processDexFiles(files, mainDexFilter.negate()); + out.processDexes( + dexFilesAndContainingZip, contextClassesToSyntheticClasses, mainDexFilter.negate()); } } } @@ -215,6 +232,23 @@ private static ImmutableSet expectedEntries(Path filterJar) throws IOExc } } + private static void parseSyntheticContextsMap( + InputStream inputStream, TreeMultimap syntheticClassContexts) { + Scanner scanner = new Scanner(inputStream, UTF_8); + scanner.useDelimiter("[;\n]"); + while (scanner.hasNext()) { + String syntheticClass = scanner.next(); + String context = scanner.next(); + // DexFileSplitter mostly expects filenames which all end in .class.dex, while the synthetic + // context map has class names, so add the extension here to make this easier to work with in + // the rest of the code. + syntheticClassContexts.put( + context + CLASS_DEX_EXTENSION, syntheticClass + CLASS_DEX_EXTENSION); + } + } + + private static final String CLASS_DEX_EXTENSION = ".class.dex"; + private final int maxNumberOfIdxPerDex; private final Path outputDirectory; /** Collect written zip files so we can conveniently wait for all of them to close when done. */ @@ -269,23 +303,31 @@ public void close() throws IOException { closer.close(); } - private void processDexFiles( - ImmutableList> filesToProcess, Predicate filter) + private void processDexes( + Map dexFilesAndContainingZip, + TreeMultimap contextClassesToSyntheticClasses, + Predicate filter) throws IOException { - for (Map.Entry entry : filesToProcess) { + + Set syntheticClasses = new HashSet<>(contextClassesToSyntheticClasses.values()); + for (Map.Entry entry : dexFilesAndContainingZip.entrySet()) { String filename = entry.getKey(); if (filter.test(filename)) { - ZipFile zipFile = entry.getValue(); - processDexEntry(zipFile, zipFile.getEntry(filename)); + // Synthetic classes will be gathered with their context classes and added to the dex file + // all together as a unit, so skip them here. + if (!syntheticClasses.contains(filename)) { + ZipFile zipFile = entry.getValue(); + processDex(zipFile, filename, contextClassesToSyntheticClasses.get(filename)); + } } } } - private void processDexEntry(ZipFile zip, ZipEntry entry) throws IOException { - String filename = entry.getName(); - checkState(filename.endsWith(".class.dex"), - "%s isn't a dex archive: %s", zip.getName(), filename); - checkState(entry.getMethod() == ZipEntry.STORED, "Expect to process STORED: %s", filename); + private void processDex(ZipFile zip, String filename, Set syntheticClasses) + throws IOException { + + // Synthetic classes base their names on their context classes, so this check only needs to be + // done for the context class. if (inCoreLib == null) { inCoreLib = filename.startsWith("j$/"); } else if (inCoreLib != filename.startsWith("j$/")) { @@ -299,6 +341,53 @@ private void processDexEntry(ZipFile zip, ZipEntry entry) throws IOException { filename); } + List zipEntryDexAndContents = new ArrayList<>(); + ZipEntryDexAndContent contextZdc = processDex(zip, filename); + checkNotNull(contextZdc, "Context class %s expected to be in %s", filename, zip.getName()); + zipEntryDexAndContents.add(contextZdc); + + for (String syntheticClass : syntheticClasses) { + ZipEntryDexAndContent syntheticClassZdc = processDex(zip, syntheticClass); + // Some synthetic classes are contained within the same dex as their enclosing class, + // so they won't be standalone dexes in the zip file, and some synthetic classes are present + // in synthetic-contexts.map but aren't standalone dexes in the zip nor are they in the + // dex with their enclosing class, so just skip these. + if (syntheticClassZdc != null) { + zipEntryDexAndContents.add(syntheticClassZdc); + } + } + + if (tracker.outsideLimits()) { + nextShard(); + for (ZipEntryDexAndContent zdc : zipEntryDexAndContents) { + tracker.track(zdc.dex); + } + checkState( + !tracker.outsideLimits(), + "Impossible to fit %s and all of its synthetic classes (count: %s) in a single shard", + filename, + syntheticClasses.size()); + } + + for (ZipEntryDexAndContent zdc : zipEntryDexAndContents) { + curOut.writeAsync(zdc.zipEntry, zdc.content); + } + } + + @Nullable + private ZipEntryDexAndContent processDex(ZipFile zip, String filename) throws IOException { + ZipEntry entry = zip.getEntry(filename); + if (entry == null) { + return null; + } + + checkState( + filename.endsWith(CLASS_DEX_EXTENSION), + "%s isn't a dex archive: %s", + zip.getName(), + filename); + checkState(entry.getMethod() == ZipEntry.STORED, "Expect to process STORED: %s", filename); + try (InputStream entryStream = zip.getInputStream(entry)) { // We don't want to use the Dex(InputStream) constructor because it closes the stream, // which will break the for loop, and it has its own bespoke way of reading the file into @@ -306,15 +395,27 @@ private void processDexEntry(ZipFile zip, ZipEntry entry) throws IOException { // TODO(kmb) since entry is stored, mmap content and give to Dex(ByteBuffer) and output zip byte[] content = new byte[(int) entry.getSize()]; ByteStreams.readFully(entryStream, content); // throws if file is smaller than expected - checkState(entryStream.read() == -1, - "Too many bytes in jar entry %s, expected %s", entry, entry.getSize()); + checkState( + entryStream.read() == -1, + "Too many bytes in jar entry %s, expected %s", + entry, + entry.getSize()); Dex dexFile = new Dex(content); - if (tracker.track(dexFile)) { - nextShard(); - tracker.track(dexFile); - } - curOut.writeAsync(entry, content); + tracker.track(dexFile); + return new ZipEntryDexAndContent(entry, content, dexFile); + } + } + + private static final class ZipEntryDexAndContent { + final ZipEntry zipEntry; + final byte[] content; + final Dex dex; + + ZipEntryDexAndContent(ZipEntry zipEntry, byte[] content, Dex dex) { + this.zipEntry = zipEntry; + this.content = content; + this.dex = dex; } } } diff --git a/src/tools/android/java/com/google/devtools/build/android/dexer/DexLimitTracker.java b/src/tools/android/java/com/google/devtools/build/android/dexer/DexLimitTracker.java index a0bfb51dfd054e..c8383e367d697e 100644 --- a/src/tools/android/java/com/google/devtools/build/android/dexer/DexLimitTracker.java +++ b/src/tools/android/java/com/google/devtools/build/android/dexer/DexLimitTracker.java @@ -38,14 +38,12 @@ public DexLimitTracker(int maxNumberOfIdxPerDex) { } /** - * Tracks the field and method references in the given file and returns whether we're within - * limits. + * Returns whether we're within limits. * - * @return {@code true} if method or field references are outside limits, {@code false} both - * are within limits. + * @return {@code true} if method or field references are outside limits, {@code false} both are + * within limits. */ - public boolean track(Dex dexFile) { - trackFieldsAndMethods(dexFile); + public boolean outsideLimits() { return fieldsSeen.size() > maxNumberOfIdxPerDex || methodsSeen.size() > maxNumberOfIdxPerDex; } @@ -55,7 +53,7 @@ public void clear() { methodsSeen.clear(); } - private void trackFieldsAndMethods(Dex dexFile) { + public void track(Dex dexFile) { int fieldCount = dexFile.fieldIds().size(); for (int fieldIndex = 0; fieldIndex < fieldCount; ++fieldIndex) { fieldsSeen.add(FieldDescriptor.fromDex(dexFile, fieldIndex));