Re: RFR: 8257596: Clarify trusted final fields for record classes
On Thu, 10 Dec 2020 22:59:16 GMT, Mandy Chung wrote: >> Hi Remi, >> >>> For me, it's backout JDK-8247444, as i said on the amber-spec-expert, i >>> prefer VM to be oblivious about java.lang.Record. >>> And in the future, the real fix is to change the spec of Field.set() to say >>> that it's only allowed for plain java classes and have the implementation >>> to directly asks the VM is a non static field is trusted or not. >> >> This reply was before you realized that records are are permanent Java SE 16 >> feature :-) If the future ended up requiring/desiring to allow final fields >> of a record class be modifiable reflectively (I double that!), `Field::set` >> spec can be updated to remove that restriction with no compatibility risk >> >>> And there are also a related issue with the validation of the >>> InnerClass/Enclosing attribute were the VM does a handshake between the >>> inner class attribute and the enclosing class attribute when calling >>> Class.getSimpleName(), again using the JLS definition of what an inner >>> class is instead of using the VM definition, the content of the InnerClass >>> attribute with no validation. >> >> It's the implementation details of the core reflection how to determine if a >> class is a member of another class. The `getSimpleName` spec and other >> related APIs (see JDK-8250226) need to define the behavior when there is an >> issue in determining the declaring class. This indeed needs some >> investigation what the best way to address this. > > I have created a new branch off jdk16: > https://github.com/mlchung/jdk16/tree/8257596. It fixed the attribute name > as Harold pointed out. > > @hseigel tier1-tier3 and jck-runtime:vm and jck-runtime:lang tests all passed. Target this fix for jdk16 (https://github.com/openjdk/jdk16/pull/14). - PR: https://git.openjdk.java.net/jdk/pull/1706
Re: RFR: 8257596: Clarify trusted final fields for record classes
On Thu, 10 Dec 2020 22:56:34 GMT, Mandy Chung wrote: >> Marked as reviewed by chegar (Reviewer). > > Hi Remi, > >> For me, it's backout JDK-8247444, as i said on the amber-spec-expert, i >> prefer VM to be oblivious about java.lang.Record. >> And in the future, the real fix is to change the spec of Field.set() to say >> that it's only allowed for plain java classes and have the implementation to >> directly asks the VM is a non static field is trusted or not. > > This reply was before you realized that records are are permanent Java SE 16 > feature :-) If the future ended up requiring/desiring to allow final fields > of a record class be modifiable reflectively (I double that!), `Field::set` > spec can be updated to remove that restriction with no compatibility risk > >> And there are also a related issue with the validation of the >> InnerClass/Enclosing attribute were the VM does a handshake between the >> inner class attribute and the enclosing class attribute when calling >> Class.getSimpleName(), again using the JLS definition of what an inner class >> is instead of using the VM definition, the content of the InnerClass >> attribute with no validation. > > It's the implementation details of the core reflection how to determine if a > class is a member of another class. The `getSimpleName` spec and other > related APIs (see JDK-8250226) need to define the behavior when there is an > issue in determining the declaring class. I have created a new branch off jdk16: https://github.com/mlchung/jdk16/tree/8257596. It fixed the attribute name as Harold pointed out. @hseigel tier1-tier3 and jck-runtime:vm and jck-runtime:lang tests all passed. - PR: https://git.openjdk.java.net/jdk/pull/1706
Re: RFR: 8257596: Clarify trusted final fields for record classes
On Thu, 10 Dec 2020 14:13:27 GMT, Chris Hegarty wrote: >> This is a follow-up on JDK-8255342 that removes non-specified JVM checks on >> classes with `RecordComponents` attributes. That introduces a regression in >> `InstanceKlass::is_record` that returns true on a non-record class which has >> `RecordComponents` attribute present. This causes unexpected semantics in >> `JVM_IsRecord` and `JVM_GetRecordComponents` and also a regression to trust >> final fields for non-record classes. >> >> I propose to change `InstanceKlass::is_record` to match the JLS semantic of >> a record class, i.e. final direct subclass of `java.lang.Record` with the >> presence of `RecordComponents` attribute. There is no change to JVM class >> file validation. Also I propose clearly define: >> - `JVM_IsRecord` returns true if the given class is a record i.e. final >> and direct subclass of `java.lang.Record` with `RecordComponents` attribute >> - `JVM_GetRecordComponents` returns an `RecordComponents` array if >> `RecordComponents` attribute is present; otherwise, returns NULL. This does >> not check if it's a record class or not. So it may return non-null on a >> non-record class if it has `RecordComponents` attribute. So >> `JVM_GetRecordComponents` returns the content of the classfile. > > Marked as reviewed by chegar (Reviewer). Hi Remi, > For me, it's backout JDK-8247444, as i said on the amber-spec-expert, i > prefer VM to be oblivious about java.lang.Record. > And in the future, the real fix is to change the spec of Field.set() to say > that it's only allowed for plain java classes and have the implementation to > directly asks the VM is a non static field is trusted or not. This reply was before you realized that records are are permanent Java SE 16 feature :-) If the future ended up requiring/desiring to allow final fields of a record class be modifiable reflectively (I double that!), `Field::set` spec can be updated to remove that restriction with no compatibility risk > And there are also a related issue with the validation of the > InnerClass/Enclosing attribute were the VM does a handshake between the inner > class attribute and the enclosing class attribute when calling > Class.getSimpleName(), again using the JLS definition of what an inner class > is instead of using the VM definition, the content of the InnerClass > attribute with no validation. It's the implementation details of the core reflection how to determine if a class is a member of another class. The `getSimpleName` spec and other related APIs (see JDK-8250226) need to define the behavior when there is an issue in determining the declaring class. - PR: https://git.openjdk.java.net/jdk/pull/1706
Re: RFR: 8257596: Clarify trusted final fields for record classes
- Mail original - > De: "Mandy Chung" > À: "core-libs-dev" , "hotspot-runtime-dev" > > Envoyé: Mercredi 9 Décembre 2020 01:43:34 > Objet: Re: RFR: 8257596: Clarify trusted final fields for record classes > On Tue, 8 Dec 2020 22:52:37 GMT, Mandy Chung wrote: > >> This is a follow-up on JDK-8255342 that removes non-specified JVM checks on >> classes with Record attributes. That introduces a regression in >> `InstanceKlass::is_record` that returns true on a non-record class which has >> `RecordComponents` attribute present. This causes unexpected semantics in >> `JVM_IsRecord` and `JVM_GetRecordComponents` and also a regression to trust >> final fields for non-record classes. >> >> I propose to change `InstanceKlass::is_record` to match the JLS semantic of a >> record class, i.e. final direct subclass of `java.lang.Record` with the >> presence of `RecordComponents` attribute. There is no change to JVM class >> file >> validation. Also I propose clearly define: >> - `JVM_IsRecord` returns true if the given class is a record i.e. final >> and >> direct subclass of `java.lang.Record` with `RecordComponents` attribute >> - `JVM_GetRecordComponents` returns an `RecordComponents` array if >> `RecordComponents` attribute is present; otherwise, returns NULL. This >> does >> not check if it's a record class or not. So it may return non-null on a >> non-record class if it has `RecordComponents` attribute. So >> `JVM_GetRecordComponents` returns the content of the classfile. > > Hi Remi, > >> It's not an issue, the JVM view of what a record is and the JLS view of what >> a >> record is doesn't have to be identical, >> only aligned. It's fine for the VM to consider any class that have a record >> Attribute is a record. >> >> We already had this discussion on amber-spec-expert list, >> see >> https://mail.openjdk.java.net/pipermail/amber-spec-experts/2020-November/002630.html > > What is the conclusion (sorry it was unclear to me)? Drop TNSFF for records? > > This issue is to fix the regression introduced by JDK-8255342. I expect > someone else will file a new JBS issue and implement what amber-spec-experts > decided. > >> We already know that the JLS definition of a record will have to be changed >> for >> inline record (an inline record is not direct subclass of j.l.Record because >> you have the reference projection in the middle). > > Yes I saw that. I updated > [JDK-8251041](https://bugs.openjdk.java.net/browse/JDK-8251041) to follow up. > >> The real issue is that the JIT optimisation and Field.set() should be >> aligned, >> VarHandle deosn't let you change a final field and Unsafe is unsafe, so the >> current problem is that Field.set() relies on the reflection api by calling >> Class.isRecord() which is not good because Classs.isRecord() returns the JLS >> view of the world not the JVM view of the world. >> >> As said in the mail chain above, for the JIT optimization, instead of listing >> all the new constructs, record, inline, etc, i propose to check the other >> way, >> only allow to set a final field is only allowed for plain old java class, so >> the VM should not have a method InstanceKlass::is_record but a method that >> return if a class is a plain class or not and this method should be called by >> the JIT and by Field.set (through a JVM entry point) >> so the fact the optimization will be done or not is not related to what the >> JLS >> think a record is, those are separated concern. > > I agree the current situation is not ideal which requires to declare all the > new > constructs explicitly for TNSFF. However, it's a reasonable tradeoff to get > the JIT optimization for records in time while waiting for true TNSFF > investigation like JMM and other relevant specs. I see this just a stop-gap > solution. When the long-term TNSFF is in place, the spec can be updated to > drop the explicit list of record, inline, etc. > > A related issue is > [JDK-8233873](https://bugs.openjdk.java.net/browse/JDK-8233873). I'm happy > if > we can do TNSFF in a different solution. > > Again this PR intends to fix the regression. Two options: > 1. Keep [JDK-8247444](https://bugs.openjdk.java.net/browse/JDK-8247444) and > implement as this proposed patch > 2. Backout [JDK-8247444](https://bugs.openjdk.java.net/browse/JDK-8247444) > > I expect Chris and/or others will follow up the decision made by the > amber-spec-experts w.r.t. trusting finals in records. I'm okay with either
Re: RFR: 8257596: Clarify trusted final fields for record classes
On Tue, 8 Dec 2020 22:52:37 GMT, Mandy Chung wrote: > This is a follow-up on JDK-8255342 that removes non-specified JVM checks on > classes with `RecordComponents` attributes. That introduces a regression in > `InstanceKlass::is_record` that returns true on a non-record class which has > `RecordComponents` attribute present. This causes unexpected semantics in > `JVM_IsRecord` and `JVM_GetRecordComponents` and also a regression to trust > final fields for non-record classes. > > I propose to change `InstanceKlass::is_record` to match the JLS semantic of a > record class, i.e. final direct subclass of `java.lang.Record` with the > presence of `RecordComponents` attribute. There is no change to JVM class > file validation. Also I propose clearly define: > - `JVM_IsRecord` returns true if the given class is a record i.e. final > and direct subclass of `java.lang.Record` with `RecordComponents` attribute > - `JVM_GetRecordComponents` returns an `RecordComponents` array if > `RecordComponents` attribute is present; otherwise, returns NULL. This does > not check if it's a record class or not. So it may return non-null on a > non-record class if it has `RecordComponents` attribute. So > `JVM_GetRecordComponents` returns the content of the classfile. Marked as reviewed by chegar (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/1706
Re: RFR: 8257596: Clarify trusted final fields for record classes
On Tue, 8 Dec 2020 22:52:37 GMT, Mandy Chung wrote: > This is a follow-up on JDK-8255342 that removes non-specified JVM checks on > classes with `RecordComponents` attributes. That introduces a regression in > `InstanceKlass::is_record` that returns true on a non-record class which has > `RecordComponents` attribute present. This causes unexpected semantics in > `JVM_IsRecord` and `JVM_GetRecordComponents` and also a regression to trust > final fields for non-record classes. > > I propose to change `InstanceKlass::is_record` to match the JLS semantic of a > record class, i.e. final direct subclass of `java.lang.Record` with the > presence of `RecordComponents` attribute. There is no change to JVM class > file validation. Also I propose clearly define: > - `JVM_IsRecord` returns true if the given class is a record i.e. final > and direct subclass of `java.lang.Record` with `RecordComponents` attribute > - `JVM_GetRecordComponents` returns an `RecordComponents` array if > `RecordComponents` attribute is present; otherwise, returns NULL. This does > not check if it's a record class or not. So it may return non-null on a > non-record class if it has `RecordComponents` attribute. So > `JVM_GetRecordComponents` returns the content of the classfile. Hi Mandy, Could you replace the comment starting at line 1854 in jvm.cpp with: // A class is a record if and only if it is final and a direct subclass // of java.lang.Record and the presence of `Record` attribute; // otherwise, it is not a record. Also, replace the comment starting at line 1872 in jvm.cpp with: // Returns an array containing the components of the Record attribute, // or NULL if the attribute is not present. // // Note that this function returns the components of the Record // attribute even if the class is not a Record. Also, please change the name of the attribute in the comments added to Class.java from RecordComponent to Record. Thanks, Harold - PR: https://git.openjdk.java.net/jdk/pull/1706
Re: RFR: 8257596: Clarify trusted final fields for record classes
On Tue, 8 Dec 2020 22:52:37 GMT, Mandy Chung wrote: > This is a follow-up on JDK-8255342 that removes non-specified JVM checks on > classes with `RecordComponents` attributes. That introduces a regression in > `InstanceKlass::is_record` that returns true on a non-record class which has > `RecordComponents` attribute present. This causes unexpected semantics in > `JVM_IsRecord` and `JVM_GetRecordComponents` and also a regression to trust > final fields for non-record classes. > > I propose to change `InstanceKlass::is_record` to match the JLS semantic of a > record class, i.e. final direct subclass of `java.lang.Record` with the > presence of `RecordComponents` attribute. There is no change to JVM class > file validation. Also I propose clearly define: > - `JVM_IsRecord` returns true if the given class is a record i.e. final > and direct subclass of `java.lang.Record` with `RecordComponents` attribute > - `JVM_GetRecordComponents` returns an `RecordComponents` array if > `RecordComponents` attribute is present; otherwise, returns NULL. This does > not check if it's a record class or not. So it may return non-null on a > non-record class if it has `RecordComponents` attribute. So > `JVM_GetRecordComponents` returns the content of the classfile. Hi Mandy, Could you regression test this change against the JCK lang and vm test suites and Mach5 tiers 1 and 2? Thanks, Harold - PR: https://git.openjdk.java.net/jdk/pull/1706
Re: RFR: 8257596: Clarify trusted final fields for record classes
On Tue, 8 Dec 2020 22:52:37 GMT, Mandy Chung wrote: > This is a follow-up on JDK-8255342 that removes non-specified JVM checks on > classes with Record attributes. That introduces a regression in > `InstanceKlass::is_record` that returns true on a non-record class which has > `RecordComponents` attribute present. This causes unexpected semantics in > `JVM_IsRecord` and `JVM_GetRecordComponents` and also a regression to trust > final fields for non-record classes. > > I propose to change `InstanceKlass::is_record` to match the JLS semantic of a > record class, i.e. final direct subclass of `java.lang.Record` with the > presence of `RecordComponents` attribute. There is no change to JVM class > file validation. Also I propose clearly define: > - `JVM_IsRecord` returns true if the given class is a record i.e. final > and direct subclass of `java.lang.Record` with `RecordComponents` attribute > - `JVM_GetRecordComponents` returns an `RecordComponents` array if > `RecordComponents` attribute is present; otherwise, returns NULL. This does > not check if it's a record class or not. So it may return non-null on a > non-record class if it has `RecordComponents` attribute. So > `JVM_GetRecordComponents` returns the content of the classfile. Hi Remi, > It's not an issue, the JVM view of what a record is and the JLS view of what > a record is doesn't have to be identical, > only aligned. It's fine for the VM to consider any class that have a record > Attribute is a record. > > We already had this discussion on amber-spec-expert list, > see > https://mail.openjdk.java.net/pipermail/amber-spec-experts/2020-November/002630.html What is the conclusion (sorry it was unclear to me)? Drop TNSFF for records? This issue is to fix the regression introduced by JDK-8255342. I expect someone else will file a new JBS issue and implement what amber-spec-experts decided. > We already know that the JLS definition of a record will have to be changed > for inline record (an inline record is not direct subclass of j.l.Record > because you have the reference projection in the middle). Yes I saw that. I updated [JDK-8251041](https://bugs.openjdk.java.net/browse/JDK-8251041) to follow up. > The real issue is that the JIT optimisation and Field.set() should be > aligned, VarHandle deosn't let you change a final field and Unsafe is unsafe, > so the current problem is that Field.set() relies on the reflection api by > calling Class.isRecord() which is not good because Classs.isRecord() returns > the JLS view of the world not the JVM view of the world. > > As said in the mail chain above, for the JIT optimization, instead of listing > all the new constructs, record, inline, etc, i propose to check the other > way, only allow to set a final field is only allowed for plain old java > class, so the VM should not have a method InstanceKlass::is_record but a > method that return if a class is a plain class or not and this method should > be called by the JIT and by Field.set (through a JVM entry point) > so the fact the optimization will be done or not is not related to what the > JLS think a record is, those are separated concern. I agree the current situation is not ideal which requires to declare all the new constructs explicitly for TNSFF. However, it's a reasonable tradeoff to get the JIT optimization for records in time while waiting for true TNSFF investigation like JMM and other relevant specs. I see this just a stop-gap solution. When the long-term TNSFF is in place, the spec can be updated to drop the explicit list of record, inline, etc. A related issue is [JDK-8233873](https://bugs.openjdk.java.net/browse/JDK-8233873). I'm happy if we can do TNSFF in a different solution. Again this PR intends to fix the regression. Two options: 1. Keep [JDK-8247444](https://bugs.openjdk.java.net/browse/JDK-8247444) and implement as this proposed patch 2. Backout [JDK-8247444](https://bugs.openjdk.java.net/browse/JDK-8247444) I expect Chris and/or others will follow up the decision made by the amber-spec-experts w.r.t. trusting finals in records. I'm okay with either option. - PR: https://git.openjdk.java.net/jdk/pull/1706
Re: RFR: 8257596: Clarify trusted final fields for record classes
- Mail original - > De: "Mandy Chung" > À: "core-libs-dev" , "hotspot-dev" > > Envoyé: Mardi 8 Décembre 2020 23:57:39 > Objet: RFR: 8257596: Clarify trusted final fields for record classes Hi Mandy, > This is a follow-up on JDK-8255342 that removes non-specified JVM checks on > classes with Record attributes. That introduces a regression in > `InstanceKlass::is_record` that returns true on a non-record class which has > `RecordComponents` attribute present. This causes unexpected semantics in > `JVM_IsRecord` and `JVM_GetRecordComponents` and also a regression to trust > final fields for non-record classes. It's not an issue, the JVM view of what a record is and the JLS view of what a record is doesn't have to be identical, only aligned. It's fine for the VM to consider any class that have a record Attribute is a record. We already had this discussion on amber-spec-expert list, see https://mail.openjdk.java.net/pipermail/amber-spec-experts/2020-November/002630.html We already know that the JLS definition of a record will have to be changed for inline record (an inline record is not direct subclass of j.l.Record because you have the reference projection in the middle). The real issue is that the JIT optimisation and Field.set() should be aligned, VarHandle deosn't let you change a final field and Unsafe is unsafe, so the current problem is that Field.set() relies on the reflection api by calling Class.isRecord() which is not good because Classs.isRecord() returns the JLS view of the world not the JVM view of the world. As said in the mail chain above, for the JIT optimization, instead of listing all the new constructs, record, inline, etc, i propose to check the other way, only allow to set a final field is only allowed for plain old java class, so the VM should not have a method InstanceKlass::is_record but a method that return if a class is a plain class or not and this method should be called by the JIT and by Field.set (through a JVM entry point) so the fact the optimization will be done or not is not related to what the JLS think a record is, those are separated concern. The more we inject the Java (the language) semantics in the JVM the less it is useful for other languages that run one the JVM. Rémi > > I propose to change `InstanceKlass::is_record` to match the JLS semantic of a > record class, i.e. final direct subclass of `java.lang.Record` with the > presence of `RecordComponents` attribute. There is no change to JVM class > file > validation. Also I propose clearly define: >- `JVM_IsRecord` returns true if the given class is a record i.e. final and >direct subclass of `java.lang.Record` with `RecordComponents` attribute >- `JVM_GetRecordComponents` returns an `RecordComponents` array if >`RecordComponents` attribute is present; otherwise, returns NULL. This > does >not check if it's a record class or not. So it may return non-null on a >non-record class if it has `RecordComponents` attribute. So >`JVM_GetRecordComponents` returns the content of the classfile. Rémi > > - > > Commit messages: > - 8257596: Clarify trusted final fields for record classes > > Changes: https://git.openjdk.java.net/jdk/pull/1706/files > Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1706=00 > Issue: https://bugs.openjdk.java.net/browse/JDK-8257596 > Stats: 60 lines in 4 files changed: 30 ins; 10 del; 20 mod > Patch: https://git.openjdk.java.net/jdk/pull/1706.diff > Fetch: git fetch https://git.openjdk.java.net/jdk pull/1706/head:pull/1706 > > PR: https://git.openjdk.java.net/jdk/pull/1706
