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

2021-06-29 Thread Peter Levart
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 >

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

2021-06-29 Thread Peter Levart
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 >

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

2021-06-29 Thread Jaroslav Tulach
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 >

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

2021-06-03 Thread David Holmes
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

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

2021-06-03 Thread Peter Levart
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

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

2021-06-03 Thread Peter Levart
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 >

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

2021-06-03 Thread David Holmes
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

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

2021-06-03 Thread Peter Levart
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

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

2021-06-03 Thread Peter Levart
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

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

2021-06-02 Thread Jaroslav Tulach
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

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

2021-06-02 Thread Brian Goetz
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

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

2021-06-02 Thread Jaroslav Tulach
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 >

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

2021-06-02 Thread David Holmes
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

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

2021-06-02 Thread David Holmes
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

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

2021-06-02 Thread David Holmes
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

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

2021-06-02 Thread Peter Levart
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

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

2021-06-02 Thread Joe Darcy
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

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

2021-06-02 Thread Peter Levart
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

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

2021-06-02 Thread Peter Levart
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 >

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

2021-06-02 Thread Peter Levart
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

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

2021-06-02 Thread 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 wrote: ...hm, it seems that mere presence of any RUNTIME annotation that was RUNTIME already at the use compile time somehow affects

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

2021-06-02 Thread David Holmes
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

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

2021-06-02 Thread Peter Levart
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:

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

2021-06-02 Thread Peter Levart
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 >

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

2021-06-02 Thread Peter Levart
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

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

2021-06-02 Thread Peter Levart
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

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

2021-06-02 Thread Peter Levart
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 >>

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

2021-06-02 Thread Peter Levart
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 >

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

2021-06-02 Thread Peter Levart
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 >

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

2021-06-02 Thread Peter Levart
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 >

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

2021-06-02 Thread Alan Bateman
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

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

2021-06-01 Thread David Holmes
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

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

2021-06-01 Thread Jaroslav Tulach
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 >

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

2021-06-01 Thread Peter Levart
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

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

2021-06-01 Thread John Rose
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

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

2021-06-01 Thread Brian Goetz
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