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

[David Holmes]

On 29/03/2021 3:53 pm, Ioi Lam wrote:
> On Fri, 26 Mar 2021 16:21:03 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()`.
> 
>>
>> ... 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.

Sorry I'm confused, if both support setting the origin why can't you use 
this version with the macros?

Thanks,
David
-----

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