RFR: 8257596: Clarify trusted final fields for record classes

[Remi Forax]

----- Mail original -----
> De: "Mandy Chung" <mchung at openjdk.java.net>
> À: "core-libs-dev" <core-libs-dev at openjdk.java.net>, "hotspot-dev" <hotspot-dev at openjdk.java.net>
> 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&pr=1706&range=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