Page MenuHomePhabricator

[Support] On Windows, handle interrupt signals without crash message
ClosedPublic

Authored by jhenderson on May 13 2020, 3:58 AM.

Details

Summary

On Linux, the signal handlers are not run on signals such as SIGINT due to CTRL-C. This makes sense, as such signals are not really crashes. Prior to this change, this wasn't the case on Windows, however. This patch changes the Windows behaviour to be consistent with Linux, and adds testing that verifies this.

The test uses llvm-symbolizer, but any tool with an interactive mode would do the job.

Fixes https://bugs.llvm.org/show_bug.cgi?id=45754.

Diff Detail

Event Timeline

jhenderson created this revision.May 13 2020, 3:58 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 13 2020, 3:58 AM
Herald added a subscriber: hiraditya. · View Herald Transcript

If anybody would be willing to verify the test on cygwin, that would be helpful. I'm not 100% confident it will work there, but don't have cygwin installed to verify.

jhenderson planned changes to this revision.May 13 2020, 4:00 AM

Forgot to run this with python 3, and there's a small problem which I need to fix.

Made test python3 compatible.

I do not think I am a good person to accept/review this. Your comments in PR45754 looks reasonable to me though.

aganea accepted this revision.May 20 2020, 8:55 AM

LGTM.

This revision is now accepted and ready to land.May 20 2020, 8:55 AM
rnk accepted this revision.May 20 2020, 2:30 PM

lgtm

I don't have Cygwin anymore. IMO your code change seems low risk for cygwin, although the test could always fail with Cygwin Python, but IMO it's not worth worrying about.

MaskRay accepted this revision.May 20 2020, 3:18 PM
MaskRay added inline comments.
llvm/test/Support/interrupts.test
5

count 0 < %t.err

or 2>&1 | count 0

On Linux, the signal handlers are not run on signals such as SIGINT due to CTRL-C.

That is the default behavior on a *NIX platform: on SIGINT, unregister the handler and call raise(SIGINT) to let the kernel terminate the process.

If a callback is registered via llvm::sys::SetInterruptFunction (used by bugpoint and clangd), that callback will run instead.

jhenderson marked 2 inline comments as done.May 21 2020, 3:03 AM

On Linux, the signal handlers are not run on signals such as SIGINT due to CTRL-C.

That is the default behavior on a *NIX platform: on SIGINT, unregister the handler and call raise(SIGINT) to let the kernel terminate the process.

If a callback is registered via llvm::sys::SetInterruptFunction (used by bugpoint and clangd), that callback will run instead.

I wasn't quite sure what this comment was suggesting/discussing. Either way, I'll tweak my commit message to say that this matches the default behaviour for LLVM on Unix (see sys::CleanupOnSignal).

llvm/test/Support/interrupts.test
5

I'm deliberately avoiding the latter, since llvm-symbolizer prints to stdout as part of the script below.

This revision was automatically updated to reflect the committed changes.
jhenderson marked an inline comment as done.