RFR: 8257596: Clarify trusted final fields for record classes

[Mandy Chung]

On Tue, 8 Dec 2020 22:52:37 GMT, Mandy Chung <mchung at 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