RFR: 8224878: Use JVMFlag parameters instead of name strings

David Holmes david.holmes at oracle.com
Wed May 29 10:19:45 UTC 2019


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
> 


More information about the hotspot-runtime-dev mailing list