On Tue, 8 Dec 2020 22:52:37 GMT, Mandy Chung <mch...@openjdk.org> 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
