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

Wed Jan 18 07:00:30 PST 2012

Updated webrev: http://cr.openjdk.java.net/~rottenha/7130391/webrev.01

Changes to the previous version are:
   * src/share/vm/runtime/globals.cpp:
Remove validation control from <type>AtPut(CommandLineFlagsWithType, 
...), that is only used by FLAG_SET_* macros in globals_extension.hpp.

   * src/share/vm/runtime/{globals.hpp, globals.cpp, globals_ext.hpp}:
Replace multiple public type safe functions 
Flag::is_valid[_ext]_<type>(<type> value, ...) by single protected type 
generic functions CommandLineFlags::is_valid[_ext (const Flag*, const 
void*, ...),  then do internal type casts on the values based on the 
type of the targeted flag (and assert on type correctness).

   * src/share/vm/services/management.cpp:
Use a better error message (David Holmes).

/Robert

On 01/17/2012 02:41 PM, Robert Ottenhag wrote:
> 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
--