RFR (L): 8073500: Prevent certain commercial flags from being changed at runtime
Coleen Phillimore
coleen.phillimore at oracle.com
Tue May 10 17:18:36 UTC 2016
On 5/10/16 11:11 AM, Ioi Lam wrote:
>
>
> 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 thought other flags would get writeability control, and this puts the
framework in.
>>>
>>> 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.
This looks like an improvement but I think we want to go further and not
use macros. Have something preprocess some lists instead, but I don't
have any good suggestions now. It's definitely something worth working on.
Thanks,
Coleen
>>
>>
>> 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