8280798: com.sun.jdi.ObjectReference::setValue spec should prohibit any final field modification

[alexmenkov]

com.sun.jdi.ObjectReference::setValue spec says that final static fields cannot be modified, but openjdk implementation throws IllegalArgumentException for any final fields (static or instance).

The fix updates the spec to prohibit any final field modification
CSR: JDK-8281652

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• Change requires a CSR request to be approved

Issues

• JDK-8280798: com.sun.jdi.ObjectReference::setValue spec should prohibit any final field modification
• JDK-8281652: com.sun.jdi.ObjectReference::setValue spec should prohibit any final field modification (CSR)

Reviewers

• Alan Bateman (@AlanBateman - Reviewer) ⚠️ Review applies to 5d98874c
• Chris Plummer (@plummercj - Reviewer)
• Serguei Spitsyn (@sspitsyn - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 11279

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/11279.diff

[bridgekeeper]

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

[alexmenkov]

/csr needed

[openjdk]

@alexmenkov this pull request will not be integrated until the CSR request JDK-8281652 for issue JDK-8280798 has been approved.

[openjdk]

@alexmenkov The following label will be automatically applied to this pull request:

• serviceability

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

[mlbridge]

Webrevs

• 03: Full - Incremental (ab58e581)
• 02: Full - Incremental (55a4616b)
• 01: Full - Incremental (5d98874c)
• 00: Full (24b580eb)

[sspitsyn]

This issue need a release note.
The fix itself looks good.

[dholmes-ora]

I commented in the bug report, but shouldn't a debugger be able to set a final field?

[AlanBateman]

I think this one is going to require more investigation as I can't immediately tell if this is a spec or implementation issue.

I checked the JDWP spec and the ClassReference/SetValues has "Final fields cannot be set" in its spec, so this means that static finals can't be set. ObjectReference/SetValues command doesn't have this, so JDWP allows a tool to set final fields. So if the change proposed in this PR goes ahead it means that JDI and JDWP specs won't be aligned (which is okay, it just needs to be fully understood).

Maybe start by digging into the history of ReferenceTypeImpl to see if the spec vs. implementation discrepancy dates goes back to JDK 1.2 (when JPDA was added) or it crept in at some other time. Maybe also dig into bug reports to see if there has been any complaints about the current behavior.

If the outcome is that spec is changed as proposed then it will require the description for IllegalArgumentException to be expanded. If the outcome is to allow for changing of some final fields then the spec may need to be updated to take account of "non-modifiable final field", as in final fields in hidden classes and records. In both cases, the JDWP spec will need to looked at it too.

[plummercj]

ObjectReference.setValue() calls ReferenceType.validateFieldSet(), which does:

void validateFieldSet(Field field) {
    validateFieldAccess(field);
    if (field.isFinal()) {
        throw new IllegalArgumentException("Cannot set value of final field");
    }
}

This code has been in place since the file was introduced on 2007-12-01. If there is a version that predates it, I don't know where to look. I'm not sure of the history relative to JDK 1.2 and the introduction of JPDA.

Yes, what is being proposed makes the JDI and JDWP specs out of sync, but the implementations have always been out of sync. The proposed spec change is just clarifying what the implementation already does. However, Eclipse does allow the user to set a non-static final field, but only after clicking through a warning dialog.

[dholmes-ora]

So changing the spec would make Eclipse non-compliant. Perhaps to bring the spec and implementation in-line we have to make the implementation behaviour allowable by the spec but not required - that is what we often do in other areas (e.g. JNI spec - my personal point of experience).

But I remain surprised/perplexed that a debugger would not be allowed to change a final field. Can't we still change final fields via reflection anyway? (I know the constraints have been tightening in that area.)

[plummercj]

https://docs.oracle.com/en/java/javase/18/docs/api/java.base/java/lang/reflect/Field.html#set(java.lang.Object,java.lang.Object)

It is allowed, for the most part, but with the warning:

Setting a final field in this way is meaningful only during deserialization or reconstruction of instances of classes with blank final fields, before they are made available for access by other parts of a program. Use in any other context may have unpredictable effects, including cases in which other parts of a program continue to use the original value of this field.

In JDK-8280798 Alex has documented a use case that shows the dangers of allowing this:

https://bugs.openjdk.org/browse/JDK-8280798?focusedCommentId=14539651&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14539651

[alexmenkov]

The change makes Eclipse non-compliant, but it shouldn't be a problem as Eclipse JDI implementation is used only by Eclipse debugger (we discussed it with one of the Eclipse debugger contributors - he doesn't see problem here). And it's up to Eclipse team to decide if they want to update the implementation or keep it.
Relaxing the spec was one of the solutions considered, but looks like it doesn't make much sense.

Reflection allows to modify final instance fields, but its spec has a warning that changing final fields is meaningful only in certain cases (which are not common for debuggers). So if we decide to update implementation to modify final fields, it would be better to add similar warning to JDI spec (additionally to "non-modifiable final field" changes described by Alan)

[plummercj]

I also have to wonder about JIT behavior in this context (and this might be what the RMI warning is alluding to). What if the JIT sees a static final reference to an object with a final field. It probably generates code that references the value of the field rather than generating code that fetches from the field. If the field value changes, the JIT won't know and the existing code will continue to reference the old value.

[AlanBateman]

I dug into the pre-OpenJDK history as most of the action on this was in 1999.

In 1999, ObjectReference.setValue's docs changed from "The field must not be final" to "If static, the field must not be final". The exception message started out as "Cannot set value of final field" and changed to "Cannot set value of static final field".

