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

[Harold Seigel]

On Tue, 16 Aug 2022 16:18:30 GMT, Thomas Stuefe <stuefe 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.
>
> The problem in [JDK-8292054](https://bugs.openjdk.org/browse/JDK-8292054) is that the output appears more than once, right?
> 
> The hotspot code warns exactly once per encountered signal change. So if someone changes SIGFPE, then SIGILL, VM should warn twice. That is by design since such changes can be unrelated and equally important, so if they are hours apart one wants to know.
> 
> [JDK-8285792](https://bugs.openjdk.org/browse/JDK-8285792) changed the behavior somewhat so that if multiple handler changes were encountered in *the same check*, we only print all signal handlers once. But that part does not always work, since thats a bit racy:
> 
> - os::run_periodic_checks()
>   - *other thread changes signal handler for FPE*
>   - check signal handler for FPE (8)
>     - changed. Disarm future checks for FPE and return true
>   - all other handlers still unchanged
>   - *other thread changes ILL (4)*
>   - print "Warning SIGFPE changed!"
>   - print all handlers
> - Next loop: 
> - os::run_periodic_checks()
>   - check signal handler for ILL (4)
>     - changed. Disarm future checks for ILL and return true
>   - check signal handler for FPE (8) - returns false since already disarmed
>   - print "SIGILL changed!"
>   - print all handlers again
> 
> .. and the test complains over the fact that the signal handler section is printed twice.
> 
> ---
> 
> I'm not sure if transforming the test to a gtest is the right way to do it. Since the test checks the ability of the VM to process `-XX:+CheckJNICalls` and notice signal handler changes on its own. Calling the function manually removes one aspect of the test.
> 
> Could we just make the existing java test more lenient, so that "Warning: SIGILL handler modified!" and "Warning: SIGFPE handler modified!" appear only once?

@tstuefe 
Thanks for looking at this change.  The purpose of this test, and the test that it is replacing, is to check that os::run_periodic_checks() does not print a complete list of signal handlers every time it finds a changed signal handler.  The tests purpose is not to test that -XX:+CheckJNICalls causes the JVM to check for changed signal handlers.

Making existing test TestPosixSig.java more lenient wouldn't help because the test wouldn't know if the multiple Warnings are caused by issues with the fix for JDK-8285792.

How about if we add the gtest proposed here as the fix for JDK-8292054 and open a new RFE to add a test to check that -XX:+CheckJNICalls causes the JVM to check for changed signal handlers?  That new test would be similar to TestPosixSig.java.

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

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