RFR (L): 8073500: Prevent certain commercial flags from being changed at runtime
Ioi Lam
ioi.lam at oracle.com
Tue May 10 15:11:20 UTC 2016
On 5/10/16 7:59 AM, Gerard Ziemski wrote:
> hi Ioi,
>
> Thank you very much for the review and excellent feedback. Please see my comments in-line:
>
>
>> On May 10, 2016, at 7:51 AM, Ioi Lam <ioi.lam at oracle.com> wrote:
>>
>> 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)
>> ….
> I like the suggestion. The only concern that I have is that at this time I would prefer to wait with implementing this suggestion, as we also have ranges and constraints that would benefit from this improvement. At this point I would prefer to check it is as is (tested) and file a followup issue to handle this improvement in ranges, constraints and writeables code all at the same time.
>
> Is that OK?
>
Yes, that seems fine.
Thanks
- Ioi
>
>> 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.
>>
> Excellent suggestion - I will file a follow-up issue for this improvement.
>
>
> Thank you.
>
>
>> 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