8304846: Provide a shared utility to dump generated classes defined via Lookup API #13182

mlchung · 2023-03-24T20:41:41Z

This implements a shared utility to dump generated classes defined as normal/hidden classes via Lookup API. This replaces the implementation in LambdaMetaFactory and method handle implementation that dumps the hidden class bytes on disk for debugging.

For classes defined via Lookup::defineClass, Lookup::defineHiddenClass and Lookup::defineHiddenClassWithClassData, by default they will be dumped to the path specified in -Djava.lang.invoke.Lookup.dumpClasses=<dumpDir>

The hidden classes generated for lambdas, LambdaForms and method handle implementation use non-default dumper so that they can be controlled via a separate system property and path as in the current implementation.

To dump lambda proxy classes, set this system property:
-Djdk.internal.lambda.dumpProxyClasses=

To dump LambdaForms and method handle implementation, set this system property:
-Djava.lang.invoke.MethodHandle.DUMP_CLASS_FILES=true

P.S. ProxyClassesDumper is renamed to ClassFileDumper but for some reason, it's not shown as rename.

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-8304846: Provide a shared utility to dump generated classes defined via Lookup API

Reviewers

Jorn Vernee (@JornVernee - Reviewer) ⚠️ Review applies to 2eeb7b04

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 13182

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

Using diff file

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

Webrev

Link to Webrev Comment

…ia Lookup API

bridgekeeper · 2023-03-24T20:42:59Z

👋 Welcome back mchung! 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 · 2023-03-24T20:45:28Z

@mlchung The following labels will be automatically applied to this pull request:

core-libs
security

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

mlbridge · 2023-03-24T20:49:22Z

Webrevs

src/java.base/share/classes/java/lang/invoke/ClassFileDumper.java

mlchung · 2023-03-25T22:53:08Z

/label remove security

openjdk · 2023-03-25T22:53:57Z

@mlchung
The security label was successfully removed.

liach · 2023-03-26T00:57:17Z

On a side note, can the class dumper be moved to jdk.internal so infrastructure like j.l.r.Proxy can use it too?

mlbridge · 2023-03-26T18:41:12Z

Mailing list message from Brian Goetz on core-libs-dev:

Since LMF goes through Lookup::defineHiddenClass, does this mean that
they will be potentially dumped twice, once through Lookup, and once
through LMF?

Now that there is a shared implementation, perhaps we should migrate use
in LMF to something more like

-Djava.lang.invoke.MethodHandle.DUMP_LAMBDA_PROXY_CLASS_FILES=true

and retire the separate dumpProxyClasses flag.

On 3/24/2023 4:49 PM, Mandy Chung wrote:

For classes defined via `Lookup::defineClass`, `Lookup::defineHiddenClass` and `Lookup::defineHiddenClassWithClassData`, by default they will be dumped to the path specified in `-Djava.lang.invoke.Lookup.dumpClasses=<dumpDir>`

The hidden classes generated for lambdas, `LambdaForms` and method handle implementation use non-default dumper so that they can be controlled via a separate system property and path as in the current implementation.

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/core-libs-dev/attachments/20230326/92fb55cf/attachment.htm>

mlchung · 2023-03-27T17:03:56Z

Since LMF goes through Lookup::defineHiddenClass, does this mean that they will be potentially dumped twice, once through Lookup, and once through LMF?

No, only once. The old dumping code in LMF is removed.

Now that there is a shared implementation, perhaps we should migrate use in LMF to something more like

-Djava.lang.invoke.MethodHandle.DUMP_LAMBDA_PROXY_CLASS_FILES=true

and retire the separate dumpProxyClasses flag.

+1. I think it's simpler to use a boolean property and dump to a known directory.

As this is implementation-specific, I will rename the property name with jdk. prefix.

mlchung · 2023-03-27T18:20:32Z

My thought on the property names:

Lookup::defineClass and Lookup::defineHiddenClass default to enable dumping of the class files via:
-Djdk.invoke.MethodHandle.dumpClassFiles=true

