RFR (S): 7130391: Add a framework for vendor-specific validation control of setting command-line switches in Hotspot

[Robert Ottenhag]

David,

Regarding the FLAG_SET_* macros, I am thinking that we can leave them to 
a follow up bug instead.

The reason is that it can be verified by code inspection (of 
preprocessed sources) if any FLAG_SET_* macro writes to a variable known 
to have validation control.

Also, fixing that hole would require any access to the variables to 
occur through interface get/set functions, preventing direct read and 
write access (wrapping the variable in a class to prevent direct 
writes), a change too intrusive for now.

Will come back with an updated and cleaned up patch.

/Robert

On 01/17/2012 01:07 PM, Robert Ottenhag wrote:
> David,
>
> Thanks for the review.
>
> On 01/17/2012 04:09 AM, David Holmes wrote:
>> Hi Robert,
>>
>> I've added serviceability to the cc list.
>
> Good, will try to remember that ;-)
>
>>
>> On 17/01/2012 12:04 PM, Robert Ottenhag wrote:
>>> Webrev: http://cr.openjdk.java.net/~rottenha/7130391/webrev.00
>>>
>>> This fix adds optional validation control to the setting of 
>>> command-line switches in Hotspot, and allows it to have 
>>> vendor-specific extensions if necessary.
>>
>> Does this imply that the Java management APIs (eg 
>> com.sun.management.VMOption) need to be changed to reflect these 
>> restrictions? Presently VMOptions are either writeable or not, but 
>> this makes them conditionally-writeable.
>
> No, since the Java management  APIs already cares for conditional 
> writes. According to 
> com.sun.management.HotSpotDiagnosticMXBean.setVMOption() it will throw 
> IllegalArgumentException if the new value is invalid.
>
>>
>>> The design follows the previously added framework for 
>>> vendor-specific command-line switch extensions in CR7117389.
>>>
>>> The validation control is handled by new boolean methods 
>>> Flag::is_valid_<type>(value,origin) that are called at the beginning 
>>> of each call to CommandLineFlags[Ex]::<type>AtPut() to verify that 
>>> the new value and origin are valid replacements for the current 
>>> value and origin for this flag.
>>>
>>> When parsing the command line options, a failed validation will 
>>> typically result in an error message and exit with "Unrecognized VM 
>>> option '<flag-name>'".  When used dynamically using the attach API 
>>> or management API the resulting operation will fail, leaving it up 
>>> to the caller to handle it as appropriate.
>>
>> The error message doesn't really seem appropriate - it may well be a 
>> recognized option, you just can't set it to that value in that way. 
>> Ideally there would be a way for the validation logic to supply a 
>> meaningful error message. In its absence the top-level message should 
>> reflect the new type of error.
>
> You are absolutely right, but the current fix is in line with the 
> existing bad error messages where any kind of malformatted command 
> line flags results in Unrecognized VM option, whether the reason is an 
> unknown name, bad type semantics (using +- for bool semantics on an 
> integer flag), or if the flag is locked.
>
> I will target meaningful error messages for command line parsing in a 
> direct follow up bug to this fix.
>
>>
>> Also some of the failures lead to crashes - which seems wrong to me - 
>> see below.
>>
>> ----
>>
>> src/share/vm/services/management.cpp:
>>
>> 1821   if (!succeed) {
>> 1822     THROW_MSG(vmSymbols::java_lang_IllegalArgumentException(),
>> 1823               "This flag is not writeable with this value or 
>> origin.");
>>
>> That's a rather cryptic error message. How about:
>>
>> "Flag can not be set to the requested value using this API"
>>
>> ?
>
> Yes, "origin" does not make sense to the upper Java layer. I will use 
> your suggestion.
>
>>
>> ----
>>
>> src/share/vm/runtime/globals_ext.hpp
>>
>> With all the
>>
>> inline bool Flag::is_valid_ext_T(T value, FlagValueOrigin origin)
>>
>> functions, is it necessary to include the type T in the function name?
>
> It is necessary if using type safe variants with T value as argument 
> since overloading does not differ between different typedef names that 
> resolves to the same native types, e.g. uintx and uint64_t are both 
> unsigned long int.
>
> I am considering a condensed variant that replaces T by void* instead, 
> and do the type casting based on the targeted flag, reducing the 
> number of functions.
>
>>
>>
>> -----
>>
>> src/share/vm/runtime/globals.cpp
>>
>> The use of the guarantees seems wrong as it means an invalid option 
>> will trigger a VM crash rather than a clean exit during 
>> initialization. It seems to me that none of the code in arguments.cpp 
>> that uses the FLAG_SET_* macros (which in turn use the 
>> CommandLineFlagsEx functions you added the guarantees to) anticipates 
>> any possibility for failure. I think if you are going this path then 
>> you have no choice but to change the CommandLineFlagsEx methods to 
>> return bool and update the FLAG_SET macros to try and perform 
>> appropriate error handling.
>
> I see your point, and in theory such as VM crash could occur anytime 
> later in a JVM session if rarely running code would make use of 
> FLAG_SET_* to change the value of a VM flag to an invalid value or 
> origin.
>
> Seems as if the options are either to
> a) ignore validation tests for the FLAG_SET_* macros, and trust that 
> they always set valid values. This can be partly verified by static 
> code inspection by looking for any variables that actually have 
> validation logic associated to them (since the variable name is 
> defined at compile time), assuming one has access to all code, but it 
> is not perfect in case code for changing a variable with validation 
> logic exists.
> b) contain the error handling within the FLAG_SET_* macros, like using 
> guarantee(), but maybe exception logic can help?
> c) require usage of the FLAG_SET_* macros to handle result codes and 
> pass it up the call chain.
>
> Also, the current macro FLAG_SET_DEFAULT does a direct write to the 
> flag value without going through <type>AtPut(). This macro must be 
> rewritten to have validation control to close the holes. The current 
> call format will require all call sites to include type name as with 
> FLAG_SET_{CMDLINE,ERGO} has, or to use slower lookup by variable name.
>
> /Robert
>
>>
>> David
>> -----
>>
>>
>>> A simple use case for validation is a manageable flag whose current 
>>> value can not be less than the previous value, while a more complex 
>>> example may base the validation on multiple other flags, etc.
>>>
>>> Thanks,
>>>
>>> /Robert
>>>
>
>


-- 
Oracle
Robert Ottenhag | Senior Member of Technical Staff
Phone: +46850630961 | Fax: +46850630911 | Mobile: +46707106161
Oracle Java HotSpot Virtual Machine
ORACLE Sweden | Folkungagatan 122 | SE-116 30 Stockholm

Oracle Svenska AB, Kronborgsgränd 17, S-164 28 KISTA, reg.no. 556254-6746

Green Oracle

Oracle is committed to developing practices and products that help protect the environment
--