8349945: Implement strict static fields (proposed JVM feature) #1465

matias9927 · 2025-05-20T22:15:29Z

This patch enables and implements verification for fields with the ACC_STATIC and ACC_STRICT modifiers. To enforce strictness on static fields, the reads and writes on the field are tracked dynamically to ensure that the field is written before being read and written to before.

Progress

Change must not contain extraneous whitespace

Issue

JDK-8349945: Implement strict static fields (proposed JVM feature) (Enhancement - P4)

Reviewers

Dan Heidinga (@DanHeidinga - no project role) ⚠️ Review applies to 95c76e6c
Chen Liang (@liach - Committer) ⚠️ Review applies to 95c76e6c
Frederic Parain (@fparain - Committer) ⚠️ Review applies to 8631c3c0

Contributors

John R Rose <[email protected]>

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/valhalla.git pull/1465/head:pull/1465
$ git checkout pull/1465

Update a local copy of the PR:
$ git checkout pull/1465
$ git pull https://git.openjdk.org/valhalla.git pull/1465/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1465

View PR using the GUI difftool:
$ git pr show -t 1465

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/valhalla/pull/1465.diff

Using Webrev

Link to Webrev Comment

bridgekeeper · 2025-05-20T22:16:05Z

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

openjdk · 2025-05-20T22:16:05Z

@matias9927 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:

8349945: Implement strict static fields (proposed JVM feature)

Co-authored-by: John R Rose <[email protected]>
Reviewed-by: heidinga, liach, fparain

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 1 new commit pushed to the lworld branch:

6a08148: 8353180: [lworld] C2: Meeting two constant TypeAryPtr with different nullness is wrongly treated as exact

Please see this link for an up-to-date comparison between the source branch of this pull request and the lworld branch.
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 lworld branch, type /integrate in a new comment.

mlbridge · 2025-05-20T22:20:28Z

Webrevs

liach · 2025-05-21T18:52:41Z

test/hotspot/jtreg/runtime/valhalla/inlinetypes/verifier/Brbefore_BAD.jasm

+ * questions.
+ */
+
+identity class Brbefore_BAD version 69:65535


Can't this be compiled with javac? I don't think we are going to add any checks for strict initialization on the javac side as pre-write reads cannot be detected because they can easily happen in nested method calls.

Even if we decide to use jasm, I think the best practice seems to leave the source to generate the jasm in a .test file, and comment in the jasm what was changed.

Javac can catch this because the static field hasn't been initialized yet:
error: variable F2__STRICT might not have been initialized int x = F2__STRICT; //FAIL
It may be possible to write a java program with nested calls that does this but I think it's best to keep the test cases simple.

I agree with the last comment, I'll add the equivalent java code to the jasm files.

DanHeidinga · 2025-05-22T11:49:01Z

src/hotspot/share/c1/c1_Runtime1.cpp

@@ -1165,6 +1166,9 @@ JRT_ENTRY(void, Runtime1::patch_code(JavaThread* current, C1StubId stub_id ))
    // field because it was unknown at compile time.
    deoptimize_for_flat = result.is_flat();

+    // Strict statics may require tracking if their class is not fully initialized.
+    // For now we can bail out of the compiler and let the interpreter handle it.
+    deoptimize_for_strict_static = result.is_strict_static_unset();


Deoptimizing on strict fields makes sense when the field is unset as an initial implementation.

As a possible future optimization, we could make some additional checks here to decide whether we need to deopt or not and instead simulate the strict static is_set bits here. current Thread is either the thread initializing the class and therefore able to read/update the set/unset bits or it is some other thread that would need to wait.

If current is the initializing thread, and this is a strict static field write, we know we're currently executing the field set or we wouldn't be patching the code. Given that, it would be possible to avoid the deopt by calling a helper from this code to toggle the bit.

Something to think about for the future when strict statics are more common to avoid spurious deopts

DanHeidinga · 2025-05-22T12:18:24Z

src/hotspot/share/oops/fieldInfo.hpp

