On Tue, 23 Sep 2025 05:09:24 GMT, David Holmes <[email protected]> wrote:
>> Implementation changes for [JEP 500: Prepare to Make Final Mean >> Final](https://openjdk.org/jeps/500). >> >> Field.set (and Lookup.unreflectSetter) are changed to allow/warn/debug/deny >> when mutating a final instance field. JFR event recorded if final field >> mutated. Spec updates to Field.set, Field.setAccessible and Module.addOpens >> to align with the proposal in the JEP. >> >> HotSpot is updated to add support for the new command line options. To aid >> diagnosability, -Xcheck:jni reports a fatal error when a mutating a final >> field with JNI, and -Xlog:jni=debug can help identity when JNI code mutates >> finals. For now, JNI code is allowed to set the "write-protected" fields >> System.in/out/err, we can re-visit once we change the >> System.setIn/setOut/setErr methods to not use JNI (I prefer to keep this >> separate to this PR because there is a small startup regression to address >> when changing System.setXXX). >> >> There are many new tests. A small number of existing tests are changed to >> run /othervm as reflectively opening a package isn't sufficient. Changing >> the tests to /othervm means that jtreg will launch the agent with the >> command line options to open the package. >> >> Testing: tier1-6 > > src/hotspot/share/prims/jni.cpp line 1873: > >> 1871: if (log_is_enabled(Debug, jni)) { >> 1872: fieldDescriptor fd; >> 1873: bool found = >> InstanceKlass::cast(klass)->find_field_from_offset(offset, true, &fd); > > If you are assuming/expecting an `InstanceKlass` then that should be the type > of the parameter. (The existing code needs a cleanup in this area too but > that is outside the scope of this PR.) Okay, but it will mean a cast in the jni_SetXXField callers, I don't think we avoid that before more changes to the existing code. > src/hotspot/share/prims/jniCheck.cpp line 1224: > >> 1222: WRAPPER_GetField(jdouble, Double, T_DOUBLE) >> 1223: >> 1224: static void checkCanSetInstanceField(JavaThread* thr, jfieldID fid, >> jobject obj) { > > There is a lot of duplication here with logic already performed in > `checkInstanceFieldID` - can you not just pass an extra argument `is_set` to > that and include the new checks there? And similarly for the static case. Given that checkInstanceFieldID and checkStaticFieldID already have a list of checks then adding it to these functions, rather than new functions, would be better, I'll do that. > test/jdk/java/lang/reflect/Field/mutateFinals/jni/JNIAttachMutatorTest.java > line 29: > >> 27: * @summary Test native thread attaching to the VM with JNI >> AttachCurrentThread and directly >> 28: * invoking Field.set to set a final field >> 29: * @requires (os.family == "linux" | os.family == "mac") > > This test should work for any non-Windows platform using pthreads. We can make it work on Windows too but means `#ifdef WIN32` and CreateThread in the test, easy to do, and means the `@requires` can do away. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/25115#discussion_r2372772061 PR Review Comment: https://git.openjdk.org/jdk/pull/25115#discussion_r2372771913 PR Review Comment: https://git.openjdk.org/jdk/pull/25115#discussion_r2372773863
