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

[Liam Miller-Cushon]

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>
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> wrote:
>
>> Hi Liam,
>>
>> Thanks for the report and patch, I've filled:
>> 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/Cla
>>> ssReader.javaThu
>>> Dec 01 17:25:45 2016 -0800
>>> +++
>>> b/src/jdk.compiler/share/classes/com/sun/tools/javac/jvm/Cla
>>> ssReader.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
>>>
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/compiler-dev/attachments/20161207/ec8fb4a0/attachment-0001.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 8170667.2.patch
Type: application/octet-stream
Size: 4688 bytes
Desc: not available
URL: <http://mail.openjdk.java.net/pipermail/compiler-dev/attachments/20161207/ec8fb4a0/8170667.2-0001.patch>