RFR: JDK-8267936: PreserveAllAnnotations option isn't tested

[David Holmes]

Never mind ...

On 2/06/2021 10:47 pm, David Holmes wrote:
> On 2/06/2021 10:17 pm, Peter Levart wrote:
>> On Wed, 2 Jun 2021 11:40:13 GMT, Peter Levart <plevart at openjdk.org> 
>> wrote:
>>
>>>> ...hm, it seems that mere presence of any RUNTIME annotation that 
>>>> was RUNTIME already at the use compile time somehow affects 
>>>> -XX:+PreserveAllAnnotations option so that now RUNTIME annotations 
>>>> that were CLASS annotations at use compile time are not returned. 
>>>> Checking VM logic...
>>>
>>> For example, if I try this:
>>>
>>>      @Retention(CLASS)
>>>      public @interface AnnA_v1 {}
>>>
>>>      // An alternative version of AnnA_v1 with RUNTIME retention 
>>> instead.
>>>      // Used to simulate separate compilation (see AltClassLoader 
>>> below).
>>>      @Retention(RUNTIME)
>>>      public @interface AnnA_v2 {}
>>>
>>>      @Retention(RUNTIME)
>>>      public @interface AnnB {}
>>>
>>>      @AnnA_v1
>>>      @AnnB
>>>      public static class TestTask implements Runnable {
>>>          @Override
>>>          public void run() {
>>>              AnnA_v1 ann1 = 
>>> TestTask.class.getDeclaredAnnotation(AnnA_v1.class);
>>>
>>> ... then `ann1` is not returned, but if I comment out the `@AnnB` 
>>> annotation use on TestTask, `ann1` is returned.
>>
>> I think we found a bug here. After all, this endeavor was not in vain. 
>> The bug is in AnnotationParser:
>> https://github.com/openjdk/jdk/blob/6765f902505fbdd02f25b599f942437cd805cad1/src/java.base/share/classes/sun/reflect/annotation/AnnotationParser.java#L111 
>>
>>
>> You see, the parser reads a 16bit integer from the start of 
>> `rawAnnotations` and interprets it as the number of annotations to 
>> parse. But OTOH, `rawAnnotations` may in case when 
>> `-XX:+PreserveAllAnnotations` was used, contain two concatenated sets 
>> of encoded annotations, each starting with its own count of 
>> annotations. See how this concatenation is performed here:
>> https://github.com/openjdk/jdk/blob/6765f902505fbdd02f25b599f942437cd805cad1/src/hotspot/share/classfile/classFileParser.cpp#L3965 
>>
> 
> Two arrays are combined into one array and that is returned. What do you 
> mean by "each starting with its own count" ?? Where is the code that 
> shoves the apparent length into the start of the rawAnnotations byte[] ?

Sorry now I see what happens. We aren't combining two arrays of 
annotations we're concatenating two streams of byes, each of which 
represents a set of annotations, starting with the length.

The code that receives this on the JDK side doesn't seem to understand 
that this is a possibility.

Though maybe this isn't a bug, maybe the AnnotationParser is 
deliberately ignoring the second byte stream. (Though if it were 
deliberate there should be some commentary to that affect!)

David

> Thanks,
> David
> -----
> 
>> The concatenation is simply joining the arrays of u1 elements into a 
>> concatenated array. So when both RuntimeVisibleAnnotations and 
>> RuntimeInvisibleAnnotations are present (in case 
>> -XX:+PreserveAllAnnotations was used), the concatenated array starts 
>> with RuntimeVisibleAnnotations followed by RuntimeInvisibleAnnotations.
>>
>> AnnotationParser should not stop parsing when the 1st loop is finished 
>> and the rawAnnotations is not exhausted yet. It should continue with 
>> the 2nd round of parsing.
>>
>> -------------
>>
>> PR: https://git.openjdk.java.net/jdk/pull/4280
>>