eclipse-jdt / eclipse.jdt.core

Eclipse Public License 2.0
164 stars 130 forks source link

[23] generate workaround for new JIT bug into eclipse.ini #2954

Closed stephan-herrmann closed 1 month ago

stephan-herrmann commented 1 month ago

As per https://github.com/eclipse-jdt/eclipse.jdt.ui/issues/1639 many test failures could be traced back to a bug in the JIT of JVM 23, whereby an NPE was raised in a code location that provably never saw a null.

The bug can be worked around by passing the following JVM arg:

-XX:CompileCommand=exclude,org.eclipse.jdt.internal.core.dom.rewrite.ASTRewriteAnalyzer::getExtendedRange

I'll add a p2.inf file to jdt.core to automatically add this option to eclipse.ini.

Now, the following would cause p2 to wrongly detect , as the end of the argument:

addJvmArg(jvmArg:-XX:CompileCommand=exclude,org.eclipse.jdt.internal.core.dom.rewrite.ASTRewriteAnalyzer::getExtendedRange);

Documentation of the java launcher mentions that , can be replaced with a space, and recommends to put the command in double quotes.

The following is understood by p2, would, however, cause the eclipse launcher to garble the entry such that the JVM cannot grok it:

addJvmArg(jvmArg:-XX:CompileCommand="exclude org.eclipse.jdt.internal.core.dom.rewrite.ASTRewriteAnalyzer::getExtendedRange");

In the end it seems that space is accepted even without quotes, both in p2.inf and in eclipse.ini.

stephan-herrmann commented 1 month ago

Resolved by #2955

stephan-herrmann commented 1 month ago

Verified: updating Eclipse SDK 4.33 with https://download.eclipse.org/eclipse/updates/4.33-Y-builds/Y20240914-1000 adds the JIT-exclusion to eclipse.ini. After that, launching eclipse prints:

CompileCommand: exclude org/eclipse/jdt/internal/core/dom/rewrite/ASTRewriteAnalyzer.getExtendedRange bool exclude = true

This means the syntax is accepted by java.

merks commented 1 month ago

@stephan-herrmann

I'm in the process of producing JustJ 23 JREs. Once I complete that, Java 23 will be available in the installers and users could use it to create an 2024-09 installation with it. That makes me wonder about this vm argument that I noticed. I.e., I could add it here:

image

But I don't really understand its purpose exactly...

stephan-herrmann commented 1 month ago

I'm in the process of producing JustJ 23 JREs. Once I complete that, Java 23 will be available in the installers and users could use it to create an 2024-09 installation with it. That makes me wonder about this vm argument that I noticed. I.e., I could add it here: [...]

But I don't really understand its purpose exactly...

I'm glad someone notices :)

Java 23 VM has the interesting capability to throw NPE from provably null-free code. More specifically, for one particular method in JDT when the JIT compiles it twice (assumeably because it was found to be relevant for optimization), the resulting code is broken. I guess the JIT re-orders statements so that the code dereferences a local variable before it is assigned. Of course such re-ordering is illegal.

The only way we can defend against this particular case is by telling the JIT not to touch that specific method. That's what this directive does:

-XX:CompileCommand=exclude,org.eclipse.jdt.internal.core.dom.rewrite.ASTRewriteAnalyzer::getExtendedRange

Obviously that is a point fix only for that one observed instance of the bug (which caused hundreds of failures in our test suite), it does not fix the JIT bug, which could still affect other methods as well...

Perhaps you saw me fighting with separator chars (, or ) and quoting. So the way to test if the result is effective is launching eclipse and looking for a line on console saying

CompileCommand: exclude org/eclipse/jdt/internal/core/dom/rewrite/ASTRewriteAnalyzer.getExtendedRange bool exclude = true
merks commented 1 month ago

Thank you 🙏