LMF and LambdaForms (and other method handle internal classes) are special cases to dump via a different system property respectively:
-Djdk.invoke.LambdaMetafactory.dumpProxyClassFiles=true
-Djdk.invoke.MethodHandle.dumpLambdaForms=true or -Djdk.invoke.MethodHandle.dumpInternalClassFiles=true

Thoughts/comments?

liach · 2023-03-27T18:28:32Z

Is there any other place in JDK where classfile dumping is needed but isn't covered by this new utility? I know j.l.r.Proxy for one, but how it declares classes is incompatible with lookups.

mlchung · 2023-03-27T18:36:42Z

j.l.r.Proxy needs to define the class file with null protection domain; that's why it can't be converted to use Lookup::defineClass yet until security manager is removed.

It's okay to move ClassFileDumper to jdk.internal.util package as a general dumping facility for j.l.r.Proxy and other areas to use.

RogerRiggs · 2023-03-28T21:51:21Z

src/java.base/share/classes/jdk/internal/util/ClassFileDumper.java

+    private static String encodeForFilename(String className) {
+        final int len = className.length();
+        StringBuilder sb = new StringBuilder(len);
+


This could be fewer lines of adhoc conversions, the replacements are hex encoding of the characters, so the replacement array is not needed.
java.util.HexFormat should be used for the hex encoding. The formatHex methods append to a StringBuilder.

This old code was from ProxyClassesDumper. Yes it can be converted to use HexFormat.

Hi @ASISBusiness, thanks for making a comment in an OpenJDK project!

All comments and discussions in the OpenJDK Community must be made available under the OpenJDK Terms of Use. If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please Use "Add GitHub user ASISBusiness for the summary.

If you are not an OpenJDK Author, Committer or Reviewer, simply check the box below to accept the OpenJDK Terms of Use for your comments.

I agree to the OpenJDK Terms of Use for all comments I make in a project in the OpenJDK GitHub organization.

Your comment will be automatically restored once you have accepted the OpenJDK Terms of Use.

turbanoff · 2023-03-29T08:01:30Z

src/java.base/share/classes/java/lang/invoke/InvokerBytecodeGenerator.java

-        }
-    }
+    private static final HashMap<String,Integer> DUMP_CLASS_FILES_COUNTERS =
+            dumper().isEnabled() ?  new HashMap<>(): null;


Suggested change

dumper().isEnabled() ? new HashMap<>(): null;

dumper().isEnabled() ? new HashMap<>(): null;

JornVernee

Looks mostly good.

JornVernee · 2023-04-03T17:58:49Z

src/java.base/share/classes/jdk/internal/util/ClassFileDumper.java

