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
--
