From 3a8c07699fd33889cd7c27cb0885d480553f4c52 Mon Sep 17 00:00:00 2001 From: Alex Humesky Date: Thu, 29 Jun 2023 04:07:09 -0400 Subject: [PATCH] Cherrypick fixes for #16368 into Bazel 6.3.0 (#18808) * Update D8/R8 dependency in bazel to 8.0.40 PiperOrigin-RevId: 520331100 Change-Id: Ia150c637747c980fc5cdfa07c80edd3a9e30e04b * Add options converters specific to R8 to avoid depending on android_builder_lib which causes classpath conflicts with D8 classes. RELNOTE: None PiperOrigin-RevId: 526149555 Change-Id: Iacfce24cc3d2327ef63c47f3bfd76813beaefc7a * Add android_gmaven_r8 to bazel's WORKSPACE file so that it overrides the version of R8 that might come from android_remote_tools.WORKSPACE in a released version of bazel used to run the test (which would be potentially out of date with respect to the source code). Also merge this with android_gmaven_r8_for_testing since they would be redundant. RELNOTES: None. PiperOrigin-RevId: 529830525 Change-Id: Ic5c18ce5beb7fb915b084dc19590a05b04675548 * Collect the contexts of D8 compiler-synthesized classes. This completes steps 2 and 3 in b/241351268#comment9 towards resolving https://github.com/bazelbuild/bazel/issues/16368 PiperOrigin-RevId: 530238344 Change-Id: I569bf8e3bfa81f7005f3e9b79338d5a5b868e339 * 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 --------- Co-authored-by: Googler --- WORKSPACE | 28 ++- distdir.bzl | 20 ++- distdir_deps.bzl | 11 ++ src/BUILD | 2 +- .../android/android_remote_tools.WORKSPACE | 4 +- .../android/dexer/DexLimitTrackerTest.java | 45 +++-- .../google/devtools/build/android/r8/BUILD | 20 +++ .../android/r8/CompatDexBuilderTest.java | 40 ++++- .../android/r8/testdata/lambda/Lambda.java | 30 ++++ src/test/shell/bazel/android/BUILD | 14 ++ .../DexFileSplitter_synthetic_classes_test.sh | 142 +++++++++++++++ src/test/shell/testenv.sh.tmpl | 2 +- .../android/dexer/DexFileAggregator.java | 3 +- .../build/android/dexer/DexFileSplitter.java | 163 ++++++++++++++---- .../build/android/dexer/DexLimitTracker.java | 12 +- .../google/devtools/build/android/r8/BUILD | 23 ++- .../build/android/r8/CompatDexBuilder.java | 60 +++++++ .../build/android/r8/CoreLibraryDesugar.java | 4 +- .../devtools/build/android/r8/Desugar.java | 4 +- .../build/android/r8/DexFileMerger.java | 4 +- .../build/android/r8/OptionsConverters.java | 70 ++++++++ tools/android/android_extensions.bzl | 4 +- 22 files changed, 619 insertions(+), 86 deletions(-) create mode 100644 src/test/java/com/google/devtools/build/android/r8/testdata/lambda/Lambda.java create mode 100755 src/test/shell/bazel/android/DexFileSplitter_synthetic_classes_test.sh create mode 100644 src/tools/android/java/com/google/devtools/build/android/r8/OptionsConverters.java diff --git a/WORKSPACE b/WORKSPACE index ff47163e724ec2..6b142957a35fa3 100644 --- a/WORKSPACE +++ b/WORKSPACE @@ -1,7 +1,7 @@ workspace(name = "io_bazel") load("//tools/build_defs/repo:http.bzl", "http_archive", "http_file", "http_jar") -load("//:distdir.bzl", "dist_http_archive", "dist_http_file", "distdir_tar") +load("//:distdir.bzl", "dist_http_archive", "dist_http_file", "dist_http_jar", "distdir_tar") load("//:distdir_deps.bzl", "DIST_DEPS") # These can be used as values for the patch_cmds and patch_cmds_win attributes @@ -126,20 +126,20 @@ distdir_tar( archives = [ "android_tools_pkg-0.27.0.tar.gz", # for android_gmaven_r8 - "r8-3.3.28.jar", + "r8-8.0.40.jar", ], dirname = "derived/distdir", dist_deps = {dep: attrs for dep, attrs in DIST_DEPS.items() if "additional_distfiles" in attrs["used_in"]}, sha256 = { "android_tools_pkg-0.27.0.tar.gz": "1afa4b7e13c82523c8b69e87f8d598c891ec7e2baa41d9e24e08becd723edb4d", - "r8-3.3.28.jar": "8626ca32fb47aba7fddd2c897615e2e8ffcdb4d4b213572a2aefb3f838f01972", + "r8-8.0.40.jar": "ab1379835c7d3e5f21f80347c3c81e2f762e0b9b02748ae5232c3afa14adf702", }, urls = { "android_tools_pkg-0.27.0.tar.gz": [ "https://mirror.bazel.build/bazel_android_tools/android_tools_pkg-0.27.0.tar.gz", ], - "r8-3.3.28.jar": [ - "https://maven.google.com/com/android/tools/r8/3.3.28/r8-3.3.28.jar", + "r8-8.0.40.jar": [ + "https://maven.google.com/com/android/tools/r8/8.0.40/r8-8.0.40.jar", ], }, ) @@ -360,21 +360,16 @@ distdir_tar( name = "test_WORKSPACE_files", archives = [ "android_tools_pkg-0.27.0.tar.gz", - "r8-3.3.28.jar", ], dirname = "test_WORKSPACE/distdir", dist_deps = {dep: attrs for dep, attrs in DIST_DEPS.items() if "test_WORKSPACE_files" in attrs["used_in"]}, sha256 = { "android_tools_pkg-0.27.0.tar.gz": "1afa4b7e13c82523c8b69e87f8d598c891ec7e2baa41d9e24e08becd723edb4d", - "r8-3.3.28.jar": "8626ca32fb47aba7fddd2c897615e2e8ffcdb4d4b213572a2aefb3f838f01972", }, urls = { "android_tools_pkg-0.27.0.tar.gz": [ "https://mirror.bazel.build/bazel_android_tools/android_tools_pkg-0.27.0.tar.gz", ], - "r8-3.3.28.jar": [ - "https://maven.google.com/com/android/tools/r8/3.3.28/r8-3.3.28.jar", - ], }, ) @@ -413,12 +408,13 @@ http_archive( url = "https://mirror.bazel.build/bazel_android_tools/android_tools_pkg-0.27.0.tar.gz", ) -# This must be kept in sync with src/main/java/com/google/devtools/build/lib/bazel/rules/android/android_remote_tools.WORKSPACE -# and tools/android/android_extensions.bzl -http_jar( - name = "android_gmaven_r8_for_testing", - sha256 = "8626ca32fb47aba7fddd2c897615e2e8ffcdb4d4b213572a2aefb3f838f01972", - url = "https://maven.google.com/com/android/tools/r8/3.3.28/r8-3.3.28.jar", +# This is here to override the android_gmaven_r8 rule from +# src/main/java/com/google/devtools/build/lib/bazel/rules/android/android_remote_tools.WORKSPACE +# so that tests like src/test/java/com/google/devtools/build/android/r8:AllTests +# use the most recent version of R8 rather than the one might be referenced in a released +# version of bazel that might have an outdated android_remote_tools.WORKSPACE relative to the tests. +dist_http_jar( + name = "android_gmaven_r8", ) dist_http_archive( diff --git a/distdir.bzl b/distdir.bzl index b4dab33424830d..d01467cc72cd1c 100644 --- a/distdir.bzl +++ b/distdir.bzl @@ -14,7 +14,7 @@ """Defines a repository rule that generates an archive consisting of the specified files to fetch""" load("//:distdir_deps.bzl", "DEPS_BY_NAME") -load("//tools/build_defs/repo:http.bzl", "http_archive", "http_file") +load("//tools/build_defs/repo:http.bzl", "http_archive", "http_file", "http_jar") _BUILD = """ load("@rules_pkg//pkg:tar.bzl", "pkg_tar") @@ -115,3 +115,21 @@ def dist_http_file(name, **kwargs): urls = info["urls"], **kwargs ) + +def dist_http_jar(name, **kwargs): + """Wraps http_jar, providing attributes like sha and urls from the central list. + + dist_http_jar wraps an http_jar invocation, but looks up relevant attributes + from distdir_deps.bzl so the user does not have to specify them. + + Args: + name: repo name + **kwargs: see http_jar for allowed args. + """ + info = DEPS_BY_NAME[name] + http_jar( + name = name, + sha256 = info["sha256"], + urls = info["urls"], + **kwargs + ) diff --git a/distdir_deps.bzl b/distdir_deps.bzl index e48b28c3e4b3e8..8df4c75e7887e7 100644 --- a/distdir_deps.bzl +++ b/distdir_deps.bzl @@ -261,6 +261,17 @@ DIST_DEPS = { # Build time dependencies for testing and packaging # ################################################### + "android_gmaven_r8": { + "archive": "r8-8.0.40.jar", + "sha256": "ab1379835c7d3e5f21f80347c3c81e2f762e0b9b02748ae5232c3afa14adf702", + "urls": [ + "https://maven.google.com/com/android/tools/r8/8.0.40/r8-8.0.40.jar", + ], + "used_in": [ + "test_WORKSPACE_files", + ], + "package_version": "8.0.40", + }, "bazel_skylib": { "archive": "bazel-skylib-1.0.3.tar.gz", "sha256": "1c531376ac7e5a180e0237938a2536de0c54d93f5c278634818e0efc952dd56c", diff --git a/src/BUILD b/src/BUILD index b8e62240ae641e..e0b9cc9cf4007a 100644 --- a/src/BUILD +++ b/src/BUILD @@ -645,7 +645,7 @@ release_archive( filegroup( name = "test_repos", srcs = [ - "@android_gmaven_r8_for_testing//jar:file", + "@android_gmaven_r8//jar:file", "@android_tools_for_testing//:WORKSPACE", "@bazel_skylib//:WORKSPACE", "@com_google_protobuf//:WORKSPACE", diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/android/android_remote_tools.WORKSPACE b/src/main/java/com/google/devtools/build/lib/bazel/rules/android/android_remote_tools.WORKSPACE index 7dc69d00c2ca36..f567dafd842e48 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/rules/android/android_remote_tools.WORKSPACE +++ b/src/main/java/com/google/devtools/build/lib/bazel/rules/android/android_remote_tools.WORKSPACE @@ -13,6 +13,6 @@ maybe( maybe( http_jar, name = "android_gmaven_r8", - sha256 = "8626ca32fb47aba7fddd2c897615e2e8ffcdb4d4b213572a2aefb3f838f01972", - url = "https://maven.google.com/com/android/tools/r8/3.3.28/r8-3.3.28.jar", + sha256 = "ab1379835c7d3e5f21f80347c3c81e2f762e0b9b02748ae5232c3afa14adf702", + url = "https://maven.google.com/com/android/tools/r8/8.0.40/r8-8.0.40.jar", ) 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/java/com/google/devtools/build/android/r8/BUILD b/src/test/java/com/google/devtools/build/android/r8/BUILD index fde5a95bdceac2..5af3a24f6a4928 100644 --- a/src/test/java/com/google/devtools/build/android/r8/BUILD +++ b/src/test/java/com/google/devtools/build/android/r8/BUILD @@ -52,6 +52,7 @@ java_test( ":barray", ":dexmergersample", ":naming001", + ":testdata_lambda_desugared.jar", ":twosimpleclasses", ], jvm_flags = [ @@ -59,6 +60,7 @@ java_test( "-DCompatDexBuilderTests.naming001=$(location :naming001)", "-DCompatDxTests.arithmetic=$(location :arithmetic)", "-DCompatDxTests.barray=$(location :barray)", + "-DCompatDexBuilderTests.lambda=$(location :testdata_lambda_desugared.jar)", ], runtime_deps = [ ":tests", @@ -114,3 +116,21 @@ java_library( "-target 8", ], ) + +java_library( + name = "testdata_lambda", + srcs = glob(["testdata/lambda/*.java"]), +) + +genrule( + name = "desugar_testdata_lambda", + srcs = [ + ":testdata_lambda", + "@bazel_tools//tools/android:android_jar", + ], + outs = ["testdata_lambda_desugared.jar"], + cmd = "$(location //src/tools/android/java/com/google/devtools/build/android/r8:desugar) " + + "-i $(location :testdata_lambda) -o $@ " + + "--bootclasspath_entry $(location @bazel_tools//tools/android:android_jar)", + tools = ["//src/tools/android/java/com/google/devtools/build/android/r8:desugar"], +) diff --git a/src/test/java/com/google/devtools/build/android/r8/CompatDexBuilderTest.java b/src/test/java/com/google/devtools/build/android/r8/CompatDexBuilderTest.java index 158200b7c78f07..8c88caedff91f9 100644 --- a/src/test/java/com/google/devtools/build/android/r8/CompatDexBuilderTest.java +++ b/src/test/java/com/google/devtools/build/android/r8/CompatDexBuilderTest.java @@ -21,13 +21,15 @@ import com.android.tools.r8.OutputMode; import com.google.common.collect.ImmutableList; import com.google.devtools.common.options.OptionsParsingException; +import java.io.BufferedReader; import java.io.IOException; +import java.io.InputStreamReader; import java.nio.file.Files; import java.nio.file.Path; import java.util.HashSet; -import java.util.List; import java.util.Set; import java.util.concurrent.ExecutionException; +import java.util.zip.ZipEntry; import java.util.zip.ZipFile; import org.junit.Rule; import org.junit.Test; @@ -45,7 +47,7 @@ public void compileManyClasses() throws IOException, InterruptedException, ExecutionException, OptionsParsingException { // Random set of classes from the R8 example test directory naming001. final String inputJar = System.getProperty("CompatDexBuilderTests.naming001"); - final List classNames = + final ImmutableList classNames = ImmutableList.of( "A", "B", @@ -86,6 +88,40 @@ public void compileManyClasses() assertThat(expectedNames).isEmpty(); } + @Test + public void compileWithSyntheticLambdas() throws Exception { + final String contextName = "com/google/devtools/build/android/r8/testdata/lambda/Lambda"; + final String inputJar = System.getProperty("CompatDexBuilderTests.lambda"); + final Path outputZip = temp.getRoot().toPath().resolve("out.zip"); + CompatDexBuilder.main( + new String[] {"--input_jar", inputJar, "--output_zip", outputZip.toString()}); + assertThat(Files.exists(outputZip)).isTrue(); + + try (ZipFile zipFile = new ZipFile(outputZip.toFile(), UTF_8)) { + assertThat(zipFile.getEntry(contextName + ".class.dex")).isNotNull(); + ZipEntry entry = zipFile.getEntry("META-INF/synthetic-contexts.map"); + assertThat(entry).isNotNull(); + try (BufferedReader reader = + new BufferedReader(new InputStreamReader(zipFile.getInputStream(entry), UTF_8))) { + String line = reader.readLine(); + assertThat(line).isNotNull(); + // Format of mapping is: ;\n + int sep = line.indexOf(';'); + String syntheticNameInMap = line.substring(0, sep); + String contextNameInMap = line.substring(sep + 1); + // The synthetic will be prefixed by the context type. This checks the synthetic name + // is larger than the context to avoid hardcoding the synthetic names, which may change. + assertThat(syntheticNameInMap).startsWith(contextName); + assertThat(syntheticNameInMap).isNotEqualTo(contextName); + // Check expected context. + assertThat(contextNameInMap).isEqualTo(contextName); + // Only one synthetic and its context should be present. + line = reader.readLine(); + assertThat(line).isNull(); + } + } + } + @Test public void compileTwoClassesAndRun() throws Exception { // Run CompatDexBuilder on dexMergeSample.jar diff --git a/src/test/java/com/google/devtools/build/android/r8/testdata/lambda/Lambda.java b/src/test/java/com/google/devtools/build/android/r8/testdata/lambda/Lambda.java new file mode 100644 index 00000000000000..2fa5fedc6cb45c --- /dev/null +++ b/src/test/java/com/google/devtools/build/android/r8/testdata/lambda/Lambda.java @@ -0,0 +1,30 @@ +// 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. +package com.google.devtools.build.android.r8.testdata.lambda; + +import java.util.function.Supplier; + +/** Test class */ +public final class Lambda { + + private Lambda() {} + + private static T foo(Supplier fn) { + return fn.get(); + } + + public static void main(String[] args) { + String unused = foo(() -> "Hello, world!"); + } +} 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/test/shell/testenv.sh.tmpl b/src/test/shell/testenv.sh.tmpl index 20d3a6205e316c..7ee2e772951205 100755 --- a/src/test/shell/testenv.sh.tmpl +++ b/src/test/shell/testenv.sh.tmpl @@ -272,7 +272,7 @@ EOF repos=( "android_tools_for_testing" - "android_gmaven_r8_for_testing" + "android_gmaven_r8" "bazel_skylib" "bazel_toolchains" "com_google_protobuf" 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)); diff --git a/src/tools/android/java/com/google/devtools/build/android/r8/BUILD b/src/tools/android/java/com/google/devtools/build/android/r8/BUILD index eb4ff9e3a936bd..55e6289887d281 100644 --- a/src/tools/android/java/com/google/devtools/build/android/r8/BUILD +++ b/src/tools/android/java/com/google/devtools/build/android/r8/BUILD @@ -47,7 +47,6 @@ java_library( deps = [ "//src/main/java/com/google/devtools/build/lib/worker:work_request_handlers", "//src/main/java/com/google/devtools/common/options", - "//src/tools/android/java/com/google/devtools/build/android:android_builder_lib", "//third_party:asm", "//third_party:auto_value", "//third_party:guava", @@ -90,3 +89,25 @@ java_library( "//src/main/java/com/google/devtools/build/lib/worker:work_request_handlers", ], ) + +java_binary( + name = "desugar", + jvm_flags = [ + # b/71513487 + "-XX:+TieredCompilation", + "-XX:TieredStopAtLevel=1", + "-Xms8g", + "-Xmx8g", + # b/172508621 + "-Dcom.android.tools.r8.sortMethodsOnCfWriting", + "-Dcom.android.tools.r8.allowAllDesugaredInput", + "-Dcom.android.tools.r8.noCfMarkerForDesugaredCode", + "-Dcom.android.tools.r8.lambdaClassFieldsNotFinal", + "-Dcom.android.tools.r8.createSingletonsForStatelessLambdas", + ], + main_class = "com.google.devtools.build.android.r8.Desugar", + visibility = ["//src/test/java/com/google/devtools/build/android/r8:__pkg__"], + runtime_deps = [ + ":r8", + ], +) diff --git a/src/tools/android/java/com/google/devtools/build/android/r8/CompatDexBuilder.java b/src/tools/android/java/com/google/devtools/build/android/r8/CompatDexBuilder.java index 0de6e36c63c260..491f99b0580911 100644 --- a/src/tools/android/java/com/google/devtools/build/android/r8/CompatDexBuilder.java +++ b/src/tools/android/java/com/google/devtools/build/android/r8/CompatDexBuilder.java @@ -24,8 +24,11 @@ import com.android.tools.r8.D8Command; import com.android.tools.r8.DexIndexedConsumer; import com.android.tools.r8.DiagnosticsHandler; +import com.android.tools.r8.SyntheticInfoConsumer; +import com.android.tools.r8.SyntheticInfoConsumerData; import com.android.tools.r8.origin.ArchiveEntryOrigin; import com.android.tools.r8.origin.PathOrigin; +import com.android.tools.r8.references.ClassReference; import com.google.auto.value.AutoValue; import com.google.common.cache.Cache; import com.google.common.cache.CacheBuilder; @@ -67,8 +70,47 @@ public class CompatDexBuilder { private static final long ONE_MEG = 1024 * 1024; + private static class ContextConsumer implements SyntheticInfoConsumer { + + // After compilation this will be non-null iff the compiled class is a D8 synthesized class. + ClassReference sythesizedPrimaryClass = null; + + // If the above is non-null then this will be the non-synthesized context class that caused + // D8 to synthesize the above class. + ClassReference contextOfSynthesizedClass = null; + + @Nullable + String getContextMapping() { + if (sythesizedPrimaryClass != null) { + return sythesizedPrimaryClass.getBinaryName() + + ";" + + contextOfSynthesizedClass.getBinaryName(); + } + return null; + } + + @Override + public synchronized void acceptSyntheticInfo(SyntheticInfoConsumerData data) { + verify( + sythesizedPrimaryClass == null || sythesizedPrimaryClass.equals(data.getSyntheticClass()), + "The single input classfile should ensure this has one value."); + verify( + contextOfSynthesizedClass == null + || contextOfSynthesizedClass.equals(data.getSynthesizingContextClass()), + "The single input classfile should ensure this has one value."); + sythesizedPrimaryClass = data.getSyntheticClass(); + contextOfSynthesizedClass = data.getSynthesizingContextClass(); + } + + @Override + public void finished() { + // Do nothing. + } + } + private static class DexConsumer implements DexIndexedConsumer { + final ContextConsumer contextConsumer = new ContextConsumer(); byte[] bytes; @Override @@ -86,6 +128,10 @@ void setBytes(byte[] byteCode) { this.bytes = byteCode; } + ContextConsumer getContextConsumer() { + return contextConsumer; + } + @Override public void finished(DiagnosticsHandler handler) { // Do nothing. @@ -269,10 +315,23 @@ private void dexEntries(@Nullable Cache dexCache, List { + + private final boolean mustExist; + + public PathConverter() { + this.mustExist = false; + } + + protected PathConverter(boolean mustExist) { + this.mustExist = mustExist; + } + + @Override + public Path convert(String input) throws OptionsParsingException { + try { + Path path = FileSystems.getDefault().getPath(input); + if (mustExist && !Files.exists(path)) { + throw new OptionsParsingException( + String.format("%s is not a valid path: it does not exist.", input)); + } + return path; + } catch (InvalidPathException e) { + throw new OptionsParsingException( + String.format("%s is not a valid path: %s.", input, e.getMessage()), e); + } + } + + @Override + public String getTypeDescription() { + return "a valid filesystem path"; + } + } + + /** + * Validating converter for Paths. A Path is considered valid if it resolves to a file and exists. + */ + public static class ExistingPathConverter extends PathConverter { + public ExistingPathConverter() { + super(true); + } + } + + private OptionsConverters() {} +} diff --git a/tools/android/android_extensions.bzl b/tools/android/android_extensions.bzl index ea84807d8487e7..7358edfb38e7c7 100644 --- a/tools/android/android_extensions.bzl +++ b/tools/android/android_extensions.bzl @@ -24,8 +24,8 @@ def _remote_android_tools_extensions_impl(_ctx): ) http_jar( name = "android_gmaven_r8", - sha256 = "8626ca32fb47aba7fddd2c897615e2e8ffcdb4d4b213572a2aefb3f838f01972", - url = "https://maven.google.com/com/android/tools/r8/3.3.28/r8-3.3.28.jar", + sha256 = "ab1379835c7d3e5f21f80347c3c81e2f762e0b9b02748ae5232c3afa14adf702", + url = "https://maven.google.com/com/android/tools/r8/8.0.40/r8-8.0.40.jar", ) remote_android_tools_extensions = module_extension(