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

[Ioi Lam]

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.

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
>>>>
>>