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

8263567: gtests don't terminate the VM safely #4986

Closed
wants to merge 4 commits into from
Closed
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
@@ -61,8 +61,7 @@ static bool is_suffix(const char* suffix, const char* str) {
return strncmp(str + (str_len - suffix_len), suffix, suffix_len) == 0;
}


static int init_jvm(int argc, char **argv, bool disable_error_handling) {
static int init_jvm(int argc, char **argv, bool disable_error_handling, JavaVM** jvm_ptr) {
// don't care about the program name
argc--;
argv++;
@@ -90,10 +89,9 @@ static int init_jvm(int argc, char **argv, bool disable_error_handling) {
args.options = options;
args.ignoreUnrecognized = JNI_FALSE;

JavaVM* jvm;
JNIEnv* env;

int ret = JNI_CreateJavaVM(&jvm, (void**)&env, &args);
int ret = JNI_CreateJavaVM(jvm_ptr, (void**)&env, &args);
if (ret == JNI_OK) {
// CreateJavaVM leaves WXExec context, while gtests
// calls internal functions assuming running in WXWwrite.
@@ -112,27 +110,37 @@ class JVMInitializerListener : public ::testing::EmptyTestEventListener {
int _argc;
char** _argv;
bool _is_initialized;
JavaVM* jvm;
dholmes-ora marked this conversation as resolved.
Show resolved Hide resolved

void initialize_jvm() {
}

public:
JVMInitializerListener(int argc, char** argv) :
_argc(argc), _argv(argv), _is_initialized(false) {
_argc(argc), _argv(argv), _is_initialized(false), jvm(NULL) {
}

virtual void OnTestStart(const ::testing::TestInfo& test_info) {
const char* name = test_info.name();
if (!_is_initialized && is_same_vm_test(name)) {
// we want to have hs_err and core files when we execute regular tests
int ret_val = init_jvm(_argc, _argv, false);
int ret_val = init_jvm(_argc, _argv, false, &jvm);
dholmes-ora marked this conversation as resolved.
Show resolved Hide resolved
if (ret_val != 0) {
ADD_FAILURE() << "Could not initialize the JVM";
exit(1);
}
_is_initialized = true;
}
}

void destroy_jvm() {
if (_is_initialized && jvm != NULL) {
int ret = jvm->DestroyJavaVM();
if (ret != 0) {
fprintf(stderr, "Warning: DestroyJavaVM error %d\n", ret);
}
}
}
};

static char* get_java_home_arg(int argc, char** argv) {
@@ -208,6 +216,14 @@ static char** remove_test_runner_arguments(int* argcp, char **argv) {
return new_argv;
}

// This is generally run once for a set of tests, but if we have vm_assert, or other_vm
// tests, a new process is forked and this will be called, for each of those tests.
dholmes-ora marked this conversation as resolved.
Show resolved Hide resolved
// When we execute a vm_assert or other_vm test we create and initialize the JVM below.
// A vm_assert test crashes the VM so no cleanup is needed, but for other_vm we call
// DestroyJavaVM via the TEST_OTHER_VM macro prior to the call to exit().
// For same_vm tests we use an event listener to create the JVM when the first same_vm
// test is executed. Once all tests are completed we can then call DestroyJavaVM on that
// JVM directly.
static void runUnitTestsInner(int argc, char** argv) {
::testing::InitGoogleMock(&argc, argv);
::testing::GTEST_FLAG(death_test_style) = "threadsafe";
@@ -253,22 +269,36 @@ static void runUnitTestsInner(int argc, char** argv) {
#endif // _WIN32
argv = remove_test_runner_arguments(&argc, argv);


JVMInitializerListener* jvm_listener = NULL;

if (is_vmassert_test || is_othervm_test) {
JavaVM* jvm = NULL;
// both vmassert and other vm tests require inited jvm
// but only vmassert tests disable hs_err and core file generation
if (init_jvm(argc, argv, is_vmassert_test) != 0) {
if (init_jvm(argc, argv, is_vmassert_test, &jvm) != 0) {
abort();
}
} else {
::testing::TestEventListeners& listeners = ::testing::UnitTest::GetInstance()->listeners();
listeners.Append(new JVMInitializerListener(argc, argv));
jvm_listener = new JVMInitializerListener(argc, argv);
listeners.Append(jvm_listener);
}

int result = RUN_ALL_TESTS();

// vm_assert and other_vm tests never reach this point as they either abort, or call
// exit() - see TEST_OTHER_VM macro. We will reach here when all same_vm tests have
// completed for this run, so we can terminate the VM used for that case.

if (result != 0) {
fprintf(stderr, "ERROR: RUN_ALL_TESTS() failed. Error %d\n", result);
exit(2);
}

if (jvm_listener != NULL) {
jvm_listener->destroy_jvm();
}
}

// Thread support for -new-thread option
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2016, 2020, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2016, 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
@@ -88,6 +88,15 @@
static void child_ ## category ## _ ## name ## _() { \
::testing::GTEST_FLAG(throw_on_failure) = true; \
test_ ## category ## _ ## name ## _(); \
JavaVM* jvm[1]; \
jsize nVMs = 0; \
JNI_GetCreatedJavaVMs(&jvm[0], 1, &nVMs); \
if (nVMs == 1) { \
int ret = jvm[0]->DestroyJavaVM(); \
if (ret != 0) { \
fprintf(stderr, "Warning: DestroyJavaVM error %d\n", ret); \
} \
} \
Copy link
Member

@tstuefe tstuefe Aug 4, 2021

Choose a reason for hiding this comment

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

For better readibility I'd factor this out to an own function.

Copy link
Member Author

@dholmes-ora dholmes-ora Aug 6, 2021

Choose a reason for hiding this comment

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

The problem is I don't have anywhere to put that function so that this macro can invoke it.

Copy link
Member Author

@dholmes-ora dholmes-ora Aug 6, 2021

Choose a reason for hiding this comment

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

To be more clear this is a .hpp file included by many .cpp files. If I put the function in the .hpp file and a specific .cpp file does not use the TEST_OTHER_VM macro then we have a function defined but not used and so hit -Werror=unused-function.
Is there some .cpp file I can put this in that would be globally accessible to all the test .cpp files?

fprintf(stderr, "OKIDOKI"); \
exit(0); \
} \