Do not workaround javac bug that leads to wrong parameter annotations. ASM internally corrects the number of parameter annotations when javac gets it wrong. It does so by using a non-existing java.lang.Synthetic annotation. In the MethodWriter it then undoes that again so that the output will maintain the javac bug. Since we are not using the MethodWriter, we get only half of the workaround, namely the bit that adds these fake annotations. We therefore have to filter them out ourselves to get the right (wrong) behavior. :) R=zerny@google.com Bug: 62300145 Change-Id: Ieec7d165d8b2a6eb664f965693de76801e691bb8
diff --git a/src/main/java/com/android/tools/r8/graph/JarClassFileReader.java b/src/main/java/com/android/tools/r8/graph/JarClassFileReader.java index 1673470..3cc3ac4 100644 --- a/src/main/java/com/android/tools/r8/graph/JarClassFileReader.java +++ b/src/main/java/com/android/tools/r8/graph/JarClassFileReader.java
@@ -24,7 +24,6 @@ import com.android.tools.r8.graph.DexValue.DexValueString; import com.android.tools.r8.graph.DexValue.DexValueType; import com.android.tools.r8.graph.JarCode.ReparseContext; -import com.android.tools.r8.utils.InternalResource; import java.io.IOException; import java.io.InputStream; import java.util.ArrayList; @@ -396,6 +395,7 @@ private final int parameterCount; private List<DexAnnotation> annotations = null; private DexValue defaultAnnotation = null; + private int fakeParameterAnnotations = 0; private List<List<DexAnnotation>> parameterAnnotations = null; private List<DexValue> parameterNames = null; private List<DexValue> parameterFlags = null; @@ -447,15 +447,28 @@ @Override public AnnotationVisitor visitParameterAnnotation(int parameter, String desc, boolean visible) { + // ASM decided to workaround a javac bug that incorrectly deals with synthesized parameter + // annotations. However, that leads us to have different behavior than javac+jvm and + // dx+art. The workaround is to use a non-existing descriptor "Ljava/lang/Synthetic;" for + // exactly this case. In order to remove the workaround we ignore all annotations + // with that descriptor. If javac is fixed, the ASM workaround will not be hit and we will + // never see this non-existing annotation descriptor. ASM uses the same check to make + // sure to undo their workaround for the javac bug in their MethodWriter. + if (desc.equals("Ljava/lang/Synthetic;")) { + assert parameterAnnotations == null; + fakeParameterAnnotations++; + return null; + } if (parameterAnnotations == null) { - parameterAnnotations = new ArrayList<>(parameterCount); - for (int i = 0; i < parameterCount; i++) { + int adjustedParameterCount = parameterCount - fakeParameterAnnotations; + parameterAnnotations = new ArrayList<>(adjustedParameterCount); + for (int i = 0; i < adjustedParameterCount; i++) { parameterAnnotations.add(new ArrayList<>()); } } return createAnnotationVisitor(desc, visible, mv == null ? null : mv.visitParameterAnnotation(parameter, desc, visible), - parameterAnnotations.get(parameter), parent.application); + parameterAnnotations.get(parameter - fakeParameterAnnotations), parent.application); } @Override
diff --git a/src/test/examples/regress_62300145/Regress.java b/src/test/examples/regress_62300145/Regress.java new file mode 100644 index 0000000..77aa6bd --- /dev/null +++ b/src/test/examples/regress_62300145/Regress.java
@@ -0,0 +1,31 @@ +// Copyright (c) 2017, 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 regress_62300145; + +import java.lang.annotation.Annotation; +import java.lang.annotation.Retention; +import java.lang.annotation.RetentionPolicy; +import java.lang.reflect.Constructor; + +public class Regress { + @Retention(RetentionPolicy.RUNTIME) + public @interface A { + } + + public class InnerClass { + public InnerClass(@A String p) {} + } + + public static void main(String[] args) throws NoSuchMethodException { + Constructor<InnerClass> constructor = + InnerClass.class.getDeclaredConstructor(Regress.class, String.class); + int i = 0; + for (Annotation[] annotations : constructor.getParameterAnnotations()) { + System.out.print(i++ + ": "); + for (Annotation annotation : annotations) { + System.out.println(annotation); + } + } + } +}
diff --git a/src/test/java/com/android/tools/r8/R8RunExamplesTest.java b/src/test/java/com/android/tools/r8/R8RunExamplesTest.java index 4a06085..ba4eea0 100644 --- a/src/test/java/com/android/tools/r8/R8RunExamplesTest.java +++ b/src/test/java/com/android/tools/r8/R8RunExamplesTest.java
@@ -57,58 +57,58 @@ @Parameters(name = "{0}{1}") public static Collection<String[]> data() { - String[][] tests = { - {"arithmetic.Arithmetic", null}, - {"arrayaccess.ArrayAccess", "37=37"}, - {"barray.BArray", "bits 42 and bool true"}, - {"bridge.BridgeMethod", null}, - {"cse.CommonSubexpressionElimination", "1\n1\n2 2\n2\n3\n3\n4 4\n4\nA\nB\n"}, - {"constants.Constants", null}, - {"controlflow.ControlFlow", null}, - {"conversions.Conversions", null}, - {"floating_point_annotations.FloatingPointValuedAnnotationTest", null}, - {"filledarray.FilledArray", null}, - {"hello.Hello", "Hello, world"}, - {"ifstatements.IfStatements", null}, - {"instancevariable.InstanceVariable", "144=144"}, - {"instanceofstring.InstanceofString", "is-string:true"}, - {"invoke.Invoke", null}, - {"jumbostring.JumboString", null}, - {"loadconst.LoadConst", null}, - {"newarray.NewArray", null}, - {"regalloc.RegAlloc", null}, - {"returns.Returns", null}, - {"staticfield.StaticField", "101010\n101010\nABC\nABC\n"}, - {"stringbuilding.StringBuilding", - "a2c-xyz-abc7xyz\ntrueABCDE1234232.21.101an Xstringbuilder"}, - {"switches.Switches", null}, - {"sync.Sync", null}, - {"throwing.Throwing", "Throwing"}, - {"trivial.Trivial", null}, - {"trycatch.TryCatch", "Success!"}, - {"nestedtrycatches.NestedTryCatches", "EXCEPTION: PRIMARY"}, - {"trycatchmany.TryCatchMany", "Success!"}, - {"invokeempty.InvokeEmpty", "AB"}, - {"regress.Regress", null}, - {"regress2.Regress2", "START\nLOOP\nLOOP\nLOOP\nLOOP\nLOOP\nEND"}, - {"regress_37726195.Regress", null}, - {"regress_37658666.Regress", null}, - {"regress_37875803.Regress", null}, - {"regress_37955340.Regress", null}, - {"memberrebinding2.Test", Integer.toString((8 * 9) / 2)}, - {"memberrebinding3.Test", null}, - {"minification.Minification", null}, - {"enclosingmethod.Main", null}, - {"interfaceinlining.Main", null}, - {"switchmaps.Switches", null}, + String[] tests = { + "arithmetic.Arithmetic", + "arrayaccess.ArrayAccess", + "barray.BArray", + "bridge.BridgeMethod", + "cse.CommonSubexpressionElimination", + "constants.Constants", + "controlflow.ControlFlow", + "conversions.Conversions", + "floating_point_annotations.FloatingPointValuedAnnotationTest", + "filledarray.FilledArray", + "hello.Hello", + "ifstatements.IfStatements", + "instancevariable.InstanceVariable", + "instanceofstring.InstanceofString", + "invoke.Invoke", + "jumbostring.JumboString", + "loadconst.LoadConst", + "newarray.NewArray", + "regalloc.RegAlloc", + "returns.Returns", + "staticfield.StaticField", + "stringbuilding.StringBuilding", + "switches.Switches", + "sync.Sync", + "throwing.Throwing", + "trivial.Trivial", + "trycatch.TryCatch", + "nestedtrycatches.NestedTryCatches", + "trycatchmany.TryCatchMany", + "invokeempty.InvokeEmpty", + "regress.Regress", + "regress2.Regress2", + "regress_37726195.Regress", + "regress_37658666.Regress", + "regress_37875803.Regress", + "regress_37955340.Regress", + "regress_62300145.Regress", + "memberrebinding2.Test", + "memberrebinding3.Test", + "minification.Minification", + "enclosingmethod.Main", + "interfaceinlining.Main", + "switchmaps.Switches", }; List<String[]> fullTestList = new ArrayList<>(tests.length * 2); - for (String[] test : tests) { - String qualified = test[0]; + for (String test : tests) { + String qualified = test; String pkg = qualified.substring(0, qualified.lastIndexOf('.')); - fullTestList.add(new String[]{pkg, DEX_EXTENSION, qualified, test[1]}); - fullTestList.add(new String[]{pkg, JAR_EXTENSION, qualified, test[1]}); + fullTestList.add(new String[]{pkg, DEX_EXTENSION, qualified}); + fullTestList.add(new String[]{pkg, JAR_EXTENSION, qualified}); } return fullTestList; } @@ -118,7 +118,6 @@ private final String name; private final String mainClass; - private final String expectedOutput; private final String fileType; private static Map<DexVm, List<String>> failsOn = ImmutableMap.of( @@ -150,11 +149,10 @@ ) ); - public R8RunExamplesTest(String name, String fileType, String mainClass, String expectedOutput) { + public R8RunExamplesTest(String name, String fileType, String mainClass) { this.name = name; this.fileType = fileType; this.mainClass = mainClass; - this.expectedOutput = expectedOutput; } private Path getInputFile() { @@ -249,10 +247,6 @@ } String output = ToolHelper.checkArtOutputIdentical(original, generated.toString(), mainClass, version); - if (expectedOutput != null && !expectedToFail) { - assertTrue("'" + output + "' lacks '" + expectedOutput + "'", - output.contains(expectedOutput)); - } } } }