RFR 8170667: ClassReader assigns method parameters from MethodParameters incorrectly when long/double parameters are present

[Vicente Romero]

On 12/09/2016 12:13 PM, Jan Lahoda wrote:
> Thanks Liam, Vicente!
>
> Pushed:
> http://hg.openjdk.java.net/jdk9/dev/langtools/rev/5d43af61155b
>
> Jan

thank you,
Vicente

>
> On 7.12.2016 21:10, Liam Miller-Cushon wrote:
>> Thanks for the review! I moved the entire test into one file, and fixed
>> the style issues.
>>
>> On Mon, Dec 5, 2016 at 3:36 PM, Vicente Romero
>> <vicente.romero at oracle.com <mailto:vicente.romero at oracle.com>> wrote:
>>
>>     Hi Liam,
>>
>>     Thanks for the patch. It's probably a question of style but it seems
>>     to me like the number of classes used could be reduced. For example,
>>     the annotation processor could define the annotated methods that it
>>     will later discover and process. This is just a suggestion. Also
>>     regarding indentation, it's preferable to use 4 spaces instead of 2.
>>
>>     Thanks,
>>     Vicente
>>
>>
>>     On 12/02/2016 11:41 AM, Liam Miller-Cushon wrote:
>>>     Thanks Jan, I've attached the full patch with a test.
>>>
>>>     On Fri, Dec 2, 2016 at 5:31 AM, Jan Lahoda <jan.lahoda at oracle.com
>>>     <mailto:jan.lahoda at oracle.com>> wrote:
>>>
>>>         Hi Liam,
>>>
>>>         Thanks for the report and patch, I've filled:
>>>         https://bugs.openjdk.java.net/browse/JDK-8170667
>>> <https://bugs.openjdk.java.net/browse/JDK-8170667>
>>>
>>>         I think you are right about the problem, and the fix seems
>>>         very reasonable to me.
>>>
>>>         Jan
>>>
>>>
>>>         On 2.12.2016 03:09, Liam Miller-Cushon wrote:
>>>
>>>             I think I found a MethodParameters class reading bug that
>>>             affects
>>>             annotation processing.
>>>
>>>             If MethodParameters attributes are not available parameter
>>>             names are
>>>             inferred from the LVT, which uses two slots for doubles
>>>             and longs. The
>>>             logic for skipping the extra slot is used even if the data
>>>             was read from
>>>             a MethodParameters attribute, where the indices are always
>>>             contiguous.
>>>             So if javac sees a parameter of type double or long it
>>>             skips the next
>>>             entry in the MethodParameters table.
>>>
>>>             There's a simple fix:
>>>
>>>             diff -r 775a385c4af6
>>> src/jdk.compiler/share/classes/com/sun/tools/javac/jvm/ClassReader.java
>>>             ---
>>> a/src/jdk.compiler/share/classes/com/sun/tools/javac/jvm/ClassReader.javaThu
>>>             Dec 01 17:25:45 2016 -0800
>>>             +++
>>> b/src/jdk.compiler/share/classes/com/sun/tools/javac/jvm/ClassReader.javaThu
>>>             Dec 01 18:05:14 2016 -0800
>>>             @@ -2321,7 +2321,7 @@
>>>                                   ? parameterNameIndices[index] : 0);
>>>                           Name name = nameIdx == 0 ? names.empty :
>>>             readName(nameIdx);
>>>                           paramNames = paramNames.prepend(name);
>>>             -            index += Code.width(t);
>>>             +            index += sawMethodParameters ? 1 : 
>>> Code.width(t);
>>>                       }
>>>                       sym.savedParameterNames = paramNames.reverse();
>>>                   }
>>>
>>>             If my diagnosis sounds correct, I can clean up the patch
>>>             and add a test.
>>>
>>>             Here's the full repro:
>>>
>>>             === Lib.java
>>>             class Lib {
>>>                void f(int a, long b, int c, int d) {}
>>>             }
>>>             === Test.java
>>>             @Deprecated
>>>             class Test {}
>>>             === ParameterProcessor.java
>>>             import java.util.Set;
>>>             import javax.annotation.processing.*;
>>>             import javax.lang.model.SourceVersion;
>>>             import javax.lang.model.element.*;
>>>
>>>             @SupportedAnnotationTypes("*")
>>>             public class ParameterProcessor extends AbstractProcessor {
>>>                @Override
>>>                public boolean process(Set<? extends TypeElement>
>>>             annotations,
>>>             RoundEnvironment roundEnv) {
>>>                  if (roundEnv.processingOver()) {
>>>                    return false;
>>>                  }
>>>                  TypeElement s =
>>> processingEnv.getElementUtils().getTypeElement("Lib");
>>>                  for (Element e : s.getEnclosedElements()) {
>>>                    if (e instanceof ExecutableElement) {
>>>                      for (VariableElement v : ((ExecutableElement)
>>>             e).getParameters()) {
>>>                        System.err.printf("  %s %s\n", v.asType(),
>>>             v.getSimpleName());
>>>                      }
>>>                    }
>>>                  }
>>>                  return false;
>>>                }
>>>
>>>                @Override
>>>                public SourceVersion getSupportedSourceVersion() {
>>>                  return SourceVersion.latest();
>>>                }
>>>             }
>>>
>>>             $ javac -parameters Lib.java
>>>             $ javac ParameterProcessor.java
>>>             $ javac -processor ParameterProcessor Test.java
>>>                int a
>>>                long b
>>>                int d
>>>                int arg3
>>>
>>>             Note that the third parameter is given the fourth
>>>             parameter's name, and
>>>             the fourth parameter's name is unknown.
>>>
>>>             Thanks,
>>>
>>>             Liam
>>>
>>>
>>
>>