Skip to content

rustllvm: Use report_fatal_error instead of llvm_unreachable #44020

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
hanna-kruppe opened this issue Aug 21, 2017 · 3 comments
Closed

rustllvm: Use report_fatal_error instead of llvm_unreachable #44020

hanna-kruppe opened this issue Aug 21, 2017 · 3 comments
Labels
A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. C-bug Category: This is a bug. P-medium Medium priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@hanna-kruppe
Copy link
Contributor

hanna-kruppe commented Aug 21, 2017

Some functions in rustllvm rely on llvm_unreachable to catch errors. However, in release mode llvm_unreachable expands to the C++ compiler's equivalent of intrinsics::unreachable() (i.e., it's a hint to the optimizer and UB to execute). This subverts the intent of several functions, causing us to execute UB instead of crashing as intended. All uses of llvm_unreachable in rustllvm should be audited and replaced with report_fatal_error unless the potential-UB is intended.

@Mark-Simulacrum Mark-Simulacrum added A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. C-bug Category: This is a bug. I-nominated T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Aug 22, 2017
@Mark-Simulacrum
Copy link
Member

Nominating for prioritization. I feel like this might be somewhat severe, and as such should get P-med/P-high but I don't really know what the effects of this can be.

@hanna-kruppe
Copy link
Contributor Author

In practice, it's been fine so far, so it'll probably be fine for a while longer. I don't think P-high is warranted, honestly. Most uses of llvm_unreachable that I recall are in code that specifically translates between C++ enums and Rust enums to ensure we don't get UB from C++ adding extending their enums without us noticing. That code will actually be fine as long as the corresponding C++ enums don't change.

Besides, instead of "release mode" I should have said "without assertions" (the macro definition depends on NDEBUG). That is, Release+Assert builds (as nightlies currently are, IIRC?) will also crash instead of causing UB.

Still, it's an easy fix, so someone with a little bit of time (coughnot mecough) should do it.

@nikomatsakis
Copy link
Contributor

triage: P-medium

Would be good to do!

@rust-highfive rust-highfive added P-medium Medium priority and removed I-nominated labels Aug 24, 2017
hanna-kruppe pushed a commit to hanna-kruppe/rust that referenced this issue Nov 20, 2017
This makes it more robust when assertions are disabled,
crashing instead of causing UB.

Also introduces a tidy check to enforce this rule,
which in turn necessitated making tidy run on src/rustllvm.

Fixes rust-lang#44020
kennytm added a commit to kennytm/rust that referenced this issue Nov 20, 2017
[rustllvm] Use report_fatal_error over llvm_unreachable

This makes it more robust when assertions are disabled, crashing instead of causing UB.

Also introduces a tidy check to enforce this rule, which in turn necessitated making tidy run on `src/rustllvm`.

Fixes rust-lang#44020
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-bug Category: This is a bug. P-medium Medium priority 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

4 participants