RFR/RFC 8231503: [TESTBUG] compiler/{jvmci, aot} tests should not run with GCs that do not support JVMCI/AOT

[Stefan Karlsson]

[I wrote this before realizing that the mail thread had forked. Some of 
the comments might have already been covered]

Hi Aleksey,

Thanks for fixing this!

Inlined:

On 2019-09-27 17:26, Aleksey Shipilev wrote:
> Replying with proper "To:". Friday, eh.
> 
> On 9/27/19 5:23 PM, Aleksey Shipilev wrote:
>> Testbug:
>>    https://bugs.openjdk.java.net/browse/JDK-8231503
>>
>> Candidate fix:
>>    https://cr.openjdk.java.net/~shade/8231503/webrev.01/

One suggestion would be to rewrite the GC checking to use the enum 
values instead of string compares:

         switch (GC.selected()) {
             case Serial:
             case Parallel:
             case G1:
                 // These GCs are supported with AOT
                 return "true";
             default:
                 // Every other GC is not supported
                 return "false";
         }


Maybe even create a function that returns a boolean.

>>
>> I am actually not sure it should be fixed this way. The alternative is to add the @requires lines to
>> every affected test (there are about a hundred of them), like this:
>>    @requires (vm.gc == "null" || vm.gc == "G1" || vm.gc = "Serial" || vm.gc = "Parallel")
>>
>> ...but that seems too intrusive. Or maybe not? Open for opinions from those who maintain JVMCI/AOT.

I would also be interested in hearing what the JVMCI/AOT maintainers think.

Isn't it odd that we prevent these tests from being run when JVMCI is 
enabled? Shouldn't we only skip the tests when an unsupported compiler 
is used? For all practical purposes that would be when Graal is used.

A lot of the tests are run with -XX:-UseJVMCICompiler and it's not 
obvious to me that those have to be excluded when running with ZGC or 
Shenandoah.

I think there might be a bug in check_jvmci_supported_gc and that we 
probably should change this to:
---
diff --git a/src/hotspot/share/jvmci/jvmci_globals.cpp 
b/src/hotspot/share/jvmci/jvmci_globals.cpp
--- a/src/hotspot/share/jvmci/jvmci_globals.cpp
+++ b/src/hotspot/share/jvmci/jvmci_globals.cpp
@@ -123,7 +123,7 @@
  }

  void JVMCIGlobals::check_jvmci_supported_gc() {
-  if (EnableJVMCI) {
+  if (EnableJVMCI && UseJVMCICompiler) {
      // Check if selected GC is supported by JVMCI and Java compiler
      if (!(UseSerialGC || UseParallelGC || UseParallelOldGC || UseG1GC)) {
        vm_exit_during_initialization("JVMCI Compiler does not support 
selected GC", GCConfig::hs_err_name());
---

I tested this with ZGC (without your patch) and ran the tests in 
test/hotspot/jtreg/compiler/jvmci. 90% of the tests passed, some of the 
tests failed (as expected) because they actually ran with 
-XX:+UseJVMCICompiler, and some (unexpectedly) crashed with 
-XX:-UseJVMCICompiler. The latter might be a real bug in the JVMCI API.

With all this said, maybe we should:

0) Add your "gc supports graal" checks to a new helper function 
(selectedGCSupportsGraal() ?).
1) Use the UseJVCMICompiler patch above
2) Leave the vmJvmci function as is
3) Tag tests actually requiring graal with vm.graal.enabled
4) Maybe change @requires vm.aot to to use vm.aot.enabled
5) Use this function to implement vm.graal.enabled and vm.aot.enabled

Of course, I could have misunderstood the meaning of 
-XX:-UseJVMCICompiler, but then it would be good to get some 
clarification about that.

Also, if you are in a hurry to get this resolved, then we could consider 
making these changes after your patch has been pushed.

Thanks,
StefanK

>>
>> Testing: compiler/aot, compiler/jvmci {serial, parallel, g1, shenandoah, zgc}; tier1
>