8257596: Clarify trusted final fields for record classes

[mlchung]

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.

---

Progress

• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• Change must be properly reviewed

Issue

• JDK-8257596: Clarify trusted final fields for record classes

Reviewers

• Chris Hegarty (@ChrisHegarty - Reviewer)

Download

$ git fetch https://git.openjdk.java.net/jdk pull/1706/head:pull/1706
$ git checkout pull/1706

[bridgekeeper]

👋 Welcome back mchung! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

@mlchung The following labels will be automatically applied to this pull request:

• core-libs
• hotspot

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

[mlchung]

/label add hotspot-runtime
/label remove hotspot

[mlbridge]

Webrevs

• 00: Full (3659879)

[openjdk]

@mlchung
The hotspot-runtime label was successfully added.

[openjdk]

@mlchung
The hotspot label was successfully removed.

[mlbridge]

Mailing list message from Remi Forax on hotspot-dev:

----- 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

[mlchung]

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 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. 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 and implement as this proposed patch
2. Backout 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.

[hseigel]

Hi Mandy,
Could you regression test this change against the JCK lang and vm test suites and Mach5 tiers 1 and 2?
Thanks, Harold

[hseigel]

Hi Mandy,
Could you replace the comment starting at line 1854 in jvm.cpp with:
// A class is a record if and only if it is final and a direct subclass
// of java.lang.Record and the presence of Record attribute;
// otherwise, it is not a record.

Also, replace the comment starting at line 1872 in jvm.cpp with:
// Returns an array containing the components of the Record attribute,
// or NULL if the attribute is not present.
//
// Note that this function returns the components of the Record
// attribute even if the class is not a Record.

Also, please change the name of the attribute in the comments added
to Class.java from RecordComponent to Record.

Thanks, Harold

[openjdk]

@mlchung This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8257596: Clarify trusted final fields for record classes

Reviewed-by: chegar

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 44 new commits pushed to the master branch:

• 502a524: 8257602: Introduce JFR Event Throttling and new jdk.ObjectAllocationSample event (enabled by default)
• 026b09c: 8257483: C2: Split immediate vector rotate from RotateLeftV and RotateRightV nodes
• 0a0691e: 8257901: ZGC: Take virtual memory usage into account when sizing heap
• 29ffffa: 8257997: sun/security/ssl/SSLSocketImpl/SSLSocketLeak.java again reports leaks after JDK-8257884
• db5da96: 8257876: Avoid Reference.isEnqueued in tests
• 4a839e9: 8256459: java/net/httpclient/ManyRequests.java and java/net/httpclient/LineBodyHandlerTest.java fail infrequently with java.net.ConnectException: Connection timed out: no further information
• d93293f: 8256730: Code that uses Object.checkIndex() range checks doesn't optimize well
• 869dcb6: 8257806: Optimize x86 allTrue and anyTrue vector mask operations of Vector API
• 34650f5: 8257872: UL: -Xlog does not check number of options
• 6847bbb: 8255918: XMLStreamFilterImpl constructor consumes XMLStreamException
• ... and 34 more: https://git.openjdk.java.net/jdk/compare/c47ab5f6b72bd3ad6ab22f5461b4f24d4e816773...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

[mlbridge]

Mailing list message from Remi Forax on core-libs-dev:

----- Mail original -----

De: "Mandy Chung" <mchung at openjdk.java.net>
?: "core-libs-dev" <core-libs-dev at openjdk.java.net>, "hotspot-runtime-dev" <hotspot-runtime-dev at 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 <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.

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

[mlchung]

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. This indeed needs some investigation what the best way to address this.

[mlchung]

I have created a new branch off jdk16: https://github.com/mlchung/jdk16/tree/8257596. It fixed the attribute name as Harold pointed out.

@hseigel tier1-tier3 and jck-runtime:vm and jck-runtime:lang tests all passed.

[mlchung]

Target this fix for jdk16 (openjdk/jdk16#14).