Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Conversation

iklam
Copy link
Member

@iklam iklam commented Mar 26, 2021

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, 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().


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8264285: Do not support FLAG_SET_XXX for VM flags of string type

Download

To checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/3219/head:pull/3219
$ git checkout pull/3219

To update a local copy of the PR:
$ git checkout pull/3219
$ git pull https://git.openjdk.java.net/jdk pull/3219/head

@bridgekeeper
Copy link

bridgekeeper bot commented Mar 26, 2021

👋 Welcome back iklam! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk
Copy link

openjdk bot commented Mar 26, 2021

@iklam The following label will be automatically applied to this pull request:

  • hotspot-runtime

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added the hotspot-runtime hotspot-runtime-dev@openjdk.org label Mar 26, 2021
@iklam
Copy link
Member Author

iklam commented Mar 26, 2021

/label remove hotspot-runtime
/label add hotspot-dev

@openjdk openjdk bot removed the hotspot-runtime hotspot-runtime-dev@openjdk.org label Mar 26, 2021
@openjdk
Copy link

openjdk bot commented Mar 26, 2021

@iklam
The hotspot-runtime label was successfully removed.

@openjdk
Copy link

openjdk bot commented Mar 26, 2021

@iklam
The hotspot label was successfully added.

@openjdk openjdk bot added the hotspot hotspot-dev@openjdk.org label Mar 26, 2021
@iklam iklam marked this pull request as ready for review Mar 26, 2021
@openjdk openjdk bot added the rfr Pull request is ready for review label Mar 26, 2021
@mlbridge
Copy link

mlbridge bot commented Mar 26, 2021

Webrevs

@mlbridge
Copy link

mlbridge bot commented Mar 29, 2021

Mailing list message from David Holmes on hotspot-dev:

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

@iklam
Copy link
Member Author

iklam commented Mar 29, 2021

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.

@mlbridge
Copy link

mlbridge bot commented Mar 29, 2021

Mailing list message from David Holmes on hotspot-dev:

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

@iklam
Copy link
Member Author

iklam commented Mar 29, 2021

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 (#3254) for the new fix.

@iklam iklam closed this Mar 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot hotspot-dev@openjdk.org rfr Pull request is ready for review
2 participants