jmx-dev RFR: 8264285: Clean the modification of ccstr JVM flags [v2]

[David Holmes]

On 1/04/2021 4:19 pm, Ioi Lam wrote:
> 
> 
> On 3/31/21 10:47 PM, David Holmes wrote:
>> On 1/04/2021 3:29 pm, Ioi Lam wrote:
>>>
>>>
>>> On 3/31/21 10:22 PM, David Holmes wrote:
>>>> On 1/04/2021 5:05 am, Ioi Lam wrote:
>>>>> On Wed, 31 Mar 2021 12:58:50 GMT, Coleen Phillimore 
>>>>> <coleenp at openjdk.org> wrote:
>>>>>
>>>>>>
>>>>>>> 36: // have any MANAGEABLE flags of the ccstr type, but we really 
>>>>>>> need to
>>>>>>> 37: // make sure the implementation is correct (in terms of 
>>>>>>> memory allocation)
>>>>>>> 38: // just in case someone may add such a flag in the future.
>>>>>>
>>>>>> Could you have just added a develop flag to the manageable flags 
>>>>>> instead?
>>>>>
>>>>> I had to use a `product` flag due to the following code, which 
>>>>> should have been removed as part of 
>>>>> [JDK-8243208](https://bugs.openjdk.java.net/browse/JDK-8243208), 
>>>>> but I was afraid to do so because I didn't have a test case. I.e., 
>>>>> all of our diagnostic/manageable/experimental flags were `product` 
>>>>> flags.
>>>>>
>>>>> With this PR, now I have a test case -- I changed 
>>>>> `DummyManageableStringFlag` to a `notproduct` flag, and removed the 
>>>>> following code. I am re-running tiers1-4 now.
>>>>
>>>> Sorry but I don't understand how a test involving one flag replaces 
>>>> a chunk of code that validated every flag declaration ??
>>>>
>>>
>>> When I did JDK-8243208, I wasn't sure if the VM would actually 
>>> support diagnostic/manageable/experimental flags that were not 
>>> `product`. So I added check_all_flag_declarations() to prevent anyone 
>>> from adding such a flag "casually" without thinking about.
>>>
>>> Now that I have added such a flag, and I believe I have tested pretty 
>>> thoroughly (tiers 1-4), I think the VM indeed supports these flags. 
>>> So now I feel it's safe to remove check_all_flag_declarations().
>>
>> But the check was checking two things:
>>
>> 1. That diagnostic/manageable/experimental are mutually exclusive
>> 2. That they only apply to product flags
>>
>> I believe both of these rules still stand based on what we defined 
>> such attributes to mean. And even if you think #2 should not apply, #1 
>> still does and so could still be checked. And if #2 is no longer our 
>> rule then the documentation in globals.hpp should be updated - though 
>> IMHO #2 should remain as-is.
>>
> 
> I think neither #1 and #2 make sense. These were limitation introduced 
> by the old flags implementation, where you had to declare a flag using 
> one of these macros
> 
>      diagnostic(type, name, ....
>      manageable(type, name, ....
>      experimental(type, name, ....
> 
> That's why you have #1 (mutual exclusion).
> 
> #2 was also due to the implementation. It makes no sense that you can't 
> have an develop flag for an experimental feature.

I don't agree with either of those claims. This is about semantics not 
implementation. diagnostic/manageable/experimental are distinct kinds of 
product flags with different levels of "support" based on their intended 
use in the product. We don't need such distinctions with non-product 
flags because the level of "support" is all the same and all flags are 
equal.

David
-----

> However, in the old implementation, adding more variations would cause 
> macro explosion. See 
> https://github.com/openjdk/jdk/blame/7d8519fffe46b6b5139b3aa51b18fcf0249a9e14/src/hotspot/share/runtime/flags/jvmFlag.cpp#L776 
> 
> 
> Anyway, changing this should be done in a separate RFE. I have reverted 
> [v2] from this PR, so we are back to [v1].
> 
> Thanks
> - Ioi
> 
> 
>> David
>> -----
>>
>>
>>> Thanks
>>> - Ioi
>>>
>>>
>>>
>>>> David
>>>> -----
>>>>
>>>>> void JVMFlag::check_all_flag_declarations() {
>>>>>    for (JVMFlag* current = &flagTable[0]; current->_name != NULL; 
>>>>> current++) {
>>>>>      int flags = static_cast<int>(current->_flags);
>>>>>      // Backwards compatibility. This will be relaxed/removed in 
>>>>> JDK-7123237.
>>>>>      int mask = JVMFlag::KIND_DIAGNOSTIC | JVMFlag::KIND_MANAGEABLE 
>>>>> | JVMFlag::KIND_EXPERIMENTAL;
>>>>>      if ((flags & mask) != 0) {
>>>>>        assert((flags & mask) == JVMFlag::KIND_DIAGNOSTIC ||
>>>>>               (flags & mask) == JVMFlag::KIND_MANAGEABLE ||
>>>>>               (flags & mask) == JVMFlag::KIND_EXPERIMENTAL,
>>>>>               "%s can be declared with at most one of "
>>>>>               "DIAGNOSTIC, MANAGEABLE or EXPERIMENTAL", 
>>>>> current->_name);
>>>>>        assert((flags & KIND_NOT_PRODUCT) == 0 &&
>>>>>               (flags & KIND_DEVELOP) == 0,
>>>>>               "%s has an optional DIAGNOSTIC, MANAGEABLE or 
>>>>> EXPERIMENTAL "
>>>>>               "attribute; it must be declared as a product flag", 
>>>>> current->_name);
>>>>>      }
>>>>>    }
>>>>> }
>>>>>
>>>>> -------------
>>>>>
>>>>> PR: https://git.openjdk.java.net/jdk/pull/3254
>>>>>
>>>
>