JDK-8184349 : There should be some verification that EnableJVMCI is disabled if a GC not supporting JVMCI is selected
  • Type: Enhancement
  • Component: hotspot
  • Sub-Component: compiler
  • Affected Version: 10
  • Priority: P4
  • Status: Resolved
  • Resolution: Fixed
  • Submitted: 2017-07-13
  • Updated: 2020-11-25
  • Resolved: 2018-06-15
The Version table provides details related to the release that this issue/RFE will be addressed.

Unresolved : Release in which this issue/RFE will be addressed.
Resolved: Release in which this issue/RFE has been resolved.
Fixed : Release in which this issue/RFE has been fixed. The release containing this fix may be available for download as an Early Access Release or a General Availability Release.

To download the current JDK release, click here.
JDK 11
11 b19Fixed
Related Reports
Relates :  
Relates :  
Relates :  
Description
If there was to be a GC that did not support JVMCI, it would be desirable that if such a GC was selected, sanity checks should make sure that create_vm can not successfully return with EnableJVMCI set.

This RFE comes from the reasoning in JDK-8184269. This is a snippet from the email conversation when this was discussed, where a check was removed that was previously made during JVMCI bootstrapping called when Graal is bootstrapping, checking that the currently selected BarrierSet is known to support JVMCI:

<EMAIL>
> One thing - we may want to preserve JVMCI_ERROR() to check known by Graal concrete barriers.
I did think about the JVMCI_ERROR() check. Here is a summary of my thoughts:
1) The current check is strange. It explicitly does not complain if the ModRef barrier set is selected. However, that is not a concrete barrier set and if it was indeed found to be the selected barrier set, it would most definitely be an error, and the JVM would arguably not work.
2) I do not think that the error checking for finding incompatible barrier sets should be done here in the first place. It should be done earlier.
Current collectors all support JVMCI. If a hypothetical new GC did not support JVMCI, e.g., does not provide enough information to JVMCI to make it possible for JVMCI compilers to JIT bytecode, and a user selects that hypothetical GC in combination with e.g. -XX:+UseJVMCICompiler, then that is already an invalid JVM argument combination, and it should not even be possible to create_vm with invalid JVM arguments.
Therefore, I think such sanity checking for JVMCI compliance should be done earlier in the bootstrapping during argument parsing, so that once bootstrapping is done, the invariant that we have a sane GC and compiler combination is already established. Then when this code is run, we already know that the user has selected a GC compatible with JVMCI, rather than sprinkling the code after create_vm with error checks, checking for conditions due to invalid JVM arguments that should never have been allowed.
3) The contract in hotspot is between the GC and JVMCI, not Graal. That is, the question we should ask ourselves in hotspot during this bootstrapping is whether a GC provides enough information to JVMCI to allow external JIT compilation, not whether Graal will do the right thing. Then it has to be the responsibility of external JITs like Graal to correctly detect what GCs it supports, rather than for the GCs to detect what external compilers it supports through JVMCI.

Due to the above reasoning, I believe it is best not to perform the sanity check here that a valid BarrierSet was provided. If an invalid GC and compiler combination exists, then that should be validated before create_vm succeeds during argument parsing, if such invalid combinations are introduced. And after bootstrapping, we should be able to trust that we do not at runtime need to detect incompatible BarrierSet and compiler combinations.
</EMAIL>
Comments
Testing found only failures related to JDK-8204209.
15-06-2018

Updated changes because recent ZGC and EpsilonGC pushes. Fixed additional tests. http://cr.openjdk.java.net/~kvn/8184349/webrev.05/
15-06-2018

webrev.04 testing passed without failures.
11-06-2018

Several tests failed when run with -Xint. I updated changes to fix it: http://cr.openjdk.java.net/~kvn/8184349/webrev.04/
09-06-2018

I updated changes after I pushed tests fix JDK-8202611. I also reduced scope of changes only refactoring code in Arguments::apply_ergo(). http://cr.openjdk.java.net/~kvn/8184349/webrev.03/
08-06-2018

