On Mon, 25 Jul 2022 17:21:27 GMT, Harold Seigel <[email protected]> wrote:
> Please review this fix for JDK-8285792. The fix removes print statements
> from check_signal_handler() so that it doesn't print all the handlers every
> time it finds one that is modified. Instead, it returns true if the handler
> is modified, false otherwise. Its caller, user_handler(), then prints all
> the handlers just once even if multiple signal handlers were modified.
>
> The fix also adds a check for VMError::crash_handler_adress() to
> check_signal_handler() to prevent it from being treated as a signal handler
> modification.
>
> The fix was tested with Mach5 tiers 1-2 on Linux and Mac OS and Mach 5 tiers
> 3-5 on Linux x64. The regression test is excluded on Windows.
>
> Thanks, Harold
Hi Harold,
Overall seems fine. A couple of nits.
We could simplify the test by just having (as is common) on Java file which
launches itself in a new VM when invoked with no-args, and does the real test
when passed a (dummy) arg. Then we will have one less file and clearer names.
Thanks.
src/hotspot/os/posix/signals_posix.cpp line 843:
> 841: // Compare both sigaction structures (intelligently; only the members
> we care about).
> 842: // Ignore if the handler is our own crash handler.
> 843: if (!are_handlers_equal(&act, expected_act) &&
Pre-existing nit: really this should be called `are_actions_equal`. The poor
naming is clearer when you now follow this with `HANDLER_IS`.
test/hotspot/jtreg/runtime/posixSig/TestPsig.java line 28:
> 26: System.loadLibrary("TestPsig");
> 27: }
> 28: public static native void doSomething(int val);
Suggestion:
`private static native void changeSigActionFor(int sig);`
-------------
Changes requested by dholmes (Reviewer).
PR: https://git.openjdk.org/jdk/pull/9631