+                public Path run() {
+                    if (!Files.exists(path)) {
+                        try {
+                            Files.createDirectory(path);


Maybe this could use createDirectories instead, in case the path is nested. dumpClass also uses that.

JornVernee · 2023-04-03T18:00:09Z

src/java.base/share/classes/jdk/internal/util/ClassFileDumper.java

+        if (dumper.isEnabled() && !path.equals(dumper.dumpPath())) {
+            throw new IllegalArgumentException("mismatched dump path for " + key);
+        }


I don't see how this exception case could ever occur, given that dumper.dumpPath is directly coming from dir, and validateDumpDir doesn't return a modified path either.

Can this check be removed?

This is to check if two callers get the ClassFileDumper of the same key but different path.

ClassFileDumper.getInstance("jdk.foo.dump", "DUMP_DIR"); ClassFileDumper.getInstance("jdk.foo.dump", "DUMP_NEW_DIR"); <--- IAE

JornVernee · 2023-04-03T18:03:32Z

src/java.base/share/classes/jdk/internal/util/ClassFileDumper.java

+    public Path pathname(String internalName) {
+        return dumpDir.resolve(encodeForFilename(internalName) + ".class");
+    }


Could be private I think

It's public in case the caller who invokes ClassDefiner::defineClass wants to print the path to which the classfile is written.

JornVernee · 2023-04-03T18:07:52Z

src/java.base/share/classes/java/lang/invoke/InnerClassLambdaMetafactory.java

@@ -89,17 +85,19 @@
    private static final AtomicInteger counter = new AtomicInteger();


This counter could be removed now, it looks like.

thanks for catching it. will fix.

JornVernee · 2023-04-03T18:13:54Z

src/java.base/share/classes/java/lang/invoke/MethodHandles.java

+                                assert ((classFlags & HIDDEN_CLASS) == 0);
+                            }
+                        } else {
+                            name += ".failed-" + dumper.incrementAndGetCounter();


I think it makes more sense to move the counter to ClassDefiner. It is not used by ClassFileDumper itself.

(make it static if it's possible to have multiple definer instances with the same class name)

I see your point. If it were a static counter for all dumpers, multiple.failed-xxx dumped by a single dumper may not be in sequence if other dumpers have .failed-xxx class files.

If the counter has to be per dumper, maybe it makes sense to push the logic that derives the file name into ClassFileDumper too. e.g. have a dumpClass(name, Class<?>, bytes) and dumpFailed(name, bytes).

that's a good idea. Updated.

JornVernee

Nice! Thanks.

openjdk · 2023-04-03T20:39:41Z

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

8304846: Provide a shared utility to dump generated classes defined via Lookup API

Reviewed-by: jvernee

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

b062b1b: 8304743: Compile_lock and SystemDictionary updates
df819cf: 8304945: StringBuilder and StringBuffer should implement Appendable explicitly
312bbe7: 8305485: Problemlist runtime/Thread/TestAlwaysPreTouchStacks.java
50e31e0: 8305442: (bf) Direct and view implementations of CharBuffer.toString(int, int) do not need to catch SIOBE
85e3974: 8304014: Convert test/jdk/java/util/zip/ZipFile/CorruptedZipFiles.java to junit
40aea04: 8278268: (ch) InputStream returned by Channels.newInputStream should have fast path for FileChannel targets
9b9b5a7: 8302323: Add repeat methods to StringBuilder/StringBuffer
dd7ca75: 8305478: [REDO] disable gtest/NMTGtests.java sub-tests failing due to JDK-8305414
f9827ad: 8288109: HttpExchangeImpl.setAttribute does not allow null value after JDK-8266897
6010de0: 8305417: disable gtest/NMTGtests.java sub-tests failing due to JDK-8305414
... and 103 more: https://git.openjdk.org/jdk/compare/765a94258d84ac6f22bb2dedd1fc1afdbabb2b14...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.

forax · 2023-04-03T21:05:07Z

src/java.base/share/classes/jdk/internal/util/ClassFileDumper.java

+            return AccessController.doPrivileged(new PrivilegedAction<>() {
+                @Override
+                public Path run() {
+                    if (!Files.exists(path)) {


I do not think this is necessary, Files.createDirectories() already only create directories if they do not exist.
Moreover, someone can change the state of the files in between the two calls (Files.exists and Files.createDirectories) given that those two calls are not atomic from the filesystem POV.

that's right. updated

forax · 2023-04-03T21:05:38Z

src/java.base/share/classes/jdk/internal/util/ClassFileDumper.java

+            });
+    }
+
+    private static HexFormat HEX = HexFormat.of().withUpperCase();


forax · 2023-04-03T21:06:22Z

src/java.base/share/classes/jdk/internal/util/ClassFileDumper.java

+            char c = className.charAt(i);
+            // control characters
+            if (c <= 31 || BAD_CHARS.contains(c)) {
+                sb.append("%");


sb.append('%') // using a character instead of a String

mlchung · 2023-04-04T18:05:31Z

/integrate

openjdk · 2023-04-04T18:07:16Z

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

2ee4245: 8305509: C1 fails "assert(k != nullptr) failed: illegal use of unloaded klass"
7baccd9: 8303260: (fc) FileChannel::transferFrom should support position > size()
3ef834f: 8298619: java/io/File/GetXSpace.java is failing
c594119: 8305511: Remove ignore from com/sun/jdi/PopAndInvokeTest.java
62bd2eb: 8303147: [s390x] fast & slow debug builds are broken
f9ed825: 8303082: [AIX] Missing C++ name demangling with XLClang++
73a084c: 8305504: stutter typo in java.compiler files
7b92210: 8305525: Problemlist runtime/ErrorHandling/TestDwarf on x86
5881b09: 8305500: SystemDictionary::find_placeholder/find_class declared but not used
7ca2aec: 8304033: JFR: Missing thread
... and 122 more: https://git.openjdk.org/jdk/compare/765a94258d84ac6f22bb2dedd1fc1afdbabb2b14...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-04-04T18:07:40Z

@mlchung Pushed as commit dd59471.

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

ExE-Boss · 2024-08-31T10:56:23Z

src/java.base/share/classes/java/lang/invoke/ClassSpecializer.java

@@ -570,22 +566,9 @@ S loadSpecies(S speciesData) {
        @SuppressWarnings("removal")


This @SuppressWarnings is no longer needed:

Suggested change

@SuppressWarnings("removal")

mlchung added 2 commits March 24, 2023 13:32

8304846: Provide a shared utility to dump generated classes defined v…

7ae14a4

…ia Lookup API

rename the system property

d1c739c

openjdk bot added the rfr Pull request is ready for review label Mar 24, 2023

openjdk bot added security security-dev@openjdk.org core-libs core-libs-dev@openjdk.org labels Mar 24, 2023

liach reviewed Mar 24, 2023

View reviewed changes

src/java.base/share/classes/java/lang/invoke/ClassFileDumper.java Outdated Show resolved Hide resolved

avoid another map lookup

04406e3

openjdk bot removed the security security-dev@openjdk.org label Mar 25, 2023

mlchung added 2 commits March 27, 2023 13:40

rename the system property for dumping class files

f7aa26a

move ClassFileDumper to jdk.internal.util

e424810

RogerRiggs reviewed Mar 28, 2023

View reviewed changes

Convert to use HexFormat

2c305fa

turbanoff reviewed Mar 29, 2023

View reviewed changes

set -D<key> or -D<key>=true will enable dumping

351574a

JornVernee reviewed Apr 3, 2023

View reviewed changes

comments from Jorn Vernee

2eeb7b0

JornVernee approved these changes Apr 3, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Apr 3, 2023

forax reviewed Apr 3, 2023

View reviewed changes

mlchung added 2 commits April 3, 2023 15:02

comments from Remi

120bc2f

update test

a680620

openjdk bot added the integrated Pull request has been integrated label Apr 4, 2023

openjdk bot closed this Apr 4, 2023

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

mlchung deleted the JDK-8304846 branch August 2, 2023 20:51

ExE-Boss reviewed Aug 31, 2024

View reviewed changes

	dumper().isEnabled() ? new HashMap<>(): null;
	dumper().isEnabled() ? new HashMap<>(): null;

		@@ -89,17 +85,19 @@
		private static final AtomicInteger counter = new AtomicInteger();

		@@ -570,22 +566,9 @@ S loadSpecies(S speciesData) {
		@SuppressWarnings("removal")

8304846: Provide a shared utility to dump generated classes defined via Lookup API #13182

8304846: Provide a shared utility to dump generated classes defined via Lookup API #13182

Conversation

mlchung commented Mar 24, 2023 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Mar 24, 2023

openjdk bot commented Mar 24, 2023

mlbridge bot commented Mar 24, 2023 • edited Loading

Webrevs

mlchung commented Mar 25, 2023

openjdk bot commented Mar 25, 2023

liach commented Mar 26, 2023

mlbridge bot commented Mar 26, 2023

mlchung commented Mar 27, 2023

mlchung commented Mar 27, 2023 • edited Loading

liach commented Mar 27, 2023

mlchung commented Mar 27, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ASISBusiness Mar 29, 2023 • edited by bridgekeeper bot Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JornVernee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JornVernee left a comment

Choose a reason for hiding this comment

openjdk bot commented Apr 3, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mlchung commented Apr 4, 2023

openjdk bot commented Apr 4, 2023

openjdk bot commented Apr 4, 2023

Choose a reason for hiding this comment

mlchung commented Mar 24, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Mar 24, 2023 •

edited

Loading

mlchung commented Mar 27, 2023 •

edited

Loading

ASISBusiness Mar 29, 2023 •

edited by bridgekeeper bot

Loading