Version 2.1.56 Cherry-pick: Add phi check when checking in value definition in service rewriter CL: https://r8-review.googlesource.com/c/r8/+/52665 Cherry-pick: Reland "Ensure no service loader rewriting if it crosses a feature split" CL: https://r8-review.googlesource.com/c/r8/+/52542 bug: 162568140 Change-Id: I05cb372595d3755e81690cb8caab06c01b4c9c0b
diff --git a/src/main/java/com/android/tools/r8/FeatureSplit.java b/src/main/java/com/android/tools/r8/FeatureSplit.java index 5947786..9bb637e 100644 --- a/src/main/java/com/android/tools/r8/FeatureSplit.java +++ b/src/main/java/com/android/tools/r8/FeatureSplit.java
@@ -29,6 +29,9 @@ */ @Keep public final class FeatureSplit { + + public static final FeatureSplit BASE = new FeatureSplit(null, null); + private final ProgramConsumer programConsumer; private final List<ProgramResourceProvider> programResourceProviders;
diff --git a/src/main/java/com/android/tools/r8/Version.java b/src/main/java/com/android/tools/r8/Version.java index ae9866f..4037c85 100644 --- a/src/main/java/com/android/tools/r8/Version.java +++ b/src/main/java/com/android/tools/r8/Version.java
@@ -11,7 +11,7 @@ // This field is accessed from release scripts using simple pattern matching. // Therefore, changing this field could break our release scripts. - public static final String LABEL = "2.1.55"; + public static final String LABEL = "2.1.56"; private Version() { }
diff --git a/src/main/java/com/android/tools/r8/graph/AppServices.java b/src/main/java/com/android/tools/r8/graph/AppServices.java index 5c1c7ad..0dbaa30 100644 --- a/src/main/java/com/android/tools/r8/graph/AppServices.java +++ b/src/main/java/com/android/tools/r8/graph/AppServices.java
@@ -8,6 +8,8 @@ import com.android.tools.r8.DataEntryResource; import com.android.tools.r8.DataResourceProvider; import com.android.tools.r8.DataResourceProvider.Visitor; +import com.android.tools.r8.FeatureSplit; +import com.android.tools.r8.ProgramResourceProvider; import com.android.tools.r8.ResourceException; import com.android.tools.r8.errors.CompilationError; import com.android.tools.r8.origin.Origin; @@ -21,7 +23,7 @@ import java.nio.charset.Charset; import java.util.ArrayList; import java.util.Collection; -import java.util.IdentityHashMap; +import java.util.LinkedHashMap; import java.util.List; import java.util.Map; import java.util.Map.Entry; @@ -36,9 +38,9 @@ private final AppView<?> appView; // Mapping from service types to service implementation types. - private final Map<DexType, List<DexType>> services; + private final Map<DexType, Map<FeatureSplit, List<DexType>>> services; - private AppServices(AppView<?> appView, Map<DexType, List<DexType>> services) { + private AppServices(AppView<?> appView, Map<DexType, Map<FeatureSplit, List<DexType>>> services) { this.appView = appView; this.services = services; } @@ -54,66 +56,124 @@ public List<DexType> serviceImplementationsFor(DexType serviceType) { assert verifyRewrittenWithLens(); - assert services.containsKey(serviceType); - List<DexType> serviceImplementationTypes = services.get(serviceType); - if (serviceImplementationTypes == null) { + Map<FeatureSplit, List<DexType>> featureSplitListMap = services.get(serviceType); + if (featureSplitListMap == null) { assert false : "Unexpected attempt to get service implementations for non-service type `" + serviceType.toSourceString() + "`"; return ImmutableList.of(); } - return serviceImplementationTypes; + ImmutableList.Builder<DexType> builder = ImmutableList.builder(); + for (List<DexType> implementations : featureSplitListMap.values()) { + builder.addAll(implementations); + } + return builder.build(); + } + + public boolean hasServiceImplementationsInFeature(DexType serviceType) { + if (appView.options().featureSplitConfiguration == null) { + return false; + } + Map<FeatureSplit, List<DexType>> featureImplementations = services.get(serviceType); + if (featureImplementations == null || featureImplementations.isEmpty()) { + assert false + : "Unexpected attempt to get service implementations for non-service type `" + + serviceType.toSourceString() + + "`"; + return true; + } + if (featureImplementations.size() > 1 + || !featureImplementations.containsKey(FeatureSplit.BASE)) { + return true; + } + // Check if service is defined feature + DexProgramClass serviceClass = appView.definitionForProgramType(serviceType); + if (appView.options().featureSplitConfiguration.isInFeature(serviceClass)) { + return true; + } + for (DexType dexType : featureImplementations.get(FeatureSplit.BASE)) { + DexProgramClass implementationClass = appView.definitionForProgramType(dexType); + if (appView.options().featureSplitConfiguration.isInFeature(implementationClass)) { + return true; + } + } + return false; } public AppServices rewrittenWithLens(GraphLense graphLens) { - ImmutableMap.Builder<DexType, List<DexType>> rewrittenServices = ImmutableMap.builder(); - for (Entry<DexType, List<DexType>> entry : services.entrySet()) { + ImmutableMap.Builder<DexType, Map<FeatureSplit, List<DexType>>> rewrittenFeatureMappings = + ImmutableMap.builder(); + for (Entry<DexType, Map<FeatureSplit, List<DexType>>> entry : services.entrySet()) { DexType rewrittenServiceType = graphLens.lookupType(entry.getKey()); - ImmutableList.Builder<DexType> rewrittenServiceImplementationTypes = ImmutableList.builder(); - for (DexType serviceImplementationType : entry.getValue()) { - rewrittenServiceImplementationTypes.add(graphLens.lookupType(serviceImplementationType)); + ImmutableMap.Builder<FeatureSplit, List<DexType>> rewrittenFeatureImplementations = + ImmutableMap.builder(); + for (Entry<FeatureSplit, List<DexType>> featureSplitImpls : entry.getValue().entrySet()) { + ImmutableList.Builder<DexType> rewrittenServiceImplementationTypes = + ImmutableList.builder(); + for (DexType serviceImplementationType : featureSplitImpls.getValue()) { + rewrittenServiceImplementationTypes.add(graphLens.lookupType(serviceImplementationType)); + } + rewrittenFeatureImplementations.put( + featureSplitImpls.getKey(), rewrittenServiceImplementationTypes.build()); } - rewrittenServices.put(rewrittenServiceType, rewrittenServiceImplementationTypes.build()); + rewrittenFeatureMappings.put(rewrittenServiceType, rewrittenFeatureImplementations.build()); } - return new AppServices(appView, rewrittenServices.build()); + return new AppServices(appView, rewrittenFeatureMappings.build()); } public AppServices prunedCopy(Collection<DexType> removedClasses) { - ImmutableMap.Builder<DexType, List<DexType>> rewrittenServicesBuilder = ImmutableMap.builder(); - for (Entry<DexType, List<DexType>> entry : services.entrySet()) { - if (!removedClasses.contains(entry.getKey())) { - DexType serviceType = entry.getKey(); + ImmutableMap.Builder<DexType, Map<FeatureSplit, List<DexType>>> rewrittenServicesBuilder = + ImmutableMap.builder(); + for (Entry<DexType, Map<FeatureSplit, List<DexType>>> entry : services.entrySet()) { + if (removedClasses.contains(entry.getKey())) { + continue; + } + ImmutableMap.Builder<FeatureSplit, List<DexType>> prunedFeatureSplitImpls = + ImmutableMap.builder(); + for (Entry<FeatureSplit, List<DexType>> featureSplitEntry : entry.getValue().entrySet()) { ImmutableList.Builder<DexType> rewrittenServiceImplementationTypesBuilder = ImmutableList.builder(); - for (DexType serviceImplementationType : entry.getValue()) { + for (DexType serviceImplementationType : featureSplitEntry.getValue()) { if (!removedClasses.contains(serviceImplementationType)) { rewrittenServiceImplementationTypesBuilder.add(serviceImplementationType); } } - List<DexType> rewrittenServiceImplementationTypes = + List<DexType> prunedFeatureSplitImplementations = rewrittenServiceImplementationTypesBuilder.build(); - if (rewrittenServiceImplementationTypes.size() > 0) { - rewrittenServicesBuilder.put( - serviceType, rewrittenServiceImplementationTypesBuilder.build()); + if (prunedFeatureSplitImplementations.size() > 0) { + prunedFeatureSplitImpls.put( + featureSplitEntry.getKey(), rewrittenServiceImplementationTypesBuilder.build()); } } + ImmutableMap<FeatureSplit, List<DexType>> prunedServiceImplementations = + prunedFeatureSplitImpls.build(); + if (prunedServiceImplementations.size() > 0) { + rewrittenServicesBuilder.put(entry.getKey(), prunedServiceImplementations); + } } return new AppServices(appView, rewrittenServicesBuilder.build()); } private boolean verifyRewrittenWithLens() { - for (Entry<DexType, List<DexType>> entry : services.entrySet()) { + for (Entry<DexType, Map<FeatureSplit, List<DexType>>> entry : services.entrySet()) { assert entry.getKey() == appView.graphLense().lookupType(entry.getKey()); - for (DexType type : entry.getValue()) { - assert type == appView.graphLense().lookupType(type); + for (Entry<FeatureSplit, List<DexType>> featureEntry : entry.getValue().entrySet()) { + for (DexType type : featureEntry.getValue()) { + assert type == appView.graphLense().lookupType(type); + } } } return true; } public void visit(BiConsumer<DexType, List<DexType>> consumer) { - services.forEach(consumer); + services.forEach( + (type, featureImpls) -> { + ImmutableList.Builder<DexType> builder = ImmutableList.builder(); + featureImpls.values().forEach(builder::addAll); + consumer.accept(type, builder.build()); + }); } public static Builder builder(AppView<?> appView) { @@ -123,7 +183,7 @@ public static class Builder { private final AppView<?> appView; - private final Map<DexType, List<DexType>> services = new IdentityHashMap<>(); + private final Map<DexType, Map<FeatureSplit, List<DexType>>> services = new LinkedHashMap<>(); private Builder(AppView<?> appView) { this.appView = appView; @@ -131,14 +191,27 @@ public AppServices build() { for (DataResourceProvider provider : appView.appInfo().app().dataResourceProviders) { - readServices(provider); + readServices(provider, FeatureSplit.BASE); + } + if (appView.options().featureSplitConfiguration != null) { + List<FeatureSplit> featureSplits = + appView.options().featureSplitConfiguration.getFeatureSplits(); + for (FeatureSplit featureSplit : featureSplits) { + for (ProgramResourceProvider provider : featureSplit.getProgramResourceProviders()) { + DataResourceProvider dataResourceProvider = provider.getDataResourceProvider(); + if (dataResourceProvider != null) { + readServices(dataResourceProvider, featureSplit); + } + } + } } return new AppServices(appView, services); } - private void readServices(DataResourceProvider dataResourceProvider) { + private void readServices( + DataResourceProvider dataResourceProvider, FeatureSplit featureSplit) { try { - dataResourceProvider.accept(new DataResourceProviderVisitor()); + dataResourceProvider.accept(new DataResourceProviderVisitor(featureSplit)); } catch (ResourceException e) { throw new CompilationError(e.getMessage(), e); } @@ -146,6 +219,12 @@ private class DataResourceProviderVisitor implements Visitor { + private final FeatureSplit featureSplit; + + public DataResourceProviderVisitor(FeatureSplit featureSplit) { + this.featureSplit = featureSplit; + } + @Override public void visit(DataDirectoryResource directory) { // Ignore. @@ -162,8 +241,10 @@ DexType serviceType = appView.dexItemFactory().createType(serviceDescriptor); byte[] bytes = ByteStreams.toByteArray(file.getByteStream()); String contents = new String(bytes, Charset.defaultCharset()); + Map<FeatureSplit, List<DexType>> featureSplitImplementations = + services.computeIfAbsent(serviceType, k -> new LinkedHashMap<>()); List<DexType> serviceImplementations = - services.computeIfAbsent(serviceType, (key) -> new ArrayList<>()); + featureSplitImplementations.computeIfAbsent(featureSplit, f -> new ArrayList<>()); readServiceImplementationsForService( contents, file.getOrigin(), serviceImplementations); }
diff --git a/src/main/java/com/android/tools/r8/ir/optimize/ServiceLoaderRewriter.java b/src/main/java/com/android/tools/r8/ir/optimize/ServiceLoaderRewriter.java index d0ad73a..235ede5 100644 --- a/src/main/java/com/android/tools/r8/ir/optimize/ServiceLoaderRewriter.java +++ b/src/main/java/com/android/tools/r8/ir/optimize/ServiceLoaderRewriter.java
@@ -130,8 +130,16 @@ continue; } - // Check that ClassLoader used is the ClassLoader defined for the the service configuration + // Check that we are not service loading anything from a feature into base. + if (appView.appServices().hasServiceImplementationsInFeature(constClass.getValue())) { + continue; + } + + // Check that ClassLoader used is the ClassLoader defined for the service configuration // that we are instantiating or NULL. + if (serviceLoaderLoad.inValues().get(1).isPhi()) { + continue; + } InvokeVirtual classLoaderInvoke = serviceLoaderLoad.inValues().get(1).definition.asInvokeVirtual(); boolean isGetClassLoaderOnConstClassOrNull =
diff --git a/src/test/java/com/android/tools/r8/rewrite/ServiceLoaderRewritingTest.java b/src/test/java/com/android/tools/r8/rewrite/ServiceLoaderRewritingTest.java index 5f46a45..1ccedb0 100644 --- a/src/test/java/com/android/tools/r8/rewrite/ServiceLoaderRewritingTest.java +++ b/src/test/java/com/android/tools/r8/rewrite/ServiceLoaderRewritingTest.java
@@ -135,6 +135,20 @@ } } + public static class LoadWhereClassLoaderIsPhi { + + public static void main(String[] args) { + ServiceLoader.load( + Service.class, + System.currentTimeMillis() > 0 + ? Thread.currentThread().getContextClassLoader() + : null) + .iterator() + .next() + .print(); + } + } + @Parameterized.Parameters(name = "{0}") public static TestParametersCollection data() { return getTestParameters().withAllRuntimesAndApiLevels().build(); @@ -207,7 +221,7 @@ testForR8(parameters.getBackend()) .addInnerClasses(ServiceLoaderRewritingTest.class) .addKeepMainRule(MainWithTryCatchRunner.class) - .setMinApi(parameters.getRuntime()) + .setMinApi(parameters.getApiLevel()) .addDataEntryResources( DataEntryResource.fromBytes( StringUtils.lines(ServiceImpl.class.getTypeName(), ServiceImpl2.class.getTypeName()) @@ -267,7 +281,7 @@ .addInnerClasses(ServiceLoaderRewritingTest.class) .addKeepMainRule(EscapingRunner.class) .enableInliningAnnotations() - .setMinApi(parameters.getRuntime()) + .setMinApi(parameters.getApiLevel()) .noMinification() .addDataEntryResources( DataEntryResource.fromBytes( @@ -291,6 +305,37 @@ } @Test + public void testDoNoRewriteWhenClassLoaderIsPhi() + throws IOException, CompilationFailedException, ExecutionException { + Path path = temp.newFile("out.zip").toPath(); + CodeInspector inspector = + testForR8(parameters.getBackend()) + .addInnerClasses(ServiceLoaderRewritingTest.class) + .addKeepMainRule(LoadWhereClassLoaderIsPhi.class) + .enableInliningAnnotations() + .setMinApi(parameters.getApiLevel()) + .addDataEntryResources( + DataEntryResource.fromBytes( + StringUtils.lines(ServiceImpl.class.getTypeName()).getBytes(), + "META-INF/services/" + Service.class.getTypeName(), + Origin.unknown())) + .compile() + .writeToZip(path) + .run(parameters.getRuntime(), LoadWhereClassLoaderIsPhi.class) + .assertSuccessWithOutputLines("Hello World!") + .inspector(); + + // Check that we have not rewritten the calls to ServiceLoader.load. + assertEquals(1, getServiceLoaderLoads(inspector, LoadWhereClassLoaderIsPhi.class)); + + // Check that we have not removed the service configuration from META-INF/services. + ZipFile zip = new ZipFile(path.toFile()); + ClassSubject serviceImpl = inspector.clazz(ServiceImpl.class); + assertTrue(serviceImpl.isPresent()); + assertNotNull(zip.getEntry("META-INF/services/" + serviceImpl.getFinalName())); + } + + @Test public void testKeepAsOriginal() throws IOException, CompilationFailedException, ExecutionException { // The CL that changed behaviour after Nougat is: