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

[David Holmes]

Hi Ioi,

On 27/03/2021 3:01 am, Ioi Lam 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.

I agree it may not work because it is untested but I don't agree it 
should be removed as we may want to set a string flag this way ...

> 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.

I'd rather see a test introduced to sanity check the operation if possible.

There is a tension between writing balanced API's and not introducing 
"dead code". We seem to be swinging the style pendulum more to the "get 
rid of all dead code" end, rather than considering the utility of 
writing a balanced API that makes a subsystem functionally complete. :(

David
-----

> -------------
> 
> Commit messages:
>   - 8264285: Do not support FLAG_SET_XXX for VM flags of string type
> 
> Changes: https://git.openjdk.java.net/jdk/pull/3219/files
>   Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=3219&range=00
>    Issue: https://bugs.openjdk.java.net/browse/JDK-8264285
>    Stats: 33 lines in 3 files changed: 7 ins; 17 del; 9 mod
>    Patch: https://git.openjdk.java.net/jdk/pull/3219.diff
>    Fetch: git fetch https://git.openjdk.java.net/jdk pull/3219/head:pull/3219
> 
> PR: https://git.openjdk.java.net/jdk/pull/3219
>