search

eclipse-openj9 / openj9

Eclipse OpenJ9: A Java Virtual Machine for OpenJDK that's optimized for small footprint, fast start-up, and high throughput. Builds on Eclipse OMR (https://github.com/eclipse/omr) and combines with the Extensions for OpenJDK for OpenJ9 repo.
Other
3.23k stars 711 forks source link

New NullRestricted Attribute in Project Valhalla #17340

Open hangshao0 opened 1 year ago

hangshao0 commented 1 year ago

In the last spec: https://cr.openjdk.org/~dlsmith/jep401/jep401-20230428/specs/flattened-heap-jvms.html, a NullRestricted attribute is introduced.

It indicates that the field cannot be null. putfield and putstatic will throw NPE if there is an attempt to assigned null to such fields. The null restricted field needs to be initialized to the default instance.

hangshao0 commented 1 year ago

The null restricted field needs to be initialized to the default instance.

OpenJ9 is already doing this for Q types.

hangshao0 commented 1 year ago

FYI @theresa-m

theresa-m commented 1 year ago

@hangshao0 can you point me to where this is happening in the code?

hangshao0 commented 1 year ago

can you point me to where this is happening in the code?

The question is regarding https://github.com/eclipse-openj9/openj9/issues/17340#issuecomment-1534954375, i.e. where Q type is initialized to the default instance ?

theresa-m commented 1 year ago

can you point me to where this is happening in the code?

The question is regarding #17340 (comment), i.e. where Q type is initialized to the default instance ?

yes.

hangshao0 commented 1 year ago

The default instance of a value type is allocated here:

https://github.com/eclipse-openj9/openj9/blob/5567b8c768d6212fdf0113ed85a6c7b82be5eb0f/runtime/vm/ClassInitialization.cpp#L521-L535

The Q type instance fields are set here: https://github.com/eclipse-openj9/openj9/blob/5567b8c768d6212fdf0113ed85a6c7b82be5eb0f/runtime/gc_modron_startup/mgcalloc.cpp#L518 https://github.com/eclipse-openj9/openj9/blob/5567b8c768d6212fdf0113ed85a6c7b82be5eb0f/runtime/vm/ValueTypeHelpers.cpp#L78-L96

defaultValueWithUnflattenedFlattenables() uses the default instance to set the fields if they are not flattened (inlined). Note the flattenedClassCache struct has all the J9Class of the Q type fields.

theresa-m commented 10 months ago

The remaining tasks for this issue are:

Remaining tasks:

theresa-m commented 10 months ago

@hangshao0 are there any other class verification checks you think should be added for NullRestricted? See the previous comment for all prs that are open against this issue.

hangshao0 commented 10 months ago

I haven't reviewed https://github.com/eclipse-openj9/openj9/pull/18030 yet. But notice the following in the spec:

Section 5.3.5. 5. For each non-static field of C declared with a NullRestricted attribute......the class or interface named by the field's type must be a value class with an ImplicitCreation attribute whose ACC_DEFAULT flag is set. If not, loading throws an IncompatibleClassChangeError

Section 5.4.2 1. For each static field of C declared with a NullRestricted attribute......The resolved type must be of a value class with an ImplicitCreation attribute whose ACC_DEFAULT flag set. If not, preparation fails with an IncompatibleClassChangeError.

Section 6.5 if the value to store is null and the resolved field is declared with a NullRestricted attribute, the withfield instruction throws a NullPointerException.

theresa-m commented 10 months ago

Thanks, https://github.com/eclipse-openj9/openj9/pull/18030 should cover the first two but I didn't notice there was a specific error to be thrown. I'll update that.

theresa-m commented 10 months ago

What about this one? Section 6.5.

The initial value of any instance field declared with a NullRestricted attribute is the initial instance of the field's class. The initial value of any instance field of a primitive type is the default value of the type ([2.3](https://docs.oracle.com/javase/specs/jvms/se16/html/jvms-2.html#jvms-2.3)). The initial value of any other instance field is null.
hangshao0 commented 10 months ago

What about this one? Section 6.5.

We are already doing that. See defaultValueWithUnflattenedFlattenables() where we are using the default initial instance (flattenedClassCache->defaultValue) to initialize the fields that are not flattened. For flattened fields, they are initialized to all zeros.

We need to change J9_IS_FIELD_FLATTENED() to include the check of null restricted attribute. Currently, it checks if the field class is flattened and it is not a volatile field > 8 bytes. We should only flatten null restricted fields, so the check for null restricted field should be added. In addition, we need to eventually replace all the places doing 'Q' type checks with null restricted attribute checks.

But I guess making the above changes now will break some tests, as javac is not updated to generate null restricted attribute yet in the default branch of Valhalla repo.