RFR (L): 8073500: Prevent certain commercial flags from being changed at runtime
Ioi Lam
ioi.lam at oracle.com
Tue May 10 12:51:10 UTC 2016
I have the same concern as David. There is a large amount of
boiler-plate changes for just a few flags that require the writeability
control. I am also guilty for this since this based on a style of macro
usage that I suggested before :-(
I understand the reason to push this now because it's an REF. The
changes look OK to me (at least no worse than before) so I think it's OK
to check it in (but only for scheduling reasons).
Just one suggestion:
How about replacing the following
64 void emit_writeable_bool(const char* /*name*/) { /* NOP */ }
65 ...
74
75 // CommandLineFlagWriteable emitting code functions if range
arguments are provided
76 void emit_writeable_bool(const char* name,
CommandLineFlagWriteable::WriteableType type) {
77 CommandLineFlagWriteableList::add(new
CommandLineFlagWriteable(name, type));
78 }
...
with macros like
#define EMIT_WRITABLE_FUNC(kind) \
/* No control emitting if type argument is NOT provided */ \
void emit_writeable_#kind(const char* /*name*/) { /* NOP */ } \
/* CommandLineFlagWriteable emitting code functions if range arguments
are provided */ \
void emit_writeable_#kind(const char* name,
CommandLineFlagWriteable::WriteableType type) {
CommandLineFlagWriteableList::add(new
CommandLineFlagWriteable(name, type));
}
EMIT_WRITABLE_FUNC(bool)
EMIT_WRITABLE_FUNC(ccstr)
....
For the future:
To avoid the large amount of boiler-plate changes, I would suggest doing
the following as an update RFE:
[0] Remove range/constrain/writable from all _XXX_FLAGS macros
[1] Unify all the XXX_FLAGS into a single macro.
Instead of using multiple blocks of XXX_FLAGS everywhere, like this
RUNTIME_FLAGS(MATERIALIZE_DEVELOPER_FLAG, \
MATERIALIZE_PD_DEVELOPER_FLAG, \
MATERIALIZE_PRODUCT_FLAG, \
MATERIALIZE_PD_PRODUCT_FLAG, \
MATERIALIZE_DIAGNOSTIC_FLAG, \
MATERIALIZE_EXPERIMENTAL_FLAG, \
MATERIALIZE_NOTPRODUCT_FLAG, \
MATERIALIZE_MANAGEABLE_FLAG, \
MATERIALIZE_PRODUCT_RW_FLAG, \
MATERIALIZE_LP64_PRODUCT_FLAG, \
IGNORE_RANGE, \
IGNORE_CONSTRAINT)
RUNTIME_OS_FLAGS(MATERIALIZE_DEVELOPER_FLAG, \
MATERIALIZE_PD_DEVELOPER_FLAG, \
MATERIALIZE_PRODUCT_FLAG, \
MATERIALIZE_PD_PRODUCT_FLAG, \
MATERIALIZE_DIAGNOSTIC_FLAG, \
MATERIALIZE_NOTPRODUCT_FLAG, \
IGNORE_RANGE, \
IGNORE_CONSTRAINT)
Define a global ALL_FLAGS macro:
ADD all_globals.hpp:
#include <globals.hpp>
#if INCLUDE_ALL_GCS
#include "gc/g1/g1_globals.hpp"
#endif // INCLUDE_ALL_GCS
#ifdef COMPILER1
#include "c1/c1_globals.hpp"
#endif
#if INCLUDE_JVMCI
#include "jvmci/jvmci_globals.hpp"
#endif
#ifdef COMPILER2
#include "opto/c2_globals.hpp"
#endif
#ifdef SHARK
#include "shark/shark_globals.hpp"
#endif
#include
.... etc
#ifndef JVMCI_FLAGS
#define JVMCI_FLAGS(develop, develop_pd, product, product_pd,
diagnostic, \
experimental, notproduct) /* do nothing */
#endif
#define ALL_FLAGS(develop, develop_pd, product, product_pd,
diagnostic, \
experimental, notproduct, manageable, product_rw) \
RUNTIME_FLAGS(develop, develop_pd, product, product_pd, diagnostic, \
experimental, notproduct, manageable, product_rw) \
RUNTIME_OS_FLAGS(develop, develop_pd, product, product_pd, diagnostic, \
experimental, notproduct) \
JVMCI_FLAGS(develop, develop_pd, product, product_pd, diagnostic, \
experimental, notproduct) \
....
I.e., skip any flags that RUNTIME_OS_FLAGS doesn't need.
Add an empty JVMCI_FLAGS macro if one doesn't exist.
Then, all flag materialization can be done in a single block in globals.cpp:
#define range(a, b) IGNORE_RANGE(a,b)
#define constraint(func, type) IGNORE_CONSTRAINT(func,type)
ALL_FLAGS(MATERIALIZE_DEVELOPER_FLAG, \
MATERIALIZE_PD_DEVELOPER_FLAG, \
MATERIALIZE_PRODUCT_FLAG, \
MATERIALIZE_PD_PRODUCT_FLAG, \
MATERIALIZE_DIAGNOSTIC_FLAG, \
MATERIALIZE_EXPERIMENTAL_FLAG, \
MATERIALIZE_NOTPRODUCT_FLAG, \
MATERIALIZE_MANAGEABLE_FLAG, \
MATERIALIZE_PRODUCT_RW_FLAG, \
MATERIALIZE_LP64_PRODUCT_FLAG) \
#undef range
#undef constraint
In commandLineFlagRangeList.cpp:
#define range(a, b) EMIT_RANGE(a,b)
#define constraint(func, type) EMIT_CONSTRAINT(func,type)
emit_range_no(NULL ALL_FLAGS(EMIT_RANGE_DEVELOPER_FLAG,
EMIT_RANGE_PD_DEVELOPER_FLAG,
EMIT_RANGE_PRODUCT_FLAG,
EMIT_RANGE_PD_PRODUCT_FLAG,
EMIT_RANGE_DIAGNOSTIC_FLAG,
EMIT_RANGE_EXPERIMENTAL_FLAG,
EMIT_RANGE_NOTPRODUCT_FLAG,
EMIT_RANGE_MANAGEABLE_FLAG,
EMIT_RANGE_PRODUCT_RW_FLAG,
EMIT_RANGE_LP64_PRODUCT_FLAG);
and then you can delete all the files like g1_globals.cpp whose sole
purpose is to materialize flags for a given XXX_FLAGS macro.
In then future, if you want to add a new attribute, or change the usage
of an existing attribute (such as range), you don't need to modify a
large number of pd files.
Thanks
- Ioi
On 5/8/16 4:39 PM, David Holmes wrote:
> On 7/05/2016 2:02 AM, Gerard Ziemski wrote:
>> hi David,
>>
>>> On May 5, 2016, at 3:55 PM, David Holmes <david.holmes at oracle.com>
>>> wrote:
>>>
>>> Hi Gerard,
>>>
>>> How does this relate to the existing Manageable and Product_rw flags ??
>>
>> Good questions:
>>
>> #1 Re: Manageable
>>
>> There is some overlap when it comes to setting the flag during the
>> runtime (ie. vie jcmd), which will need to be reconciled (in a follow
>> up task), but this mechanism is more general (with possibly more
>> writeable types added in the future) and applies to all stages of the
>> VM runtime, including the startup.
>>
>>
>> #2 Re: product_rw
>>
>> I don’t see that used anywhere, we should remove it (in a follow up
>> task).
>
> I think every time we add yet-another-kind-of-flag we lament that
> there is not a better way to express this as an attribute of the
> existing flags type. Writability is an orthogonal concept (as is
> "managed") to product/develop/experimental/etc and I hate to see
> yet-another-flag-type added.
>
> That's just a comment, not a blocker, but also not a review as I only
> skimmed this.
>
> David
> -----
>
>>
>> cheers
>>
>>
>>> Thanks,
>>> David
>>>
>>> On 3/05/2016 2:29 AM, Gerard Ziemski wrote:
>>>> hi all,
>>>>
>>>> Please review this fix (large code change, but relatively
>>>> straightforward implementation wise), which implements a mechanism
>>>> for enforcing when and how a runtime flag may be modified. There
>>>> are 3 new types, which can be assigned:
>>>>
>>>> - writeable(Always) is optional and default value, which places no
>>>> limits on when or how often a flag may be changed
>>>>
>>>> - writeable(Once) denotes that a flag may be >> changed << only
>>>> once. We allow case such as “java -XX:+UnlockCommercialFeatures ...
>>>> -XX:+UnlockCommercialFeatures ...” to support existing usage. This
>>>> is OK, because here the flag only changes its value >> once << ,
>>>> ie. from false to true.
>>>>
>>>> - writeable(CommandLineOnly) denotes that a flag may only be
>>>> changed from the command line (tools like jcmd may not change such
>>>> a flag during the runtime)
>>>>
>>>> The implementation is based on the same mechanism as that in
>>>> JEP-245 (https://bugs.openjdk.java.net/browse/JDK-8059557)
>>>>
>>>>
>>>> bug: https://bugs.openjdk.java.net/browse/JDK-8073500
>>>> webrev: http://cr.openjdk.java.net/~gziemski/8073500_rev1/
>>>>
>>>> Tested with “JPRT hotspot” and “RBT hs-nightly-runtime”
>>>>
>>>>
>>>> cheers
>>>>
>>
More information about the hotspot-runtime-dev
mailing list