-
Notifications
You must be signed in to change notification settings - Fork 6.1k
8340840: jshell ClassFormatError when making inner class static #27665
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
base: master
Are you sure you want to change the base?
Conversation
8368999: jshell crash when existing sealed class is updated to also be abstract
👋 Welcome back jlahoda! A progress list of the required criteria for merging this PR into |
@lahodaj 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 195 new commits pushed to the
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 |
@lahodaj |
} catch (EngineTerminationException ex) { | ||
throw ex; | ||
} catch (Exception ex) { | ||
} catch (Exception | LinkageError ex) { |
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.
How was ClassFormatError previously thrown here? Was it wrapped in some exception?
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 was simply thrown out of the redefine method, as the stacktrace in the summary suggests. Note that the javadoc for redefineClasses
is very clear it may throw ClassFormatError
:
https://docs.oracle.com/en/java/javase/25/docs/api/jdk.jdi/com/sun/jdi/VirtualMachine.html#redefineClasses(java.util.Map)
if (c == outer && member.owner == c) { | ||
member.flags_field = flags; | ||
enterMember(c, member); | ||
if ((member.flags_field & FROM_SOURCE) == 0) { |
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.
I agree with the fix -- I guess I'd slightly prefer to see a warning if a mismatch is detected, but I'll leave that to you.
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.
javac changes look good (did not look at the jshell part)
Consider a JShell interaction like:
There are two problems here (although the stack trace immediately only shows one of them):
Redefining Classes
When a snippet is redefined, JShell first tries to redefine it using JDI (
VirtualMachine.redefineClasses)
. This usually throwsUnsupportedOperationException
is the redefine cannot happen, andJdiExecutionControl
handles that gracefully. JShell will recompile and reload the given snippet, and dependent snippets, under different names.But the
redefineClasses
method can also throw variousLinkageError
s. These are properly documented for the method: https://docs.oracle.com/en/java/javase/25/docs/api/jdk.jdi/com/sun/jdi/VirtualMachine.html#redefineClasses(java.util.Map)But
JdiExecutionControl
is not handling theseLinkageError
s.The proposed solution herein is to simply catch and handle the
LinkageError
s in the same way as theUnsupportedOperationException
(and other exceptions).InnerClasses attribute data overriding information from sources
Consider situation when compiling the third input:
class O { static class I {} }
.When this is being compiled, a classfile for
var i = new O().new I();
exists, and itsInnerClasses
attribute recordsO.I
to be a non-static
inner class.While compiling the code for
class O { static class I {} }
, the classfile forvar i
is also read from the classfile, and itsInnerClasses
attribute is read as well. And as a consequence, theO.I
class will be marked as non-static
, which conflicts with what is in the source file.This is not related to JShell as such, it can be reproduced with javac. Please see the
test/langtools/tools/javac/recovery/SourceAndInnerClassInconsistency.java
test.The proposal herein is to not use the information from the
InnerClasses
classfile attribute to manipulate classes that originate in a source file. More generally, I think the information from the source should always prevail over information from other/unrelated classfiles. Note theInnerClasses
attribute is in a classfile that has no real relation to the source code that is being compiled, it is simply an classfile on the classpath./issue JDK-8368999
Progress
Warning
8340840: jshell ClassFormatError when making inner class static
,8368999: jshell crash when existing sealed class is updated to also be abstract
Issues
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/27665/head:pull/27665
$ git checkout pull/27665
Update a local copy of the PR:
$ git checkout pull/27665
$ git pull https://git.openjdk.org/jdk.git pull/27665/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 27665
View PR using the GUI difftool:
$ git pr show -t 27665
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/27665.diff
Using Webrev
Link to Webrev Comment