RFR: 8144185: javac produces incorrect RuntimeInvisibleTypeAnnotations length attribute

[Srikanth]

Hi Liam,

OK. I have studied these differences and I am satisfied with the patch 
you have and the explanations you have provided. Thanks so much.

Can you please generate a mercurial change set for JDK10 and mail it to 
me privately with
this one comment incorporated so I can take it forward:

      - Rename test class from being T8144185.java to a more meaningful 
one such as say TypeAnnotationPropagationTest,java

Thanks!
Srikanth



On Thursday 11 May 2017 08:20 PM, Srikanth wrote:
> I made the assertions about the differences in the patches simply by 
> looking at the patches without ascertaining the behavior on tip due to 
> other fixes.
>
> I will look into this in detail to see what is the minimal set of 
> changes that would suffice.
>
> Srikanth
>
> On Thursday 11 May 2017 07:47 PM, Liam Miller-Cushon wrote:
>> Thanks for reviewing,
>>
>> On Thu, May 11, 2017 at 2:46 AM, Srikanth 
>> <srikanth.adayapalam at oracle.com 
>> <mailto:srikanth.adayapalam at oracle.com>> wrote:
>>
>>
>>     I am looking into this one - turns out I had a patch for this
>>     that was put through internal review process and some open issues
>>     that needed addressing caused it to be deferred.
>>
>>
>> If it'd be less work to continue with your patch that sounds good to 
>> me, thanks. As I said I ran into an ASM crash related to this, and 
>> thought it might be a simple fix.
>>
>>     (1) Your proposed patch simply switches on the kind to determine
>>     whether annotations need to be carried over. This looks
>>     incorrect/insufficient. Type annotations and declarations
>>     annotations have different rules to be carried over.
>>
>>     From
>>     http://mail.openjdk.java.net/pipermail/compiler-dev/2015-December/009865.html
>>     <http://mail.openjdk.java.net/pipermail/compiler-dev/2015-December/009865.html>:
>>
>>         - Declaration annotations on a lambda formal should not make
>>     it to the
>>            class file at all.
>>
>>
>> I think that is handled in ClassWriter: 
>> http://hg.openjdk.java.net/jdk9/dev/langtools/file/ee84b7d44339/src/jdk.compiler/share/classes/com/sun/tools/javac/jvm/ClassWriter.java#l1183 
>>
>>
>>          - Type annotations on the type of a lambda formal should be
>>     carried
>>     over to the type
>>            of the formal parameter of the synthetic method that
>>     implements
>>     the lambda[*]
>>
>>     I don't think this is handled by your patch ???
>>
>>
>> As far as I can tell this is handled correctly before and after the 
>> patch:
>>
>> class T {
>>   @Retention(RUNTIME) @Target(TYPE_USE) @interface A {}
>>   Function<String, String> r = (@A String x) -> x;
>> }
>> ...
>>     RuntimeVisibleTypeAnnotations:
>>       0: #20(): METHOD_FORMAL_PARAMETER, param_index=0
>>
>>     (2) From https://bugs.openjdk.java.net/browse/JDK-8144185
>>     <https://bugs.openjdk.java.net/browse/JDK-8144185>:
>>
>>     Not only is the range incorrect, but also LVT index appears
>>     incorrect:
>>
>>     I don't think this is handled by your patch ???
>>
>>
>> I thought it was handled by the patch and exercised by the included 
>> test case. What am I missing? The incorrect LVT index comes from 
>> associating the annotation with the capture parameter in the 
>> synthetic lambda method instead of with the original local variable. 
>> In the test, the LVT index of the variable in f() is 1, and the LVT 
>> index of the capture parameter in the synthetic lambda method is 0.
>>
>> Before the patch, I see:
>>
>>       RuntimeVisibleTypeAnnotations:
>>         0: #23(): LOCAL_VARIABLE, {start_pc=0, length=31, index=0}
>>
>> With the patch applied:
>>
>>       RuntimeVisibleTypeAnnotations:
>>         0: #23(): LOCAL_VARIABLE, {start_pc=3, length=8, index=1}
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/compiler-dev/attachments/20170612/90f79f4f/attachment.html>