RFR: 8292054: Test runtime/posixSig/TestPosixSig.java fails with 'Test failed, bad output.' [v2]

[Thomas Stuefe]

On Wed, 17 Aug 2022 13:44:35 GMT, Harold Seigel <hseigel at openjdk.org> wrote:

>> Please review this fix for JDK-8292054.  The existing regression test for JDK-8285792, test/hotspot/jtreg/runtime/posixSig/TestPosixSig.java, intermittently fails because it depends on periodic calls to JVM function os::run_periodic_checks().  This fix replaces test TestPosixSig.java with a gtest that does its own explicit call to os::run_periodic_checks().
>> 
>> The fix was tested by running the new test 150+ times on Linux, Mac OS, and Windows.
>
> Harold Seigel has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Improve gtest based on review comments

Hi @hseigel, thanks for explaining the motivation. See remarks inline.

test/hotspot/gtest/runtime/test_signals.cpp line 29:

> 27: #include "memory/resourceArea.hpp"
> 28: #include "runtime/os.hpp"
> 29: #include "utilities/defaultStream.hpp"

Probably don't need this, but need `ostream.hpp`

test/hotspot/gtest/runtime/test_signals.cpp line 53:

> 51:     act.sa_flags = 0;
> 52:     ASSERT_NE(sigaction(SIGFPE, &act, &old_SIGFPE_act), -1) << "Setting SIGFPE handler failed";
> 53:     ASSERT_NE(sigaction(SIGILL, &act, &old_SIGILL_act), -1) << "Setting SIGILL handler failed";

Could you add `<< "(" << errno << ")"` to the error text? May need errno.h.

test/hotspot/gtest/runtime/test_signals.cpp line 59:

> 57:     stringStream st;
> 58:     os::run_periodic_checks(&st);
> 59:     char* res = st.as_string();

By using "const char* res = st.base()" instead of as_string(), you can access the string buffer without copying and don't need to use a ResourceMark.

test/hotspot/gtest/runtime/test_signals.cpp line 71:

> 69:     modified = strstr(modified + 1, "Handler was modified!");
> 70:     ASSERT_NE(modified, nullptr) << "Only one message found";
> 71:     ASSERT_EQ(strstr(modified + 1, "Handler was modified!"), nullptr) << "Too many messages found";

This is nitpicking and feel free to disregard this. But maybe simpler to understand would be to scan that "Signal Handlers:" (the header line) appears just once, and maybe that "Warning: SIGFPE handler modified!" and "Warning: SIGILL handler modified!" appear just once too.

test/hotspot/gtest/runtime/test_signals.cpp line 76:

> 74: 
> 75: // This tests the fix for JDK-8285792.
> 76: TEST_VM(PosixSignalTest, check_handlers) {

Please use TEST_OTHER_VM. Your test changes global state and thus pollutes the VM for followup tests.

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

Changes requested by stuefe (Reviewer).

PR: https://git.openjdk.org/jdk/pull/9882