RFR: 8263567: gtests don't terminate the VM safely

[Thomas Stuefe]

On Wed, 4 Aug 2021 05:55:11 GMT, David Holmes <dholmes at openjdk.org> wrote:

> Please review this fix to shutdown cleanly any JVMs created by gtests that aren't aborted.
> 
> Testing:
>   - manual instrumentation to check all JVM construction and then deletion during gtest runs
>   - local gtest testing
>   - tiers 1-3 gtest testing
> 
> Thanks,
> David

Hi David,

looks good, small remarks inline and one general remark here:

I wonder whether we should not just always initialize the VM at launch and destroy it after finish, regardless of the tests we run. We even could abolish TEST and just always do TEST_VM.

While in theory TEST (without _VM) is a good idea because we save time on the VM initialization, in practice
1) usually one just runs always the full suite, or a subset of it which includes TEST_VM tests, so most of the time we pay for VM init anyway.
2) tests are done using copy+paste and use TEST_VM unnecessarily. OTOH I also already had TEST tests that needed VM stuff. I mean that maintaining the correct designation for the tests is cumbersome and error-prone.
3) The _VM flexibility, tied to order and selection of tests, means the time VM initialization happens is unpredictable, which makes analysis more difficult and behavior sometimes surprising.

We could simplify all this and the runner code by just unconditionally initializing the VM at start of the gtestlauncher. I think the benefits of differentiating between TEST and TEST_VM are slim in practice.

..Thomas

test/hotspot/gtest/gtestMain.cpp line 113:

> 111:   char** _argv;
> 112:   bool _is_initialized;
> 113:   JavaVM* jvm;

underscore missing?

test/hotspot/gtest/gtestMain.cpp line 127:

> 125:     if (!_is_initialized && is_same_vm_test(name)) {
> 126:       // we want to have hs_err and core files when we execute regular tests
> 127:       int ret_val = init_jvm(_argc, _argv, false, &jvm);

init_jvm should just return the JavaVM pointer instead of a (rather boolean) int. The result int does not carry any information beyond "success". We can do the same with != NULL.

test/hotspot/gtest/gtestMain.cpp line 220:

> 218: 
> 219: // This is generally run once for a set of tests, but if we have vm_assert, or other_vm
> 220: // tests, a new process is forked and this will be called, for each of those tests.

is the comma needed?

test/hotspot/gtest/unittest.hpp line 99:

> 97:         fprintf(stderr, "Warning: DestroyJavaVM error %d\n", ret);  \
> 98:       }                                                             \
> 99:     }                                                               \

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

-------------

Changes requested by stuefe (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/4986