RFR: 8224878: Use JVMFlag parameters instead of name strings

[David Holmes]

Thanks Stefan! Looks good.

David

On 29/05/2019 10:51 pm, Stefan Karlsson wrote:
> Hi David,
> 
> Thanks for reviewing this! I've updated the patch with your suggested 
> name change:
>   http://cr.openjdk.java.net/~stefank/8224878/webrev.03.delta/
>   http://cr.openjdk.java.net/~stefank/8224878/webrev.03/
> 
> Thanks,
> StefanK
> 
> On 2019-05-29 12:19, David Holmes wrote:
>> Hi Stefan,
>>
>> This all looks okay to me - working with the Flag instead of the name 
>> seems a lot clearer.
>>
>> My only suggestion, as per other email, is to change 
>> find_flag_unconstrained to find_declared_flag. But up to you.
>>
>> Thanks,
>> David
>>
>> On 29/05/2019 3:15 am, Stefan Karlsson wrote:
>>> To restrict the usages of flags fetched with find_flag_constrained 
>>> (or whatever we'll call it), I changed it to return a const JVMFlag*. 
>>> This way flags retrieved from that function won't be writable. If you 
>>> want to write to a flag, you need to use the find_flag function, 
>>> which does the appropriate checks (not locked, nor 
>>> is_constant_in_binary).
>>>
>>> Updated webrevs:
>>>   https://cr.openjdk.java.net/~stefank/8224878/webrev.02.delta
>>>   https://cr.openjdk.java.net/~stefank/8224878/webrev.02
>>>
>>> Thanks,
>>> StefanK
>>>
>>> On 2019-05-28 15:23, Stefan Karlsson wrote:
>>>> Hi David,
>>>>
>>>> On 2019-05-28 15:05, David Holmes wrote:
>>>>> Hi Stefan,
>>>>>
>>>>> Just in relation to the find_flag(name) versus find_flag(name, len) 
>>>>> issue, I'm trying to see the simplest thing to "flip" so that it 
>>>>> makes more sense, but to be honest I don't understand why we use 
>>>>> the find_flag(name, len) form in the places that we do. For example:
>>>>>
>>>>> JVMFlag::Error JVMFlag::boolAtPut(const char* name, size_t len, 
>>>>> bool* value, JVMFlag::Flags origin) {
>>>>>    JVMFlag* result = JVMFlag::find_flag(name, len);
>>>>>    return boolAtPut(result, value, origin);
>>>>> }
>>>>>
>>>>> won't find a locked flag - but why not? I don't see why this should 
>>>>> be a function that only applies to unlocked flags. ??
>>>>
>>>> The patch above actually removes that code:
>>>> -JVMFlag::Error JVMFlag::boolAtPut(const char* name, size_t len, 
>>>> bool* value, JVMFlag::Flags origin) {
>>>> -  JVMFlag* result = JVMFlag::find_flag(name, len);
>>>> -  return boolAtPut(result, value, origin);
>>>> -}
>>>>
>>>> and left is the version that doesn't care if the flag is locked or not:
>>>> JVMFlag::Error JVMFlag::boolAtPut(JVMFlag* flag, bool* value, 
>>>> JVMFlag::Flags origin) {
>>>>
>>>> However, now that you mention it, maybe that's unwise. Maybe we 
>>>> actually should check that we don't set locked (or 
>>>> is_constant_in_binary) flags.
>>>>
>>>>>
>>>>> But given we are talking about locked versus unlocked flags then 
>>>>> find_flag_unlocked() would seem more appropriate than 
>>>>> find_flag_unconstrained().
>>>>
>>>> There's an extra dimension to this:
>>>>
>>>>  896 // Search the flag table for a named flag
>>>>  897 JVMFlag* JVMFlag::find_flag(const char* name, size_t length, 
>>>> bool allow_locked, bool return_flag) {
>>>>  898   for (JVMFlag* current = &flagTable[0]; current->_name != 
>>>> NULL; current++) {
>>>>  899     if (str_equal(current->_name, current->get_name_length(), 
>>>> name, length)) {
>>>>  900       // Found a matching entry.
>>>>  901       // Don't report notproduct and develop flags in product 
>>>> builds.
>>>>  902       if (current->is_constant_in_binary()) {
>>>>  903         return (return_flag ? current : NULL);
>>>>  904       }
>>>>  905       // Report locked flags only if allowed.
>>>>  906       if (!(current->is_unlocked() || current->is_unlocker())) {
>>>>  907         if (!allow_locked) {
>>>>  908           // disable use of locked flags, e.g. diagnostic, 
>>>> experimental,
>>>>  909           // etc. until they are explicitly unlocked
>>>>  910           return NULL;
>>>>  911         }
>>>>  912       }
>>>>  913       return current;
>>>>  914     }
>>>>  915   }
>>>>  916   // JVMFlag name is not in the flag table
>>>>  917   return NULL;
>>>>  918 }
>>>>
>>>> it's not only locked flags that get returned. See lines 902-903. Do 
>>>> you still think find_flag_unlocked would be a better name?
>>>>
>>>>>
>>>>> More tomorrow ...
>>>>
>>>> Thanks,
>>>> StefanK
>>>>
>>>>>
>>>>> Cheers,
>>>>> David
>>>>>
>>>>> On 28/05/2019 10:44 pm, Stefan Karlsson wrote:
>>>>>> And here's the webrev:
>>>>>>   http://cr.openjdk.java.net/~stefank/8224878/webrev.01/
>>>>>>
>>>>>> Thanks,
>>>>>> StefanK
>>>>>>
>>>>>> On 2019-05-28 13:35, Stefan Karlsson wrote:
>>>>>>> Hi all,
>>>>>>>
>>>>>>> Please review this patch to use JVMFlag parameters instead of 
>>>>>>> name strings in the JVM flag handling code.
>>>>>>>
>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8224878
>>>>>>>
>>>>>>> The intention is to reduce the places where the API uses "const 
>>>>>>> char* name" parameters to describe a JVM flag, and instead use a 
>>>>>>> JVMFlag* when performing various flag handling code. The places 
>>>>>>> where we translate from "const char* name" to JVMFlag* has been 
>>>>>>> pushed to the outer layers of the code, which explicitly uses the 
>>>>>>> JVMFlag::find_flag functions.
>>>>>>>
>>>>>>> This allows us to replace strcmp with simple JVMFlag pointer 
>>>>>>> equality checks in jvmFlagRangeList.cpp and 
>>>>>>> jvmFlagConstraintList.cpp.
>>>>>>>
>>>>>>> This gets rid of the need to store a typed pointer to the flag in 
>>>>>>> jvmFlagRangeList.cpp and jvmFlagConstraintList.cpp.
>>>>>>>
>>>>>>> This gets rid of the the need to propagate the 'allow_locked' and 
>>>>>>> 'return_flag' parameters down the call chain. For example, see 
>>>>>>> the changes to JVMFlag::<type>At and JVMFlag::<type>AtPut:
>>>>>>> -  static JVMFlag::Error intAt(const char* name, int* value, bool 
>>>>>>> allow_locked = false, bool return_flag = false)      { return 
>>>>>>> intAt(name, strlen(name), value, allow_locked, return_flag); }
>>>>>>> +  static JVMFlag::Error intAt(const JVMFlag* flag, int* value);
>>>>>>>    static JVMFlag::Error intAtPut(JVMFlag* flag, int* value, 
>>>>>>> JVMFlag::Flags origin);
>>>>>>> -  static JVMFlag::Error intAtPut(const char* name, size_t len, 
>>>>>>> int* value, JVMFlag::Flags origin);
>>>>>>> -  static JVMFlag::Error intAtPut(const char* name, int* value, 
>>>>>>> JVMFlag::Flags origin)   { return intAtPut(name, strlen(name), 
>>>>>>> value, origin); }
>>>>>>>
>>>>>>>   It changes the JVMFlag::find_flag API. The way it uses default 
>>>>>>> values have a surprising effect for users of those functions. 
>>>>>>> find_flag(name) searches among all flags (even locked and 
>>>>>>> constants), while find_flag(name, strlen(name)) doesn't return 
>>>>>>> locked or constants flags. To make it less likely to accidentally 
>>>>>>> call the wrong version, this has been changed to the following:
>>>>>>> * JVMFlag::find_flag(name) - Fetches the flag if it is available 
>>>>>>> (unlocked and not constant).
>>>>>>> * JVMFlag::find_flag_unrestricted(name) - Fetches the flag even 
>>>>>>> if it is locked or a constant.
>>>>>>>
>>>>>>> Removed the unused JVMFlag::wasSetOnCmdline.
>>>>>>>
>>>>>>> Small cleanups
>>>>>>> - Renamed address_of_flag to flag_from_enum
>>>>>>> - Renamed local JVMFlag* variable from results to flag
>>>>>>> - Use initializer lists
>>>>>>> - Removed superfluous semicolons
>>>>>>>
>>>>>>> I don't mind splitting patch up into more than one RFE, if that 
>>>>>>> seems more appropriate.
>>>>>>>
>>>>>>> Tested with:
>>>>>>> - test/hotspot/jtreg/runtime/CommandLine
>>>>>>> - tier1 (tier2-3 80% done)
>>>>>>>
>>>>>>> Suggestions on other testing is appreciated.
>>>>>>>
>>>>>>> Thanks,
>>>>>>> StefanK
>>>