Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NullRestricted attribute field class checks #18030

Merged
merged 2 commits into from
Sep 21, 2023

Conversation

theresa-m
Copy link
Contributor

@theresa-m theresa-m commented Aug 28, 2023

  • NullRestricted fields must be a value type with an ImplicitCreation attribute that has the ACC_DEFAULT flag set. For non-static fields this should be checked during class creation and for static fields during the preparation stage of linking.
  • This pr also adds static NullRestricted fields to the flattenedClassCache to make the described failure possible. Changes to allow non-static NullRestricted fields to be flattened will come in a later pull request.
  • Move ImplicitCreation flag declarations because flags are no longer part of extramodifiers, and correct default flag value

Related: #17340

@theresa-m theresa-m marked this pull request as draft August 29, 2023 13:41
@theresa-m theresa-m marked this pull request as ready for review August 29, 2023 14:10
@theresa-m theresa-m requested a review from hangshao0 August 29, 2023 14:10
@hangshao0 hangshao0 added project:valhalla Used to track Project Valhalla related work comp:vm comp:test labels Aug 29, 2023
@theresa-m theresa-m marked this pull request as draft August 30, 2023 14:16
@hangshao0
Copy link
Contributor

There are merge conflicts that need to be resolved.

@theresa-m theresa-m changed the title NullRestricted field attribute class checks NullRestricted attribute field class checks Aug 30, 2023
@theresa-m theresa-m marked this pull request as ready for review August 30, 2023 20:11
@theresa-m theresa-m force-pushed the nr_verification_3 branch 3 times, most recently from c354487 to 19b3767 Compare September 18, 2023 21:08
@theresa-m
Copy link
Contributor Author

@hangshao0 I'm planning to add changes to flatten non-static NullRestricted field classes in another pull request since its not needed for these checks.

@theresa-m theresa-m requested a review from hangshao0 September 18, 2023 21:10
runtime/oti/j9javaaccessflags.h Outdated Show resolved Hide resolved
runtime/vm/createramclass.cpp Show resolved Hide resolved
Flags are no longer part of extramodifiers

Signed-off-by: Theresa Mammarella <[email protected]>
Copy link
Contributor

@hangshao0 hangshao0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@hangshao0
Copy link
Contributor

Jenkins test sanity,extended xlinuxval jdknext

@hangshao0
Copy link
Contributor

Jenkins test sanity,extended winval jdknext

@theresa-m
Copy link
Contributor Author

theresa-m commented Sep 20, 2023

Looking into the failure.

edit: linking here so the failure isn't lost https://openj9-jenkins.osuosl.org/job/Test_openjdkValhalla_j9_sanity.functional_x86-64_linux_valhalla_Personal/79/

NullRestricted fields must be a value type with an ImplicitCreation attribute that has the ACC_DEFAULT flag set. For non-static fields this should be checked during class creation and for static fields during the preparation stage of linking.

This pr also adds static NullRestricted fields to the flattenedClassCache to make the described failure possible. Changes to allow non-static NullRestricted fields to be flattened will come in a later pull request.

Signed-off-by: Theresa Mammarella <[email protected]>
* ImplicitCreation attribute. The attribute must have the ACC_DEFAULT flag set.
* Static fields will be checked during class preparation.
*/
if (J9_ARE_ALL_BITS_SET(modifiers, J9FieldFlagIsNullRestricted)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Forgot this important line here :) the functional/Valhalla tests are passing for me locally now.

@hangshao0
Copy link
Contributor

Jenkins test sanity,extended xlinuxval jdknext

@hangshao0
Copy link
Contributor

Jenkins test sanity,extended winval jdknext

@hangshao0 hangshao0 merged commit 27f0069 into eclipse-openj9:master Sep 21, 2023
@theresa-m theresa-m deleted the nr_verification_3 branch December 6, 2023 14:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp:test comp:vm project:valhalla Used to track Project Valhalla related work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants