On Thu, 10 Dec 2020 14:13:27 GMT, Chris Hegarty <che...@openjdk.org> 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
