From faa0690d2152c090607a8db136ba9104bc22bb4e Mon Sep 17 00:00:00 2001 From: kmb Date: Fri, 16 Mar 2018 18:52:15 -0700 Subject: Reflect core library moves in super calls, even in default method stubs. Always generate default method stubs for emulated methods. RELNOTES: None. PiperOrigin-RevId: 189423933 GitOrigin-RevId: 44a26afb091f2d23d68bcad53e45a319b299867a Change-Id: I8eaecb5a1a29051a14d0529005a56a225b2f4d8b --- .../desugar/CoreLibraryInvocationRewriter.java | 9 +++- .../build/android/desugar/CoreLibrarySupport.java | 45 +++++++++++++---- .../android/desugar/DefaultMethodClassFixer.java | 59 ++++++++++++++++++---- .../android/desugar/CoreLibrarySupportTest.java | 53 +++++++++++++++++++ 4 files changed, 145 insertions(+), 21 deletions(-) diff --git a/java/com/google/devtools/build/android/desugar/CoreLibraryInvocationRewriter.java b/java/com/google/devtools/build/android/desugar/CoreLibraryInvocationRewriter.java index 77db915..381a344 100644 --- a/java/com/google/devtools/build/android/desugar/CoreLibraryInvocationRewriter.java +++ b/java/com/google/devtools/build/android/desugar/CoreLibraryInvocationRewriter.java @@ -14,6 +14,7 @@ package com.google.devtools.build.android.desugar; 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 org.objectweb.asm.ClassVisitor; @@ -65,9 +66,13 @@ public class CoreLibraryInvocationRewriter extends ClassVisitor { } if (opcode == Opcodes.INVOKESTATIC || opcode == Opcodes.INVOKESPECIAL) { - checkArgument(itf, "Expected interface to rewrite %s.%s : %s", owner, name, desc); - owner = InterfaceDesugaring.getCompanionClassName(coreInterfaceName); + checkArgument(itf || opcode == Opcodes.INVOKESPECIAL, + "Expected interface to rewrite %s.%s : %s", owner, name, desc); + owner = coreInterface.isInterface() + ? InterfaceDesugaring.getCompanionClassName(coreInterfaceName) + : checkNotNull(support.getMoveTarget(coreInterfaceName, name)); } else { + checkState(coreInterface.isInterface()); owner = coreInterfaceName + "$$Dispatch"; } diff --git a/java/com/google/devtools/build/android/desugar/CoreLibrarySupport.java b/java/com/google/devtools/build/android/desugar/CoreLibrarySupport.java index fd10e5e..f247074 100644 --- a/java/com/google/devtools/build/android/desugar/CoreLibrarySupport.java +++ b/java/com/google/devtools/build/android/desugar/CoreLibrarySupport.java @@ -32,6 +32,7 @@ import java.lang.reflect.Method; import java.util.Collection; import java.util.HashMap; import java.util.Iterator; +import java.util.LinkedHashMap; import java.util.LinkedHashSet; import java.util.List; import java.util.Objects; @@ -89,7 +90,8 @@ class CoreLibrarySupport { this.emulatedInterfaces = classBuilder.build(); // We can call isRenamed and rename below b/c we initialized the necessary fields above - ImmutableMap.Builder movesBuilder = ImmutableMap.builder(); + // Use LinkedHashMap to tolerate identical duplicates + LinkedHashMap movesBuilder = new LinkedHashMap<>(); Splitter splitter = Splitter.on("->").trimResults().omitEmptyStrings(); for (String move : memberMoves) { List pair = splitter.splitToList(move); @@ -103,9 +105,12 @@ class CoreLibrarySupport { checkArgument(!this.excludeFromEmulation.contains(pair.get(0)), "Retargeted invocation %s shouldn't overlap with excluded", move); - movesBuilder.put(pair.get(0), renameCoreLibrary(pair.get(1))); + String value = renameCoreLibrary(pair.get(1)); + String existing = movesBuilder.put(pair.get(0), value); + checkArgument(existing == null || existing.equals(value), + "Two move destinations %s and %s configured for %s", existing, value, pair.get(0)); } - this.memberMoves = movesBuilder.build(); + this.memberMoves = ImmutableMap.copyOf(movesBuilder); } public boolean isRenamedCoreLibrary(String internalName) { @@ -165,9 +170,13 @@ class CoreLibrarySupport { * core interface, this methods returns that interface. This is a helper method for * {@link CoreLibraryInvocationRewriter}. * - *

Always returns an interface (or {@code null}), even if {@code owner} is a class. Can only - * return non-{@code null} if {@code owner} is a core library type. + *

This method can only return non-{@code null} if {@code owner} is a core library type. + * It usually returns an emulated interface, unless the given invocation is a super-call to a + * core class's implementation of an emulated method that's being moved (other implementations + * of emulated methods in core classes are ignored). In that case the class is returned and the + * caller can use {@link #getMoveTarget} to find out where to redirect the invokespecial to. */ + // TODO(kmb): Rethink this API and consider combining it with getMoveTarget(). @Nullable public Class getCoreInterfaceRewritingTarget( int opcode, String owner, String name, String desc, boolean itf) { @@ -175,15 +184,20 @@ class CoreLibrarySupport { // Regular desugaring handles generated classes, no emulation is needed return null; } - if (!itf && (opcode == Opcodes.INVOKESTATIC || opcode == Opcodes.INVOKESPECIAL)) { - // Ignore staticly dispatched invocations on classes--they never need rewriting + if (!itf && opcode == Opcodes.INVOKESTATIC) { + // Ignore static invocations on classes--they never need rewriting (unless moved but that's + // handled separately). return null; } + if ("".equals(name)) { + return null; // Constructors aren't rewritten + } + Class clazz; if (isRenamedCoreLibrary(owner)) { // For renamed invocation targets we just need to do what InterfaceDesugaring does, that is, // only worry about invokestatic and invokespecial interface invocations; nothing to do for - // invokevirtual and invokeinterface. InterfaceDesugaring ignores bootclasspath interfaces, + // classes and invokeinterface. InterfaceDesugaring ignores bootclasspath interfaces, // so we have to do its work here for renamed interfaces. if (itf && (opcode == Opcodes.INVOKESTATIC || opcode == Opcodes.INVOKESPECIAL)) { @@ -213,6 +227,19 @@ class CoreLibrarySupport { if (isExcluded(callee)) { return null; } + + if (!itf && opcode == Opcodes.INVOKESPECIAL) { + // See if the invoked implementation is moved; note we ignore all other overrides in classes + Class impl = clazz; // we know clazz is not an interface because !itf + while (impl != null) { + String implName = impl.getName().replace('.', '/'); + if (getMoveTarget(implName, name) != null) { + return impl; + } + impl = impl.getSuperclass(); + } + } + Class result = callee.getDeclaringClass(); if (isRenamedCoreLibrary(result.getName().replace('.', '/')) || emulatedInterfaces.stream().anyMatch(emulated -> emulated.isAssignableFrom(result))) { @@ -232,7 +259,7 @@ class CoreLibrarySupport { checkState(!roots.hasNext(), "Ambiguous emulation substitute: %s", callee); return substitute; } else { - checkArgument(opcode != Opcodes.INVOKESPECIAL, + checkArgument(!itf || opcode != Opcodes.INVOKESPECIAL, "Couldn't resolve interface super call %s.super.%s : %s", owner, name, desc); } return null; diff --git a/java/com/google/devtools/build/android/desugar/DefaultMethodClassFixer.java b/java/com/google/devtools/build/android/desugar/DefaultMethodClassFixer.java index 960cfeb..f0fc6d1 100644 --- a/java/com/google/devtools/build/android/desugar/DefaultMethodClassFixer.java +++ b/java/com/google/devtools/build/android/desugar/DefaultMethodClassFixer.java @@ -233,11 +233,13 @@ public class DefaultMethodClassFixer extends ClassVisitor { // superclass is also rewritten and already implements this interface, so we _must_ skip it. continue; } - stubMissingDefaultAndBridgeMethods(interfaceToVisit.getName().replace('.', '/')); + stubMissingDefaultAndBridgeMethods( + interfaceToVisit.getName().replace('.', '/'), mayNeedStubsForSuperclass); } } - private void stubMissingDefaultAndBridgeMethods(String implemented) { + private void stubMissingDefaultAndBridgeMethods( + String implemented, boolean mayNeedStubsForSuperclass) { ClassReader bytecode; boolean isBootclasspath; if (bootclasspath.isKnown(implemented)) { @@ -258,7 +260,9 @@ public class DefaultMethodClassFixer extends ClassVisitor { "Couldn't find interface %s implemented by %s", implemented, internalName); isBootclasspath = false; } - bytecode.accept(new DefaultMethodStubber(isBootclasspath), ClassReader.SKIP_DEBUG); + bytecode.accept( + new DefaultMethodStubber(isBootclasspath, mayNeedStubsForSuperclass), + ClassReader.SKIP_DEBUG); } private Class loadFromInternal(String internalName) { @@ -281,7 +285,8 @@ public class DefaultMethodClassFixer extends ClassVisitor { } private void recordInheritedMethods() { - InstanceMethodRecorder recorder = new InstanceMethodRecorder(); + InstanceMethodRecorder recorder = + new InstanceMethodRecorder(mayNeedInterfaceStubsForEmulatedSuperclass()); String internalName = superName; while (internalName != null) { ClassReader bytecode = bootclasspath.readIfKnown(internalName); @@ -429,11 +434,15 @@ public class DefaultMethodClassFixer extends ClassVisitor { private class DefaultMethodStubber extends ClassVisitor { private final boolean isBootclasspathInterface; + private final boolean mayNeedStubsForSuperclass; + private String stubbedInterfaceName; - public DefaultMethodStubber(boolean isBootclasspathInterface) { + public DefaultMethodStubber( + boolean isBootclasspathInterface, boolean mayNeedStubsForSuperclass) { super(Opcodes.ASM6); this.isBootclasspathInterface = isBootclasspathInterface; + this.mayNeedStubsForSuperclass = mayNeedStubsForSuperclass; } @Override @@ -472,6 +481,21 @@ public class DefaultMethodClassFixer extends ClassVisitor { MethodVisitor stubMethod = DefaultMethodClassFixer.this.visitMethod(access, name, desc, (String) null, exceptions); + String receiverName = stubbedInterfaceName; + String owner = InterfaceDesugaring.getCompanionClassName(stubbedInterfaceName); + if (mayNeedStubsForSuperclass) { + // Reflect what CoreLibraryInvocationRewriter would do if it encountered a super-call to a + // moved implementation of an emulated method. Equivalent to emitting the invokespecial + // super call here and relying on CoreLibraryInvocationRewriter for the rest + Class emulatedImplementation = + coreLibrarySupport.getCoreInterfaceRewritingTarget( + Opcodes.INVOKESPECIAL, superName, name, desc, /*itf=*/ false); + if (emulatedImplementation != null && !emulatedImplementation.isInterface()) { + receiverName = emulatedImplementation.getName().replace('.', '/'); + owner = checkNotNull(coreLibrarySupport.getMoveTarget(receiverName, name)); + } + } + int slot = 0; stubMethod.visitVarInsn(Opcodes.ALOAD, slot++); // load the receiver Type neededType = Type.getMethodType(desc); @@ -481,10 +505,10 @@ public class DefaultMethodClassFixer extends ClassVisitor { } stubMethod.visitMethodInsn( Opcodes.INVOKESTATIC, - InterfaceDesugaring.getCompanionClassName(stubbedInterfaceName), + owner, name, - InterfaceDesugaring.companionDefaultMethodDescriptor(stubbedInterfaceName, desc), - /*itf*/ false); + InterfaceDesugaring.companionDefaultMethodDescriptor(receiverName, desc), + /*itf=*/ false); stubMethod.visitInsn(neededType.getReturnType().getOpcode(Opcodes.IRETURN)); stubMethod.visitMaxs(0, 0); // rely on class writer to compute these @@ -563,8 +587,13 @@ public class DefaultMethodClassFixer extends ClassVisitor { private class InstanceMethodRecorder extends ClassVisitor { - public InstanceMethodRecorder() { + private final boolean ignoreEmulatedMethods; + + private String className; + + public InstanceMethodRecorder(boolean ignoreEmulatedMethods) { super(Opcodes.ASM6); + this.ignoreEmulatedMethods = ignoreEmulatedMethods; } @Override @@ -576,13 +605,23 @@ public class DefaultMethodClassFixer extends ClassVisitor { String superName, String[] interfaces) { checkArgument(BitFlags.noneSet(access, Opcodes.ACC_INTERFACE)); + className = name; // updated every time we start visiting another superclass super.visit(version, access, name, signature, superName, interfaces); } @Override public MethodVisitor visitMethod( int access, String name, String desc, String signature, String[] exceptions) { - // TODO(kmb): what if this method only exists on some devices, e.g., ArrayList.spliterator? + if (ignoreEmulatedMethods + && BitFlags.noneSet(access, Opcodes.ACC_STATIC) // short-circuit + && coreLibrarySupport.getCoreInterfaceRewritingTarget( + Opcodes.INVOKEVIRTUAL, className, name, desc, /*itf=*/ false) + != null) { + // *don't* record emulated core library method implementations in immediate subclasses of + // emulated core library clasess so that they can be stubbed (since the inherited + // implementation may be missing at runtime). + return null; + } recordIfInstanceMethod(access, name, desc); return null; } diff --git a/test/java/com/google/devtools/build/android/desugar/CoreLibrarySupportTest.java b/test/java/com/google/devtools/build/android/desugar/CoreLibrarySupportTest.java index 42f1f78..9b43207 100644 --- a/test/java/com/google/devtools/build/android/desugar/CoreLibrarySupportTest.java +++ b/test/java/com/google/devtools/build/android/desugar/CoreLibrarySupportTest.java @@ -19,6 +19,7 @@ import com.google.common.collect.ImmutableList; import com.google.devtools.build.android.desugar.io.CoreLibraryRewriter; import java.util.Collection; import java.util.Comparator; +import java.util.LinkedHashMap; import java.util.Map; import java.util.concurrent.ConcurrentMap; import org.junit.Test; @@ -164,6 +165,58 @@ public class CoreLibrarySupportTest { .isNull(); } + @Test + public void testGetCoreInterfaceRewritingTarget_emulatedImplementationMoved() throws Exception { + CoreLibrarySupport support = + new CoreLibrarySupport( + new CoreLibraryRewriter(""), + Thread.currentThread().getContextClassLoader(), + ImmutableList.of("java/util/Moved"), + ImmutableList.of("java/util/Map"), + ImmutableList.of("java/util/LinkedHashMap#forEach->java/util/Moved"), + ImmutableList.of()); + assertThat( + support.getCoreInterfaceRewritingTarget( + Opcodes.INVOKEINTERFACE, + "java/util/Map", + "forEach", + "(Ljava/util/function/BiConsumer;)V", + true)) + .isEqualTo(Map.class); + assertThat( + support.getCoreInterfaceRewritingTarget( + Opcodes.INVOKESPECIAL, + "java/util/Map", + "forEach", + "(Ljava/util/function/BiConsumer;)V", + true)) + .isEqualTo(Map.class); + assertThat( + support.getCoreInterfaceRewritingTarget( + Opcodes.INVOKEVIRTUAL, + "java/util/LinkedHashMap", + "forEach", + "(Ljava/util/function/BiConsumer;)V", + false)) + .isEqualTo(Map.class); + assertThat( + support.getCoreInterfaceRewritingTarget( + Opcodes.INVOKESPECIAL, + "java/util/LinkedHashMap", + "forEach", + "(Ljava/util/function/BiConsumer;)V", + false)) + .isEqualTo(LinkedHashMap.class); + assertThat( + support.getCoreInterfaceRewritingTarget( + Opcodes.INVOKESPECIAL, + "java/util/HashMap", + "forEach", + "(Ljava/util/function/BiConsumer;)V", + false)) + .isEqualTo(Map.class); + } + @Test public void testGetCoreInterfaceRewritingTarget_abstractMethod() throws Exception { CoreLibrarySupport support = -- cgit v1.2.3