On Sat, 22 Aug 2026 00:44:02 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: > > Only print success message if truely a success. Fix variable name in > message. Posted more nits. There are some typos there. test/hotspot/jtreg/vmTestbase/nsk/jdwp/ClassType/SetValues/setvalues001.java line 456: > 454: void checkJDWPValuesChanged(long testedClassID, long > testedFieldIDs[], > 455: JDWP.Value targetValues[]) { > 456: // verify that JDWP ClassType.Getvalues returns the expected > values Nit: Typo? : s/ClassType.Getvalues/ClassType.GetValues/ test/hotspot/jtreg/vmTestbase/nsk/jdwp/ClassType/SetValues/setvalues001.java line 458: > 456: // verify that JDWP ClassType.Getvalues returns the expected > values > 457: int count = targetValues.length; > 458: log.display("\n>>> getting field values using JDWP > ClassType.GetValues \n"); Nit: Start with a capital letter: "Getting field values..." for consistency with other messages like this. test/hotspot/jtreg/vmTestbase/nsk/jdwp/ClassType/SetValues/setvalues001.java line 469: > 467: log.display(" fieldID: " + testedFieldIDs[i]); > 468: > 469: JDWP.Value actuallValue = actualValues[i]; Nit: This looks like a typo: s/actuallValue/actualValue . test/hotspot/jtreg/vmTestbase/nsk/jdwp/ClassType/SetValues/setvalues001.java line 483: > 481: } > 482: if (success) { > 483: log.display("Verfied using JDWP ClassType.Getvalues that all > static fields values have been correctly set"); Nit: Typo? : s/ClassType.Getvalues/ClassType.GetValues/ test/hotspot/jtreg/vmTestbase/nsk/jdwp/ObjectReference/SetValues/setvalues001.java line 369: > 367: return values; > 368: } catch (BoundException e) { > 369: log.complain("Unable to parse reply packet for > ReferenceType.GetValues command:\n\t" Nit: Typo? : s/ReferenceType.GetValues/ObjectReference.GetValues/ test/hotspot/jtreg/vmTestbase/nsk/jdwp/ObjectReference/SetValues/setvalues001.java line 544: > 542: // verify that JDWP ObjectReference.Getvalues returns the > expected values > 543: int count = targetValues.length; > 544: log.display("\n>>> getting field values using JDWP > ObjectReference.GetValues \n"); Nit: Start with a capital letter: "Getting field values..." for consistency with other messages like this. test/hotspot/jtreg/vmTestbase/nsk/jdwp/ObjectReference/SetValues/setvalues001.java line 555: > 553: log.display(" fieldID: " + testedFieldIDs[i]); > 554: > 555: JDWP.Value actuallValue = actualValues[i]; Nit: This looks like a typo: s/actuallValue/actualValue . test/hotspot/jtreg/vmTestbase/nsk/jdwp/ObjectReference/SetValues/setvalues001.java line 569: > 567: } > 568: if (success) { > 569: log.display("Verfied using JDWP ObjectReference.Getvalues > that all fields values have been correctly set"); Nit: Typo? : s/ClassType.Getvalues/ClassType.GetValues/ test/hotspot/jtreg/vmTestbase/nsk/jdwp/ObjectReference/SetValues/setvalues001a.java line 357: > 355: // class with static fields with the tested objects > 356: public static class ObjectClass { > 357: // static field with the tested objects Nit: Now it needs a plural form: "_static fields_". ------------- PR Review: https://git.openjdk.org/jdk/pull/32029#pullrequestreview-4998427572 PR Review Comment: https://git.openjdk.org/jdk/pull/32029#discussion_r3834646297 PR Review Comment: https://git.openjdk.org/jdk/pull/32029#discussion_r3834671028 PR Review Comment: https://git.openjdk.org/jdk/pull/32029#discussion_r3834632380 PR Review Comment: https://git.openjdk.org/jdk/pull/32029#discussion_r3834647535 PR Review Comment: https://git.openjdk.org/jdk/pull/32029#discussion_r3834655600 PR Review Comment: https://git.openjdk.org/jdk/pull/32029#discussion_r3834671544 PR Review Comment: https://git.openjdk.org/jdk/pull/32029#discussion_r3834638088 PR Review Comment: https://git.openjdk.org/jdk/pull/32029#discussion_r3834650361 PR Review Comment: https://git.openjdk.org/jdk/pull/32029#discussion_r3834663140
