Re: RFR: JDK-8267936: PreserveAllAnnotations option isn't tested
On Tue, 1 Jun 2021 09:30:40 GMT, Jaroslav Tulach wrote: > There doesn't seem to be much support for the complete changes in #4245. To > get at least something useful from that endeavor I have extracted the test > for existing behavior of `-XX:+PreserveAllAnnotations` and I am offering it > in this pull request without any changes to the JVM behavior. To share the information... I contacted one of the original authors of Java annotations, but he doesn't remember anything about the option. Here's the conversation... On Thu, Jun 3, 2021 at 11:53 PM Peter Levart wrote: > > There is an undocumented JVM flag called > "-XX+PreserveAllAnnotations" > which according to JVM code affects the way > "RuntimeInvisibleAnnotations" kind of class attributes are > treated at > runtime. Normally they are ignored (since they normally contain > only > metadata for annotation types with CLASS retention), but when > this flag > is passed to JVM, such attributes are also passed to > AnnotationParser > together with "RuntimeVisibleAnnotations" kind of class > attributes. > > So the question is: "What was the original intent of this JVM > flag". A > debate sprung in a review thread on JDK GitHub and some say that > this is > probably just a "leftover from an early implementation" and are > proposing to deprecate and remove it. Others (me mostly) see some > sense > in the feature as it allows migrating an annotation type from > CLASS > retention to RUNTIME without recompiling all the classes that use > the > annotation. Do you happen to know the original intent of this JVM > flag? On Fri, Jun 4, 2021 at 6:54 AM Neal Gafter wrote: > > I don’t recall that flag, but my suspicion is that it was used to > assist testing. > If you ask me, I think the ecosystem would be better without it. On 04/06/2021 15:56, Neal Gafter wrote: > > One more thing. You don’t have to recompile uses of an annotation to > migrate > from class to runtime retention. You only have to recompile the > attribute declaration. On Fri, Jun 4, 2021 at 7:15 AM Peter Levart wrote: > > Not really. The AnnotationParser does filter out annotations based on > current "retention" > of annotation type which is just a meta-annotation on the annotation > type, so it would appear > that only the annotation declaration has to be recompiled, but as I > described, the annotation > uses in classes that use the annotation get compiled into two kinds of > class attributes: > RuntimeVisibleAnnotations (those with RUNTIME retention) and > RuntimeInvisibleAnnotations > (those with CLASS retention). Only those from RuntimeVisibleAnnotations > are passed to > AnnotationParser at runtime unless this VM flag is given which makes the > VM pass a concatenation > of RuntimeVisibleAnnotations and RuntimeInvisibleAnnotations to the > AnnotationParser. > So merely changing the annotation retention and recompiling the > annotation declaration is not enough. > You have to also either recompile annotation uses or pass this VM flag. > So I was wondering if the flag was actually meant for this purpose. On 04/06/2021 17:30, Neal Gafter wrote: > > Got it. I don't remember anything about the VM flag, but I doubt it was > designed to be used for this purpose. - PR: https://git.openjdk.java.net/jdk/pull/4280
Re: RFR: JDK-8267936: PreserveAllAnnotations option isn't tested
On Tue, 1 Jun 2021 09:30:40 GMT, Jaroslav Tulach wrote: > There doesn't seem to be much support for the complete changes in #4245. To > get at least something useful from that endeavor I have extracted the test > for existing behavior of `-XX:+PreserveAllAnnotations` and I am offering it > in this pull request without any changes to the JVM behavior. Well, if you are willing to take this further, I would suggest you modify the test so that it would only test the "correct" behavior and not also the "incorrect" one. Meaning, you should test annotated elements (class in you case) that are annotated with CLASS retention annotations only (at the time the annotation uses are compiled) and not with mixed set (CLASS and RUNTIME retention) annotations. In that case the outcome is sensible (and useful). If others agree that adding such test is OK, I can sponsor it. - PR: https://git.openjdk.java.net/jdk/pull/4280
Re: RFR: JDK-8267936: PreserveAllAnnotations option isn't tested
On Tue, 1 Jun 2021 09:30:40 GMT, Jaroslav Tulach wrote: > There doesn't seem to be much support for the complete changes in #4245. To > get at least something useful from that endeavor I have extracted the test > for existing behavior of `-XX:+PreserveAllAnnotations` and I am offering it > in this pull request without any changes to the JVM behavior. Is there anyone willing to sponsor this PR? What changes shall I make to get your sponsorship? - PR: https://git.openjdk.java.net/jdk/pull/4280
Re: RFR: JDK-8267936: PreserveAllAnnotations option isn't tested
Hi Peter,
On 3/06/2021 6:52 pm, Peter Levart wrote:
On Thu, 3 Jun 2021 07:31:14 GMT, David Holmes wrote:
The code is confusing because it gives no indication that it is aware
that runtime invisible annotations could be present:
/**
* Parses the annotations described by the specified byte array.
* resolving constant references in the specified constant pool.
* The array must contain an array of annotations as described
* in the RuntimeVisibleAnnotations_attribute:
but at the same time it checks for the RUNTIME retention, which means it
must have recognised the possibility there could be something else
there.
Yes, there could be a CLASS retention annotation there (even though
`-XX+PreserveAllAnnotations` was not used at runtime, so rawAnnotations
contains the content of `RuntimeVisibleAnnotations` only). Again, such
annotation was RUNTIME retention when its use was compiled into a class, but at
runtime such annotation may be updated to have CLASS or even SOURCE retention.
Such annotation use is filtered out.
Sorry Peter I'm not following you. I am only talking about runtime here.
The VM loads a class at runtime and examines the annotation attributes
that exist in that classfile.
Right, but into which annotation attribute (`RuntimeVisibleAnnotations` vs.
`RuntimeInvisibleAnnotations`) of that class the annotation use was encoded
depends on what retention the annotation had at compile time, because those
attributes are created by `javac` compiler.
Okay
Some will be visible annotations (which
implies RUNTIME retention), others may be invisible (which implies
!RUNTIME which I assume means CLASS). It provides access to these via
the "raw annotations" and the reflection code then processes that
through the AnnotationProcessor.
Right, so reflection code at runtime accesses the annotations that were
compiled into the class annotation attributes by parsing them and filtering out
all but RUNTIME annotations. But this filtering is done at runtime and uses
annotation's current set of meta-annotations values (i.e. `@Retention`) which
can differ from what this same annotation had at class compile time. So this is
how current RUNTIME annotations can end up in the `RuntimeInvisibleAnnotations`
class attribute and how current CLASS annotations can end up in
`RuntimeVisibleAnnotations` class attribute. It's the consequence of separate
compilation.
Okay I see what you are talking about in regards to separate compilation.
I don't know exactly what you mean by an annotation use being compiled
into a class, but I assume it is something like the way compile-time
constants are compiled in. But I don't see how that relates to the
current discussion.
An annotation use is the use of annotation in the source code to annotate
something (Class, Method, Field, ...). For example:
@AnnA
public class Use { ... }
`javac` decides into which class attribute (`RuntimeVisibleAnnotations` vs.
`RuntimeInvisibleAnnotations`) of Use.class file it will encode the `@AnnA` use
by examining `AnnA`'s `@Retention` meta-annotation at that time. Say that at
that time the annotation was this:
@Retention(CLASS)
public @interface AnnA {}
`javac` would encode the `Use`'s use of that annotation into the
`RuntimeInvisibleAnnotations` attribute of `Use.class`.
Now comes runtime. Annotation maintainer decides to lift the retention of
`AnnA` annotation to RUNTIME and now it looks like this:
@Retention(RUNTIME)
public @interface AnnA {}
He tries to run some new code to extract the annotation value from `Use` class
at runtime via reflection, because now at runtime the annotation is updated to
have RUNTIME retention. But he also doesn't have access to `Use.java` to
recompile it with updated annotation. He just has access to `Use.class` that
was compiled with an old version of the same annotation. As we said,
`Use.class` has encoded the annotation use in its `RuntimeInvisibleAnnotations`
attribute. Voila, here comes `-XX+PreserveAllAnnotations` option to enable
passing `RuntimeInvisibleAnnotations` attribute with encoded annotations to the
reflection runtime and annotation parser which would return such annotation use.
I see the picture you are painting and that things could work that way,
but I don't know if I agree that this was an intention or that they
should work that way.
The separate compilation story with annotations is somewhat different to
other separate compilation issues as the JVMS does not really provide
any guidance here. The RuntimeInvisibleAnnotations are not even parsed
by the VM so it know nothing about which annotation types are referred
to, never loads those types or does any checking to see if the
properties of that type (like Retention) remain the same. Even
RuntimeVisibleAnnotations are only structurally parsed to see of there
are annotations the VM has to be aware of (ie @Contended) but those can
never be out-of-sync through separate compilation.
So in essence
Re: RFR: JDK-8267936: PreserveAllAnnotations option isn't tested
On Thu, 3 Jun 2021 07:31:14 GMT, David Holmes wrote:
> > > The code is confusing because it gives no indication that it is aware
> > > that runtime invisible annotations could be present:
> > > /**
> > > * Parses the annotations described by the specified byte array.
> > > * resolving constant references in the specified constant pool.
> > > * The array must contain an array of annotations as described
> > > * in the RuntimeVisibleAnnotations_attribute:
> > > but at the same time it checks for the RUNTIME retention, which means it
> > > must have recognised the possibility there could be something else
> > > there.
> >
> >
> > Yes, there could be a CLASS retention annotation there (even though
> > `-XX+PreserveAllAnnotations` was not used at runtime, so rawAnnotations
> > contains the content of `RuntimeVisibleAnnotations` only). Again, such
> > annotation was RUNTIME retention when its use was compiled into a class,
> > but at runtime such annotation may be updated to have CLASS or even SOURCE
> > retention. Such annotation use is filtered out.
>
> Sorry Peter I'm not following you. I am only talking about runtime here.
> The VM loads a class at runtime and examines the annotation attributes
> that exist in that classfile.
Right, but into which annotation attribute (`RuntimeVisibleAnnotations` vs.
`RuntimeInvisibleAnnotations`) of that class the annotation use was encoded
depends on what retention the annotation had at compile time, because those
attributes are created by `javac` compiler.
> Some will be visible annotations (which
> implies RUNTIME retention), others may be invisible (which implies
> !RUNTIME which I assume means CLASS). It provides access to these via
> the "raw annotations" and the reflection code then processes that
> through the AnnotationProcessor.
Right, so reflection code at runtime accesses the annotations that were
compiled into the class annotation attributes by parsing them and filtering out
all but RUNTIME annotations. But this filtering is done at runtime and uses
annotation's current set of meta-annotations values (i.e. `@Retention`) which
can differ from what this same annotation had at class compile time. So this is
how current RUNTIME annotations can end up in the `RuntimeInvisibleAnnotations`
class attribute and how current CLASS annotations can end up in
`RuntimeVisibleAnnotations` class attribute. It's the consequence of separate
compilation.
>
> I don't know exactly what you mean by an annotation use being compiled
> into a class, but I assume it is something like the way compile-time
> constants are compiled in. But I don't see how that relates to the
> current discussion.
An annotation use is the use of annotation in the source code to annotate
something (Class, Method, Field, ...). For example:
@AnnA
public class Use { ... }
`javac` decides into which class attribute (`RuntimeVisibleAnnotations` vs.
`RuntimeInvisibleAnnotations`) of Use.class file it will encode the `@AnnA` use
by examining `AnnA`'s `@Retention` meta-annotation at that time. Say that at
that time the annotation was this:
@Retention(CLASS)
public @interface AnnA {}
`javac` would encode the `Use`'s use of that annotation into the
`RuntimeInvisibleAnnotations` attribute of `Use.class`.
Now comes runtime. Annotation maintainer decides to lift the retention of
`AnnA` annotation to RUNTIME and now it looks like this:
@Retention(RUNTIME)
public @interface AnnA {}
He tries to run some new code to extract the annotation value from `Use` class
at runtime via reflection, because now at runtime the annotation is updated to
have RUNTIME retention. But he also doesn't have access to `Use.java` to
recompile it with updated annotation. He just has access to `Use.class` that
was compiled with an old version of the same annotation. As we said,
`Use.class` has encoded the annotation use in its `RuntimeInvisibleAnnotations`
attribute. Voila, here comes `-XX+PreserveAllAnnotations` option to enable
passing `RuntimeInvisibleAnnotations` attribute with encoded annotations to the
reflection runtime and annotation parser which would return such annotation use.
Now imagine whole libraries with classes that use an annotation which began its
life as CLASS annotation because it was paired with AnnotationProcessor(s) that
processed those annotation uses at compile time, but now some runtime tool
emerges that would like to see those annotations too. The maintainer accepts
the proposal to promote annotation's retention from CLASS to RUNTIME in new
version of annotation. But who is going to re-compile all those libraries?
>
> David
Regards, Peter
-
PR: https://git.openjdk.java.net/jdk/pull/4280
Re: RFR: JDK-8267936: PreserveAllAnnotations option isn't tested
On Tue, 1 Jun 2021 09:30:40 GMT, Jaroslav Tulach wrote: > There doesn't seem to be much support for the complete changes in #4245. To > get at least something useful from that endeavor I have extracted the test > for existing behavior of `-XX:+PreserveAllAnnotations` and I am offering it > in this pull request without any changes to the JVM behavior. > _Mailing list message from [Brian Goetz](mailto:brian.go...@oracle.com) on > [core-libs-dev](mailto:core-libs-...@mail.openjdk.java.net):_ > > It seems pretty clear that this "feature" is a leftover from an early > implementation, doesn't clearly say what it is supposed to do, is more > complicated than it looks, and is buggily implemented.? I agree that this "feature" is poorly documented and buggily implemented, but I can't agree that it was not created for a reason. At least I can't so easily classify it as a "leftover". In particular because it makes sense. > While I > understand the temptation to "fix" it, at this point we'd realistically > be adding a basically entirely new reflection feature that hasn't gone > through any sort of design analysis. That "feature" is there already from JDK 5 days. We would not be adding it. The reason why we are dealing with it here is very simple. It was/is useful for solving a real world problem and a user (looking a Jaroslav) tried to use it, but because it was not documented, the user miss-interpreted its meaning and tried to propose a "patch" that would solve his problem in the wrong way. Through the discussion we established a more correct way to use the "feature" (he just has to modify the annotation). Jaroslav can use it. Nothing has to be done to it. Now because the feature wasn't documented and didn't even have a unit test, Jaroslav volunteered to create a test for it. Reviewing that test, I found an inconsistency and discovered a corner case where the "feature" behaves inconsistently. Now at this point we can either fix this inconsistency or not. I guess not many were affected by it since nobody officially complained and from that we can predict that it is highly unlikely that anybody will. > -- we'd just be guessing about what > the original intent of this vestigial feature is.? The feature makes sense to me. But would it help if we invited original authors (Joshua Bloch and/or Neal Gafter) to explain? Of course, it they're willing to participate and if they remember what the heck they were doing :-) ... > It seems the > reasonable options are to either leave it alone, deprecate it, or engage > in a much more deliberate redesign.? (And given the fact that I can > think of at least 1,000 things that are higher priority, I'd not be > inclined to pursue the third option.) I see deprecating a feature so easily on the terms of "it is not documented and it is buggy" and without even trying to understand the feature and the consequences of its deprecation as a more "ruthless" act than trying to fix its corner-case inconsistency by carefully paying attention to compatibility. The feature is clearly useful (a user is using it to solve the problem). Deprecating it and later removing it would leave such user(s) without a means to solve such problem(s). So I'm at least for leaving it alone at this point. Regards, Peter - PR: https://git.openjdk.java.net/jdk/pull/4280
Re: RFR: JDK-8267936: PreserveAllAnnotations option isn't tested
On 3/06/2021 4:49 pm, Peter Levart wrote: On Wed, 2 Jun 2021 22:44:10 GMT, David Holmes wrote: I think this is not deliberate. Since `rawAnnotations` may end up having any of the following: - `RuntimeVisibleAnnotations` (when there were just `RUNTIME` annotation usages compiled into the class or `-XX+PreserveAllAnnotations` was not used at runtime) - `RuntimeInvisibleAnnotations` (when there were just `CLASS` annotation usages compiled into the class and `-XX+PreserveAllAnnotations` was used at runtime) - `RuntimeVisibleAnnotations + RuntimeInvisibleAnnotations` (when there were `RUNTIME` and `CLASS` annotation usages compiled into the class and `-XX+PreserveAllAnnotations` was used at runtime) So why would `RuntimeInvisibleAnnotations` be skipped in 3rd case but not in 2nd case? Well in the second case there is no way to know it is looking only at invisible annotations, so it has to read them and then discard them as they were in fact CLASS retention. The skipping could be seen as an optimization. Not all annotations in the 2nd case need to be CLASS retention. They were CLASS retention when the class that uses them was compiled, but at runtime, the retention may be different (separate compilation). So they are actually returned by the reflection in that case. The code is confusing because it gives no indication that it is aware that runtime invisible annotations could be present: /** * Parses the annotations described by the specified byte array. * resolving constant references in the specified constant pool. * The array must contain an array of annotations as described * in the RuntimeVisibleAnnotations_attribute: but at the same time it checks for the RUNTIME retention, which means it must have recognised the possibility there could be something else there. Yes, there could be a CLASS retention annotation there (even though `-XX+PreserveAllAnnotations` was not used at runtime, so rawAnnotations contains the content of `RuntimeVisibleAnnotations` only). Again, such annotation was RUNTIME retention when its use was compiled into a class, but at runtime such annotation may be updated to have CLASS or even SOURCE retention. Such annotation use is filtered out. Sorry Peter I'm not following you. I am only talking about runtime here. The VM loads a class at runtime and examines the annotation attributes that exist in that classfile. Some will be visible annotations (which implies RUNTIME retention), others may be invisible (which implies !RUNTIME which I assume means CLASS). It provides access to these via the "raw annotations" and the reflection code then processes that through the AnnotationProcessor. I don't know exactly what you mean by an annotation use being compiled into a class, but I assume it is something like the way compile-time constants are compiled in. But I don't see how that relates to the current discussion. David - That check is day 2 code though, on day 1 there was this comment: /* XXX Uncomment when this meta-attribute on meta-attributes (Neal's putback) if (type.retention() == VisibilityLevel.RUNTIME) XXX */ I guess this was just code in creation before release. At some point, the `RetentionPolicy` enum was added, but above comment refers to it by the name `VisibilityLevel` But the end result is that the code in AnnotationParser correctly filters out all non-RUNTIME annotations, either directly, or by skipping them in the stream in the first place. So in that sense there is no bug, but the code could do with some additional comments. The problem is that it sometimes skips RUNTIME annotations too. I consider this a bug. Cheers, David Regard, Peter - PR: https://git.openjdk.java.net/jdk/pull/4280
Re: RFR: JDK-8267936: PreserveAllAnnotations option isn't tested
On Wed, 2 Jun 2021 22:47:13 GMT, David Holmes wrote: > On 3/06/2021 2:54 am, Joe Darcy wrote: > > > If the reflection runtime doesn't implement the semantics of > > -XX+PreserveAllAnnotations, I suggest deprecating/obsoleting/etc. that > > option now. > > I have to agree with Joe now. I mistakenly thought the raw annotation > stream was exposed to some parts of reflection, but now I see that it > all goes through AnnotationParser, which strips out all non-Runtime > retention annotations. So the flag is useless as the data it causes to > be passed to the JDK is thrown away anyway. > > Cheers, > David We are constantly dancing around the same problem which is a false assumption that `RuntimeVisibleAnnotations` attribute contains only RUNTIME retention annotations and that `RuntimeInvisibleAnnotations` attribute contains only CLASS retention annotations. This is simply not true in a real world. It is only true in a world where either nothing changes from the day 1 when it is 1st created or in a world where all the sources that compose the application are always recompiled. Those two worlds are ideal worlds that don't exist. In other words, the flag is useless in the dream world(s) but not in the real world. - PR: https://git.openjdk.java.net/jdk/pull/4280
Re: RFR: JDK-8267936: PreserveAllAnnotations option isn't tested
On Wed, 2 Jun 2021 22:44:10 GMT, David Holmes wrote: > > I think this is not deliberate. Since `rawAnnotations` may end up having > > any of the following: > > - `RuntimeVisibleAnnotations` (when there were just `RUNTIME` annotation > > usages compiled into the class or `-XX+PreserveAllAnnotations` was not used > > at runtime) > > - `RuntimeInvisibleAnnotations` (when there were just `CLASS` annotation > > usages compiled into the class and `-XX+PreserveAllAnnotations` was used at > > runtime) > > - `RuntimeVisibleAnnotations + RuntimeInvisibleAnnotations` (when there > > were `RUNTIME` and `CLASS` annotation usages compiled into the class and > > `-XX+PreserveAllAnnotations` was used at runtime) > > So why would `RuntimeInvisibleAnnotations` be skipped in 3rd case but not > > in 2nd case? > > Well in the second case there is no way to know it is looking only at > invisible annotations, so it has to read them and then discard them as > they were in fact CLASS retention. The skipping could be seen as an > optimization. Not all annotations in the 2nd case need to be CLASS retention. They were CLASS retention when the class that uses them was compiled, but at runtime, the retention may be different (separate compilation). So they are actually returned by the reflection in that case. > > The code is confusing because it gives no indication that it is aware > that runtime invisible annotations could be present: > > /** > * Parses the annotations described by the specified byte array. > * resolving constant references in the specified constant pool. > * The array must contain an array of annotations as described > * in the RuntimeVisibleAnnotations_attribute: > > but at the same time it checks for the RUNTIME retention, which means it > must have recognised the possibility there could be something else > there. Yes, there could be a CLASS retention annotation there (even though `-XX+PreserveAllAnnotations` was not used at runtime, so rawAnnotations contains the content of `RuntimeVisibleAnnotations` only). Again, such annotation was RUNTIME retention when its use was compiled into a class, but at runtime such annotation may be updated to have CLASS or even SOURCE retention. Such annotation use is filtered out. > That check is day 2 code though, on day 1 there was this comment: > > /* XXX Uncomment when this meta-attribute on meta-attributes (Neal's > putback) > if (type.retention() == VisibilityLevel.RUNTIME) > XXX */ I guess this was just code in creation before release. At some point, the `RetentionPolicy` enum was added, but above comment refers to it by the name `VisibilityLevel` > > But the end result is that the code in AnnotationParser correctly > filters out all non-RUNTIME annotations, either directly, or by skipping > them in the stream in the first place. So in that sense there is no bug, > but the code could do with some additional comments. The problem is that it sometimes skips RUNTIME annotations too. I consider this a bug. > > Cheers, > David Regard, Peter - PR: https://git.openjdk.java.net/jdk/pull/4280
Re: RFR: JDK-8267936: PreserveAllAnnotations option isn't tested
On Thu, 3 Jun 2021 03:23:13 GMT, Brian Goetz wrote: > reasonable options are to either leave it alone, deprecate it, or engage > in a much more deliberate redesign.? My favorite option is to leave it alone and test it a bit with the test already provided in this PR. That however requires one approval, me to type `/integrate` and someone to type `/sponsor` - if my understanding of the skara bot is correct. - PR: https://git.openjdk.java.net/jdk/pull/4280
Re: RFR: JDK-8267936: PreserveAllAnnotations option isn't tested
It seems pretty clear that this "feature" is a leftover from an early implementation, doesn't clearly say what it is supposed to do, is more complicated than it looks, and is buggily implemented. While I understand the temptation to "fix" it, at this point we'd realistically be adding a basically entirely new reflection feature that hasn't gone through any sort of design analysis -- we'd just be guessing about what the original intent of this vestigial feature is. It seems the reasonable options are to either leave it alone, deprecate it, or engage in a much more deliberate redesign. (And given the fact that I can think of at least 1,000 things that are higher priority, I'd not be inclined to pursue the third option.) On 6/2/2021 11:06 PM, Jaroslav Tulach wrote: On Tue, 1 Jun 2021 09:30:40 GMT, Jaroslav Tulach wrote: There doesn't seem to be much support for the complete changes in #4245. To get at least something useful from that endeavor I have extracted the test for existing behavior of `-XX:+PreserveAllAnnotations` and I am offering it in this pull request without any changes to the JVM behavior. FYI: The current test is a "mirror" copy of [AnnotationTypeRuntimeAssumptionTest.java](https://github.com/openjdk/jdk/blob/0f689ea8f29be113cc8a917a86fd9e0f85f921fc/test/jdk/java/lang/annotation/AnnotationType/AnnotationTypeRuntimeAssumptionTest.java) with adjustments to cover the `CLASS`->`RUNTIME` migration. What I would do is to add a patch for the parser bug That's an interesting discovery! I see a patch and I can include it as your commit in this PR, if there is a test... and then extend the test in 3 dimensions: ...or just take my PR and expand it. Also the checkbox _"Allow edits and access to secrets by maintainers"_ is on - e.g. this PR is open for modifications. - PR: https://git.openjdk.java.net/jdk/pull/4280
Re: RFR: JDK-8267936: PreserveAllAnnotations option isn't tested
On Tue, 1 Jun 2021 09:30:40 GMT, Jaroslav Tulach wrote: > There doesn't seem to be much support for the complete changes in #4245. To > get at least something useful from that endeavor I have extracted the test > for existing behavior of `-XX:+PreserveAllAnnotations` and I am offering it > in this pull request without any changes to the JVM behavior. FYI: The current test is a "mirror" copy of [AnnotationTypeRuntimeAssumptionTest.java](https://github.com/openjdk/jdk/blob/0f689ea8f29be113cc8a917a86fd9e0f85f921fc/test/jdk/java/lang/annotation/AnnotationType/AnnotationTypeRuntimeAssumptionTest.java) with adjustments to cover the `CLASS`->`RUNTIME` migration. > What I would do is to add a patch for the parser bug That's an interesting discovery! I see a patch and I can include it as your commit in this PR, if there is a test... > and then extend the test in 3 dimensions: ...or just take my PR and expand it. Also the checkbox _"Allow edits and access to secrets by maintainers"_ is on - e.g. this PR is open for modifications. - PR: https://git.openjdk.java.net/jdk/pull/4280
Re: RFR: JDK-8267936: PreserveAllAnnotations option isn't tested
On 3/06/2021 2:54 am, Joe Darcy wrote: If the reflection runtime doesn't implement the semantics of -XX+PreserveAllAnnotations, I suggest deprecating/obsoleting/etc. that option now. I have to agree with Joe now. I mistakenly thought the raw annotation stream was exposed to some parts of reflection, but now I see that it all goes through AnnotationParser, which strips out all non-Runtime retention annotations. So the flag is useless as the data it causes to be passed to the JDK is thrown away anyway. Cheers, David -Joe On 6/2/2021 7:48 AM, Peter Levart wrote: On Tue, 1 Jun 2021 09:30:40 GMT, Jaroslav Tulach wrote: There doesn't seem to be much support for the complete changes in #4245. To get at least something useful from that endeavor I have extracted the test for existing behavior of `-XX:+PreserveAllAnnotations` and I am offering it in this pull request without any changes to the JVM behavior. What I would do is to add a patch for the parser bug and then extend the test in 3 dimensions: - run it with and without the `-XX+PreserveAllAnnotations` option (adapt expected results accordingly) - test annotation use when besides the annotation that changes retention from CLASS -> RUNTIME there is another RUNTIME annotation present on the annotated element or not (this would fail before the bugfix) - test with different annotated elements (Class, Method, Field, Constructor, Parameter) to exercise different code paths in the VM. - PR: https://git.openjdk.java.net/jdk/pull/4280
Re: RFR: JDK-8267936: PreserveAllAnnotations option isn't tested
On 3/06/2021 1:38 am, Peter Levart wrote: On Wed, 2 Jun 2021 13:24:10 GMT, David Holmes wrote: 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 I think this is not deliberate. Since `rawAnnotations` may end up having any of the following: - `RuntimeVisibleAnnotations` (when there were just `RUNTIME` annotation usages compiled into the class or `-XX+PreserveAllAnnotations` was not used at runtime) - `RuntimeInvisibleAnnotations` (when there were just `CLASS` annotation usages compiled into the class and `-XX+PreserveAllAnnotations` was used at runtime) - `RuntimeVisibleAnnotations + RuntimeInvisibleAnnotations` (when there were `RUNTIME` and `CLASS` annotation usages compiled into the class and `-XX+PreserveAllAnnotations` was used at runtime) So why would `RuntimeInvisibleAnnotations` be skipped in 3rd case but not in 2nd case? Well in the second case there is no way to know it is looking only at invisible annotations, so it has to read them and then discard them as they were in fact CLASS retention. The skipping could be seen as an optimization. The code is confusing because it gives no indication that it is aware that runtime invisible annotations could be present: /** * Parses the annotations described by the specified byte array. * resolving constant references in the specified constant pool. * The array must contain an array of annotations as described * in the RuntimeVisibleAnnotations_attribute: but at the same time it checks for the RUNTIME retention, which means it must have recognised the possibility there could be something else there. That check is day 2 code though, on day 1 there was this comment: /* XXX Uncomment when this meta-attribute on meta-attributes (Neal's putback) if (type.retention() == VisibilityLevel.RUNTIME) XXX */ But the end result is that the code in AnnotationParser correctly filters out all non-RUNTIME annotations, either directly, or by skipping them in the stream in the first place. So in that sense there is no bug, but the code could do with some additional comments. Cheers, David - - PR: https://git.openjdk.java.net/jdk/pull/4280
Re: RFR: JDK-8267936: PreserveAllAnnotations option isn't tested
On 3/06/2021 12:39 am, Peter Levart wrote: On Wed, 2 Jun 2021 05:59:05 GMT, David Holmes wrote: Sorry Jaroslav but I don't really see this test as a basic functional test of the PreserveAllAnnotations flag. There is no need for any dynamic retention mode switch. All you need as I've said previously is a class with all the CLASS retention annotations of interest (8 IIRC) applied and a programs that reads them, and either expects to find them or not, based on the PreserveAllAnnotations flag. `CLASS` retention annotations are never returned by reflection regardless of whether the `PreserveAllAnnotations` option was used Sorry yes - my bad. I missed that getRawAnnotations() output was then fed through the AnnotationsParser. Thanks, David - or not. The only way (apart from hacking into encapsulated state and doing your own parsing) is to compile a class that uses an annotation with `CLASS` retention and then run a test that looks up the annotation on this class after the annotation has been changed to have `RUNTIME` retention, but the class that uses it has not been recompiled. `AltClassLoader` does the trick which replaces the class file of _v1 class with the class file of _v2 class when loading the class which simulates this change and recompilation of the annotation without changing anything else. - PR: https://git.openjdk.java.net/jdk/pull/4280
Re: RFR: JDK-8267936: PreserveAllAnnotations option isn't tested
On Wed, 2 Jun 2021 16:56:07 GMT, Joe Darcy wrote: > If the reflection runtime doesn't implement the semantics of > -XX+PreserveAllAnnotations, I suggest deprecating/obsoleting/etc. that > option now. > > -Joe What is the -XX+PreserveAllAnnotations semantics in terms of reflection? The VM spec allows it, but reflection spec doesn't mention it or anything close to it. It doesn't violate the reflection spec. It actually makes reflection behave more correctly in some cases. Without it, reflection sometimes doesn't return annotations with RUNTIME retention. It does implement a useful semantics in terms of late binding. It's just buggy in corner case, which apparently hasn't bothered anyone, since nobody officially complained. That doesn't mean it hasn't been or is not used in the way where it behaves in a useful way and it doesn't mean it won't be needed in the future. Jaroslav gave a perfect example of the case where it is needed now. Removing this option means that migrating an annotation from CLASS retention to RUNTIME becomes almost impossible thing in the real world where no single person controls the whole codebase. Peter - PR: https://git.openjdk.java.net/jdk/pull/4280
Re: RFR: JDK-8267936: PreserveAllAnnotations option isn't tested
If the reflection runtime doesn't implement the semantics of -XX+PreserveAllAnnotations, I suggest deprecating/obsoleting/etc. that option now. -Joe On 6/2/2021 7:48 AM, Peter Levart wrote: On Tue, 1 Jun 2021 09:30:40 GMT, Jaroslav Tulach wrote: There doesn't seem to be much support for the complete changes in #4245. To get at least something useful from that endeavor I have extracted the test for existing behavior of `-XX:+PreserveAllAnnotations` and I am offering it in this pull request without any changes to the JVM behavior. What I would do is to add a patch for the parser bug and then extend the test in 3 dimensions: - run it with and without the `-XX+PreserveAllAnnotations` option (adapt expected results accordingly) - test annotation use when besides the annotation that changes retention from CLASS -> RUNTIME there is another RUNTIME annotation present on the annotated element or not (this would fail before the bugfix) - test with different annotated elements (Class, Method, Field, Constructor, Parameter) to exercise different code paths in the VM. - PR: https://git.openjdk.java.net/jdk/pull/4280
Re: RFR: JDK-8267936: PreserveAllAnnotations option isn't tested
On Wed, 2 Jun 2021 13:24:10 GMT, David Holmes wrote: > 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 I think this is not deliberate. Since `rawAnnotations` may end up having any of the following: - `RuntimeVisibleAnnotations` (when there were just `RUNTIME` annotation usages compiled into the class or `-XX+PreserveAllAnnotations` was not used at runtime) - `RuntimeInvisibleAnnotations` (when there were just `CLASS` annotation usages compiled into the class and `-XX+PreserveAllAnnotations` was used at runtime) - `RuntimeVisibleAnnotations + RuntimeInvisibleAnnotations` (when there were `RUNTIME` and `CLASS` annotation usages compiled into the class and `-XX+PreserveAllAnnotations` was used at runtime) So why would `RuntimeInvisibleAnnotations` be skipped in 3rd case but not in 2nd case? - PR: https://git.openjdk.java.net/jdk/pull/4280
Re: RFR: JDK-8267936: PreserveAllAnnotations option isn't tested
On Tue, 1 Jun 2021 09:30:40 GMT, Jaroslav Tulach wrote: > There doesn't seem to be much support for the complete changes in #4245. To > get at least something useful from that endeavor I have extracted the test > for existing behavior of `-XX:+PreserveAllAnnotations` and I am offering it > in this pull request without any changes to the JVM behavior. What I would do is to add a patch for the parser bug and then extend the test in 3 dimensions: - run it with and without the `-XX+PreserveAllAnnotations` option (adapt expected results accordingly) - test annotation use when besides the annotation that changes retention from CLASS -> RUNTIME there is another RUNTIME annotation present on the annotated element or not (this would fail before the bugfix) - test with different annotated elements (Class, Method, Field, Constructor, Parameter) to exercise different code paths in the VM. - PR: https://git.openjdk.java.net/jdk/pull/4280
Re: RFR: JDK-8267936: PreserveAllAnnotations option isn't tested
On Wed, 2 Jun 2021 05:59:05 GMT, David Holmes wrote: > Sorry Jaroslav but I don't really see this test as a basic functional > test of the PreserveAllAnnotations flag. There is no need for any > dynamic retention mode switch. All you need as I've said previously is a > class with all the CLASS retention annotations of interest (8 IIRC) > applied and a programs that reads them, and either expects to find them > or not, based on the PreserveAllAnnotations flag. `CLASS` retention annotations are never returned by reflection regardless of whether the `PreserveAllAnnotations` option was used or not. The only way (apart from hacking into encapsulated state and doing your own parsing) is to compile a class that uses an annotation with `CLASS` retention and then run a test that looks up the annotation on this class after the annotation has been changed to have `RUNTIME` retention, but the class that uses it has not been recompiled. `AltClassLoader` does the trick which replaces the class file of _v1 class with the class file of _v2 class when loading the class which simulates this change and recompilation of the annotation without changing anything else. - PR: https://git.openjdk.java.net/jdk/pull/4280
Re: RFR: JDK-8267936: PreserveAllAnnotations option isn't tested
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
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
Re: RFR: JDK-8267936: PreserveAllAnnotations option isn't tested
On 2/06/2021 10:17 pm, Peter Levart wrote:
On Wed, 2 Jun 2021 11:40:13 GMT, Peter Levart 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[] ?
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
Re: RFR: JDK-8267936: PreserveAllAnnotations option isn't tested
On Wed, 2 Jun 2021 12:33:37 GMT, Peter Levart wrote: > I suggest the following patch for the bug in AnnotationParser: > An alternative would be to change the `ClassFileParser::assemble_annotations` in the VM to no be so "dumb", but to construct correct concatenation. - PR: https://git.openjdk.java.net/jdk/pull/4280
Re: RFR: JDK-8267936: PreserveAllAnnotations option isn't tested
On Tue, 1 Jun 2021 09:30:40 GMT, Jaroslav Tulach
wrote:
> There doesn't seem to be much support for the complete changes in #4245. To
> get at least something useful from that endeavor I have extracted the test
> for existing behavior of `-XX:+PreserveAllAnnotations` and I am offering it
> in this pull request without any changes to the JVM behavior.
I suggest the following patch for the bug in AnnotationParser:
Index: src/java.base/share/classes/sun/reflect/annotation/AnnotationParser.java
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===
diff --git
a/src/java.base/share/classes/sun/reflect/annotation/AnnotationParser.java
b/src/java.base/share/classes/sun/reflect/annotation/AnnotationParser.java
--- a/src/java.base/share/classes/sun/reflect/annotation/AnnotationParser.java
(revision b4371e9bcaa1c8aa394b5eca409c5afc669cc146)
+++ b/src/java.base/share/classes/sun/reflect/annotation/AnnotationParser.java
(date 1622636986705)
@@ -116,6 +116,7 @@
Map, Annotation> result =
new LinkedHashMap, Annotation>();
ByteBuffer buf = ByteBuffer.wrap(rawAnnotations);
+for (int round = 0; round < 2 && buf.hasRemaining(); round++) {
int numAnnotations = buf.getShort() & 0x;
for (int i = 0; i < numAnnotations; i++) {
Annotation a = parseAnnotation2(buf, constPool, container, false,
selectAnnotationClasses);
@@ -128,6 +129,7 @@
}
}
}
+}
return result;
}
-
PR: https://git.openjdk.java.net/jdk/pull/4280
Re: RFR: JDK-8267936: PreserveAllAnnotations option isn't tested
On Wed, 2 Jun 2021 11:40:13 GMT, Peter Levart 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
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
Re: RFR: JDK-8267936: PreserveAllAnnotations option isn't tested
On Wed, 2 Jun 2021 11:34:18 GMT, Peter Levart wrote:
>> test/jdk/java/lang/annotation/AnnotationType/AnnotationTypeChangedToRuntimeTest.java
>> line 81:
>>
>>> 79: " should not be visible at runtime");
>>> 80: }
>>> 81: }
>>
>> I'm trying to understand why the case of
>> `TestTask.class.getDeclaredAnnotation(AnnA_v1.class)` is different from
>> `AnnB.class.getDeclaredAnnotation(AnnA_v1.class)` ... `TestTask` and `AnnB`
>> are both just types annotated with the same annotation `@AnnA_v1` ... I'll
>> have to debug this to see the point (does it have something to do with the
>> fact that there is circularity of annotation uses: `AnnA_v1` is annotated
>> with `@AnnB` while `AnnB` is annotated with `@AnnA_v1` ?
>
> ...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.
-
PR: https://git.openjdk.java.net/jdk/pull/4280
Re: RFR: JDK-8267936: PreserveAllAnnotations option isn't tested
On Wed, 2 Jun 2021 10:36:06 GMT, Peter Levart wrote: >> There doesn't seem to be much support for the complete changes in #4245. To >> get at least something useful from that endeavor I have extracted the test >> for existing behavior of `-XX:+PreserveAllAnnotations` and I am offering it >> in this pull request without any changes to the JVM behavior. > > test/jdk/java/lang/annotation/AnnotationType/AnnotationTypeChangedToRuntimeTest.java > line 81: > >> 79: " should not be visible at runtime"); >> 80: } >> 81: } > > I'm trying to understand why the case of > `TestTask.class.getDeclaredAnnotation(AnnA_v1.class)` is different from > `AnnB.class.getDeclaredAnnotation(AnnA_v1.class)` ... `TestTask` and `AnnB` > are both just types annotated with the same annotation `@AnnA_v1` ... I'll > have to debug this to see the point (does it have something to do with the > fact that there is circularity of annotation uses: `AnnA_v1` is annotated > with `@AnnB` while `AnnB` is annotated with `@AnnA_v1` ? ...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... - PR: https://git.openjdk.java.net/jdk/pull/4280
Re: RFR: JDK-8267936: PreserveAllAnnotations option isn't tested
On Tue, 1 Jun 2021 09:30:40 GMT, Jaroslav Tulach
wrote:
> There doesn't seem to be much support for the complete changes in #4245. To
> get at least something useful from that endeavor I have extracted the test
> for existing behavior of `-XX:+PreserveAllAnnotations` and I am offering it
> in this pull request without any changes to the JVM behavior.
test/jdk/java/lang/annotation/AnnotationType/AnnotationTypeChangedToRuntimeTest.java
line 42:
> 40: * {@link RetentionPolicy#CLASS CLASS} retention was changed into
> 41: * {@link RetentionPolicy#RUNTIME RUNTIME} and we want to read
> 42: * its value during runtime with the help of
I would insert the phrase ", but the classes that use the annotation have not
been recompiled" after the `RUNTIME`.
-
PR: https://git.openjdk.java.net/jdk/pull/4280
Re: RFR: JDK-8267936: PreserveAllAnnotations option isn't tested
On Tue, 1 Jun 2021 09:30:40 GMT, Jaroslav Tulach
wrote:
> There doesn't seem to be much support for the complete changes in #4245. To
> get at least something useful from that endeavor I have extracted the test
> for existing behavior of `-XX:+PreserveAllAnnotations` and I am offering it
> in this pull request without any changes to the JVM behavior.
test/jdk/java/lang/annotation/AnnotationType/AnnotationTypeChangedToRuntimeTest.java
line 81:
> 79: " should not be visible at runtime");
> 80: }
> 81: }
I'm trying to understand why the case of
`TestTask.class.getDeclaredAnnotation(AnnA_v1.class)` is different from
`AnnB.class.getDeclaredAnnotation(AnnA_v1.class)` ... `TestTask` and `AnnB` are
both just types annotated with the same annotation `@AnnA_v1` ... I'll have to
debug this to see the point (does it have something to do with the fact that
there is circularity of annotation uses: `AnnA_v1` is annotated with `@AnnB`
while `AnnB` is annotated with `@AnnA_v1` ?
test/jdk/java/lang/annotation/AnnotationType/AnnotationTypeChangedToRuntimeTest.java
line 102:
> 100: */
> 101: static class AltClassLoader extends ClassLoader {
> 102: AltClassLoader(ClassLoader parent) {
I can't escape the feeling that I already saw this code somewhere. Hm... ;-)
-
PR: https://git.openjdk.java.net/jdk/pull/4280
Re: RFR: JDK-8267936: PreserveAllAnnotations option isn't tested
On Tue, 1 Jun 2021 09:30:40 GMT, Jaroslav Tulach
wrote:
> There doesn't seem to be much support for the complete changes in #4245. To
> get at least something useful from that endeavor I have extracted the test
> for existing behavior of `-XX:+PreserveAllAnnotations` and I am offering it
> in this pull request without any changes to the JVM behavior.
test/jdk/java/lang/annotation/AnnotationType/AnnotationTypeChangedToRuntimeTest.java
line 57:
> 55: @AnnB
> 56: public @interface AnnA_v2 {
> 57: }
You meant to write: "An alternative version of AnnA_v1 with RUNTIME retention
instead", right?
-
PR: https://git.openjdk.java.net/jdk/pull/4280
Re: RFR: JDK-8267936: PreserveAllAnnotations option isn't tested
On 01/06/2021 21:28, John Rose wrote: : Note to self and other reviewers of future versions of the JVMS: When you see proposed language like the “unless…” of JVMS 4.7.17, remember today's conversation and try to avoid putting dark corners like that into the JVMS. The RuntimeInvisibleAnnotations attribute is similar to the RuntimeVisibleAnnotations attribute (§4.7.16), except that the annotations represented by a RuntimeInvisibleAnnotations attribute must not be made available for return by reflective APIs, [[WE SHOULD HAVE STOPPED HERE]] unless the Java Virtual Machine has been instructed to retain these annotations via some implementation-specific mechanism such as a command line flag. In the absence of such instructions, the Java Virtual Machine ignores this attribute. https://docs.oracle.com/javase/specs/jvms/se16/html/jvms-4.html#jvms-4.7.17 Maybe the "unless ..." could be dropped from the JVMS in some future release. The XX option could be deprecated now so that it won't be a surprise when it becomes obsolete. On the other hand, maybe it's not worth doing anything as there are more important things to work on. -Alan
Re: RFR: JDK-8267936: PreserveAllAnnotations option isn't tested
On 2/06/2021 3:47 pm, Jaroslav Tulach wrote: On Tue, 1 Jun 2021 09:30:40 GMT, Jaroslav Tulach wrote: There doesn't seem to be much support for the complete changes in #4245. To get at least something useful from that endeavor I have extracted the test for existing behavior of `-XX:+PreserveAllAnnotations` and I am offering it in this pull request without any changes to the JVM behavior. Right Peter, the `AnnotationTypeChangedToRuntimeTest` mimics closely the use-case: JVM as a late-binding runtime ... There are exceptions to the rule such as compile-time constants, ... and also annotation uses where the information from one source file (annotation retention) is baked into compilation artifacts of other source files (`RuntimeVisibleAnnotations` vs. `RuntimeInvisibleAnnotations`). `PreserveAllAnnotations` option helps to overcome the situation Great formulation of the problem. The late binding allows people to ignore time of compilation when thinking about the running system. Ignoring time makes the mental model of the overall system easier. But when certain information is _baked_ elsewhere, ignoring time is may no longer be possible as the sequence of actions becomes important - an up to date system may see relics of the past (old values of compile-time constants and annotation not being visible even their most recent retention is `RUNTIME`). This PR isn't going to modify behavior of the `-XX:+PreserveAllAnnotations` option in any way. It only provides a test. Having a test is better than having none. Can we consider this PR reviewed? Can somebody with enough merit mark this _change as properly reviewed_? Can somebody restart the _Windows aarch64 - Build_ - the error seems unrelated? Sorry Jaroslav but I don't really see this test as a basic functional test of the PreserveAllAnnotations flag. There is no need for any dynamic retention mode switch. All you need as I've said previously is a class with all the CLASS retention annotations of interest (8 IIRC) applied and a programs that reads them, and either expects to find them or not, based on the PreserveAllAnnotations flag. I get that you are just trying to not waste a test you already developed. The Windows build issue seems to be a software configuration issue on the Github machines and is outside of our control. I've no idea how to report issues to the Github folk. David - PR: https://git.openjdk.java.net/jdk/pull/4280
Re: RFR: JDK-8267936: PreserveAllAnnotations option isn't tested
On Tue, 1 Jun 2021 09:30:40 GMT, Jaroslav Tulach wrote: > There doesn't seem to be much support for the complete changes in #4245. To > get at least something useful from that endeavor I have extracted the test > for existing behavior of `-XX:+PreserveAllAnnotations` and I am offering it > in this pull request without any changes to the JVM behavior. Right Peter, the `AnnotationTypeChangedToRuntimeTest` mimics closely the use-case: > JVM as a late-binding runtime ... There are exceptions to the rule such as compile-time constants, ... and also annotation uses where the information from one source file (annotation retention) is baked into compilation artifacts of other source files (`RuntimeVisibleAnnotations` vs. `RuntimeInvisibleAnnotations`). `PreserveAllAnnotations` option helps to overcome the situation Great formulation of the problem. The late binding allows people to ignore time of compilation when thinking about the running system. Ignoring time makes the mental model of the overall system easier. But when certain information is _baked_ elsewhere, ignoring time is may no longer be possible as the sequence of actions becomes important - an up to date system may see relics of the past (old values of compile-time constants and annotation not being visible even their most recent retention is `RUNTIME`). This PR isn't going to modify behavior of the `-XX:+PreserveAllAnnotations` option in any way. It only provides a test. Having a test is better than having none. Can we consider this PR reviewed? Can somebody with enough merit mark this _change as properly reviewed_? Can somebody restart the _Windows aarch64 - Build_ - the error seems unrelated? - PR: https://git.openjdk.java.net/jdk/pull/4280
Re: RFR: JDK-8267936: PreserveAllAnnotations option isn't tested
On 01/06/2021 22:28, John Rose wrote: On Jun 1, 2021, at 7:02 AM, Brian Goetz mailto:brian.go...@oracle.com>> wrote: As Alan's archaeology discovered, this flag appears to be a leftover from the original implementation, and I could find no signs of its usage. Note to self and other reviewers of future versions of the JVMS: When you see proposed language like the “unless…” of JVMS 4.7.17, remember today's conversation and try to avoid putting dark corners like that into the JVMS. The RuntimeInvisibleAnnotations attribute is similar to the RuntimeVisibleAnnotations attribute (§4.7.16), except that the annotations represented by a RuntimeInvisibleAnnotations attribute must not be made available for return by reflective APIs, [[WE SHOULD HAVE STOPPED HERE]] unless the Java Virtual Machine has been instructed to retain these annotations via some implementation-specific mechanism such as a command line flag. In the absence of such instructions, the Java Virtual Machine ignores this attribute. https://docs.oracle.com/javase/specs/jvms/se16/html/jvms-4.html#jvms-4.7.17 Hi, I do think this option, in current form, is useful as an escape hatch for exactly the case that Jaroslav has. Java/JVM as a late-binding runtime does a very good job of keeping the information from source files local in direct compilation artefacts which facilitates separate compilation where changes to and recompilation of one source file have immediate effect on the whole app. There are exceptions to the rule such as compile-time constants, ... and also annotation uses where the information from one source file (annotation retention) is baked into compilation artefacts of other source files (RuntimeVisibleAnnotations vs. RuntimeInvisibleAnnotations). PreserveAllAnnotations option helps to overcome the situation where the annotation has been updated but classes where such annotation is used have not been recompiled (yet). I see the two class file attributes merely as a runtime optimization while both kinds of annotations could be kept in a single attribute. The JVM option just disables this optimization. So I would still keep the option. Regards, Peter
Re: RFR: JDK-8267936: PreserveAllAnnotations option isn't tested
On Jun 1, 2021, at 7:02 AM, Brian Goetz mailto:brian.go...@oracle.com>> wrote: As Alan's archaeology discovered, this flag appears to be a leftover from the original implementation, and I could find no signs of its usage. Note to self and other reviewers of future versions of the JVMS: When you see proposed language like the “unless…” of JVMS 4.7.17, remember today's conversation and try to avoid putting dark corners like that into the JVMS. The RuntimeInvisibleAnnotations attribute is similar to the RuntimeVisibleAnnotations attribute (§4.7.16), except that the annotations represented by a RuntimeInvisibleAnnotations attribute must not be made available for return by reflective APIs, [[WE SHOULD HAVE STOPPED HERE]] unless the Java Virtual Machine has been instructed to retain these annotations via some implementation-specific mechanism such as a command line flag. In the absence of such instructions, the Java Virtual Machine ignores this attribute. https://docs.oracle.com/javase/specs/jvms/se16/html/jvms-4.html#jvms-4.7.17
Re: RFR: JDK-8267936: PreserveAllAnnotations option isn't tested
Since the discussion happened over the holiday weekend, I didn't get a chance to respond until now, but I think that this came to a good outcome. As Alan's archaeology discovered, this flag appears to be a leftover from the original implementation, and I could find no signs of its usage. We might consider deprecating it (though, we might also leave sleeping dogs alone), but its good to have a test for the current behavior. Allow me to make a few meta-comments about how we got here, though, before we wrap up. they are not visible via java.lang.reflect API. I assume that's just an omission. While omissions do sometimes happen, it is usually not the best first theory in a situation like this. More often than not, there is a good, but often non-obvious, explanation. More importantly, though, the PR-first approach is not how we like to approach such a change, especially when it involves the behaviors of such fundamental APIs such as reflection, because it jumps over the most important steps: - What problem are we trying to solve? - Is it really a problem that needs to be solved? - Is it really a problem that needs to be solved in the JDK? - What solutions are there, besides the obvious one? - What are the pros, cons, and tradeoffs of the various solutions? - Of the proposed solution, what future downsides and risks could we imagine? Going straight to the code of a specific solution is likely to be unsatisfying in both the short term (because its the wrong conversation) or the long term (because it might not be the right solution to the right problem.) Instead, starting with a discussion of the problem, and of potential solutions and tradeoffs, is likely to yield a better result on both counts. (As an example of the last one, suppose we committed this patch. One could easily imagine some library later saying "must be run with -Xx:PreserveAllAnnotations" because that's what the author had to do to make it work. But a behavior change like non-runtime annotations showing up unexpectedly in reflection could confuse existing code, which might not work as expected with the new behavior. Which might then call for "can we please make the PreserveAllAnnotations flag more selective (e.g., per-class/module/package)" or calls for new flags or ... yuck. We try to avoid today's solutions becoming tomorrow's problems. This is not an exact science, of course, but this is the sort of stewardship we strive for.) On 6/1/2021 5:39 AM, Jaroslav Tulach wrote: There doesn't seem to be much support for the complete changes in #4245. To get at least something useful from that endeavor I have extracted the test for existing behavior of `-XX:+PreserveAllAnnotations` and I am offering it in this pull request without any changes to the JVM behavior. - Commit messages: - Test long time existing behavior of -XX:+PreserveAllAnnotations Changes: https://git.openjdk.java.net/jdk/pull/4280/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=4280=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8267936 Stats: 172 lines in 1 file changed: 172 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/4280.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4280/head:pull/4280 PR: https://git.openjdk.java.net/jdk/pull/4280
