8343846: [lworld] implement spec changes to stack map tables #1373

matias9927 · 2025-02-21T20:10:39Z

This patch implements the spec changes needed to realize the ACC_STRICT flag. See the JBS issue for more details.

The patch is divided across 3 major commits:

VM changes for asserting that strict fields are initialized before the super constructor is called
Javac changes for generating the stack map table entries
Test

Verified with tier 1-3 tests

Progress

Change must not contain extraneous whitespace

Issue

JDK-8343846: [lworld] implement spec changes to stack map tables (Enhancement - P4)

Reviewers

Coleen Phillimore (@coleenp - Committer)
Frederic Parain (@fparain - Committer)
Chen Liang (@liach - no project role)

Contributors

Vicente Romero <vromero@openjdk.org>

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1373

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/valhalla/pull/1373.diff

Using Webrev

Link to Webrev Comment

bridgekeeper · 2025-02-21T20:11:26Z

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

openjdk · 2025-02-21T20:11:36Z

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

8343846: [lworld] implement spec changes to stack map tables

Co-authored-by: Vicente Romero <vromero@openjdk.org>
Reviewed-by: coleenp, fparain, liach

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 331 new commits pushed to the lworld branch:

ad75b32: 8351037: [lworld] Crashing in CodeBlob::print_value_on()
b22815a: 8350961: [lworld] Some VarHandles tests fail when field flattening is not applied
6761b57: 8350872: [lworld] Field is flattened although -XX:-UseFieldFlattening is set
b8652c4: Merge jdk
78c18cf: 8349399: GHA: Add static-jdk build on linux-x64
e43960a: 8350616: Skip ValidateHazardPtrsClosure in non-debug builds
9ec4696: 8350313: Include timings for leaving safepoint in safepoint logging
ec6624b: 8350649: Class unloading accesses/resurrects dead Java mirror after JDK-8346567
9477c70: 8024695: new File("").exists() returns false whereas it is the current working directory
3e46480: 8350770: [BACKOUT] Protection zone for easier detection of accidental zero-nKlass use
... and 321 more: https://git.openjdk.org/valhalla/compare/0bcf2675a5d68b9316c49b1cad3cffe7b0df022c...lworld

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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@coleenp, @fparain) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

matias9927 · 2025-02-21T20:35:18Z

/contributor add @vicente-romero-oracle

openjdk · 2025-02-21T20:35:44Z

@matias9927
Contributor Vicente Romero <vromero@openjdk.org> successfully added.

liach · 2025-02-21T20:43:05Z

Do we require complete ClassFile API support for the spec change immediately? Currently ClassFile support is reading-only, still need some tweaks to support writing the correct assert_unset_fields (either through user-specified or through auto generation)

mlbridge · 2025-02-21T21:24:51Z

Webrevs

vicente-romero-oracle · 2025-02-21T21:39:40Z

Do we require complete ClassFile API support for the spec change immediately? Currently ClassFile support is reading-only, still need some tweaks to support writing the correct assert_unset_fields (either through user-specified or through auto generation)

it would be a nice to have I think but I don't think it is a must as part of this PR. This is still an experimental feature and we don't know what the final version will be, it could even be possible for this feature to be dropped, so

liach · 2025-02-21T21:41:26Z

Sure, we can always add them later.

fparain · 2025-02-25T20:17:27Z

src/hotspot/share/classfile/verifier.cpp

