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

Closed
Closed
Changes from all commits
Commits
File filter
Filter file types
Jump to
Jump to file
Failed to load files.

Always

Just for now

@@ -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))
@@ -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, \
@@ -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.

This comment has been minimized.

@coleenp

coleenp Mar 31, 2021

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

This comment has been minimized.

@iklam

iklam Mar 31, 2021
Author Member

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);
    }
  }
}

This comment has been minimized.

@coleenp

coleenp Apr 1, 2021

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?

This comment has been minimized.

@iklam

iklam Apr 1, 2021
Author Member

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, \
product_pd, \
notproduct, \
range, \
constraint) \
\
product(ccstr, DummyManageableStringFlag, NULL, MANAGEABLE, \
"Dummy flag for testing string handling in WriteableFlags") \

This comment has been minimized.

@coleenp

coleenp Apr 1, 2021

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

This comment has been minimized.

@coleenp

coleenp Mar 31, 2021

good comment!

*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) {
@@ -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);
@@ -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");
}
Comment on lines +248 to +250

This comment has been minimized.

@dholmes-ora

dholmes-ora Mar 30, 2021
Member

The whole block should be ifdef DEBUG.

This comment has been minimized.

@iklam

iklam Mar 31, 2021
Author Member

Since this whole block can be optimized out by the C compiler in product builds, I'd rather leave out the #ifdef to avoid clutter.

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();
@@ -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);
}
@@ -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);
}
@@ -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;
}
}
ProTip! Use n and p to navigate between commits in a pull request.