From 4fe5df97444e2a274c77bc1617174b3dc11bd078 Mon Sep 17 00:00:00 2001 From: iklam Date: Fri, 26 Mar 2021 09:15:42 -0700 Subject: [PATCH 1/4] 8264285: Do not support FLAG_SET_XXX for VM flags of string type --- .../share/runtime/flags/jvmFlagAccess.cpp | 26 +++++-------------- .../share/runtime/flags/jvmFlagAccess.hpp | 4 +-- .../share/runtime/globals_extension.hpp | 12 +++++---- 3 files changed, 16 insertions(+), 26 deletions(-) diff --git a/src/hotspot/share/runtime/flags/jvmFlagAccess.cpp b/src/hotspot/share/runtime/flags/jvmFlagAccess.cpp index 34c9b949049df..dfc43e96bf9bf 100644 --- a/src/hotspot/share/runtime/flags/jvmFlagAccess.cpp +++ b/src/hotspot/share/runtime/flags/jvmFlagAccess.cpp @@ -29,6 +29,7 @@ #include "runtime/flags/jvmFlagAccess.hpp" #include "runtime/flags/jvmFlagLimit.hpp" #include "runtime/flags/jvmFlagConstraintsRuntime.hpp" +#include "runtime/globals_extension.hpp" #include "runtime/os.hpp" #include "utilities/macros.hpp" #include "utilities/ostream.hpp" @@ -328,31 +329,18 @@ JVMFlag::Error JVMFlagAccess::ccstrAtPut(JVMFlag* flag, ccstr* value, JVMFlagOri // This is called by the FLAG_SET_XXX macros. JVMFlag::Error JVMFlagAccess::set_impl(JVMFlagsEnum flag_enum, int type_enum, void* value, JVMFlagOrigin origin) { - if (type_enum == JVMFlag::TYPE_ccstr || type_enum == JVMFlag::TYPE_ccstrlist) { - return ccstrAtPut((JVMFlagsEnum)flag_enum, *((ccstr*)value), origin); - } + // The FLAG_SET_XXX macros should have caused an static_assert to fail if you try to use them on + // ccstr/ccstrlist options. + // + // Uncomment the following and verify that the C++ compilation will fail. + // FLAG_SET_ERGO(LogFile, ""); + assert(type_enum != JVMFlag::TYPE_ccstr && type_enum != JVMFlag::TYPE_ccstrlist, "sanity"); JVMFlag* flag = JVMFlag::flag_from_enum(flag_enum); assert(flag->type() == type_enum, "wrong flag type"); return set_impl(flag, type_enum, value, origin); } -// This is called by the FLAG_SET_XXX macros. -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(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; -} - JVMFlag::Error JVMFlagAccess::check_range(const JVMFlag* flag, bool verbose) { return access_impl(flag)->check_range(flag, verbose); } diff --git a/src/hotspot/share/runtime/flags/jvmFlagAccess.hpp b/src/hotspot/share/runtime/flags/jvmFlagAccess.hpp index 91e41ded3dad2..0a340bba4f3f0 100644 --- a/src/hotspot/share/runtime/flags/jvmFlagAccess.hpp +++ b/src/hotspot/share/runtime/flags/jvmFlagAccess.hpp @@ -1,5 +1,5 @@ /* - * Copyright (c) 2020, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2020, 2021, Oracle and/or its affiliates. All rights reserved. * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. * * This code is free software; you can redistribute it and/or modify it @@ -54,7 +54,6 @@ class JVMFlagAccess : AllStatic { inline static const FlagAccessImpl* access_impl(const JVMFlag* flag); static JVMFlag::Error set_impl(JVMFlagsEnum flag_enum, int type_enum, void* value, JVMFlagOrigin origin); static JVMFlag::Error set_impl(JVMFlag* flag, int type_enum, void* value, JVMFlagOrigin origin); - static JVMFlag::Error ccstrAtPut(JVMFlagsEnum flag, ccstr value, JVMFlagOrigin origin); public: static JVMFlag::Error check_range(const JVMFlag* flag, bool verbose); @@ -86,6 +85,7 @@ class JVMFlagAccess : AllStatic { // type_enum will result in an assert. template static JVMFlag::Error set(JVMFlagsEnum flag_enum, T value, JVMFlagOrigin origin) { + static_assert(type_enum != JVMFlag::TYPE_ccstr && type_enum != JVMFlag::TYPE_ccstrlist, "not supported"); return set_impl(flag_enum, type_enum, &value, origin); } diff --git a/src/hotspot/share/runtime/globals_extension.hpp b/src/hotspot/share/runtime/globals_extension.hpp index fd7b83c22a3d4..c980c75e77328 100644 --- a/src/hotspot/share/runtime/globals_extension.hpp +++ b/src/hotspot/share/runtime/globals_extension.hpp @@ -1,5 +1,5 @@ /* - * Copyright (c) 2003, 2020, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2003, 2021, Oracle and/or its affiliates. All rights reserved. * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. * * This code is free software; you can redistribute it and/or modify it @@ -49,10 +49,12 @@ enum JVMFlagsEnum : int { NUM_JVMFlagsEnum }; -// Construct set functions for all flags +// Set functions for all flags -- use a template so we can do static_assert +// at individual call sites of FLAG_SET_{CMDLINE,ERGO,MGMT} #define FLAG_MEMBER_SETTER(name) Flag_##name##_set #define FLAG_MEMBER_SETTER_(type, name) \ + template \ inline JVMFlag::Error FLAG_MEMBER_SETTER(name)(type value, JVMFlagOrigin origin) { \ return JVMFlagAccess::set(FLAG_MEMBER_ENUM(name), value, origin); \ } @@ -75,9 +77,9 @@ ALL_FLAGS(DEFINE_FLAG_MEMBER_SETTER, #define FLAG_SET_DEFAULT(name, value) ((name) = (value)) #define FLAG_SET_CMDLINE(name, value) (JVMFlag::setOnCmdLine(FLAG_MEMBER_ENUM(name)), \ - FLAG_MEMBER_SETTER(name)((value), JVMFlagOrigin::COMMAND_LINE)) -#define FLAG_SET_ERGO(name, value) (FLAG_MEMBER_SETTER(name)((value), JVMFlagOrigin::ERGONOMIC)) -#define FLAG_SET_MGMT(name, value) (FLAG_MEMBER_SETTER(name)((value), JVMFlagOrigin::MANAGEMENT)) + FLAG_MEMBER_SETTER(name)<0>((value), JVMFlagOrigin::COMMAND_LINE)) +#define FLAG_SET_ERGO(name, value) (FLAG_MEMBER_SETTER(name)<0>((value), JVMFlagOrigin::ERGONOMIC)) +#define FLAG_SET_MGMT(name, value) (FLAG_MEMBER_SETTER(name)<0>((value), JVMFlagOrigin::MANAGEMENT)) #define FLAG_SET_ERGO_IF_DEFAULT(name, value) \ do { \ From 7eca23435b3c0d64eae1f8aad4b94d0bd93ccff7 Mon Sep 17 00:00:00 2001 From: iklam Date: Mon, 29 Mar 2021 13:50:29 -0700 Subject: [PATCH 2/4] restored SET_FLAG_XXX for ccstr type, and fixed bugs in existing ccstr modification code --- src/hotspot/share/prims/whitebox.cpp | 10 ++- src/hotspot/share/runtime/flags/allFlags.hpp | 10 +++ .../share/runtime/flags/debug_globals.hpp | 72 +++++++++++++++++++ .../share/runtime/flags/jvmFlagAccess.cpp | 31 ++++---- .../share/runtime/flags/jvmFlagAccess.hpp | 1 - .../share/runtime/globals_extension.hpp | 12 ++-- src/hotspot/share/services/writeableFlags.cpp | 10 +-- test/hotspot/gtest/runtime/test_globals.cpp | 23 ++++++ .../serviceability/dcmd/vm/SetVMFlagTest.java | 22 +++++- .../HotSpotDiagnosticMXBean/SetVMOption.java | 37 ++++++++-- 10 files changed, 187 insertions(+), 41 deletions(-) create mode 100644 src/hotspot/share/runtime/flags/debug_globals.hpp diff --git a/src/hotspot/share/prims/whitebox.cpp b/src/hotspot/share/prims/whitebox.cpp index 9aafefdb2c30a..6c5a9633347ed 100644 --- a/src/hotspot/share/prims/whitebox.cpp +++ b/src/hotspot/share/prims/whitebox.cpp @@ -1332,18 +1332,16 @@ WB_ENTRY(void, WB_SetStringVMFlag(JNIEnv* env, jobject o, jstring name, jstring ccstrValue = env->GetStringUTFChars(value, NULL); CHECK_JNI_EXCEPTION(env); } - ccstr ccstrResult = ccstrValue; - bool needFree; { + ccstr param = ccstrValue; ThreadInVMfromNative ttvfn(thread); // back to VM - needFree = SetVMFlag (thread, env, name, &ccstrResult); + if (SetVMFlag (thread, env, name, ¶m)) { + assert(param == NULL, "old value is freed automatically and not returned"); + } } if (value != NULL) { env->ReleaseStringUTFChars(value, ccstrValue); } - if (needFree) { - FREE_C_HEAP_ARRAY(char, ccstrResult); - } WB_END WB_ENTRY(void, WB_LockCompilation(JNIEnv* env, jobject o, jlong timeout)) diff --git a/src/hotspot/share/runtime/flags/allFlags.hpp b/src/hotspot/share/runtime/flags/allFlags.hpp index 9d655e02a1c43..7d644e9a8061e 100644 --- a/src/hotspot/share/runtime/flags/allFlags.hpp +++ b/src/hotspot/share/runtime/flags/allFlags.hpp @@ -28,6 +28,7 @@ #include "compiler/compiler_globals.hpp" #include "gc/shared/gc_globals.hpp" #include "gc/shared/tlab_globals.hpp" +#include "runtime/flags/debug_globals.hpp" #include "runtime/globals.hpp" // Put LP64/ARCH/JVMCI/COMPILER1/COMPILER2 at the top, @@ -112,6 +113,15 @@ range, \ constraint) \ \ + DEBUG_RUNTIME_FLAGS( \ + develop, \ + develop_pd, \ + product, \ + product_pd, \ + notproduct, \ + range, \ + constraint) \ + \ GC_FLAGS( \ develop, \ develop_pd, \ diff --git a/src/hotspot/share/runtime/flags/debug_globals.hpp b/src/hotspot/share/runtime/flags/debug_globals.hpp new file mode 100644 index 0000000000000..11ea2f6010457 --- /dev/null +++ b/src/hotspot/share/runtime/flags/debug_globals.hpp @@ -0,0 +1,72 @@ +/* + * Copyright (c) 2021, Oracle and/or its affiliates. All rights reserved. + * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. + * + * This code is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License version 2 only, as + * published by the Free Software Foundation. + * + * This code is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License + * version 2 for more details (a copy is included in the LICENSE file that + * accompanied this code). + * + * You should have received a copy of the GNU General Public License version + * 2 along with this work; if not, write to the Free Software Foundation, + * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA. + * + * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA + * or visit www.oracle.com if you need additional information or have any + * questions. + * + */ + +#ifndef SHARE_RUNTIME_DEBUG_GLOBALS_HPP +#define SHARE_RUNTIME_DEBUG_GLOBALS_HPP + +#include "runtime/globals_shared.hpp" +#include "utilities/macros.hpp" + +// +// These flags are needed for testing the implementation of various flag access +// APIs. +// +// For example, DummyManageableStringFlag is needed because we don't +// have any MANAGEABLE flags of the ccstr type, but we really need to +// make sure the implementation is correct (in terms of memory allocation) +// just in case someone may add such a flag in the future. +// + +#ifndef ASSERT + +#define DEBUG_RUNTIME_FLAGS(develop, \ + develop_pd, \ + product, \ + product_pd, \ + notproduct, \ + range, \ + constraint) \ + \ + +#else + +#define DEBUG_RUNTIME_FLAGS(develop, \ + develop_pd, \ + product, \ + product_pd, \ + notproduct, \ + range, \ + constraint) \ + \ + product(ccstr, DummyManageableStringFlag, NULL, MANAGEABLE, \ + "Dummy flag for testing string handling in WriteableFlags") \ + \ + +// end of DEBUG_RUNTIME_FLAGS + +#endif // ASSERT + +DECLARE_FLAGS(DEBUG_RUNTIME_FLAGS) + +#endif // SHARE_RUNTIME_DEBUG_GLOBALS_HPP diff --git a/src/hotspot/share/runtime/flags/jvmFlagAccess.cpp b/src/hotspot/share/runtime/flags/jvmFlagAccess.cpp index dfc43e96bf9bf..8a2fdbb205218 100644 --- a/src/hotspot/share/runtime/flags/jvmFlagAccess.cpp +++ b/src/hotspot/share/runtime/flags/jvmFlagAccess.cpp @@ -29,7 +29,6 @@ #include "runtime/flags/jvmFlagAccess.hpp" #include "runtime/flags/jvmFlagLimit.hpp" #include "runtime/flags/jvmFlagConstraintsRuntime.hpp" -#include "runtime/globals_extension.hpp" #include "runtime/os.hpp" #include "utilities/macros.hpp" #include "utilities/ostream.hpp" @@ -318,27 +317,29 @@ JVMFlag::Error JVMFlagAccess::ccstrAtPut(JVMFlag* flag, ccstr* value, JVMFlagOri new_value = os::strdup_check_oom(*value); } flag->set_ccstr(new_value); - if (flag->is_default() && old_value != NULL) { - // Prior value is NOT heap allocated, but was a literal constant. - old_value = os::strdup_check_oom(old_value); - } - *value = old_value; + if (!flag->is_default() && old_value != NULL) { + // Old value is heap allocated so free it. + FREE_C_HEAP_ARRAY(char, old_value); + } + // Unlike the other APIs, the old vale is NOT returned, so the caller won't need to free it. + // The callers typically don't care what the old value is. + // If the caller really wants to know the old value, read it (and make a copy if necessary) + // before calling this API. + *value = NULL; flag->set_origin(origin); return JVMFlag::SUCCESS; } // This is called by the FLAG_SET_XXX macros. JVMFlag::Error JVMFlagAccess::set_impl(JVMFlagsEnum flag_enum, int type_enum, void* value, JVMFlagOrigin origin) { - // The FLAG_SET_XXX macros should have caused an static_assert to fail if you try to use them on - // ccstr/ccstrlist options. - // - // Uncomment the following and verify that the C++ compilation will fail. - // FLAG_SET_ERGO(LogFile, ""); - assert(type_enum != JVMFlag::TYPE_ccstr && type_enum != JVMFlag::TYPE_ccstrlist, "sanity"); - JVMFlag* flag = JVMFlag::flag_from_enum(flag_enum); - assert(flag->type() == type_enum, "wrong flag type"); - return set_impl(flag, type_enum, value, origin); + if (type_enum == JVMFlag::TYPE_ccstr || type_enum == JVMFlag::TYPE_ccstrlist) { + assert(flag->is_ccstr(), "must be"); + return ccstrAtPut(flag, (ccstr*)value, origin); + } else { + assert(flag->type() == type_enum, "wrong flag type"); + return set_impl(flag, type_enum, value, origin); + } } JVMFlag::Error JVMFlagAccess::check_range(const JVMFlag* flag, bool verbose) { diff --git a/src/hotspot/share/runtime/flags/jvmFlagAccess.hpp b/src/hotspot/share/runtime/flags/jvmFlagAccess.hpp index 0a340bba4f3f0..48198e110d968 100644 --- a/src/hotspot/share/runtime/flags/jvmFlagAccess.hpp +++ b/src/hotspot/share/runtime/flags/jvmFlagAccess.hpp @@ -85,7 +85,6 @@ class JVMFlagAccess : AllStatic { // type_enum will result in an assert. template static JVMFlag::Error set(JVMFlagsEnum flag_enum, T value, JVMFlagOrigin origin) { - static_assert(type_enum != JVMFlag::TYPE_ccstr && type_enum != JVMFlag::TYPE_ccstrlist, "not supported"); return set_impl(flag_enum, type_enum, &value, origin); } diff --git a/src/hotspot/share/runtime/globals_extension.hpp b/src/hotspot/share/runtime/globals_extension.hpp index c980c75e77328..fd7b83c22a3d4 100644 --- a/src/hotspot/share/runtime/globals_extension.hpp +++ b/src/hotspot/share/runtime/globals_extension.hpp @@ -1,5 +1,5 @@ /* - * Copyright (c) 2003, 2021, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2003, 2020, Oracle and/or its affiliates. All rights reserved. * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. * * This code is free software; you can redistribute it and/or modify it @@ -49,12 +49,10 @@ enum JVMFlagsEnum : int { NUM_JVMFlagsEnum }; -// Set functions for all flags -- use a template so we can do static_assert -// at individual call sites of FLAG_SET_{CMDLINE,ERGO,MGMT} +// Construct set functions for all flags #define FLAG_MEMBER_SETTER(name) Flag_##name##_set #define FLAG_MEMBER_SETTER_(type, name) \ - template \ inline JVMFlag::Error FLAG_MEMBER_SETTER(name)(type value, JVMFlagOrigin origin) { \ return JVMFlagAccess::set(FLAG_MEMBER_ENUM(name), value, origin); \ } @@ -77,9 +75,9 @@ ALL_FLAGS(DEFINE_FLAG_MEMBER_SETTER, #define FLAG_SET_DEFAULT(name, value) ((name) = (value)) #define FLAG_SET_CMDLINE(name, value) (JVMFlag::setOnCmdLine(FLAG_MEMBER_ENUM(name)), \ - FLAG_MEMBER_SETTER(name)<0>((value), JVMFlagOrigin::COMMAND_LINE)) -#define FLAG_SET_ERGO(name, value) (FLAG_MEMBER_SETTER(name)<0>((value), JVMFlagOrigin::ERGONOMIC)) -#define FLAG_SET_MGMT(name, value) (FLAG_MEMBER_SETTER(name)<0>((value), JVMFlagOrigin::MANAGEMENT)) + FLAG_MEMBER_SETTER(name)((value), JVMFlagOrigin::COMMAND_LINE)) +#define FLAG_SET_ERGO(name, value) (FLAG_MEMBER_SETTER(name)((value), JVMFlagOrigin::ERGONOMIC)) +#define FLAG_SET_MGMT(name, value) (FLAG_MEMBER_SETTER(name)((value), JVMFlagOrigin::MANAGEMENT)) #define FLAG_SET_ERGO_IF_DEFAULT(name, value) \ do { \ diff --git a/src/hotspot/share/services/writeableFlags.cpp b/src/hotspot/share/services/writeableFlags.cpp index dc5f62b4bf823..18553f4fcd796 100644 --- a/src/hotspot/share/services/writeableFlags.cpp +++ b/src/hotspot/share/services/writeableFlags.cpp @@ -1,5 +1,5 @@ /* - * Copyright (c) 2015, 2020, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2015, 2021, Oracle and/or its affiliates. All rights reserved. * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. * * This code is free software; you can redistribute it and/or modify it @@ -25,6 +25,7 @@ #include "precompiled.hpp" #include "classfile/javaClasses.hpp" #include "memory/allocation.inline.hpp" +#include "memory/resourceArea.hpp" #include "runtime/arguments.hpp" #include "runtime/flags/jvmFlag.hpp" #include "runtime/flags/jvmFlagAccess.hpp" @@ -244,6 +245,9 @@ JVMFlag::Error WriteableFlags::set_double_flag(const char* name, double value, J JVMFlag::Error WriteableFlags::set_ccstr_flag(const char* name, const char* value, JVMFlagOrigin origin, FormatBuffer<80>& err_msg) { JVMFlag* flag = JVMFlag::find_flag(name); JVMFlag::Error err = JVMFlagAccess::ccstrAtPut(flag, &value, origin); + if (err == JVMFlag::SUCCESS) { + assert(value == NULL, "old value is freed automatically and not returned"); + } print_flag_error_message_if_needed(err, flag, err_msg); return err; } @@ -357,11 +361,9 @@ JVMFlag::Error WriteableFlags::set_flag_from_jvalue(JVMFlag* f, const void* valu err_msg.print("flag value is missing"); return JVMFlag::MISSING_VALUE; } + ResourceMark rm; ccstr svalue = java_lang_String::as_utf8_string(str); JVMFlag::Error ret = WriteableFlags::set_ccstr_flag(f->name(), svalue, origin, err_msg); - if (ret != JVMFlag::SUCCESS) { - FREE_C_HEAP_ARRAY(char, svalue); - } return ret; } else { ShouldNotReachHere(); diff --git a/test/hotspot/gtest/runtime/test_globals.cpp b/test/hotspot/gtest/runtime/test_globals.cpp index 529e5d2557081..b6bcb0d2bcc8d 100644 --- a/test/hotspot/gtest/runtime/test_globals.cpp +++ b/test/hotspot/gtest/runtime/test_globals.cpp @@ -25,6 +25,7 @@ #include "compiler/compiler_globals.hpp" #include "gc/shared/gc_globals.hpp" #include "runtime/globals.hpp" +#include "runtime/globals_extension.hpp" #include "runtime/flags/flagSetting.hpp" #include "runtime/flags/jvmFlag.hpp" #include "unittest.hpp" @@ -71,3 +72,25 @@ TEST_VM(FlagGuard, double_flag) { TEST_VM(FlagGuard, ccstr_flag) { TEST_FLAG(PerfDataSaveFile, ccstr, "/a/random/path"); } + + +// SharedArchiveConfigFile is used only during "java -Xshare:dump", so +// it's safe to modify its value in gtest + +TEST_VM(FlagAccess, ccstr_flag) { + FLAG_SET_CMDLINE(SharedArchiveConfigFile, ""); + ASSERT_EQ(FLAG_IS_CMDLINE(SharedArchiveConfigFile), true); + ASSERT_EQ(strcmp(SharedArchiveConfigFile, ""), 0); + + FLAG_SET_ERGO(SharedArchiveConfigFile, "foobar"); + ASSERT_EQ(FLAG_IS_ERGO(SharedArchiveConfigFile), true); + ASSERT_EQ(strcmp(SharedArchiveConfigFile, "foobar") , 0); + + FLAG_SET_ERGO(SharedArchiveConfigFile, nullptr); + ASSERT_EQ(FLAG_IS_ERGO(SharedArchiveConfigFile), true); + ASSERT_EQ(SharedArchiveConfigFile, nullptr); + + FLAG_SET_ERGO(SharedArchiveConfigFile, "xyz"); + ASSERT_EQ(FLAG_IS_ERGO(SharedArchiveConfigFile), true); + ASSERT_EQ(strcmp(SharedArchiveConfigFile, "xyz"), 0); +} diff --git a/test/hotspot/jtreg/serviceability/dcmd/vm/SetVMFlagTest.java b/test/hotspot/jtreg/serviceability/dcmd/vm/SetVMFlagTest.java index 3d8b02958a15e..67ff7beb38b8c 100644 --- a/test/hotspot/jtreg/serviceability/dcmd/vm/SetVMFlagTest.java +++ b/test/hotspot/jtreg/serviceability/dcmd/vm/SetVMFlagTest.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2015, 2016, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2015, 2021, Oracle and/or its affiliates. All rights reserved. * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. * * This code is free software; you can redistribute it and/or modify it @@ -21,6 +21,7 @@ * questions. */ +import jdk.test.lib.Platform; import jdk.test.lib.process.OutputAnalyzer; import jdk.test.lib.dcmd.CommandExecutor; import jdk.test.lib.dcmd.JMXExecutor; @@ -47,6 +48,7 @@ public void run(CommandExecutor executor) { setMutableFlagWithInvalidValue(executor); setImmutableFlag(executor); setNonExistingFlag(executor); + setStringFlag(executor); } @Test @@ -147,6 +149,24 @@ private void setNonExistingFlag(CommandExecutor executor) { out.stdoutShouldContain("flag " + unknownFlag + " does not exist"); } + private void setStringFlag(CommandExecutor executor) { + // Today we don't have any manageable flags of the string type in the product build, + // so we can only test DummyManageableStringFlag in the debug build. + if (!Platform.isDebugBuild()) { + return; + } + + String flag = "DummyManageableStringFlag"; + String toValue = "DummyManageableStringFlag_Is_Set_To_Hello"; + + System.out.println("### Setting a string flag '" + flag + "'"); + OutputAnalyzer out = executor.execute("VM.set_flag " + flag + " " + toValue); + out.stderrShouldBeEmpty(); + + out = getAllFlags(executor); + out.stdoutShouldContain(toValue); + } + private OutputAnalyzer getAllFlags(CommandExecutor executor) { return executor.execute("VM.flags -all", true); } diff --git a/test/jdk/com/sun/management/HotSpotDiagnosticMXBean/SetVMOption.java b/test/jdk/com/sun/management/HotSpotDiagnosticMXBean/SetVMOption.java index 8cad178234ab8..890b54f9e334f 100644 --- a/test/jdk/com/sun/management/HotSpotDiagnosticMXBean/SetVMOption.java +++ b/test/jdk/com/sun/management/HotSpotDiagnosticMXBean/SetVMOption.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2005, 2015, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2005, 2021, Oracle and/or its affiliates. All rights reserved. * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. * * This code is free software; you can redistribute it and/or modify it @@ -29,6 +29,7 @@ * @author Mandy Chung * @author Jaroslav Bachorik * + * @library /test/lib * @run main/othervm -XX:+HeapDumpOnOutOfMemoryError SetVMOption */ @@ -37,6 +38,7 @@ import com.sun.management.HotSpotDiagnosticMXBean; import com.sun.management.VMOption; import com.sun.management.VMOption.Origin; +import jdk.test.lib.Platform; public class SetVMOption { private static final String HEAP_DUMP_ON_OOM = "HeapDumpOnOutOfMemoryError"; @@ -94,6 +96,23 @@ public static void main(String[] args) throws Exception { option.isWriteable() + " expected: " + o.isWriteable()); } + + // Today we don't have any manageable flags of the string type in the product build, + // so we can only test DummyManageableStringFlag in the debug build. + if (Platform.isDebugBuild()) { + String optionName = "DummyManageableStringFlag"; + String toValue = "DummyManageableStringFlag_Is_Set_To_Hello"; + + mbean.setVMOption(optionName, toValue); + + VMOption stringOption = findOption(optionName); + Object newValue = stringOption.getValue(); + if (!toValue.equals(newValue)) { + throw new RuntimeException("Unmatched value: " + + newValue + " expected: " + toValue); + } + } + // check if ManagementServer is not writeable List options = mbean.getDiagnosticOptions(); VMOption mgmtServerOption = null; @@ -123,18 +142,22 @@ public static void main(String[] args) throws Exception { } public static VMOption findHeapDumpOnOomOption() { + return findOption(HEAP_DUMP_ON_OOM); + } + + private static VMOption findOption(String optionName) { List options = mbean.getDiagnosticOptions(); - VMOption gcDetails = null; + VMOption found = null; for (VMOption o : options) { - if (o.getName().equals(HEAP_DUMP_ON_OOM)) { - gcDetails = o; + if (o.getName().equals(optionName)) { + found = o; break; } } - if (gcDetails == null) { - throw new RuntimeException("VM option " + HEAP_DUMP_ON_OOM + + if (found == null) { + throw new RuntimeException("VM option " + optionName + " not found"); } - return gcDetails; + return found; } } From 673aaafc4860dd7f70a3f222422ae84e85fd4219 Mon Sep 17 00:00:00 2001 From: iklam Date: Wed, 31 Mar 2021 09:30:05 -0700 Subject: [PATCH 3/4] relax flag attributions (ala JDK-7123237) --- src/hotspot/share/runtime/arguments.cpp | 1 - .../share/runtime/flags/debug_globals.hpp | 17 +---------------- src/hotspot/share/runtime/flags/jvmFlag.cpp | 19 ------------------- src/hotspot/share/runtime/flags/jvmFlag.hpp | 1 - 4 files changed, 1 insertion(+), 37 deletions(-) diff --git a/src/hotspot/share/runtime/arguments.cpp b/src/hotspot/share/runtime/arguments.cpp index e537f00594a52..c272830b55490 100644 --- a/src/hotspot/share/runtime/arguments.cpp +++ b/src/hotspot/share/runtime/arguments.cpp @@ -3782,7 +3782,6 @@ 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"; diff --git a/src/hotspot/share/runtime/flags/debug_globals.hpp b/src/hotspot/share/runtime/flags/debug_globals.hpp index 11ea2f6010457..5b2e26c555be2 100644 --- a/src/hotspot/share/runtime/flags/debug_globals.hpp +++ b/src/hotspot/share/runtime/flags/debug_globals.hpp @@ -38,19 +38,6 @@ // just in case someone may add such a flag in the future. // -#ifndef ASSERT - -#define DEBUG_RUNTIME_FLAGS(develop, \ - develop_pd, \ - product, \ - product_pd, \ - notproduct, \ - range, \ - constraint) \ - \ - -#else - #define DEBUG_RUNTIME_FLAGS(develop, \ develop_pd, \ product, \ @@ -59,14 +46,12 @@ range, \ constraint) \ \ - product(ccstr, DummyManageableStringFlag, NULL, MANAGEABLE, \ + notproduct(ccstr, DummyManageableStringFlag, NULL, MANAGEABLE, \ "Dummy flag for testing string handling in WriteableFlags") \ \ // end of DEBUG_RUNTIME_FLAGS -#endif // ASSERT - DECLARE_FLAGS(DEBUG_RUNTIME_FLAGS) #endif // SHARE_RUNTIME_DEBUG_GLOBALS_HPP diff --git a/src/hotspot/share/runtime/flags/jvmFlag.cpp b/src/hotspot/share/runtime/flags/jvmFlag.cpp index 8ad1e7fa312bc..c9de636d31201 100644 --- a/src/hotspot/share/runtime/flags/jvmFlag.cpp +++ b/src/hotspot/share/runtime/flags/jvmFlag.cpp @@ -681,25 +681,6 @@ 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(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) { diff --git a/src/hotspot/share/runtime/flags/jvmFlag.hpp b/src/hotspot/share/runtime/flags/jvmFlag.hpp index 36f2c53902bba..bd6dced189d32 100644 --- a/src/hotspot/share/runtime/flags/jvmFlag.hpp +++ b/src/hotspot/share/runtime/flags/jvmFlag.hpp @@ -179,7 +179,6 @@ 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(this - JVMFlag::flags); From 2a5e2b1943386b235402c488114bd6095d9963a8 Mon Sep 17 00:00:00 2001 From: iklam Date: Wed, 31 Mar 2021 23:04:50 -0700 Subject: [PATCH 4/4] Revert "relax flag attributions (ala JDK-7123237)" This reverts commit 673aaafc4860dd7f70a3f222422ae84e85fd4219. --- src/hotspot/share/runtime/arguments.cpp | 1 + .../share/runtime/flags/debug_globals.hpp | 17 ++++++++++++++++- src/hotspot/share/runtime/flags/jvmFlag.cpp | 19 +++++++++++++++++++ src/hotspot/share/runtime/flags/jvmFlag.hpp | 1 + 4 files changed, 37 insertions(+), 1 deletion(-) diff --git a/src/hotspot/share/runtime/arguments.cpp b/src/hotspot/share/runtime/arguments.cpp index c272830b55490..e537f00594a52 100644 --- a/src/hotspot/share/runtime/arguments.cpp +++ b/src/hotspot/share/runtime/arguments.cpp @@ -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"; diff --git a/src/hotspot/share/runtime/flags/debug_globals.hpp b/src/hotspot/share/runtime/flags/debug_globals.hpp index 5b2e26c555be2..11ea2f6010457 100644 --- a/src/hotspot/share/runtime/flags/debug_globals.hpp +++ b/src/hotspot/share/runtime/flags/debug_globals.hpp @@ -38,6 +38,19 @@ // just in case someone may add such a flag in the future. // +#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") \ \ // end of DEBUG_RUNTIME_FLAGS +#endif // ASSERT + DECLARE_FLAGS(DEBUG_RUNTIME_FLAGS) #endif // SHARE_RUNTIME_DEBUG_GLOBALS_HPP diff --git a/src/hotspot/share/runtime/flags/jvmFlag.cpp b/src/hotspot/share/runtime/flags/jvmFlag.cpp index c9de636d31201..8ad1e7fa312bc 100644 --- a/src/hotspot/share/runtime/flags/jvmFlag.cpp +++ b/src/hotspot/share/runtime/flags/jvmFlag.cpp @@ -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(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) { diff --git a/src/hotspot/share/runtime/flags/jvmFlag.hpp b/src/hotspot/share/runtime/flags/jvmFlag.hpp index bd6dced189d32..36f2c53902bba 100644 --- a/src/hotspot/share/runtime/flags/jvmFlag.hpp +++ b/src/hotspot/share/runtime/flags/jvmFlag.hpp @@ -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(this - JVMFlag::flags);