After JFR moved into OpenJDK I have to redo fix for tests.
29-05-2018

Yes, it will fix it.
29-05-2018

I was able reproduce crash in runtime tests with -Xcomp without my changes. Filed JDK-8202669. An other new failure I see is due to JDK-8016310 serviceability test. And last is a problem of comilercontrol run with Graal - filed JDK-8202666.
04-05-2018

VerboseOutTest.java test failed because it was removed from Problem list but fixed by JDK-8194968 after my last pull from jdk/jdk. Crush in runtime/negAttrLength/NegativeAttributeLength.java test ran with -Xcomp is new problem it seems (nut not related to my changes).
03-05-2018

I pulled latest source from jdk/jdk which forced me to do more changes and fixed CheckCompileThresholdScaling.java test: http://cr.openjdk.java.net/~kvn/8184349/webrev.02/
03-05-2018

I decided to split complex changes in tests into separate bug since they will be big. Filed JDK-8202611.
03-05-2018

Test compiler/arguments/CheckCompileThresholdScaling.java failed because before changes CompileThreshold flag was scaled and marked 'ergonomic' even in Interpreter mode (as result of flags combination: -XX:-TieredCompilation -XX:CompileThreshold=0). With changes compiler flags are not adjusted ergonomically in Interpreter mode because they will not be used anyway (see line 386 in compilerConfig.cpp). I will fix test: diff -r a87f2e7a527c test/hotspot/jtreg/compiler/arguments/CheckCompileThresholdScaling.java --- a/test/hotspot/jtreg/compiler/arguments/CheckCompileThresholdScaling.java +++ b/test/hotspot/jtreg/compiler/arguments/CheckCompileThresholdScaling.java @@ -125,7 +125,7 @@ "interpreted mode" }, { - "intx CompileThreshold = 0 {pd product} {command line, ergonomic}", + "intx CompileThreshold = 0 {pd product} {command line}", "double CompileThresholdScaling = 0.750000 {product} {command line}", "interpreted mode" }
03-05-2018

Updated changes to filter out tests which explicitly use CMS: http://cr.openjdk.java.net/~kvn/8184349/webrev.01/
03-05-2018

First version: http://cr.openjdk.java.net/~kvn/8184349/webrev.00/
01-05-2018

The needed changes are more complicated than above fix. GC is selected much later during flags setting based on ergonomic. But we have a lot of Compiler flags setting based on UseJVMCICompiler before that. I need to refactor compiler flags setting to that after GC is selected.
27-04-2018

Suggested fix: diff -r 938478a66ad7 src/hotspot/share/jvmci/jvmci_globals.cpp --- a/src/hotspot/share/jvmci/jvmci_globals.cpp +++ b/src/hotspot/share/jvmci/jvmci_globals.cpp @@ -66,6 +66,19 @@ JVMCI_FLAG_CHECKED(UseJVMCICompiler) JVMCI_FLAG_CHECKED(EnableJVMCI) + if (EnableJVMCI) { + // Check that used GC is supported by JVMCI and Java compiler + if (!(UseSerialGC || UseParallelGC || UseG1GC)) { + if (!FLAG_IS_DEFAULT(EnableJVMCI)) { + jio_fprintf(defaultStream::error_stream(), + "Improperly specified VM option EnableJVMCI: it is not supported with selected GC\n"); + } + FLAG_SET_DEFAULT(EnableJVMCI, false); + FLAG_SET_DEFAULT(UseJVMCICompiler, false); + return true; + } + } + CHECK_NOT_SET(BootstrapJVMCI, UseJVMCICompiler) CHECK_NOT_SET(PrintBootstrap, UseJVMCICompiler) CHECK_NOT_SET(JVMCIThreads, UseJVMCICompiler)
27-04-2018

This become urgent because of JDK-8202273. Graal dropped CMS support and tests which run with CMS and Graal as JIT failing.
26-04-2018