RFR (L): 8073500: Prevent certain commercial flags from being changed at runtime
Gerard Ziemski
gerard.ziemski at oracle.com
Tue May 10 14:59:11 UTC 2016
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?
> 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