On Wed, 29 Jul 2026 17:03:25 GMT, Chris Plummer <[email protected]> wrote:
>> The JDWP implementation (debug agent) allows the writing of static final >> fields and instance final fields. This has always been the case since no >> checks are made to prevent it, and JNI doesn't prevent it. The JDI spec >> forbids the setting of final fields, and it has a check that will throw an >> exception if attempted. However, it is important that we continue to allow >> JDWP to write final fields because some debuggers allow for it at the JDI >> level (protected by a click through warning). Although these debuggers are >> not fully JDI compliant, there is no requirement that they be compliant, so >> JDWP should continue to support this behavior. Much of this was discussed in >> the PR for [JDK-8281652](https://bugs.openjdk.org/browse/JDK-8281652), which >> clarified in the JDI spec that writing final fields was not allowed. This >> was just a spec clarification. JDI already forbid the setting of final >> fields. >> >> Having said all that, the JDWP spec for ClassType.SetValues says "Final >> fields cannot be set." This is clearly wrong as it has always been allowed. >> ObjectReference.SetValues says nothing about final fields, and also allows >> them to be set. >> >> This PR has two spec updates: >> - Get rid of the "Final fields cannot be set." text for ClassType.SetValues. >> - ObjectReference.SetValues and ReferenceType.SetValues should warn against >> setting final fields by using language similar to JNI. >> >> I've also updated two tests to test for setting both static final fields and >> instance final fields. The tests (before my changes) verified the setting of >> fields (not final) on the debuggee side. I tried the same with final fields >> and ran into problems. The compiler inlines final primitives values, so >> setting the final fields is not seen when the fields are referenced from >> java (or at least this was the case with static final fields. I'm not >> positive about instance final fields). So I added support for using >> GetValues to verify the results rather than relying on the debuggee's view >> of the fields. >> >> Testing in progress. >> >> --------- >> - [x] I confirm that I make this contribution in accordance with the >> [OpenJDK Interim AI Policy](https://openjdk.org/legal/ai). > > Chris Plummer has updated the pull request incrementally with one additional > commit since the last revision: > > fix spelling and incorrect logging This looks good in general. I've posted a couple of nits though. test/hotspot/jtreg/vmTestbase/nsk/jdwp/ClassType/SetValues/setvalues001.java line 482: > 480: } > 481: } > 482: log.display("Verfied using JDWP ClassType.Getvalues that all > static fields values have been correctly set"); Nit: Should the line 482 be executed in the success case only? test/hotspot/jtreg/vmTestbase/nsk/jdwp/ObjectReference/SetValues/setvalues001.java line 187: > 185: + " " + TESTED_FINAL_CLASS_SIGNATURE); > 186: long testedFinalClassID = > debugee.getReferenceTypeID(TESTED_FINAL_CLASS_SIGNATURE); > 187: log.display(" got tested final classID: " + > testedClassID); Q: Should it be `testedFinalClassID` instead of `testedClassID`? test/hotspot/jtreg/vmTestbase/nsk/jdwp/ObjectReference/SetValues/setvalues001.java line 568: > 566: } > 567: } > 568: log.display("Verfied using JDWP ObjectReference.Getvalues that > all fields values have been correctly set"); Nit: Should the line 568 be executed in the success case only? ------------- PR Review: https://git.openjdk.org/jdk/pull/32029#pullrequestreview-4998095049 PR Review Comment: https://git.openjdk.org/jdk/pull/32029#discussion_r3834319591 PR Review Comment: https://git.openjdk.org/jdk/pull/32029#discussion_r3834308763 PR Review Comment: https://git.openjdk.org/jdk/pull/32029#discussion_r3834326070
