On Thu, 8 May 2025 11:22:30 GMT, Alan Bateman <[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 1931: > 1929: } \ > 1930: o->Fieldname##_field_put(offset, value); \ > 1931: log_debug_if_final_instance_field(thread, "Set<Type>Field", k, > offset); \ Just curious, can we use macros to convert generic `Set<Type>Field` to specific ones, such as using ##Result## in the string? src/hotspot/share/prims/jniCheck.cpp line 1252: > 1250: IN_VM( \ > 1251: checkInstanceFieldID(thr, fieldID, obj, FieldType); \ > 1252: checkCanSetInstanceField(thr, fieldID, obj); \ Same remark, could we pass ##Result## so we avoid `Set<Type>Field`? src/java.base/share/classes/java/lang/Module.java line 950: > 948: * <p> Opening a package with this method does not allow the given > module to > 949: * {@linkplain Field#set(Object, Object) reflectively set} a final > field declared > 950: * in a class in the package, or Suggestion: * {@linkplain Field#set(Object, Object) reflectively set} or src/java.base/share/classes/java/lang/invoke/MethodHandles.java line 3449: > 3447: } > 3448: // check if write access to final field allowed > 3449: if (!field.isStatic() && isAccessible && allowedModes > != TRUSTED) { I don't think we need this allowedModes special permission - I see no scenario in which the core libraries implementation needs to perform such a reflective operation. src/java.base/share/classes/java/lang/reflect/AccessibleObject.java line 173: > 171: * <p> The {@code accessible} flag when {@code true} suppresses Java > language access > 172: * control checks to only enable {@linkplain Field#get > <em>read</em>} access to > 173: * these non-modifiable final fields. Note to reviewers: this is moved to Field.setAccessible. src/java.base/share/classes/java/lang/reflect/Field.java line 1439: > 1437: } else { > 1438: // no java caller, only allowed if field is public in > exported package > 1439: if (!Reflection.verifyPublicMemberAccess(clazz, modifiers)) > { Is this sufficient? I know core libraries has APIs as public non-static final fields, like java.lang.constant.DirectMethodHandleDesc$Kind.refKind. Don't think they should be allowed to be modified by native code, for example. src/java.base/share/classes/java/lang/reflect/Field.java line 1461: > 1459: return Modifier.isFinal(modifiers) > 1460: && !Modifier.isStatic(modifiers) > 1461: && !clazz.isRecord() This check cannot constant fold in the edge case where the declaring class extends `java.lang.Record` but does not have a `Record` attribute. I have seen such classes in proguard-stripped code. test/langtools/jdk/jshell/CompletionSuggestionTest.java line 35: > 33: * @build toolbox.ToolBox toolbox.JarTask toolbox.JavacTask > 34: * @build KullaTesting TestingInputStream Compiler > 35: * @run junit/othervm/timeout=480 CompletionSuggestionTest Why does this need an update? ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/25115#discussion_r2349051829 PR Review Comment: https://git.openjdk.org/jdk/pull/25115#discussion_r2349061108 PR Review Comment: https://git.openjdk.org/jdk/pull/25115#discussion_r2362845215 PR Review Comment: https://git.openjdk.org/jdk/pull/25115#discussion_r2349087370 PR Review Comment: https://git.openjdk.org/jdk/pull/25115#discussion_r2349097202 PR Review Comment: https://git.openjdk.org/jdk/pull/25115#discussion_r2349771906 PR Review Comment: https://git.openjdk.org/jdk/pull/25115#discussion_r2369472297 PR Review Comment: https://git.openjdk.org/jdk/pull/25115#discussion_r2369489143
