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.

Sign up for GitHub

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: Clean the modification of ccstr JVM flags #3254

Closed
wants to merge 5 commits into from
+181 −45
Closed

8264285: Clean the modification of ccstr JVM flags #3254

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
4fe5df9
8264285: Do not support FLAG_SET_XXX for VM flags of string type
iklam Mar 26, 2021
7eca234
restored SET_FLAG_XXX for ccstr type, and fixed bugs in existing ccst…
iklam Mar 29, 2021
673aaaf
relax flag attributions (ala JDK-7123237)
iklam Mar 31, 2021
2a5e2b1
Revert "relax flag attributions (ala JDK-7123237)"
iklam Apr 1, 2021
5f91222
Merge branch 'master' into 8264285-clean-up-setting-ccstr-jvm-flags
iklam Apr 1, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/hotspot/share/runtime/arguments.cpp
View file
Original file line number Diff line number Diff line change
@@ -3782,6 +3782,7 @@ static void apply_debugger_ergo() {

jint Arguments::parse(const JavaVMInitArgs* initial_cmd_args) {
assert(verify_special_jvm_flags(false), "deprecated and obsolete flag table inconsistent");
JVMFlag::check_all_flag_declarations();

// If flag "-XX:Flags=flags-file" is used it will be the first option to be processed.
const char* hotspotrc = ".hotspotrc";
17 changes: 16 additions & 1 deletion src/hotspot/share/runtime/flags/debug_globals.hpp
View file
Original file line number Diff line number Diff line change
@@ -38,6 +38,19 @@
// just in case someone may add such a flag in the future.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you have just added a develop flag to the manageable flags instead?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had to use a product flag due to the following code, which should have been removed as part of JDK-8243208, but I was afraid to do so because I didn't have a test case. I.e., all of our diagnostic/manageable/experimental flags were product flags.

With this PR, now I have a test case -- I changed DummyManageableStringFlag to a notproduct flag, and removed the following code. I am re-running tiers1-4 now.

void JVMFlag::check_all_flag_declarations() {
  for (JVMFlag* current = &flagTable[0]; current->_name != NULL; current++) {
    int flags = static_cast<int>(current->_flags);
    // Backwards compatibility. This will be relaxed/removed in JDK-7123237.
    int mask = JVMFlag::KIND_DIAGNOSTIC | JVMFlag::KIND_MANAGEABLE | JVMFlag::KIND_EXPERIMENTAL;
    if ((flags & mask) != 0) {
      assert((flags & mask) == JVMFlag::KIND_DIAGNOSTIC ||
             (flags & mask) == JVMFlag::KIND_MANAGEABLE ||
             (flags & mask) == JVMFlag::KIND_EXPERIMENTAL,
             "%s can be declared with at most one of "
             "DIAGNOSTIC, MANAGEABLE or EXPERIMENTAL", current->_name);
      assert((flags & KIND_NOT_PRODUCT) == 0 &&
             (flags & KIND_DEVELOP) == 0,
             "%s has an optional DIAGNOSTIC, MANAGEABLE or EXPERIMENTAL "
             "attribute; it must be declared as a product flag", current->_name);
    }
  }
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the difference between notproduct and develop again? Do we run tests with the optimized build and why would this flag be available in that build? ie. why not develop?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From the top of globals.hpp:

  • develop flags are settable / visible only during development and are constant in the PRODUCT version
  • notproduct flags are settable / visible only during development and are not declared in the PRODUCT version

Since this flag is only used in test cases, and specifically for modifying its value, it doesn't make sense to expose this flag to the PRODUCT version at all. So I chose notproduct, so we can save a few bytes for the PRODUCT as well.

//

#ifndef ASSERT

#define DEBUG_RUNTIME_FLAGS(develop, \
develop_pd, \
product, \
product_pd, \
notproduct, \
range, \
constraint) \
\

#else

#define DEBUG_RUNTIME_FLAGS(develop, \
develop_pd, \
product, \
@@ -46,12 +59,14 @@
range, \
constraint) \
\
notproduct(ccstr, DummyManageableStringFlag, NULL, MANAGEABLE, \
product(ccstr, DummyManageableStringFlag, NULL, MANAGEABLE, \
"Dummy flag for testing string handling in WriteableFlags") \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this is in essence a product/manageable flag in debug only mode, which doesn't make sense at all, necessitating another macro that looks honestly bizarre. So I think that means a non-product manageable flag or even a develop manageable flag is something that we want, we want to be able to write a develop flag by the management interface. I agree diagnostic and experimental flags only seem to make sense as product flags.

The doc could simply be updated. Also the doc at the top of this file refers to CCC which is no longer -> CSR.

// MANAGEABLE flags are writeable external product flags.
// They are dynamically writeable through the JDK management interface
// (com.sun.management.HotSpotDiagnosticMXBean API) and also through JConsole.
// These flags are external exported interface (see CCC). The list of
// manageable flags can be queried programmatically through the management
// interface.

I'm not going to spend time typing about this minor point. The improvement is good and should be checked in.

\

// end of DEBUG_RUNTIME_FLAGS

#endif // ASSERT

DECLARE_FLAGS(DEBUG_RUNTIME_FLAGS)

#endif // SHARE_RUNTIME_DEBUG_GLOBALS_HPP
19 changes: 19 additions & 0 deletions src/hotspot/share/runtime/flags/jvmFlag.cpp
View file
Original file line number Diff line number Diff line change
@@ -681,6 +681,25 @@ void JVMFlag::assert_valid_flag_enum(JVMFlagsEnum i) {
assert(0 <= int(i) && int(i) < NUM_JVMFlagsEnum, "must be");
}

void JVMFlag::check_all_flag_declarations() {
for (JVMFlag* current = &flagTable[0]; current->_name != NULL; current++) {
int flags = static_cast<int>(current->_flags);
// Backwards compatibility. This will be relaxed/removed in JDK-7123237.
int mask = JVMFlag::KIND_DIAGNOSTIC | JVMFlag::KIND_MANAGEABLE | JVMFlag::KIND_EXPERIMENTAL;
if ((flags & mask) != 0) {
assert((flags & mask) == JVMFlag::KIND_DIAGNOSTIC ||
(flags & mask) == JVMFlag::KIND_MANAGEABLE ||
(flags & mask) == JVMFlag::KIND_EXPERIMENTAL,
"%s can be declared with at most one of "
"DIAGNOSTIC, MANAGEABLE or EXPERIMENTAL", current->_name);
assert((flags & KIND_NOT_PRODUCT) == 0 &&
(flags & KIND_DEVELOP) == 0,
"%s has an optional DIAGNOSTIC, MANAGEABLE or EXPERIMENTAL "
"attribute; it must be declared as a product flag", current->_name);
}
}
}

#endif // ASSERT

void JVMFlag::printFlags(outputStream* out, bool withComments, bool printRanges, bool skipDefaults) {
1 change: 1 addition & 0 deletions src/hotspot/share/runtime/flags/jvmFlag.hpp
View file
Original file line number Diff line number Diff line change
@@ -179,6 +179,7 @@ class JVMFlag {
static JVMFlag* fuzzy_match(const char* name, size_t length, bool allow_locked = false);

static void assert_valid_flag_enum(JVMFlagsEnum i) NOT_DEBUG_RETURN;
static void check_all_flag_declarations() NOT_DEBUG_RETURN;

inline JVMFlagsEnum flag_enum() const {
JVMFlagsEnum i = static_cast<JVMFlagsEnum>(this - JVMFlag::flags);