RFR: 8224878: Use JVMFlag parameters instead of name strings

[John Rose]

Reviewed.  This is a very good cleanup on
a mini-framework which used to be simple
and has grown hairy over the years.

I think you should add an expression evaluator
feature now to the flag framework.  …Not.
Just kidding!  It's plenty complicated already.
Thanks for making it more manageable.

— John

On May 28, 2019, at 10:15 AM, Stefan Karlsson <stefan.karlsson at oracle.com> 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
>