8294969: Convert jdk.jdeps javap to use the Classfile API #11411

asotona · 2022-11-29T10:26:31Z

javap uses proprietary com.sun.tools.classfile library to parse class files.

This patch converts javap to use 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-8294969: Convert jdk.jdeps javap to use the Classfile API (Sub-task - P4)

Reviewers

Vicente Romero (@vicente-romero-oracle - Reviewer) ⚠️ Review applies to ae3b25bf

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 11411

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

Using diff file

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

Webrev

Link to Webrev Comment

…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

…ass cast and throw ConstantPoolException instead

fix of javap ConstantWriter to print DynamicConstantPoolEntry without accessing BSM attribute

asotona · 2023-07-27T17:20:41Z

Important fixes have been integrated and all tests are now passing.

Please review.

Thanks,
Adam

bridgekeeper · 2023-08-24T23:46:28Z

@asotona This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

asotona · 2023-08-28T08:26:25Z

keep open

vicente-romero-oracle · 2023-09-06T19:53:12Z

src/jdk.jdeps/share/classes/com/sun/tools/javap/TypeAnnotationWriter.java

            }
        }
    }

    private AnnotationWriter annotationWriter;
    private ClassWriter classWriter;
    private Map<Integer, List<Note>> pcMap;
+    private CodeAttribute lr;


nit: name lr doesn't relate much to the type of the field

Right, lr historically came from label resolver, I'll fix it.

vicente-romero-oracle · 2023-09-06T19:55:49Z

test/langtools/tools/javap/8260403/T8260403.java

+        int res = com.sun.tools.javap.Main.run(
+                new String[]{"-c", System.getProperty("test.classes") + "/InvalidSignature.class"},
+                new PrintWriter(sw));
+        System.out.println(sw);


is this for debug purpose only?

It is the test fix.
8260403 says "javap should be more robust", however to expect zero exit code for invalid class is wrong.
It suppose to expect non-zero exit code and fail only on fatal errors or unhandled CP errors.

vicente-romero-oracle · 2023-09-06T19:57:19Z

test/langtools/tools/javap/malformed/MalformedTest.java

@@ -0,0 +1,50 @@
+/*
+ * Copyright (c) 2023, Oracle and/or its affiliates. All rights reserved.


isn't this test testing the same as: test/langtools/tools/javap/8260403/T8260403.java?

The principle of the test is the same, but it tests intentionally different class malformations (on a different level).

vicente-romero-oracle · 2023-09-06T21:10:07Z

src/jdk.jdeps/share/classes/com/sun/tools/javap/CodeWriter.java

-            if (!method.access_flags.is(AccessFlags.ACC_STATIC))
-                ++n;  // for 'this'
-            argCount = Integer.toString(n);
-        } catch (ConstantPoolException e) {


nice clean-up

vicente-romero-oracle · 2023-09-06T23:08:48Z

src/jdk.jdeps/share/classes/com/sun/tools/javap/LocalVariableTableWriter.java

@@ -50,19 +46,21 @@
 public class LocalVariableTableWriter extends  InstructionDetailWriter {


nit: two spaces between extends and InstructionDetailWriter

fixed, thanks

vicente-romero-oracle · 2023-09-07T03:10:33Z

src/java.base/share/classes/jdk/internal/classfile/ClassReader.java

+    FloatEntry readFloatEntry(int offset);
+
+    /**
+     * {@return the field ref entry whose index is given at the specified


I think that ref in these APIs should be reference

fixed, thanks

vicente-romero-oracle · 2023-09-07T04:28:22Z

src/jdk.jdeps/share/classes/com/sun/tools/javap/AttributeWriter.java

+                println("ModuleTarget:");
+                indent(+1);
+                print("target_platform: #" + attr.targetPlatform().index());
+//ToDo find the spec - can be null???


what about this comment?

removed according to the available spec information

vicente-romero-oracle · 2023-09-07T04:31:26Z

src/jdk.jdeps/share/classes/com/sun/tools/javap/AttributeWriter.java

+                println("SourceFile: \"" + attr.sourceFile().stringValue() + "\"");
+            case SourceIDAttribute attr ->
+                constantWriter.write(attr.sourceId().index());
+//            case StackMapAttribute ???


I guess this comment can be removed

removed, thanks

briangoetz · 2023-09-14T16:38:36Z

src/java.base/share/classes/jdk/internal/classfile/ClassReader.java

+     *         a constant value entry
+     */
+    ConstantValueEntry readConstantValueEntry(int offset);
+


I'm not sure that all this additional API surface carries its weight in ClassReader, when each of these methods just called readEntry() and casts to the desired type? I can see two alternate ways here:

Ordinary casts at the use site (worse error handling, but ultimately the same result), or

A single method

T readEntry(int pos, Class t)

We need at least <T extends PoolEntry> T readEntry(int pos, Class<T> t) to throw ConstantPoolException instead of ClassCastException from broken entries.

# Conflicts: # src/java.base/share/classes/jdk/internal/classfile/impl/ClassReaderImpl.java

vicente-romero-oracle

looks good to me

openjdk · 2023-09-15T14:43:12Z

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

8294969: Convert jdk.jdeps javap to use the Classfile API

