8306038: SystemModulesPlugin generates code that doesn't pop when return value not used

[koppor]

This refs 8306038

(Before this referenced 8240567)

Although this change is rather small, I think, it's good to have a "more clean" SystemModulesPlugin available.

---

Progress

• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• Change must be properly reviewed (2 reviews required, with at least 1 Reviewer, 1 Author)

Issue

• JDK-8306038: SystemModulesPlugin generates code that doesn't pop when return value not used

Reviewers

• Alan Bateman (@AlanBateman - Reviewer)
• Mandy Chung (@mlchung - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 13442

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

Using diff file

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

Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back koppor! 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.

[openjdk]

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

• core-libs

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

• 00: Full (dd2ae2ee)

[AlanBateman]

I think you've got the wrong JBS issue, JDK-8240567 is about changing the SystemModulesPlugin to avoid the 64k limit on method size.

[koppor]

I think you've got the wrong JBS issue, JDK-8240567 is about changing the SystemModulesPlugin to avoid the 64k limit on method size.

For me, it's the correct one. I was not sure which one to link. I can also drop the reference.

More background: I am currently trying to get #10704 finished and saw that improvement. Just in case, I don't manage to get that PR finished, I wanted to keep my small contribution alive. 😅

[AlanBateman]

For me, it's the correct one. I was not sure which one to link. I can also drop the reference.

I checked the generated code and it looks like the code generated to invoke builder's newRequires, newExports, ... is missing a pop as the return values aren't used. It seems to be benign for now but could bite. I've created JDK-8306038 to track it .

[koppor]

Thank you for creating the issue. - I added the pop at the "missing" places as there already wore pops at other places in that class.

[AlanBateman]

/reviewers 2

[openjdk]

@AlanBateman
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 1 Reviewer, 1 Author).

[AlanBateman]

This looks right. I assume you've checked java -XX:+UnlockDiagnosticVMOptions -XX:+BytecodeVerificationLocal -version as this code is not normally verified. Also I assume you've run tier2 with the jlink tests.

You will need to edit the PR so it matches the JBS title.

@mlchung @asotona, do you want to look at this too? I think it pre-dates the conversion to the class file API.

[AlanBateman]

This looks okay.

[mlchung]

This small change is fine.

[koppor]

This looks right. I assume you've checked java -XX:+UnlockDiagnosticVMOptions -XX:+BytecodeVerificationLocal -version as this code is not normally verified

koppor@DESKTOP-KAK953S /cygdrive/c/git-repositories/jdk/jdk/build/windows-x86_64-server-release/jdk/bin
$ ./java -XX:+UnlockDiagnosticVMOptions  -XX:+BytecodeVerificationLocal -version
openjdk version "21-internal" 2023-09-19
OpenJDK Runtime Environment (build 21-internal-adhoc.koppor.jdk)
OpenJDK 64-Bit Server VM (build 21-internal-adhoc.koppor.jdk, mixed mode)

Also I assume you've run tier2 with the jlink tests.

TBH, I assumed there is a CI in place checking tier2, too.

Locally, they do not run here. Example:

test ZipFileSystemTest.testInvalidRequestURIGET(): failure
java.lang.AssertionError: expected [134] but found [146]
        at org.testng.Assert.fail(Assert.java:99)
        at org.testng.Assert.failNotEquals(Assert.java:1037)
        at org.testng.Assert.assertEqualsImpl(Assert.java:140)
...
JavaTest Message: Test threw exception: java.lang.RuntimeException: javax.security.auth.login.LoginException: F?r NT sind keine LoginModules konfiguriert
JavaTest Message: shutting down test

I assume, I should run the test on a dedicated linux machine?

[mlchung]

or you can just run jlink test with this command:

make test TEST=test/jdk/tools/jlink

[mlchung]

I ran tier1-tier3 tests with your patch and all passed.

[AlanBateman]

I ran tier1-tier3 tests with your patch and all passed.

Good. @koppor You can edit the PR title to match the JBS issue, then enter "/integrate" and one of us will sponsor.

[koppor]

/integrate

[openjdk]

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

8306038: SystemModulesPlugin generates code that doesn't pop when return value not used

Reviewed-by: alanb, mchung

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

• c738c8e: 8306278: jvmtiAgentList.cpp:253 assert(offset >= 0) failed: invariant occurs on AIX after JDK-8257967
• 9fb53ad: 8305716: Enhancements for printing age tables
• ebba42a: 8305993: Add handleSocketErrorWithMessage to extend nio Net.c exception message
• 42b7260: 8306111: PPC64: RT call after thaw with exception requires larger ABI section
• c7faf60: 8305757: Call Method::compute_has_loops_flag() when creating CDS archive
• eb8d8cd: 8299129: Enhance NameService lookups
• 77df315: 8296692: Improve String platform support
• f098b49: 8298310: Enhance TLS session negotiation
• 9e56d10: 8296832: Improve Swing platform support
• 2e5700a: 8288436: Improve Xalan supports
• ... and 74 more: https://git.openjdk.org/jdk/compare/d8af7a6014055295355a1242db6c2872299c6398...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.

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 (@AlanBateman, @mlchung) 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).

[openjdk]

@koppor
Your change (at version dd2ae2e) is now ready to be sponsored by a Committer.

[mlchung]

/sponsor

[openjdk]

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

• a31a11f: 8306006: strace001.java fails due to unknown methods on stack
• ddb8646: 8306123: Move InstanceKlass writeable flags
• 1a41e12: 8306310: Move is_shared Klass flag
• c738c8e: 8306278: jvmtiAgentList.cpp:253 assert(offset >= 0) failed: invariant occurs on AIX after JDK-8257967
• 9fb53ad: 8305716: Enhancements for printing age tables
• ebba42a: 8305993: Add handleSocketErrorWithMessage to extend nio Net.c exception message
• 42b7260: 8306111: PPC64: RT call after thaw with exception requires larger ABI section
• c7faf60: 8305757: Call Method::compute_has_loops_flag() when creating CDS archive
• eb8d8cd: 8299129: Enhance NameService lookups
• 77df315: 8296692: Improve String platform support
• ... and 77 more: https://git.openjdk.org/jdk/compare/d8af7a6014055295355a1242db6c2872299c6398...master

Your commit was automatically rebased without conflicts.

[openjdk]

@mlchung @koppor Pushed as commit c57af31.

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