8294972: Convert jdk.jlink internal plugins to use the Classfile API #12944

asotona · 2023-03-09T11:35:32Z

jdk.jlink internal plugins are heavily using ASM

This patch converts ASM calls to Classfile API.

Please review.
Thanks,
Adam

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

Issue

JDK-8294972: Convert jdk.jlink internal plugins to use the Classfile API

Reviewers

Mandy Chung (@mlchung - Reviewer) ⚠️ Review applies to b021d852
Alan Bateman (@AlanBateman - Reviewer) ⚠️ Review applies to 993ef398

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 12944

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

Using diff file

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

…e and added test (openjdk#11)

…lang.reflect.AccesFlag in Classfile API and tests

…andleInfo implemented RebuildingTransformation and added to Transforms and CorpusTest reduced CorpusTestHelper output and adjusted TEST.properties

MethodParameterInfo name parameter changed to Optional added MethodParameterInfo::ofParameter(Optional<String>,int) implemented TemporaryConstantPool::stringEntry adjusted BoundAttribute and RebuildingTransformation test helper

EnclosingMethodAttribute factory method changed to accept Optionals added EnclosingMethodAttribute::of(ClassDesc,Optional<String>,Optional<MethodTypeDesc>) added EnclosingMethodAttribute accessor methods InnerClassInfo all factory methods changed to accept Optionals added NestHostAttribute::of(ClassDesc) added SourceIDAttribute::of(String) changes reflected in BoundAttribute and RebuildTransformation test helper

* added TypeAnnotation factory methods accepting ClassDesc and AnnotationElement... AnnotationValue.OfConstant sub-classed to allow switch pattern matching RebuildingTransformation test helper adjusted * added TypeAnnotation.TargetInfo factory methods with validity checking for multi-target types adjusted RebuildTransformation test helper

…Symbol (openjdk#13) refactored to FieldModel::fieldTypeSymbol and MethodModel::methodTypeSymbol (openjdk#13) added round testing of signatures in RebuildTransformation test helper

… a frame type. Doing so, make the chop size available to consumers of frames.

…eInstructions

…ptor argument slots

…ine-tunning

asotona · 2023-03-16T08:01:52Z

What was the issue with the previous revision?
The only conversion bug I found during the code reformat/revision was missing className.replace('/', '.') and after that the test pass.

jdk/src/jdk.jlink/share/classes/jdk/tools/jlink/internal/plugins/SystemModulesPlugin.java

Line 1746 in 49c80bc

.constantInstruction(className.replace('/', '.'))

mlchung

Looks good. I reviewed the changes in all plugins except IncludeLocalePlugin and will re-review it once you restore to the previous version.

src/jdk.jlink/share/classes/jdk/tools/jlink/internal/plugins/SystemModulesPlugin.java

src/jdk.jlink/share/classes/jdk/tools/jlink/internal/plugins/IncludeLocalesPlugin.java

mlchung

Looks good.

openjdk · 2023-03-20T15:12:40Z

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

8294972: Convert jdk.jlink internal plugins to use the Classfile API

Reviewed-by: mchung, alanb

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

0156909: 8304502: Classfile API class hierarchy makes assumptions when class is not resolved
0deb648: 8290200: com/sun/jdi/InvokeHangTest.java fails with "Debuggee appears to be hung"
019fcd8: 8304139: Add and method constants to ConstantDescs
d6f20e2: 8304680: Problemlist compiler/sharedstubs/SharedStubToInterpTest.java
d788a1b: 8304180: Constant Descriptors for MethodHandles::classData and classDataAt
bbde215: 8299494: Test vmTestbase/nsk/stress/except/except011.java failed: ExceptionInInitializerError: target class not found
1c04686: 8304387: Fix positions of shared static stubs / trampolines
c65bb2c: 8304334: java/awt/color/ICC_ColorSpace/ToFromCIEXYZRoundTrip.java times out on slow platforms
4bf1fbb: 8303648: Add String.indexOf(String str, int beginIndex, int endIndex)

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
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.

src/jdk.jlink/share/classes/jdk/tools/jlink/internal/plugins/SystemModulesPlugin.java

AlanBateman

Good work

AlanBateman · 2023-03-20T19:53:29Z

src/jdk.jlink/share/classes/jdk/tools/jlink/internal/plugins/SystemModulesPlugin.java

+                                  "<init>",
+                                  MethodTypeDesc.of(CD_void, CD_String))
+                   .astore(BUILDER_VAR)
+                   .aload(BUILDER_VAR);


The methods to generate the MD all load the Builder onto the operand stack so the aload at L1021 is catching my attention, is that needed?

It seems to be redundant as the following code does not expect it on stack and loads it again as needed.

Yes, I think so. If java -XX:+UnlockDiagnosticVMOptions -XX:+BytecodeVerificationLocal -version doesn't fail and you've run all the tests then it should be okay to drop it.

I can confirm java -XX:+UnlockDiagnosticVMOptions -XX:+BytecodeVerificationLocal -version successful execution and passing all jdk/tools/jlink tests. Now waiting for all tier1 tests results.

all tier1, 2, and 3 tests passed

# Conflicts: # src/jdk.jlink/share/classes/jdk/tools/jlink/internal/plugins/SystemModulesPlugin.java

src/jdk.jlink/share/classes/jdk/tools/jlink/internal/plugins/IncludeLocalesPlugin.java

src/jdk.jlink/share/classes/jdk/tools/jlink/internal/plugins/SystemModulesPlugin.java

Co-authored-by: ExE Boss <3889017+ExE-Boss@users.noreply.github.com>

asotona · 2023-03-22T12:10:33Z

/integrate

openjdk · 2023-03-22T12:12:27Z

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

c74507e: 8304657: G1: Rename set_state_empty to set_state_untracked
ca94287: 8304144: G1: Remove unnecessary is_survivor check in G1ClearCardTableTask
eda0065: 8304286: java/net/SocketOption/OptionsTest.java failing after JDK-8302659
c039d26: 8303804: Fix some errors of If-VectorTest and CMove-VectorTest
0156909: 8304502: Classfile API class hierarchy makes assumptions when class is not resolved
0deb648: 8290200: com/sun/jdi/InvokeHangTest.java fails with "Debuggee appears to be hung"
019fcd8: 8304139: Add and method constants to ConstantDescs
d6f20e2: 8304680: Problemlist compiler/sharedstubs/SharedStubToInterpTest.java
d788a1b: 8304180: Constant Descriptors for MethodHandles::classData and classDataAt
bbde215: 8299494: Test vmTestbase/nsk/stress/except/except011.java failed: ExceptionInInitializerError: target class not found
... and 3 more: https://git.openjdk.org/jdk/compare/c4df9b5f176672617f29bd253f01df2ea81dac36...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-03-22T12:12:49Z

@asotona Pushed as commit 358c61b.

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

asotona and others added 30 commits June 10, 2022 14:20

Initial commit of Classfile Processing API

34c6057

fix of javadoc link

6663f05

fixed ClassPrinterTest on Windows

63d0e3b

Update README.md

02bf8b4

fixed dead code patching in StackMapGenerator::removeRangeFromExcTabl…

e1386df

…e and added test (openjdk#11)

Tweak JEP link

0daac4a

Merge master branch into classfile-api-branch

a5a2580

removal of jdk.classfile.jdktypes.AccessFlag and redirection to java.…

b5267ad

…lang.reflect.AccesFlag in Classfile API and tests

Classfile API javadoc build fix

941dfb0

fixed InvokeDynamicEntry::asSymbol and BytecodeHelpers::handleDescToH…

0fe61c5

…andleInfo implemented RebuildingTransformation and added to Transforms and CorpusTest reduced CorpusTestHelper output and adjusted TEST.properties

added ConstantValueAttribute::of(ConstantDesc)

a89d822

MethodParameterInfo name parameter changed to Optional added MethodParameterInfo::ofParameter(Optional<String>,int) implemented TemporaryConstantPool::stringEntry adjusted BoundAttribute and RebuildingTransformation test helper

refactored to FieldModel::fieldTypeSymbol and MethodModel::methodType…

d8f30a7

…Symbol (openjdk#13) refactored to FieldModel::fieldTypeSymbol and MethodModel::methodTypeSymbol (openjdk#13) added round testing of signatures in RebuildTransformation test helper

Model stack map frames in a type hierarchy where each type represents…

59479ff

… a frame type. Doing so, make the chop size available to consumers of frames.

Merge branch 'master' into classfile-api-branch

28f560e

Added some testing of local variable management, with BlockBuilder

99a17b3

test/jdk/jdk/classfile/examples fixed

f2ac770

Fixed frame offset valid boundaries in StackMapGenerator + added test

8480163

Adds toString methods to all classes that appear within a CodeModel.

ace0f43

Update README.md

a98f860

new JMH micro benchmark focused on clean StackMapGenerator throughput

4b69c9c

PerformancePatch - avoid ArrayList in StackMapGenerator::processInvok…

0631349

…eInstructions

StackMapGenerator code cleanup

1950b1f

PerformancePatch - StackMapGenerator faster counting of method descri…

e14f3a5

…ptor argument slots

PerformancePatch - StackMapGenerator more effective loops and other f…

5cbe442

…ine-tunning

CodeBuilder and ClassPrinterImpl minor fixes

a1502e3

update of AdvancedTransformationsTest::instrument

4b5620e

CodeBuilder fixes and increased test coverage

0c80bff

Classfile try catch

4c730ed

asotona added 2 commits March 16, 2023 11:16

implementation of custom ResourceHelper in IncludeLocalesPlugin

ee508f7

remaining cleanup in SystemModulesPlugin

3dbe1e5

mlchung reviewed Mar 16, 2023

View reviewed changes

src/jdk.jlink/share/classes/jdk/tools/jlink/internal/plugins/SystemModulesPlugin.java Outdated Show resolved Hide resolved

src/jdk.jlink/share/classes/jdk/tools/jlink/internal/plugins/SystemModulesPlugin.java Outdated Show resolved Hide resolved

asotona added 4 commits March 17, 2023 12:27

Revert "implementation of custom ResourceHelper in IncludeLocalesPlugin"

c784e62

SystemModulesPlugin::getClassBytes rename

fc98d7c

SystemModulesPlugin::genClassBytes rename

f01475a

Merge branch 'master' into JDK-8294972-jlink-plugins

4e5b965

mlchung reviewed Mar 17, 2023

View reviewed changes

src/jdk.jlink/share/classes/jdk/tools/jlink/internal/plugins/IncludeLocalesPlugin.java Show resolved Hide resolved

added default to thrown for unknown CP tag in IncludeLocalesPlugin

b021d85

mlchung approved these changes Mar 20, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Mar 20, 2023

AlanBateman reviewed Mar 20, 2023

View reviewed changes

src/jdk.jlink/share/classes/jdk/tools/jlink/internal/plugins/SystemModulesPlugin.java Show resolved Hide resolved

AlanBateman reviewed Mar 20, 2023

View reviewed changes

src/jdk.jlink/share/classes/jdk/tools/jlink/internal/plugins/SystemModulesPlugin.java Outdated Show resolved Hide resolved

fixed SystemModulesClassGenerator.moduleInfos comment

993ef39

AlanBateman approved these changes Mar 20, 2023

View reviewed changes

AlanBateman reviewed Mar 20, 2023

View reviewed changes

asotona added 2 commits March 21, 2023 09:57

Merge branch 'master' into JDK-8294972-jlink-plugins

71ed9a4

# Conflicts: # src/jdk.jlink/share/classes/jdk/tools/jlink/internal/plugins/SystemModulesPlugin.java

removed obsolete instruction from SystemModulesPlugin

3c12e88

ExE-Boss reviewed Mar 22, 2023

View reviewed changes

asotona and others added 2 commits March 22, 2023 07:10

Apply suggestions from code review

c1d5a77

Co-authored-by: ExE Boss <3889017+ExE-Boss@users.noreply.github.com>

improved exception message as suggested

2af22a2

openjdk bot added the integrated Pull request has been integrated label Mar 22, 2023

openjdk bot closed this Mar 22, 2023

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Mar 22, 2023

8294972: Convert jdk.jlink internal plugins to use the Classfile API #12944

8294972: Convert jdk.jlink internal plugins to use the Classfile API #12944

Uh oh!

Conversation

asotona commented Mar 9, 2023 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

asotona commented Mar 16, 2023

Uh oh!

mlchung left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mlchung left a comment

Choose a reason for hiding this comment

Uh oh!

openjdk bot commented Mar 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AlanBateman left a comment

Choose a reason for hiding this comment

Uh oh!

AlanBateman Mar 20, 2023

Choose a reason for hiding this comment

Uh oh!

asotona Mar 21, 2023

Choose a reason for hiding this comment

Uh oh!

AlanBateman Mar 21, 2023

Choose a reason for hiding this comment

Uh oh!

asotona Mar 21, 2023

Choose a reason for hiding this comment

Uh oh!

asotona Mar 22, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

asotona commented Mar 22, 2023

Uh oh!

openjdk bot commented Mar 22, 2023

Uh oh!

openjdk bot commented Mar 22, 2023

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

18 participants

asotona commented Mar 9, 2023 •

edited by openjdk bot

Loading

mlchung left a comment •

edited

Loading

openjdk bot commented Mar 20, 2023 •

edited

Loading