RFR (S): JDK-8129855: -XX:+IgnoreUnrecognizedVMOptions hides out of range VM options.

[Dmitry Dmitriev]

Hi Gerard,

Hotspot changes look good to me. Comments about new test.
1) I think that test should be placed in "test/runtime/CommandLine", 
because recently we don't use bugid folder to put tests.
2) Since this is a new test, then we need only 2015 in copyright year at 
line 2.
3) Extra spaces between "@bug" and "8129855" on line 28.
4) Lines 42-49: use Platform.isDebugBuild() from test 
library(test/testlibrary/jdk/test/lib/Platform.java). It do the same thing.
5) Test perform similar things each step: create process builder, start 
it and verify exit value. I think you can introduce additional function 
which perform these things and in this case you can remove duplicated code.
For example, you can introduce following function:
private static void ranJavaCheckExitValue(boolean shouldSuccess, 
String... args) {
      ProcessBuilder pb = ProcessTools.createJavaProcessBuilder(args);
      OutputAnalyzer output = new OutputAnalyzer(pb.start());
      if (shouldSuccess) {
          output.shouldHaveExitValue(0);
      } else {
          output.shouldHaveExitValue(1);
      }
}

And test will looks like this(lines 51-69 in the original test):
     /*
       #1.1 normal flag:
                                     exists, invalid value does not exist
                                     -XX:MinHeapFreeRatio=notnum 
-XX:THIS_FLAG_DOESNT_EXIST
       -IgnoreUnrecognizedVMOptions ERR                           ERR
       +IgnoreUnrecognizedVMOptions ERR                           OK
     */
     ranJavaCheckExitValue(false, "-XX:-IgnoreUnrecognizedVMOptions", 
"-XX:MinHeapFreeRatio=notnum", "-version");
     ranJavaCheckExitValue(false, "-XX:-IgnoreUnrecognizedVMOptions", 
"-XX:THIS_FLAG_DOESNT_EXIST", "-version");
     ranJavaCheckExitValue(false, "-XX:+IgnoreUnrecognizedVMOptions", 
"-XX:MinHeapFreeRatio=notnum", "-version");
     ranJavaCheckExitValue(true, "-XX:+IgnoreUnrecognizedVMOptions", 
"-XX:THIS_FLAG_DOESNT_EXIST", "-version");

6) I think that empty line 232 is not needed.

Thanks,
Dmitry

On 29.09.2015 0:55, gerard ziemski wrote:
> hi all,
>
> We are fixing how IgnoreUnrecognizedVMOptions treats those flags whose 
> values are out of range. Before the fix, the VM would continue even if 
> flag’s value was out of range, ex:
>
> java_old -XX:+IgnoreUnrecognizedVMOptions -XX:MinTLABSize=0 -version
> size_t MinTLABSize=0 is outside the allowed range [ 1 ... 4294967295 ]
> java version "1.9.0-internal-fastdebug"
> Java(TM) SE Runtime Environment (build 
> 1.9.0-internal-fastdebug-20150624183527.jesper.main2rt-b00)
> Java HotSpot(TM) Server VM (build 
> 1.9.0-internal-fastdebug-20150624183527.jesper.main2rt-b00, mixed mode)
>
> now, we correctly exit the VM with an error, ex:
>
> java_new -XX:+IgnoreUnrecognizedVMOptions -XX:MinTLABSize=0 -version
> size_t MinTLABSize=0 is outside the allowed range [ 1 ... 
> 18446744073709551615 ]
> Improperly specified VM option 'MinTLABSize=0'
> Error: Could not create the Java Virtual Machine.
> Error: A fatal exception has occurred. Program will exit.
>
> In addition IgnoreUnrecognizedVMOptions nows strictly follows the spec 
> as outlined in JDK-8129855. The behavior changes depending on whether 
> the build is product or debug.
>
> We also introduce a new test that verifies all known use cases that we 
> care about.
>
> References:
>      bugid: https://bugs.openjdk.java.net/browse/JDK-8129855
>     webrev: http://cr.openjdk.java.net/~gziemski/8129855_rev0
> discussion: 
> http://mail.openjdk.java.net/pipermail/hotspot-dev/2015-June/019213.html
>
> Passes "JPRT hotspot" and "RBT testlist,noncolo.testlist quick"
>
>
> cheers