-
Notifications
You must be signed in to change notification settings - Fork 5.8k
8334714: Implement JEP 484: Class-File API #19826
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
👋 Welcome back asotona! A progress list of the required criteria for merging this PR into |
@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:
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 4 new commits pushed to the
Please see this link for an up-to-date comparison between the source branch of this pull request and the ➡️ To integrate this PR with the above commit message to the |
Do we intend to do it after we finish our planned API changes or as soon as possible? Also we might need to look at the preview-enabled test treatments. |
Looks like this has been accidentally created as a sub-task of the JEP issue, I assume you'll fix that. Will there be another issue to update the tests and drop |
Any potential API changes should be merged first, to avoid impression of changing a non-preview API. Cleaning up preview-enabled tests has a lower priority and should follow. |
I thought the implementation is usually created as a subtask of JEP, however I can make it a standalone bug if neede. Yes, there will be follow up task to clean all the |
Note that |
@ExE-Boss Your report has been promoted to https://bugs.openjdk.org/browse/JDK-8336430 and will be addressed in a separate patch. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
@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! |
I've been trying to work with classfile api, and:
|
Unfortunately, ClassFile API's scope is only to interpret correctly the Class Files that are accepted by the current VM and support writing such class files; for example, for release N, we will not support correct parsing or writing of preview class files from N-1, N-2, etc. While your account of oak format seems interesting (from a search, it seems to use u1 for max stacks/locals, u2 for Code size), it is neither recognized by hotspot (the reference implementation for JVM): jdk/src/hotspot/share/classfile/classFileParser.cpp Lines 2332 to 2334 in 5671f83
Nor by the JVMS: https://docs.oracle.com/javase/specs/jvms/se22/html/jvms-4.html#jvms-4.7.3 And as such, it falls out of the goals of the API, and is likely not going to be added. You most likely will resort to a third party library to handle such frozen format, as the main issue ClassFile API is solving is that a library built against JDK 21 may not run on JDK 23 unless it bumps its ASM dependency to support reading JDK 23 Class File format, which doesn't exist for the said oak format. |
Interesting. I guess handling of oak classfiles was removed a long time ago by this commit.
Also now, this library which took special care to parse oak files byte[] bytes = ClassFile.of(ClassFile.StackMapsOption.GENERATE_STACK_MAPS)
.build(ClassDesc.ofInternalName("Test"), builder -> {
builder.withVersion(ClassFile.JAVA_1_VERSION, 0);
builder.withMethod(
"main",
MethodTypeDesc.ofDescriptor("([Ljava/lang/String;)V"),
AccessFlags.ofMethod(AccessFlag.PUBLIC, AccessFlag.STATIC).flagsMask(),
mb -> {
mb.withCode(cb -> {
ClassDesc printStream = PrintStream.class.describeConstable().orElseThrow();
cb.getstatic(System.class.describeConstable().orElseThrow(), "out", printStream)
.ldc("Hello, World")
.invokevirtual(printStream, "println", MethodTypeDesc.ofDescriptor("(Ljava/lang/String;)V"))
.return_();
});
});
builder.with(SourceFileAttribute.of("Test Attr"));
});
Path path = Path.of("Test.class");
Files.write(path, bytes);
var file = new ClassFileReader().read(bytes); As noted about Hotspot technically parsing invalid data now (if you could consider that "invalid") and we make a correct oak classfile:
|
I think so. See https://mail.openjdk.org/pipermail/hotspot-dev/2019-October/039795.html for some context. There are also historical evaluations available at https://bugs.openjdk.org/browse/JDK-8232890 and https://bugs.openjdk.org/browse/JDK-8232967, notably the judgement in Compatibility Risk Description:
It appears so. See note in jdk/src/java.base/share/classes/java/lang/reflect/ClassFileFormatVersion.java Lines 413 to 414 in 5671f83
It seems that now major version 45, regardless of the minor version, is simply seen as the Class File format for Java 1.1.
Indeed, it would be a good RFE to allow users to override default attribute mappers to parse attributes; this would be extremely useful if users wish to support previous previews that only differed in the attribute formats.
For the behavioral inconsistencies, we can backport this special handling removal to active jdk update projects or even request specification changes on LTS versions (see https://github.com/openjdk/jdk8u-ri) if this is deemed important enough. |
I don't think at this point in time there would be any change in the format of any existing attributes, as that would be a binary incompatible change.
I don't think that many projects use pre-Java 1 format at this point. While I saw some libraries that kept their version set to versions like Java 4 for no apparent reason... Hey, at least it's not Java 1 version. |
Edit: I missed the word |
This would be an important decision; I would wait till Monday when more engineers are back to discuss about the histroy of dropping support for such files, why this format never made its way to the JVMS, and the correct way to handle so as to avoid inconsistency between different versions.
Yes, Class-File API is supposed to handle whatever the VM handles. It's actually considered a "JVM API" in Oracle's documentations: https://docs.oracle.com/en/java/javase/22/vm/class-file-api.html OpenJDK's preview version policy allows shipping the preview features for testing to a wider audience without any implied burden of long-term maintenance. So for example, if the Nullable Types JEP preview adds Unfortunately, there seems to be some misunderstanding around the role of preview, so some are asking if preview features that is largely unchanged and finalized in a future release can become finalized in an LTS as well. Short answer, no: it's an experiment format enabled by the new 6-month release schedule. And note that javap uses ClassFile API, and as a result, javap for release N cannot correctly decompile Class Files with version N-1.65535, etc. We are planning to decompile on a best-effort basis, that we try to interpret such as class as if it's N.65535 (maybe emit a warning), trying to salvage the most out of the Class File and printing errors (but continue to output) if we encounter anything we don't expect. |
I was informed in offline discussion that this is unlikely to be changed and this change is unlikely to be backported unless for security purposes. |
@asotona this pull request can not be integrated into git checkout JDK-8334714-final
git fetch https://git.openjdk.org/jdk.git master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push |
# Conflicts: # src/java.base/share/classes/java/lang/classfile/AccessFlags.java # src/java.base/share/classes/java/lang/classfile/ClassBuilder.java # src/java.base/share/classes/java/lang/classfile/ClassElement.java # src/java.base/share/classes/java/lang/classfile/ClassFileTransform.java # src/java.base/share/classes/java/lang/classfile/ClassHierarchyResolver.java # src/java.base/share/classes/java/lang/classfile/ClassModel.java # src/java.base/share/classes/java/lang/classfile/ClassReader.java # src/java.base/share/classes/java/lang/classfile/ClassSignature.java # src/java.base/share/classes/java/lang/classfile/CodeBuilder.java # src/java.base/share/classes/java/lang/classfile/CodeElement.java # src/java.base/share/classes/java/lang/classfile/CodeModel.java # src/java.base/share/classes/java/lang/classfile/CompoundElement.java # src/java.base/share/classes/java/lang/classfile/FieldBuilder.java # src/java.base/share/classes/java/lang/classfile/FieldElement.java # src/java.base/share/classes/java/lang/classfile/Instruction.java # src/java.base/share/classes/java/lang/classfile/MethodBuilder.java # src/java.base/share/classes/java/lang/classfile/MethodElement.java # src/java.base/share/classes/java/lang/classfile/TypeKind.java # src/java.base/share/classes/java/lang/classfile/attribute/LocalVariableTableAttribute.java # src/java.base/share/classes/java/lang/classfile/attribute/LocalVariableTypeTableAttribute.java # src/java.base/share/classes/java/lang/classfile/attribute/RuntimeInvisibleAnnotationsAttribute.java # src/java.base/share/classes/java/lang/classfile/attribute/RuntimeVisibleAnnotationsAttribute.java # src/java.base/share/classes/java/lang/classfile/constantpool/AnnotationConstantValueEntry.java # src/java.base/share/classes/java/lang/classfile/constantpool/ConstantDynamicEntry.java # src/java.base/share/classes/java/lang/classfile/constantpool/ConstantPool.java # src/java.base/share/classes/java/lang/classfile/constantpool/ConstantPoolBuilder.java # src/java.base/share/classes/java/lang/classfile/constantpool/ConstantValueEntry.java # src/java.base/share/classes/java/lang/classfile/constantpool/DynamicConstantPoolEntry.java # src/java.base/share/classes/java/lang/classfile/constantpool/FieldRefEntry.java # src/java.base/share/classes/java/lang/classfile/constantpool/InterfaceMethodRefEntry.java # src/java.base/share/classes/java/lang/classfile/constantpool/LoadableConstantEntry.java # src/java.base/share/classes/java/lang/classfile/constantpool/MethodRefEntry.java # src/java.base/share/classes/java/lang/classfile/constantpool/ModuleEntry.java # src/java.base/share/classes/java/lang/classfile/constantpool/PackageEntry.java # src/java.base/share/classes/java/lang/classfile/instruction/NewMultiArrayInstruction.java
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should probably be possible for ClassFile::verify(…)
to be able to verify the bytecode of java.lang.Object
, java.lang.Class
, java.lang.String
, and java.lang.Throwable
, as the main reason the HotSpot verifier skips those is that there’s circular verification bootstrap dependencies between those, but the Class‑File API verifier uses the offline ClassHierarchyResolver
instead for determining assignability checks.
jdk/src/java.base/share/classes/jdk/internal/classfile/impl/verifier/VerifierImpl.java
Lines 144 to 150 in 18bcbf7
public static boolean is_eligible_for_verification(VerificationWrapper klass) { | |
String name = klass.thisClassName(); | |
return !java_lang_Object.equals(name) && | |
!java_lang_Class.equals(name) && | |
!java_lang_String.equals(name) && | |
!java_lang_Throwable.equals(name); | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
VerifierImpl should be the best possible 1:1 transcription of the HotSpot verifier (for easy maintenance). I also have to suppress itching to rewrite it ;) Fortunately why would someone need to verify Object, Class, String or Throwable.
# Conflicts: # src/java.base/share/classes/jdk/internal/javac/PreviewFeature.java
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The latest merge of PreviewFeature.java looks good.
# Conflicts: # src/java.base/share/classes/java/lang/classfile/CustomAttribute.java
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's finally the time to set sail!
Thank you! /integrate |
Going to push as commit 84ffb64.
Your commit was automatically rebased without conflicts. |
@@ -74,8 +74,6 @@ public enum Feature { | |||
SCOPED_VALUES, | |||
@JEP(number=480, title="Structured Concurrency", status="Third Preview") | |||
STRUCTURED_CONCURRENCY, | |||
@JEP(number=466, title="ClassFile API", status="Second Preview") | |||
CLASSFILE_API, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just remembered that we have to keep this until JDK 24 is the minimum boot JDK; with this removed bootcycle build should fail.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The |
Thanks for reporting luca, we still don't test for redundant tags so I missed this. |
I filed JDK‑8345737 for that, and would prefer that at least |
I fear that promoting creation of unbound Also for |
Thanks to |
Class-File API is leaving preview.
This is a removal of all
@PreviewFeature
annotations from Class-File API.It also bumps all
@since
tags and removesjdk.internal.javac.PreviewFeature.Feature.CLASSFILE_API
.Please review.
Thanks,
Adam
Progress
Issues
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/19826/head:pull/19826
$ git checkout pull/19826
Update a local copy of the PR:
$ git checkout pull/19826
$ git pull https://git.openjdk.org/jdk.git pull/19826/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 19826
View PR using the GUI difftool:
$ git pr show -t 19826
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/19826.diff
Using Webrev
Link to Webrev Comment