RFR: 8224878: Use JVMFlag parameters instead of name strings

Tue May 28 17:15:08 UTC 2019

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