Reviewed-by: vromero

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

8dfde28: 8315485: (fs) Move java/nio/file/Path/Misc.java tests into java/nio/file/Path/PathOps.java
3c743cf: 8312126: NullPointerException in CertStore.getCRLs after 8297955
bd26813: 8316295: Serial: Remove empty Generation::promotion_failure_occurred
42dc6b5: 8316182: RISC-V: SA stack walking code having trouble finding sender frame when invoking LambdaForms is involved
31c4e03: 8314551: More generic way to handshake GC threads with monitor deflation
89cb290: 8312165: Fix typos in java.desktop Swing
25f32f3: 8316160: Remove sun.misc.Unsafe.{shouldBeInitialized,ensureClassInitialized}
86dde5e: 8316001: GC: Make TestArrayAllocatorMallocLimit use createTestJvm
4f864fa: 8314136: Test java/net/httpclient/CancelRequestTest.java failed: WARNING: tracker for HttpClientImpl(42) has outstanding operations
4070829: 8315931: RISC-V: xxxMaxVectorTestsSmokeTest fails when using RVV
... and 10 more: https://git.openjdk.org/jdk/compare/b2e91060db82a13e993227f538c8d54b41a9796b...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.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

asotona · 2023-09-18T08:34:32Z

/integrate

openjdk · 2023-09-18T08:35:35Z

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

fbc766e: 8315988: Parallel: Make TestAggressiveHeap use createTestJvm
aa0ebee: 8316341: sun/security/pkcs11/PKCS11Test.java needs adjustment on Linux ppc64le Ubuntu 22
ca3fe7b: 8315932: runtime/InvocationTests spend a lot of time on dependency verification
f440329: 8316391: (zipfs) ZipFileSystem.readFullyAt does not tolerate short reads
4b8f5d0: 8316273: JDK-8315818 broke JVMCIPrintProperties on libgraal
e624198: 8316387: Exclude more failing multicast tests on AIX after JDK-8315651
c92bdb0: 8316187: Modernize examples in StringTokenizer and {Date,Number}Format
8f46abc: 8315889: Open source several Swing HTMLDocument related tests
0050447: 8316148: Remove sun/tools/jhsdb/JStackStressTest.java from problem list
149acd1: 8316207: Fix typos in java.io.StreamTokenizer
... and 21 more: https://git.openjdk.org/jdk/compare/b2e91060db82a13e993227f538c8d54b41a9796b...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-09-18T08:35:46Z

@asotona Pushed as commit 1203e11.

💡 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 5 commits June 30, 2023 11:51

extended ClassReader about specific entry-reading methods to avoid cl…

28ee5f3

…ass cast and throw ConstantPoolException instead

added DydnamicConstantPoolEntry::bootstrapMethodIndex

834de48

fix of javap ConstantWriter to print DynamicConstantPoolEntry without accessing BSM attribute

fixed code printing and ConstantPoolException reporting indoex

eb9d864

Merge branch 'master' into JDK-8294969-javap

b63758d

Merge branch 'master' into JDK-8294969-javap

4960751

vicente-romero-oracle reviewed Sep 6, 2023

View reviewed changes

asotona added 2 commits September 7, 2023 10:03

Merge branch 'master' into JDK-8294969

df63b5e

minor patches

697079f

vicente-romero-oracle reviewed Sep 7, 2023

View reviewed changes

applied suggested changes from review

27ff582

briangoetz reviewed Sep 14, 2023

View reviewed changes

asotona added 4 commits September 14, 2023 20:56

Merge branch 'master' into JDK-8294969

bac5817

# Conflicts: # src/java.base/share/classes/jdk/internal/classfile/impl/ClassReaderImpl.java

fixed ConstantWriter

e8cb15b

reducing ClassReader API footprint

938571f

fixed TestClassNameWarning test

ae3b25b

vicente-romero-oracle approved these changes Sep 15, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Sep 15, 2023

adding serviceability/sa/ClhsdbDumpclass.java test to problem list

3ae33d4

openjdk bot added the integrated Pull request has been integrated label Sep 18, 2023

openjdk bot closed this Sep 18, 2023

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

		@@ -0,0 +1,50 @@
		/*
		* Copyright (c) 2023, Oracle and/or its affiliates. All rights reserved.

		@@ -50,19 +46,21 @@
		public class LocalVariableTableWriter extends InstructionDetailWriter {

8294969: Convert jdk.jdeps javap to use the Classfile API #11411

8294969: Convert jdk.jdeps javap to use the Classfile API #11411

Uh oh!

Conversation

asotona commented Nov 29, 2022 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Webrev

Uh oh!

asotona commented Jul 27, 2023

Uh oh!

bridgekeeper bot commented Aug 24, 2023

Uh oh!

asotona commented Aug 28, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vicente-romero-oracle left a comment

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

asotona commented Sep 18, 2023

Uh oh!

openjdk bot commented Sep 18, 2023

Uh oh!

openjdk bot commented Sep 18, 2023

Uh oh!

Uh oh!

asotona commented Nov 29, 2022 •

edited by openjdk bot

Loading

openjdk bot commented Sep 15, 2023 •

edited

Loading