+          if (fd.access_flags().is_strict()) {
+            ResourceMark rm(THREAD);
+            if (!current_frame->satisfy_unset_field(fd.name(), fd.signature())) {
+              log_info(verification)("Attempting to initialize field not found in initial stict instance fields: %s%s",


typo: stict -> strict

fparain · 2025-02-27T13:48:19Z

src/hotspot/share/classfile/stackMapTable.cpp

+    } else if (new_fields->number_of_entries() > 0) {
+      _prev_frame->verifier()->verify_error(
+        ErrorContext::bad_strict_fields(_prev_frame->offset(), _prev_frame),
+        "Cannot have uninitialzied strict fields after class initialization");


typo: uninitialzied -> uninitialized

coleenp

This looks really good! I have a few comment and name suggestions, and was wondering if your Hashtable could be <NameAndSig, bool> where satisfied is the value?

src/hotspot/share/classfile/stackMapFrame.cpp

src/hotspot/share/classfile/stackMapFrame.hpp

src/hotspot/share/classfile/stackMapTable.cpp

src/hotspot/share/classfile/verifier.cpp

coleenp

This looks really good, and the design fits very nicely with the work of the verifier in verifying stackmap tables. I reviewed the Hotspot changes.

fparain

LGTM. Nice integration with the verifier code.

liach

The language and tooling changes look fine.

matias9927 · 2025-03-03T15:07:06Z

Thank you for the discussions and review @coleenp @fparain and @liach! Also thank you @vicente-romero-oracle for the javac component!
/integrate

openjdk · 2025-03-03T15:07:50Z

@matias9927
Your change (at version 728753b) is now ready to be sponsored by a Committer.

fparain · 2025-03-03T15:11:18Z

/sponsor

openjdk · 2025-03-03T15:11:37Z

@fparain Only the author (@matias9927) is allowed to issue the integrate command. As this pull request is ready to be sponsored, and you are an eligible sponsor, did you mean to issue the /sponsor command?

openjdk · 2025-03-03T15:12:06Z

Going to push as commit 7a1ff29.
Since your change was applied there have been 331 commits pushed to the lworld branch:

ad75b32: 8351037: [lworld] Crashing in CodeBlob::print_value_on()
b22815a: 8350961: [lworld] Some VarHandles tests fail when field flattening is not applied
6761b57: 8350872: [lworld] Field is flattened although -XX:-UseFieldFlattening is set
b8652c4: Merge jdk
78c18cf: 8349399: GHA: Add static-jdk build on linux-x64
e43960a: 8350616: Skip ValidateHazardPtrsClosure in non-debug builds
9ec4696: 8350313: Include timings for leaving safepoint in safepoint logging
ec6624b: 8350649: Class unloading accesses/resurrects dead Java mirror after JDK-8346567
9477c70: 8024695: new File("").exists() returns false whereas it is the current working directory
3e46480: 8350770: [BACKOUT] Protection zone for easier detection of accidental zero-nKlass use
... and 321 more: https://git.openjdk.org/valhalla/compare/0bcf2675a5d68b9316c49b1cad3cffe7b0df022c...lworld

Your commit was automatically rebased without conflicts.

openjdk · 2025-03-03T15:12:18Z

@fparain @matias9927 Pushed as commit 7a1ff29.

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

matias9927 added 3 commits February 21, 2025 19:21

Implemented VM changes to handle strict instance fields

0016a44

Implemented javac changes

ae94e57

Added test

Loading
Loading status checks…

a3ec5e8

matias9927 marked this pull request as ready for review February 21, 2025 21:20

Fixed build failures and changed test

Loading
Loading status checks…

77f8a1f

openjdk bot added ready rfr labels Feb 21, 2025

fparain reviewed Feb 25, 2025

View reviewed changes

fparain reviewed Feb 27, 2025

View reviewed changes

Typo corrections and method renaming

Loading
Loading status checks…

040d3ea

coleenp suggested changes Feb 27, 2025

View reviewed changes

matias9927 added 2 commits February 28, 2025 19:38

Coleen suggestions

19f1ed6

Changed AssertUnsetFieldsTable to NameAndSig to bool

Loading
Loading status checks…

728753b

coleenp approved these changes Feb 28, 2025

View reviewed changes

fparain approved these changes Feb 28, 2025

View reviewed changes

liach approved these changes Feb 28, 2025

View reviewed changes

openjdk bot added the sponsor label Mar 3, 2025

openjdk bot added the integrated label Mar 3, 2025

openjdk bot closed this Mar 3, 2025

openjdk bot removed ready rfr sponsor labels Mar 3, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8343846: [lworld] implement spec changes to stack map tables #1373

8343846: [lworld] implement spec changes to stack map tables #1373

matias9927 commented Feb 21, 2025 •

edited by openjdk bot

Loading

bridgekeeper bot commented Feb 21, 2025

openjdk bot commented Feb 21, 2025 •

edited

Loading

matias9927 commented Feb 21, 2025

openjdk bot commented Feb 21, 2025

liach commented Feb 21, 2025

mlbridge bot commented Feb 21, 2025 •

edited

Loading

vicente-romero-oracle commented Feb 21, 2025

liach commented Feb 21, 2025

fparain Feb 25, 2025

fparain Feb 27, 2025

coleenp left a comment

coleenp left a comment •

edited

Loading

fparain left a comment

liach left a comment

matias9927 commented Mar 3, 2025

openjdk bot commented Mar 3, 2025

fparain commented Mar 3, 2025

openjdk bot commented Mar 3, 2025

openjdk bot commented Mar 3, 2025

openjdk bot commented Mar 3, 2025

8343846: [lworld] implement spec changes to stack map tables #1373

8343846: [lworld] implement spec changes to stack map tables #1373

Conversation

matias9927 commented Feb 21, 2025 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Contributors

Reviewing

bridgekeeper bot commented Feb 21, 2025

openjdk bot commented Feb 21, 2025 • edited Loading

matias9927 commented Feb 21, 2025

openjdk bot commented Feb 21, 2025

liach commented Feb 21, 2025

mlbridge bot commented Feb 21, 2025 • edited Loading

Webrevs

vicente-romero-oracle commented Feb 21, 2025

liach commented Feb 21, 2025

fparain Feb 25, 2025

Choose a reason for hiding this comment

fparain Feb 27, 2025

Choose a reason for hiding this comment

coleenp left a comment

Choose a reason for hiding this comment

coleenp left a comment • edited Loading

Choose a reason for hiding this comment

fparain left a comment

Choose a reason for hiding this comment

liach left a comment

Choose a reason for hiding this comment

matias9927 commented Mar 3, 2025

openjdk bot commented Mar 3, 2025

fparain commented Mar 3, 2025

openjdk bot commented Mar 3, 2025

openjdk bot commented Mar 3, 2025

openjdk bot commented Mar 3, 2025

matias9927 commented Feb 21, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Feb 21, 2025 •

edited

Loading

mlbridge bot commented Feb 21, 2025 •

edited

Loading

coleenp left a comment •

edited

Loading