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

[gerard ziemski]

Excellent suggestions! Thank you Dmitry!


cheers

On 09/29/2015 07:50 AM, Dmitry Dmitriev wrote:
> 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
>
>