Version 2.0.75
Cherry pick: Workaround for non-determinism in service loader rewriter
CL: https://r8-review.googlesource.com/51384
Cherry pick: Update synthesized $load method names
CL: https://r8-review.googlesource.com/51402
Cherry pick: Unique ServiceLoaderRewriting for each context
CL: https://r8-review.googlesource.com/51685
Bug: 157430860
Bug: 156054499
Bug: 157223339
Change-Id: Ie2b96546adfbaf9ad3717e60eb1e1eb011005f45
diff --git a/src/main/java/com/android/tools/r8/Version.java b/src/main/java/com/android/tools/r8/Version.java
index f031f30..05557f3 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.0.74";
+ public static final String LABEL = "2.0.75";
private Version() {
}
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 4f5d248..aead60b 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
@@ -30,12 +30,14 @@
import com.android.tools.r8.ir.desugar.ServiceLoaderSourceCode;
import com.android.tools.r8.origin.SynthesizedOrigin;
import com.android.tools.r8.shaking.AppInfoWithLiveness;
+import com.android.tools.r8.utils.IntBox;
+import com.android.tools.r8.utils.StringUtils;
import com.google.common.collect.ImmutableList;
import java.util.ArrayList;
import java.util.Collections;
+import java.util.IdentityHashMap;
import java.util.List;
-import java.util.concurrent.ConcurrentHashMap;
-import java.util.concurrent.atomic.AtomicInteger;
+import java.util.Map;
import java.util.concurrent.atomic.AtomicReference;
/**
@@ -69,12 +71,9 @@
public static final String SERVICE_LOADER_CLASS_NAME = "$$ServiceLoaderMethods";
private static final String SERVICE_LOADER_METHOD_PREFIX_NAME = "$load";
+ private static final int SERVICE_LOADER_METHOD_HASH_LENGTH = 7;
private AtomicReference<DexProgramClass> synthesizedClass = new AtomicReference<>();
- private ConcurrentHashMap<DexType, DexEncodedMethod> synthesizedServiceLoaders =
- new ConcurrentHashMap<>();
-
- private AtomicInteger atomicInteger = new AtomicInteger(0);
private final AppView<? extends AppInfoWithLiveness> appView;
@@ -89,6 +88,10 @@
public void rewrite(IRCode code) {
DexItemFactory factory = appView.dexItemFactory();
InstructionListIterator instructionIterator = code.instructionListIterator();
+ IntBox synthesizedLoadMethodCounter = new IntBox();
+ // Create a map from service type to loader methods local to this context since two
+ // service loader calls to the same type in different methods and in the same wave can race.
+ Map<DexType, DexEncodedMethod> synthesizedServiceLoaders = new IdentityHashMap<>();
while (instructionIterator.hasNext()) {
Instruction instruction = instructionIterator.next();
@@ -172,7 +175,9 @@
synthesizedServiceLoaders.computeIfAbsent(
constClass.getValue(),
service -> {
- DexEncodedMethod addedMethod = createSynthesizedMethod(service, classes);
+ DexEncodedMethod addedMethod =
+ createSynthesizedMethod(
+ service, classes, code.method, synthesizedLoadMethodCounter);
if (appView.options().isGeneratingClassFiles()) {
addedMethod.upgradeClassFileVersion(code.method.getClassFileVersion());
}
@@ -184,27 +189,48 @@
}
}
- private DexEncodedMethod createSynthesizedMethod(DexType serviceType, List<DexClass> classes) {
+ private DexEncodedMethod createSynthesizedMethod(
+ DexType serviceType,
+ List<DexClass> classes,
+ DexEncodedMethod context,
+ IntBox synthesizedLoadMethodCounter) {
+ String hashCode = Integer.toString(context.method.hashCode());
+ String methodNamePrefix =
+ SERVICE_LOADER_METHOD_PREFIX_NAME
+ + "$"
+ + StringUtils.replaceAll(context.method.holder.toSourceString(), ".", "$")
+ + "$"
+ + (context.isInitializer()
+ ? (context.isClassInitializer() ? "$clinit" : "$init")
+ : context.method.name.toSourceString())
+ + "$"
+ + hashCode.substring(0, Math.min(SERVICE_LOADER_METHOD_HASH_LENGTH, hashCode.length()))
+ + "$";
DexProto proto = appView.dexItemFactory().createProto(appView.dexItemFactory().iteratorType);
- DexMethod method =
- appView
- .dexItemFactory()
- .createMethod(
- appView.dexItemFactory().serviceLoaderRewrittenClassType,
- proto,
- SERVICE_LOADER_METHOD_PREFIX_NAME + atomicInteger.incrementAndGet());
- MethodAccessFlags methodAccess =
- MethodAccessFlags.fromSharedAccessFlags(Constants.ACC_PUBLIC | Constants.ACC_STATIC, false);
- DexEncodedMethod encodedMethod =
- new DexEncodedMethod(
- method,
- methodAccess,
- DexAnnotationSet.empty(),
- ParameterAnnotationsList.empty(),
- ServiceLoaderSourceCode.generate(serviceType, classes, appView.dexItemFactory()),
- true);
- getOrSetSynthesizedClass().addDirectMethod(encodedMethod);
- return encodedMethod;
+ DexProgramClass synthesizedClass = getOrSetSynthesizedClass();
+ synchronized (synthesizedClass) {
+ DexMethod methodReference;
+ do {
+ methodReference =
+ appView
+ .dexItemFactory()
+ .createMethod(
+ appView.dexItemFactory().serviceLoaderRewrittenClassType,
+ proto,
+ methodNamePrefix + "$" + synthesizedLoadMethodCounter.getAndIncrement());
+ } while (synthesizedClass.lookupMethod(methodReference) != null);
+ DexEncodedMethod method =
+ new DexEncodedMethod(
+ methodReference,
+ MethodAccessFlags.fromSharedAccessFlags(
+ Constants.ACC_PUBLIC | Constants.ACC_STATIC, false),
+ DexAnnotationSet.empty(),
+ ParameterAnnotationsList.empty(),
+ ServiceLoaderSourceCode.generate(serviceType, classes, appView.dexItemFactory()),
+ true);
+ synthesizedClass.addDirectMethod(method);
+ return method;
+ }
}
private DexProgramClass getOrSetSynthesizedClass() {
diff --git a/src/main/java/com/android/tools/r8/utils/IntBox.java b/src/main/java/com/android/tools/r8/utils/IntBox.java
new file mode 100644
index 0000000..ebb92ca
--- /dev/null
+++ b/src/main/java/com/android/tools/r8/utils/IntBox.java
@@ -0,0 +1,32 @@
+// Copyright (c) 2020, the R8 project authors. Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+
+package com.android.tools.r8.utils;
+
+public class IntBox {
+
+ private int value;
+
+ public IntBox() {}
+
+ public IntBox(int initialValue) {
+ set(initialValue);
+ }
+
+ public int get() {
+ return value;
+ }
+
+ public int getAndIncrement() {
+ return value++;
+ }
+
+ public void increment() {
+ value++;
+ }
+
+ public void set(int value) {
+ this.value = value;
+ }
+}
diff --git a/src/test/java/com/android/tools/r8/rewrite/ServiceLoaderMultipleCallsTest.java b/src/test/java/com/android/tools/r8/rewrite/ServiceLoaderMultipleCallsTest.java
new file mode 100644
index 0000000..688a96f
--- /dev/null
+++ b/src/test/java/com/android/tools/r8/rewrite/ServiceLoaderMultipleCallsTest.java
@@ -0,0 +1,141 @@
+// Copyright (c) 2020, the R8 project authors. Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+
+package com.android.tools.r8.rewrite;
+
+import static com.android.tools.r8.utils.codeinspector.Matchers.isPresent;
+import static junit.framework.TestCase.assertEquals;
+import static junit.framework.TestCase.assertNull;
+import static junit.framework.TestCase.assertTrue;
+import static org.hamcrest.MatcherAssert.assertThat;
+
+import com.android.tools.r8.CompilationFailedException;
+import com.android.tools.r8.DataEntryResource;
+import com.android.tools.r8.NeverInline;
+import com.android.tools.r8.TestBase;
+import com.android.tools.r8.TestParameters;
+import com.android.tools.r8.TestParametersCollection;
+import com.android.tools.r8.origin.Origin;
+import com.android.tools.r8.utils.StringUtils;
+import com.android.tools.r8.utils.codeinspector.ClassSubject;
+import com.android.tools.r8.utils.codeinspector.CodeInspector;
+import com.android.tools.r8.utils.codeinspector.InstructionSubject;
+import java.io.IOException;
+import java.nio.file.Path;
+import java.util.ServiceLoader;
+import java.util.concurrent.ExecutionException;
+import java.util.zip.ZipFile;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+
+@RunWith(Parameterized.class)
+public class ServiceLoaderMultipleCallsTest extends TestBase {
+
+ private final TestParameters parameters;
+ private final String EXPECTED_OUTPUT = StringUtils.lines("Hello World!", "Hello World!");
+
+ public interface Service {
+
+ void print();
+ }
+
+ public static class ServiceImpl implements Service {
+
+ @Override
+ public void print() {
+ System.out.println("Hello World!");
+ }
+ }
+
+ public static class ServiceImpl2 implements Service {
+
+ @Override
+ public void print() {
+ System.out.println("Hello World 2!");
+ }
+ }
+
+ public static class MainRunner {
+
+ public static void main(String[] args) {
+ run1();
+ run2();
+ }
+
+ @NeverInline
+ public static void run1() {
+ for (Service x : ServiceLoader.load(Service.class, Service.class.getClassLoader())) {
+ x.print();
+ }
+ }
+
+ @NeverInline
+ public static void run2() {
+ for (Service x : ServiceLoader.load(Service.class, Service.class.getClassLoader())) {
+ x.print();
+ }
+ }
+ }
+
+ @Parameterized.Parameters(name = "{0}")
+ public static TestParametersCollection data() {
+ return getTestParameters().withAllRuntimesAndApiLevels().build();
+ }
+
+ public ServiceLoaderMultipleCallsTest(TestParameters parameters) {
+ this.parameters = parameters;
+ }
+
+ @Test
+ public void testRewritings() throws IOException, CompilationFailedException, ExecutionException {
+ Path path = temp.newFile("out.zip").toPath();
+ testForR8(parameters.getBackend())
+ .addInnerClasses(ServiceLoaderMultipleCallsTest.class)
+ .addKeepMainRule(MainRunner.class)
+ .setMinApi(parameters.getApiLevel())
+ .enableInliningAnnotations()
+ .addDataEntryResources(
+ DataEntryResource.fromBytes(
+ StringUtils.lines(ServiceImpl.class.getTypeName()).getBytes(),
+ "META-INF/services/" + Service.class.getTypeName(),
+ Origin.unknown()))
+ .compile()
+ .writeToZip(path)
+ .run(parameters.getRuntime(), MainRunner.class)
+ .assertSuccessWithOutput(EXPECTED_OUTPUT)
+ .inspect(
+ inspector -> {
+ // Check that we have actually rewritten the calls to ServiceLoader.load.
+ assertEquals(0, getServiceLoaderLoads(inspector, MainRunner.class));
+ // Check that the synthesize service loader class holds two methods, one for each
+ // context.
+ ClassSubject serviceLoaderMethods = inspector.clazz("$$ServiceLoaderMethods");
+ assertThat(serviceLoaderMethods, isPresent());
+ assertEquals(2, serviceLoaderMethods.allMethods().size());
+ });
+
+ // Check that we have removed the service configuration from META-INF/services.
+ ZipFile zip = new ZipFile(path.toFile());
+ assertNull(zip.getEntry("META-INF/services/" + Service.class.getTypeName()));
+ }
+
+ private static long getServiceLoaderLoads(CodeInspector inspector, Class<?> clazz) {
+ ClassSubject classSubject = inspector.clazz(clazz);
+ assertTrue(classSubject.isPresent());
+ return classSubject.allMethods().stream()
+ .mapToLong(
+ method ->
+ method
+ .streamInstructions()
+ .filter(ServiceLoaderMultipleCallsTest::isServiceLoaderLoad)
+ .count())
+ .sum();
+ }
+
+ private static boolean isServiceLoaderLoad(InstructionSubject instruction) {
+ return instruction.isInvokeStatic()
+ && instruction.getMethod().qualifiedName().contains("ServiceLoader.load");
+ }
+}
