8264285: Clean the modification of ccstr JVM flags

iklam · iklam · commit 585839906254 · 2021-04-01T18:25:14.000Z
Reviewed-by: dholmes, coleenp
diff --git a/src/hotspot/share/prims/whitebox.cpp b/src/hotspot/share/prims/whitebox.cpp
@@ -1372,18 +1372,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 <JVM_FLAG_TYPE(ccstr)> (thread, env, name, &ccstrResult);
+    if (SetVMFlag <JVM_FLAG_TYPE(ccstr)> (thread, env, name, &param)) {
+      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
@@ -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
@@ -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
@@ -317,40 +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);
+  if (!flag->is_default() && old_value != NULL) {
+    // Old value is heap allocated so free it.
+    FREE_C_HEAP_ARRAY(char, old_value);
   }
-  *value = 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) {
-  if (type_enum == JVMFlag::TYPE_ccstr || type_enum == JVMFlag::TYPE_ccstrlist) {
-    return ccstrAtPut((JVMFlagsEnum)flag_enum, *((ccstr*)value), origin);
-  }
-
   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<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);
+  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);
   }
-  faddr->set_origin(origin);
-  return JVMFlag::SUCCESS;
 }
 
 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
@@ -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);
diff --git 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
@@ -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
@@ -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
@@ -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<VMOption> 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<VMOption> 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;
     }
 }

Original file line number	Diff line number	Diff line change
`@@ -1372,18 +1372,16 @@ WB_ENTRY(void, WB_SetStringVMFlag(JNIEnv* env, jobject o, jstring name, jstring`
`1372`	`1372`	`ccstrValue = env->GetStringUTFChars(value, NULL);`
`1373`	`1373`	`CHECK_JNI_EXCEPTION(env);`
`1374`	`1374`	`}`
`1375`		`- ccstr ccstrResult = ccstrValue;`
`1376`		`- bool needFree;`
`1377`	`1375`	`{`
	`1376`	`+ ccstr param = ccstrValue;`
`1378`	`1377`	`ThreadInVMfromNative ttvfn(thread); // back to VM`
`1379`		`- needFree = SetVMFlag <JVM_FLAG_TYPE(ccstr)> (thread, env, name, &ccstrResult);`
	`1378`	`+ if (SetVMFlag <JVM_FLAG_TYPE(ccstr)> (thread, env, name, &param)) {`
	`1379`	`+ assert(param == NULL, "old value is freed automatically and not returned");`
	`1380`	`+ }`
`1380`	`1381`	`}`
`1381`	`1382`	`if (value != NULL) {`
`1382`	`1383`	`env->ReleaseStringUTFChars(value, ccstrValue);`
`1383`	`1384`	`}`
`1384`		`- if (needFree) {`
`1385`		`- FREE_C_HEAP_ARRAY(char, ccstrResult);`
`1386`		`- }`
`1387`	`1385`	`WB_END`
`1388`	`1386`
`1389`	`1387`	`WB_ENTRY(void, WB_LockCompilation(JNIEnv* env, jobject o, jlong timeout))`