RFR: 8264285: Do not support FLAG_SET_XXX for VM flags of string type

[Ioi Lam]

On Mon, 29 Mar 2021 05:50:40 GMT, Ioi Lam <iklam at openjdk.org> wrote:

>> We have two versions of `JVMFlagAccess::ccstrAtPut()` that are slightly different.
>> 
>> The following version is supposed to be used only by the `FLAG_SET_{CMDLINE,ERGO,MGMT}` macros. However, it's not used anywhere in the HotSpot source code:
>> 
>> JVMFlag::Error JVMFlagAccess::ccstrAtPut(JVMFlagsEnum flag, ccstr value, JVMFlagOrigin origin) {
>>   JVMFlag* faddr = JVMFlag::flag_from_enum(flag);
>>   assert(faddr->is_ccstr(), "wrong flag type");
>>   ccstr old_value = faddr->get_ccstr();
>>   trace_flag_changed<ccstr, EventStringFlagChanged>(faddr, old_value, value, origin);
>>   char* new_value = os::strdup_check_oom(value);
>>   faddr->set_ccstr(new_value);
>>   if (!faddr->is_default() && old_value != NULL) {
>>     // Prior value is heap allocated so free it.
>>     FREE_C_HEAP_ARRAY(char, old_value);
>>   }
>>   faddr->set_origin(origin);
>>   return JVMFlag::SUCCESS;
>> }
>> 
>> It's not clear whether this unused version is actually correct since the last JVMFlag rewrite in [JDK-8081833](https://bugs.openjdk.java.net/browse/JDK-8081833), due to complete lack of testing. Let's remove this version to simplify code maintenance.
>> 
>> If you need to modify flags of the string type, do not use `FLAG_SET_{CMDLINE,ERGO,MGMT}`. (A `static_assert` is added to prevent this). Instead, use the remaining version of `JVMFlagAccess::ccstrAtPut()`.
>
>> > If you need to modify flags of the string type, do not use `FLAG_SET_{CMDLINE,ERGO,MGMT}`. (A `static_assert` is added to prevent this). Instead, use the remaining version of `JVMFlagAccess::ccstrAtPut()`.
>> 
>> ... and using ccstrAtPut doesn't update the origin of the flag as might
>> be desired when using the macros.
> 
> This is the version I removed:
> 
>   static JVMFlag::Error ccstrAtPut(JVMFlagsEnum flag, ccstr value, JVMFlagOrigin origin);
> 
> This is the remaining version:
> 
>   static JVMFlag::Error ccstrAtPut(JVMFlag* flag, ccstr* value, JVMFlagOrigin origin);
> 
> So they are practically the same API. The origin is changed in both. The only difference is they have unobvious subtle difference in how they handle the buffer allocation.

Thanks to @dholmes-ora for pointing out the problems with this PR. I have changed the REF to fix the problem differently. To avoid confusion, I am closing this PR. 

I opened a new PR (https://github.com/openjdk/jdk/pull/3254) for the new fix.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3219