RFR: 8224878: Use JVMFlag parameters instead of name strings

[Stefan Karlsson]

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