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

[gerard ziemski]

hi Dan,

Thank you for the review, and please see my answers in-line:


> From: Daniel D. Daugherty <daniel.daugherty at oracle.com>
> Reply-To: daniel.daugherty at oracle.com
> To: gerard ziemski <gerard.ziemski at oracle.com>, hotspot-dev at openjdk.java.net Developers <hotspot-dev at openjdk.java.net>, Dmitry Dmitriev <dmitry.dmitriev at oracle.com>
> On 9/29/15 12:09 PM, gerard ziemski wrote:
>> Here is a webrev1 updated with fixes based on Dmitry's feedback (ie.
>> changes to the java test only):
>>
>> http://cr.openjdk.java.net/~gziemski/8129855_rev1
>
> src/share/vm/runtime/arguments.cpp
>    It took a couple of reading attempts (L893-899), but I'm
>    good with this file.
>
> src/share/vm/runtime/globals.cpp
>    L336:   get_locked_message_ext(buf, buflen);
>    L337:   return Flag::NONE;
>        Seems a bit strange for Flag::NONE to be returned after
>        we've called out to get_locked_message_ext(buf, buflen).
>
>        Looks like the protocol is to check buf[0]. If it's
>        non-NULL: (buf[0] != '\0'), then we've gotten a message
>        from the extension…

There are currently no cases from get_locked_message_ext() that we are interested in, so I didn’t bother to extend 
Flag::MsgType with more types and code there.


>
> src/share/vm/runtime/globals.hpp
>    No comments.
>
> test/runtime/CommandLine/IgnoreUnrecognizedVMOptions.java
>    L29  * @summary -XX:+IgnoreUnrecognizedVMOptions should work
> according to the spec from https:...
>
>        Is this URL persistent and will it survive the test
>        of time (and DB changes)?

I honestly don’t know, but since there are no other places anywhere else where we link in such matter, I took it away 
and replaced with:

@summary -XX:+IgnoreUnrecognizedVMOptions should work according to the spec from JDK-8129855


>
>    L84:     runJavaAndCheckExitValue(false,
> "-XX:-IgnoreUnrecognizedVMOptions", "-XX:-UnlockDiagnosticVMOptions",
> "-XX:+AlwaysSafeConstructors", "-version");
>
>        Should this be '-XX:-UnlockExperimentalVMOptions’?
>

Yes, fixed.


>
>    L85:     runJavaAndCheckExitValue(false,
> "-XX:-IgnoreUnrecognizedVMOptions", "-XX:-UnlockDiagnosticVMOptions",
> "-XX:+FlightRecorder", "-version");
>
>        Should this be '-XX:-UnlockCommercialFeatures’?

Yes, fixed.


>
>    L87:     runJavaAndCheckExitValue(false,
> "-XX:+IgnoreUnrecognizedVMOptions", "-XX:-UnlockDiagnosticVMOptions",
> "-XX:+AlwaysSafeConstructors", "-version");
>
>        Should this be '-XX:-UnlockExperimentalVMOptions’?

Yes, fixed.


>
>    L88:     runJavaAndCheckExitValue(false,
> "-XX:+IgnoreUnrecognizedVMOptions", "-XX:-UnlockDiagnosticVMOptions",
> "-XX:+FlightRecorder", "-version");
>
>        Should this be '-XX:-UnlockCommercialFeatures’?

Yes, fixed.


>
>    L126-131: seems like you should narrow down the use of
>        "-XX:-IgnoreUnrecognizedVMOptions" and
> "-XX:-UnlockDiagnosticVMOptions"
>        to just the options to which they apply.

Actually the problem here is similar to line L84-L88 fixes that you pointed out above. Fixed.

I took the opportunity here when fixing these cases to add another section, "malformed unlocked flags”, which next to 
the already existing "locked flags” and "malformed locked flags” should give the test better coverage.


Thank you for the review!

I will re-test and post rev 1 soon.


cheers


>>
>>
>> cheers
>>
>> On 09/28/2015 04:55 PM, 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
>>>
>>
>
>