From 9092303c358b87c1e027abafe135c44a27531b36 Mon Sep 17 00:00:00 2001 From: Googler Date: Wed, 28 Jun 2023 12:50:11 -0700 Subject: [PATCH] Group synthetic classes with their context classes in DexFileSplitter so that synthetic classes don't end up without their context classes in dex shards, which will causing merging to fail in DexFileMerger. Fixes #16368. RELNOTES: None PiperOrigin-RevId: 544134712 Change-Id: Ib29f6659f18dd71be96a7985bc25cfb44e719ae5 --- .../android/dexer/DexLimitTrackerTest.java | 45 +++-- src/test/shell/bazel/android/BUILD | 14 ++ .../DexFileSplitter_synthetic_classes_test.sh | 142 +++++++++++++++ .../android/dexer/DexFileAggregator.java | 3 +- .../build/android/dexer/DexFileSplitter.java | 163 ++++++++++++++---- .../build/android/dexer/DexLimitTracker.java | 12 +- 6 files changed, 325 insertions(+), 54 deletions(-) create mode 100755 src/test/shell/bazel/android/DexFileSplitter_synthetic_classes_test.sh 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 c6674fa6f65156..db8ee37d3105df 100644 --- a/src/test/shell/bazel/android/BUILD +++ b/src/test/shell/bazel/android/BUILD @@ -184,3 +184,17 @@ android_sh_test( "//src/test/shell/bazel:test-deps", ], ) + +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));