@@ -313,6 +313,8 @@ class FieldStatus {
  enum FieldStatusBitPosition {
    _fs_access_watched,       // field access is watched by JVMTI
    _fs_modification_watched, // field modification is watched by JVMTI
+    _fs_strict_static_unset,  // JVM_ACC_STRICT static field has not yet been set
+    _fs_strict_static_unread, // SS field has not yet been read (EnforceStrictStatics=2 only)


Suggested change

_fs_strict_static_unread, // SS field has not yet been read (EnforceStrictStatics=2 only)

_fs_strict_static_unread, // SS field has not yet been read

Minor comment update - I think we removed the various modes so we don't need to mention them in the comments

DanHeidinga · 2025-05-22T12:25:58Z

src/hotspot/share/oops/instanceKlass.cpp

+      throw_strict_static_exception(bad_strict_static, "is unset before first read in", CHECK);
+    }
+  } else {
+    // Experimentally, enforce additional proposed conditions after the first write.


Suggested change

// Experimentally, enforce additional proposed conditions after the first write.

// Ensure no write after read for final strict statics

I think this comment may be left over from the multi-option initial prototype. It might be worth going over all the comments and making sure they still apply now that we've settled on a single approach

DanHeidinga · 2025-05-22T12:27:34Z

src/hotspot/share/oops/instanceKlass.cpp

+        Symbol* bad_strict_static = fi.name(constants());
+        throw_strict_static_exception(bad_strict_static, "is set after read (as final) in", CHECK);
+      } else if (!is_writing && fs.is_strict_static_unread()) {
+        // log the read (this requires an extra status bit, with extra tests on it)


Suggested change

// log the read (this requires an extra status bit, with extra tests on it)

This comment also seems like a holdover from the prototype

DanHeidinga · 2025-05-22T12:29:06Z

src/hotspot/share/oops/instanceKlassFlags.hpp

@@ -59,6 +59,8 @@ class InstanceKlassFlags {
    flag(is_naturally_atomic                , 1 << 16) /* loaded/stored in one instruction*/ \
    flag(must_be_atomic                     , 1 << 17) /* doesn't allow tearing */ \
    flag(has_loosely_consistent_annotation  , 1 << 18) /* the class has the LooselyConsistentValue annotation WARNING: it doesn't automatically mean that the class allows tearing */ \
+    flag(is_implicitly_constructible        , 1 << 19) /* the class has the ImplicitlyConstrutible annotation */ \


Did this get accidentally propogated into this patch from somewhere else?

Weird, thanks for catching that!

DanHeidinga · 2025-05-22T12:31:24Z

src/hotspot/share/prims/jni.cpp

-      !InstanceKlass::cast(k)->find_field(fieldname, signame, true, &fd)) {
+      !InstanceKlass::cast(k)->find_field(fieldname, signame, true, &fd) ||
+      // strict fields need special tracking during <clinit>; do not hand them out so early
+      (fd.access_flags().is_strict() && !InstanceKlass::cast(k)->is_initialized())) {


Should this be an assert rather than a runtime check given that we force the class to be initialized a few lines above?

openjdk · 2025-05-23T19:09:00Z

@matias9927 this pull request can not be integrated into lworld due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout implement_strict_statics_8349945
git fetch https://git.openjdk.org/valhalla.git lworld
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge lworld"
git push

DanHeidinga · 2025-05-26T13:27:40Z

src/hotspot/share/prims/jni.cpp

@@ -2057,6 +2057,7 @@ JNI_ENTRY(jfieldID, jni_GetStaticFieldID(JNIEnv *env, jclass clazz,
  fieldDescriptor fd;
  if (!k->is_instance_klass() ||
      !InstanceKlass::cast(k)->find_field(fieldname, signame, true, &fd)) {
+    assert(InstanceKlass::cast(k)->is_initialized(), "must be");


The assert is right before throwing a NoSuchFieldError. If we're going to assert, we want to do it on the path that can return the jfieldID so it needs to be moved out of the if block

DanHeidinga · 2025-05-26T13:33:28Z

There are a couple of other corner cases for dealing with reflection we should probably test. Assume a single strict static final field :

bytecode write; reflective read; bytecode write --> error for write after read
reflective write; bytecode read, bytecode write --> error for write after read
reflective write; bytecode read, reflective write --> error for write after read
reflective write; reflective read, reflective write --> error for write after read

This ensures we've covered the mix of reflective vs bytecode reads and writes and ensures all paths correctly set and check the bits.

liach · 2025-05-26T15:25:28Z

I think reflective write for static final is already impossible - at least that is what I know about Method Handles and core reflection. So we probably just need to care about reflective reads.

DanHeidinga · 2025-05-26T15:34:45Z

I think reflective write for static final is already impossible - at least that is what I know about Method Handles and core reflection. So we probably just need to care about reflective reads.

Let's confirm that. It used to be possible (and maybe still is) to use setAccessible to allow reflective writes to finals. MHs won't unallow static final setters unless they come through the MethodHandles::unreflect door with an accessible reflective field

matias9927 · 2025-05-29T15:10:09Z

/contributor add @rose00

openjdk · 2025-05-29T15:10:28Z

@matias9927
Contributor John R Rose <[email protected]> successfully added.

matias9927 · 2025-05-29T16:53:51Z

I think reflective write for static final is already impossible - at least that is what I know about Method Handles and core reflection. So we probably just need to care about reflective reads.

Let's confirm that. It used to be possible (and maybe still is) to use setAccessible to allow reflective writes to finals. MHs won't unallow static final setters unless they come through the MethodHandles::unreflect door with an accessible reflective field

It looks like Chen is correct here, reflective writing of static finals doesn't seem to be possible. At the very least, setAccessible() won't prevent an exception like this:
java.lang.IllegalAccessException: Can not set static final java.lang.String field WriteAfterReadRefl1.F1__STRICT to java.lang.String

With regards to your suggested test cases, the best I can do at the moment is add the first one on that list.

DanHeidinga · 2025-05-29T16:56:45Z

That's great! It removes a bunch cases we'll never have to worry about

matias9927 · 2025-05-29T17:02:03Z

A subtle change got swept up in the merge: StrictStaticFieldsTest.java remains problem listed but is now associated with the bug JDK-8357141

liach · 2025-05-30T00:33:20Z

test/hotspot/jtreg/runtime/valhalla/inlinetypes/verifier/StrictStaticTests.java

+        if (vererrs != null && !vererrs.isEmpty()) {
+            System.out.println(vererrs);
+            var cm = ClassFile.of().parse(classBytes);
+            System.out.println("" + cm + cm.fields() + cm.methods() + cm.methods().get(0).code().orElseThrow().elementList());


Suggested change

System.out.println("" + cm + cm.fields() + cm.methods() + cm.methods().get(0).code().orElseThrow().elementList());

System.out.println(cm.toDebugString());

Note we can't fail here: ClassFile.verify is not up-to-date with strict fields. I don't know who originally wrote the VerificationTable code that is supposed to be on parity with stackMapTable.cpp, but that code is complex and is bound to diverge from whatever we have in the C++ code there.

DanHeidinga

Looks good. Thanks Matias

liach

I can keep an eye on the verification part of classfile and investigate that from the language and tooling side. Please simplify the debug output of a ClassModel as I suggested above. Otherwise, changes to java.lang.invoke looks good.

fparain

Looks good to me.
Thank you for this thorough work on strict fields.

liach · 2025-06-07T01:23:27Z

test/hotspot/jtreg/ProblemList.txt

@@ -131,6 +131,7 @@ containers/docker/TestJcmdWithSideCar.java 8341518 linux-x64
 runtime/AccModule/ConstModule.java 8294051 generic-all
 runtime/valhalla/inlinetypes/CircularityTest.java 8349037 generic-all
 runtime/valhalla/inlinetypes/verifier/StrictInstanceFieldsTest.java 8357141 generic-all
+runtime/valhalla/inlinetypes/verifier/StrictStaticFieldsTest.java 8357141 generic-all


8357141 is now closed. We might need a dedicated new issue to re-enable these tests.

Thanks for letting me know! I've updated the bug number to CODETOOLS-7904031 for both StrictStaticFieldsTest and StrictInstanceFieldsTest.

liach · 2025-06-10T15:44:42Z

I think this is ready for integration; we can fine tune the test and other things later.

matias9927 · 2025-06-10T19:15:11Z

Thank you for the reviews @DanHeidinga @fparain and @liach and thank you @rose00 for the prototype!
/integrate

openjdk · 2025-06-10T19:16:16Z

Going to push as commit f4bd868.
Since your change was applied there has been 1 commit pushed to the lworld branch:

6a08148: 8353180: [lworld] C2: Meeting two constant TypeAryPtr with different nullness is wrongly treated as exact

Your commit was automatically rebased without conflicts.

openjdk · 2025-06-10T19:16:23Z

@matias9927 Pushed as commit f4bd868.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

matias9927 added 2 commits May 20, 2025 18:13

8349945: Implement strict static fields (proposed JVM feature)

1aa7802

Added tests

81a6f5a

matias9927 marked this pull request as ready for review May 20, 2025 22:16

openjdk bot added ready rfr labels May 20, 2025

liach reviewed May 21, 2025

View reviewed changes

DanHeidinga reviewed May 22, 2025

View reviewed changes

Added reflective tests and problem listed test

b5f9b44

openjdk bot added merge-conflict and removed ready labels May 23, 2025

matias9927 added 2 commits May 23, 2025 15:11

Copyrights

fefad14

Chen and Dan comments and cleanup

702b82a

DanHeidinga reviewed May 26, 2025

View reviewed changes

Dan comments and new test case

e416b25

Merge branch 'lworld' into implement_strict_statics_8349945

95c76e6

openjdk bot added ready and removed merge-conflict labels May 29, 2025

liach reviewed May 30, 2025

View reviewed changes

DanHeidinga approved these changes Jun 4, 2025

View reviewed changes

liach approved these changes Jun 5, 2025

View reviewed changes

openjdk bot added merge-conflict and removed ready labels Jun 5, 2025

matias9927 added 3 commits June 6, 2025 10:41

Moved exception to guarantee and Chen comment

33ee4b2

Merge branch 'lworld' into implement_strict_statics_8349945

603219a

Fixed path in problem list and adjusted jasm test

68621ff

openjdk bot added ready and removed merge-conflict labels Jun 6, 2025

Problem listed test again

8631c3c

fparain approved these changes Jun 6, 2025

View reviewed changes

liach reviewed Jun 7, 2025

View reviewed changes

Updated problemlist bug numbers

22598c3

openjdk bot added the integrated label Jun 10, 2025

openjdk bot closed this Jun 10, 2025

openjdk bot removed ready rfr labels Jun 10, 2025

	_fs_strict_static_unread, // SS field has not yet been read (EnforceStrictStatics=2 only)
	_fs_strict_static_unread, // SS field has not yet been read

	// Experimentally, enforce additional proposed conditions after the first write.
	// Ensure no write after read for final strict statics

	System.out.println("" + cm + cm.fields() + cm.methods() + cm.methods().get(0).code().orElseThrow().elementList());
	System.out.println(cm.toDebugString());

8349945: Implement strict static fields (proposed JVM feature) #1465

8349945: Implement strict static fields (proposed JVM feature) #1465

Uh oh!

Conversation

matias9927 commented May 20, 2025 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Contributors

Reviewing

Uh oh!

bridgekeeper bot commented May 20, 2025

Uh oh!

openjdk bot commented May 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mlbridge bot commented May 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DanHeidinga May 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

openjdk bot commented May 23, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DanHeidinga commented May 26, 2025

Uh oh!

liach commented May 26, 2025

Uh oh!

DanHeidinga commented May 26, 2025

Uh oh!

matias9927 commented May 29, 2025

Uh oh!

openjdk bot commented May 29, 2025

Uh oh!

matias9927 commented May 29, 2025

Uh oh!

DanHeidinga commented May 29, 2025

Uh oh!

matias9927 commented May 29, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DanHeidinga left a comment

Choose a reason for hiding this comment

Uh oh!

liach left a comment

Choose a reason for hiding this comment

Uh oh!

fparain left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liach commented Jun 10, 2025

Uh oh!

matias9927 commented Jun 10, 2025

Uh oh!

matias9927 commented May 20, 2025 •

edited by openjdk bot

Loading

openjdk bot commented May 20, 2025 •

edited

Loading

mlbridge bot commented May 20, 2025 •

edited

Loading

DanHeidinga May 22, 2025 •

edited

Loading