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

[David Holmes]

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.

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