----- Mail original ----- > De: "Mandy Chung" <mch...@openjdk.java.net> > À: "core-libs-dev" <core-libs-dev@openjdk.java.net>, "hotspot-runtime-dev" > <hotspot-runtime-...@openjdk.java.net> > 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 <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. 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. 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. Rémi > > ------------- > > PR: https://git.openjdk.java.net/jdk/pull/1706