jjh re-visited it in 2005 via JDK-623152 where he changed the exception message to "Cannot set value of final field".

I don't think the ancient history sheds much more light on the original intent. I suspect the original intent was to disallow setValue only static final fields. That would have aligned with Field.set + setAccessible(true) at the time.

In general I think the JDK is on the path to a world where "final means final". We see this with final fields on hidden classes and fields in records. So the proposal to change the JDI spec may be too bad but it require reaching out to IDE maintainers. Eclipse have their own JDI implementation. VS.Code uses the Eclipse JDI implementation. IntelliJ maintain a fork of JDI. So while the JDK's implementation of JDI has disallowed setting of all final fields for 20+ years, this is not the JDI implementation that is used by at least two of the main stream IDEs.

[plummercj]

IntelliJ currently does not allow setting final fields. I'm checking with a team member to make sure they are ok with the spec change.

As mentioned earlier, we already discussed this with an Eclipse team member, and there was no objection to the proposed spec change. Eclipse currently does allow setting final fields, but you have to click through a warning dialog first. Eclipse can continue with this behavior if they wish. There is no requirement for them to be spec compliant.

[alexmenkov]

I decided to check if VS Code shows the warning as Eclipse does, so I installed VS Code and tried to set final fields in debugger.
For both static and instance fields I got:
Failed to set variable. Reason: java.lang.UnsupportedOperationException: SetVariableRequest: Final field value cannot be changed.

So VS Code DOES NOT allow to set static final fields.

As far as I got VS Code uses Eclipse JDT only for editor (code navigation, auto completion, refactoring, etc), but debugging is implemented by another extension: Debugger for Java (https://github.com/Microsoft/java-debug). I.e. VS Code does not use Eclipse
JDI implementation.

[plummercj]

IntelliJ currently does not allow setting final fields. I'm checking with a team member to make sure they are ok with the spec change.

Egor at Jetbrains (Intellij) is ok with the proposed spec changes.

[AlanBateman]

I checked with the Microsoft team that maintain the Java Debugger in VS Code. The JDI implementation comes from the Eclipse JDT project. The summary is that the VS Code debugger doesn't allow final fields to be modified. It checks if the field is final so it won't call ObjectReference::setValue for final fields.

[AlanBateman]

I think we've got enough information now to support this change.

[AlanBateman]

One other thing to add is that we should survey the existing tests to make sure that there are tests to ensure IAE is thrown for both static and instance fields when they are final.

[plummercj]

One other thing to add is that we should survey the existing tests to make sure that there are tests to ensure IAE is thrown for both static and instance fields when they are final.

I think a good way to find such tests would be to remove the !isFinal() check and see if any tests fail.

[openjdk]

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

8280798: com.sun.jdi.ObjectReference::setValue spec should prohibit any final field modification

Reviewed-by: alanb, cjplummer, sspitsyn

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 170 new commits pushed to the master branch:

• b035056: 8297455: Use the official ToolProvider API to call javac
• 257aa15: 8297444: Refactor the javacserver build tool
• e846b04: 8297875: jar should not compress the manifest directory entry
• 82031d3: 8297294: compiler/c2/irTests/TestMulNodeIdealization.java failed compilation
• 337ca10: 8297978: Exclude vmTestbase/nsk/stress/except/except012.java until 8297977 is fixed
• 770ff5a: 8297215: Update libs tests to use @enablePreview
• c69aa42: 8297968: Crash in PrintOptoAssembly
• 5a5ced3: 8297830: aarch64: Make Address a discriminated union internally
• 391599b: 8297313: Refactor APIs for calculating address of CDS archive heap regions
• 0962957: 8297449: Update JInternalFrame Metal Border code
• ... and 160 more: https://git.openjdk.org/jdk/compare/5a45c25151b1da8e329ea2be21a0e4d2652f8b4a...master

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 master branch, type /integrate in a new comment.

[alexmenkov]

The only test for final fields is vmTestbase/nsk/jdi/ObjectReference/setValue/setvalue004.
It tested only static final fields.
I added testing for instance final fields

[alexmenkov]

/integrate

[openjdk]

Going to push as commit 2821fa9.
Since your change was applied there have been 186 commits pushed to the master branch:

• fb6fd03: 8291830: jvmti/RedefineClasses/StressRedefine failed: assert(!is_null(v)) failed: narrow klass value can never be zero
• 1b92465: 8297608: JFR: Incorrect duration after chunk rotation
• 6065696: 8297982: Exclude vmTestbase/nsk/monitoring/stress/lowmem/ with ZGC until 8297979 is fixed
• 415cfd2: 8297285: Shenandoah pacing causes assertion failure during VM initialization
• df07255: 8297984: Turn on warnings as errors for javadoc
• 227364d: 8297953: Fix several C2 IR matching tests for RISC-V
• 1370228: 8297941: Add override modifier in space.hpp
• 319faa5: 8296084: javax/swing/JSpinner/4788637/bug4788637.java fails intermittently on a VM
• b73363f: 8297686: JFR: Improve documentation of EventStream::onMetadata(Consumer)
• 1376f33: 8297911: Memory leak in JfrUpcalls::on_retransform
• ... and 176 more: https://git.openjdk.org/jdk/compare/5a45c25151b1da8e329ea2be21a0e4d2652f8b4a...master

Your commit was automatically rebased without conflicts.

[openjdk]

@alexmenkov Pushed as commit 2821fa9.

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