Skip to content
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

Evaluating constants in MIR optimizations introduces const eval errors #122828

Open
tmiasko opened this issue Mar 21, 2024 · 11 comments
Open

Evaluating constants in MIR optimizations introduces const eval errors #122828

tmiasko opened this issue Mar 21, 2024 · 11 comments
Labels
A-const-eval Area: Constant evaluation, covers all const contexts (static, const fn, ...) A-mir-opt Area: MIR optimizations A-monomorphization Area: Monomorphization C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@tmiasko
Copy link
Contributor

tmiasko commented Mar 21, 2024

#![feature(inline_const)]
#![crate_type = "lib"]
pub fn f<T>() -> usize {
    g::<()>()
}
pub fn g<T>() -> usize {
    const { 0 / std::mem::size_of::<T>() }
}

This compiles fine in debug builds, but fails in release builds:

$ rustc b.rs
$ rustc b.rs -O
error[E0080]: evaluation of `g::<()>::{constant#0}` failed
 --> b.rs:7:13
  |
7 |     const { 0 / std::mem::size_of::<T>() }
  |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ attempt to divide `0_usize` by zero

error: aborting due to 1 previous error

For more information about this error, try `rustc --explain E0080`.

Based on #107503 (comment) cc @RalfJung.

But also see this example based on type sizes.

@tmiasko tmiasko added the C-bug Category: This is a bug. label Mar 21, 2024
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Mar 21, 2024
@tmiasko tmiasko added A-const-eval Area: Constant evaluation, covers all const contexts (static, const fn, ...) A-mir-opt Area: MIR optimizations A-monomorphization Area: Monomorphization T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Mar 21, 2024
@RalfJung
Copy link
Member

Ah, so there is a way to trigger this via inlining, dang.

Cc @rust-lang/wg-const-eval @rust-lang/wg-mir-opt

@RalfJung
Copy link
Member

All functions in this example are generic, so no amount of collector tweaking can possibly help here.

I don't really see an alternative to developing some sort of const-eval query that suppresses the error.

@RalfJung
Copy link
Member

Also, note that even if we get such a "silent const-eval query", all the stuff in #122568 is still needed -- collection definitely wants to use the "loud" const-eval query. That's also what makes #122814 an independent issue from this one, even though both involve "inlining":

  • This one here is about the MIR inliner monomorphizing constants, which leads to new const-eval errors.
  • The other one is about #[inline] functions being cross-crate-inlinable for LLVM which affects what happens during mono-item collection.

@RalfJung
Copy link
Member

So how could such a silent query work? I think we need a query that returns roughly Result<RawConst, ErrorInfo> where ErrorInfo contains everything needed to render the error -- in particular, all the spans of the backtrace. The the query caller can decide whether they want to ignore or show errors. We probably want a 2nd query that shows errors so that we still get deduplication if the constant is eval'd multiple times.

That sounds very doable, though perf may become a problem. What's less clear to me is how to deal with nested queries: evaluating a const may require evaluating other consts; what if those fail? If the innermost const-eval query uses silent queries for nested eval, we'll have to propagate such error info up to the result of the outer query or else we'll never see the inner failures. So probably we want ErrorInfo to be interned such that one ErrorInfo can link to the ErrorInfo of a nested query that failed, or something like that?

@oli-obk
Copy link
Contributor

oli-obk commented Mar 25, 2024

Sounds like you want what #92674 tried to do. A Reveal mode that silences const eval errors and reruns in an erroring mode if one wants to report an error. So you still get caching in the success case, but need to rerun in the error case.

That was nontrivial to actually do, but I don't remember the issues

@RalfJung
Copy link
Member

With inlining we can be evaluating failing constants even in what should be successful compilation, like the example in the OP. So not caching failures doesn't seem great.

To be clear I think the intended behavior of the example in the OP is that compilation should succeed. The fact that with optimizations enabled, we get an error -- that's a bug.

@oli-obk
Copy link
Contributor

oli-obk commented Mar 25, 2024

So not caching failures doesn't seem great.

we'd still cache them. I mean, when you actually want to report an error, then you rerun with the regular Reveal::All and it'll report an error.

@RalfJung
Copy link
Member

RalfJung commented Apr 3, 2024

As @scottmcm points out, this doesn't just affect const evaluation, it also affects the "maximum size of a type" check. So we'd not just need to handle MIR with consts that can fail to evaluate, but also MIR with types that are too big to exist.

Actually that example is more like #107503, not like this issue, since it fails i debug builds and compiles in release builds.

@RalfJung
Copy link
Member

RalfJung commented Apr 3, 2024

That said, the required_consts stuff we do currently already doesn't cover type sizes. So it's likely #107503 still exists for type sizes, and it seems pretty tricky to fix. Given that such large types are very rare, it's also unclear whether it's really worth the compile time cost to ensure that type size errors are not optimization-dependent.

EDIT: Indeed, here is an example.

@RalfJung
Copy link
Member

RalfJung commented Apr 4, 2024

A version of this issue for type sizes would look something like this:

#![crate_type = "lib"]

pub fn f<T>() -> usize {
    g::<u8>()
}
pub fn g<T>() -> usize {
    let _val = std::mem::MaybeUninit::<[T; 1 << 47]>::uninit();
    42
}

But that does not error. It seems like MIR opts already do not care about type sizes, or we just haven't found the right example.

@tmiasko
Copy link
Contributor Author

tmiasko commented Apr 4, 2024

MIR optimizations do not report any layout errors by themselves, only code generation does. #122814 can be used to generate more code in an optimized build:

#![crate_type="lib"]
pub fn f() {
    loop {}
    g();
}
#[inline(never)]
fn g() -> std::mem::MaybeUninit::<[u8; 1 << 47]> {
    std::mem::MaybeUninit::<[u8; 1 << 47]>::uninit()
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-const-eval Area: Constant evaluation, covers all const contexts (static, const fn, ...) A-mir-opt Area: MIR optimizations A-monomorphization Area: Monomorphization C-bug Category: This is a bug. 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