Skip to content

LLVM failures cause exit status 1 #54992

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
euclio opened this issue Oct 11, 2018 · 4 comments
Closed

LLVM failures cause exit status 1 #54992

euclio opened this issue Oct 11, 2018 · 4 comments
Labels
A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. C-feature-request Category: A feature request, i.e: not implemented / a PR. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@euclio
Copy link
Contributor

euclio commented Oct 11, 2018

Related to #51971.

If a fatal error occurs in LLVM, the compiler exits with status 1. This is problematic, because if a test is expecting a compilation failure, the test could still pass. It also makes it difficult to test for the presence of ICEs in general.

A simple fix would be to patch our copy of LLVM to exit with 101 (like ICEs) in report_fatal_error, but any other ideas would be appreciated.

@cuviper
Copy link
Member

cuviper commented Oct 12, 2018

Those error functions all call sys::RunInterruptHandlers(), which sounds like it could be a great hook for us, but then that only calls RemoveFilesToRemove(). :/

@cuviper
Copy link
Member

cuviper commented Oct 12, 2018

There is install_fatal_error_handler though, and that handler gets called first.

@cuviper cuviper added A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. C-feature-request Category: A feature request, i.e: not implemented / a PR. labels Oct 12, 2018
@euclio
Copy link
Contributor Author

euclio commented Oct 12, 2018

That sounds nice, but if we exit with 101 (or simply panic) in the handler won't we be skipping any other cleanup (sys::RunInterruptHandlers, plus any future cleanup that gets added there)? Or could we just call it in our handler?

I'm interested in putting a patch together, but I'm unfamiliar with how we link Rust to C++. The error handler needs to be able to consume a std::string.

@cuviper
Copy link
Member

cuviper commented Oct 12, 2018

or simply panic

We shouldn't do anything that looks like exception unwinding. See install_fatal_error_handler:

  /// It is dangerous to naively use an error handler which throws an exception.
  /// Even though some applications desire to gracefully recover from arbitrary
  /// faults, blindly throwing exceptions through unfamiliar code isn't a way to
  /// achieve this.

won't we be skipping any other cleanup (sys::RunInterruptHandlers, plus any future cleanup that gets added there)? Or could we just call it in our handler?

I think we can call it -- it's publicly declared in llvm/Support/Signals.h. We might want to keep a note to make sure we check for new cleanup needed when we update LLVM though.

I'm interested in putting a patch together, but I'm unfamiliar with how we link Rust to C++. The error handler needs to be able to consume a std::string.

We bridge C++ code in src/rustllvm/. You could just write our handler in C++ there, and add some C-FFI function for librustc_codegen_llvm to call and hook that up.

euclio added a commit to euclio/rust that referenced this issue Oct 12, 2018
bors added a commit that referenced this issue Oct 16, 2018
Exit with code 101 on fatal codegen errors

Fixes #54992.

This PR installs a custom fatal error handler that prints the error from LLVM and exits with 101. There should be no visible change in the output from LLVM. This allows distinguishing a fatal LLVM error with a compilation error by exit code.

This PR also modifies the LLVM codegen backend to ICE instead of emitting a fatal error when encountering a LLVM worker thread panic for the same reason.

r? @cuviper
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. C-feature-request Category: A feature request, i.e: not implemented / a PR. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

2 participants