-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Const-eval errors in dead functions are optimization-dependent #107503
Comments
@rustbot modify labels +T-lang |
Ahem. Thank you for this most illuminating and horrifying example. |
On a related note, there's also this following code that compiles in release mode but fails to compile in debug mode: [playground] #![feature(inline_const)]
#[inline(always)]
fn falser() -> bool {
false
}
fn has_late_error<T>() {
const {
panic!()
};
}
fn main() {
if falser() {
has_late_error::<()>();
}
} Maybe it makes more sense to track |
To be clear, this has nothing to do with inline_const, right? Associated consts show the same behavior? |
I believe so, yes; any post-mono error should work. It's just significantly easier to show with panicking inline const than any other post-mono error, and much more accessible. I believe this just results from |
@RalfJung Is it possible to produce a similarly horrifying example as #107503 (comment) that compiles on stable today, and is optimization-dependent and DCA dependent? |
@joshtriplett yes. It's not even that hard. pub fn cause_late_error<T>() {
struct S;
impl S {
const C: () = panic!();
}
S::C;
}
fn main() {
let a = 1;
let b = 2;
if a + b == 5 { // changing 5 to 3 makes compilation fail
cause_late_error::<()>();
}
} |
And of course that also works for @oskgo's example: #[inline(always)]
fn falser() -> bool {
false
}
pub fn has_late_error<T>() {
struct S;
impl S {
const C: () = panic!();
}
S::C;
}
fn main() {
if falser() {
has_late_error::<()>();
}
} Compiles in release mode but not in debug mode. |
Sorry if this sounds rude, but could we just issue a crater run and push a breaking change at this point? Panic in consteval is not something stabilized for too long. We can recognize this1 as a design mistake. Whether the code depends on something other than Footnotes
|
This is the same underlying issue as #106617, so we probably should close one of these in favor of the other (or open a new issue to track the collected problems). We don't monomorphize dead code, which means
All of this affects stable Rust with associated consts. |
This was mentioned over in #106617 (comment), but it's worth highlighting here that this same problem can be triggered without using fn cause_late_error<T>() {
cause_late_error::<((), T)>();
}
fn main() {
let (a, b) = (1, 2);
if a + b == 5 {
cause_late_error::<()>();
}
} |
I think there's a fairly obvious way to fix this, if someone wants to give it a shot: we basically mirror
The big question is what the perf impact of that will be, but the only way to figure that out is to actually implement this. Any takers? :) |
on it |
Draft: monomorphize things from dead code, too This is another attempt at fixing rust-lang#107503. The previous attempt at rust-lang#112879 seems stuck in figuring out where the perf regression comes from. So here I want to take baby steps to see the impact of each step. r? `@ghost`
Draft: monomorphize things from dead code, too This is another attempt at fixing rust-lang#107503. The previous attempt at rust-lang#112879 seems stuck in figuring out where the perf regression comes from. So here I want to take baby steps to see the impact of each step. r? `@ghost`
Draft: monomorphize things from dead code, too This is another attempt at fixing rust-lang#107503. The previous attempt at rust-lang#112879 seems stuck in figuring out where the perf regression comes from. So here I want to take baby steps to see the impact of each step. r? `@ghost`
The approximation is inherent to the problem since it is non-computable precisely. You can view the role of a reachability pass as a form of item collection that explores all possible instantiations of generic code. This work cannot be done by a mono item collector. Note that the example from #107503 (comment) is optimization level dependent. |
I have sketched a computable strategy in my previous post. I don't see why it is necessary to explore "all possible instantiations". The way I view it, Both mono item collection and EDIT: Ah... I missed the issue of a generic function calling a monomorphic internal function. You are right.
I can't reproduce that, this example fails both in debug and release builds. |
In general this behavior of skipping a bunch of checking on dead private functions is usually quite surprising anyway. Maybe the collector should just always walk all monomorphic functions, reachable or not, in "mentioned" mode? Though I guess that means it has to run MIR opts on them, which is work we currently avoid doing. How likely is it that people have large amounts of monomorphic functions that never get mentioned (transitively) in anything "reachable"...? |
In your example the constant happens to be evaluated by one of lint passes running on MIR. This evaluation is easily avoidable. The example below fails only with optimizations enabled: struct Fail<T>(T);
impl<T> Fail<T> {
const C: () = panic!();
}
pub fn f() {
loop {}; g()
}
#[inline(never)]
fn g() {
h::<i32>()
}
fn h<T>() {
Fail::<T>::C;
} |
So we have HIR-level optimizations? Where are those happening? Or is that just the same issue as your previous example above? |
Oh lol so this is the other way around then, the unoptimized build misses the const-eval. Wtf, how is that happening... |
recursively evaluate the constants in everything that is 'mentioned' This is another attempt at fixing rust-lang#107503. The previous attempt at rust-lang#112879 seems stuck in figuring out where the [perf regression](https://perf.rust-lang.org/compare.html?start=c55d1ee8d4e3162187214692229a63c2cc5e0f31&end=ec8de1ebe0d698b109beeaaac83e60f4ef8bb7d1&stat=instructions:u) comes from. In rust-lang#122258 I learned some things, which informed the approach this PR is taking. Quoting from the new collector docs, which explain the high-level idea: ```rust //! One important role of collection is to evaluate all constants that are used by all the items //! which are being collected. Codegen can then rely on only encountering constants that evaluate //! successfully, and if a constant fails to evaluate, the collector has much better context to be //! able to show where this constant comes up. //! //! However, the exact set of "used" items (collected as described above), and therefore the exact //! set of used constants, can depend on optimizations. Optimizing away dead code may optimize away //! a function call that uses a failing constant, so an unoptimized build may fail where an //! optimized build succeeds. This is undesirable. //! //! To fix this, the collector has the concept of "mentioned" items. Some time during the MIR //! pipeline, before any optimization-level-dependent optimizations, we compute a list of all items //! that syntactically appear in the code. These are considered "mentioned", and even if they are in //! dead code and get optimized away (which makes them no longer "used"), they are still //! "mentioned". For every used item, the collector ensures that all mentioned items, recursively, //! do not use a failing constant. This is reflected via the [`CollectionMode`], which determines //! whether we are visiting a used item or merely a mentioned item. //! //! The collector and "mentioned items" gathering (which lives in `rustc_mir_transform::mentioned_items`) //! need to stay in sync in the following sense: //! //! - For every item that the collector gather that could eventually lead to build failure (most //! likely due to containing a constant that fails to evaluate), a corresponding mentioned item //! must be added. This should use the exact same strategy as the ecollector to make sure they are //! in sync. However, while the collector works on monomorphized types, mentioned items are //! collected on generic MIR -- so any time the collector checks for a particular type (such as //! `ty::FnDef`), we have to just onconditionally add this as a mentioned item. //! - In `visit_mentioned_item`, we then do with that mentioned item exactly what the collector //! would have done during regular MIR visiting. Basically you can think of the collector having //! two stages, a pre-monomorphization stage and a post-monomorphization stage (usually quite //! literally separated by a call to `self.monomorphize`); the pre-monomorphizationn stage is //! duplicated in mentioned items gathering and the post-monomorphization stage is duplicated in //! `visit_mentioned_item`. //! - Finally, as a performance optimization, the collector should fill `used_mentioned_item` during //! its MIR traversal with exactly what mentioned item gathering would have added in the same //! situation. This detects mentioned items that have *not* been optimized away and hence don't //! need a dedicated traversal. enum CollectionMode { /// Collect items that are used, i.e., actually needed for codegen. /// /// Which items are used can depend on optimization levels, as MIR optimizations can remove /// uses. UsedItems, /// Collect items that are mentioned. The goal of this mode is that it is independent of /// optimizations: the set of "mentioned" items is computed before optimizations are run. /// /// The exact contents of this set are *not* a stable guarantee. (For instance, it is currently /// computed after drop-elaboration. If we ever do some optimizations even in debug builds, we /// might decide to run them before computing mentioned items.) The key property of this set is /// that it is optimization-independent. MentionedItems, } ``` And the `mentioned_items` MIR body field docs: ```rust /// Further items that were mentioned in this function and hence *may* become monomorphized, /// depending on optimizations. We use this to avoid optimization-dependent compile errors: the /// collector recursively traverses all "mentioned" items and evaluates all their /// `required_consts`. /// /// This is *not* soundness-critical and the contents of this list are *not* a stable guarantee. /// All that's relevant is that this set is optimization-level-independent, and that it includes /// everything that the collector would consider "used". (For example, we currently compute this /// set after drop elaboration, so some drop calls that can never be reached are not considered /// "mentioned".) See the documentation of `CollectionMode` in /// `compiler/rustc_monomorphize/src/collector.rs` for more context. pub mentioned_items: Vec<Spanned<MentionedItem<'tcx>>>, ``` Fixes rust-lang#107503
FYI, #109247 broke a handful of crates for example #110475 because of invalid dead code. Errors in monomorphic dead code I'm not sure, but errors in dead code surely exist, and I find it concerning that when we uncover these errors they get reported as regressions. Perhaps people would be more accepting of such a change if we announce it beforehand and expand our detection of erroneous code only once. |
Ah, good point. I was mostly worried about performance. I don't know if there are cases where code would previously build with with and without optimizations, and then after that change it now fails in both cases. But it's very possible. I'm currently running this as a perf experiment in #122568, but I don't intend to land it in the same change. The rest of #122568 I am fairly sure only affects code that already fails when built without optimizations. |
@rustbot labels -I-lang-nominated We discussed this in lang triage today. Our consensus was that this is a bug and the fix can go forward with no further action from lang. Regarding whether this should block the stabilization of inline const, we tentatively felt that it should not, but we wanted to discuss that with more of the team before resolving the blocking concern on #104087. |
Ah, reading that concern now gives me flashbacks. ;) That concern makes a difference between "Optimization-dependent errors" and "Errors in unused code". It turns out those cannot be disentangled, as basically all notions of "used" that exist in rustc depend on optimization levels. (This was not the case back when the comment was written, but is true ever since |
The only way I can imagine making On the other hand, the fact that this bug has been shipping on nightly for quite some time and nobody has complained perhaps indicates that the impact is not too widespread. I am very happy to see anyone trying to untangle the reachability/MIR collection code. |
That was three "not" in a row, you lost me here. ;)
I gave up the hope of trying to simplify it, but maybe I can at least document it a bit better. ;) |
I think library authors will be equally confused by (yeah wow that sentence is a mess) |
All uses in libraries I have seen are one of:
and I have certainly seen a few particularly... dank... usages of these kinds of "const assertions". |
To be clear, the only blessed pattern libs may use is that if a const is directly used inside some function, then whenever that function executes with a given choice of its generic parameters, the const succeeded evaluating. So we do not guarantee that the code in the OP will fail to build! The point of this issue is that we don't want optimization-level-dependent behavior. But which functions we walk and evaluate consts for is still not a stable guarantee -- except that any function that actually gets executed has been walked and had its consts evaluated. |
yeah, that seems fine? weird but fine. |
the OP's code fails to build in both debug and release now. huh. weird. |
Probably needs an inline(never).
We have tests in tree that demo the opt dependent behavior; see the PR that closes this issue.
|
Hm, I wonder if inlining can also cause trouble here... if a call like I'm beginning to wonder if MIR passes need a way to eval consts such that even if it fails, no error is emitted. |
(After some experimentation with inlining) I think we aren't even computing |
recursively evaluate the constants in everything that is 'mentioned' This is another attempt at fixing rust-lang/rust#107503. The previous attempt at rust-lang/rust#112879 seems stuck in figuring out where the [perf regression](https://perf.rust-lang.org/compare.html?start=c55d1ee8d4e3162187214692229a63c2cc5e0f31&end=ec8de1ebe0d698b109beeaaac83e60f4ef8bb7d1&stat=instructions:u) comes from. In rust-lang/rust#122258 I learned some things, which informed the approach this PR is taking. Quoting from the new collector docs, which explain the high-level idea: ```rust //! One important role of collection is to evaluate all constants that are used by all the items //! which are being collected. Codegen can then rely on only encountering constants that evaluate //! successfully, and if a constant fails to evaluate, the collector has much better context to be //! able to show where this constant comes up. //! //! However, the exact set of "used" items (collected as described above), and therefore the exact //! set of used constants, can depend on optimizations. Optimizing away dead code may optimize away //! a function call that uses a failing constant, so an unoptimized build may fail where an //! optimized build succeeds. This is undesirable. //! //! To fix this, the collector has the concept of "mentioned" items. Some time during the MIR //! pipeline, before any optimization-level-dependent optimizations, we compute a list of all items //! that syntactically appear in the code. These are considered "mentioned", and even if they are in //! dead code and get optimized away (which makes them no longer "used"), they are still //! "mentioned". For every used item, the collector ensures that all mentioned items, recursively, //! do not use a failing constant. This is reflected via the [`CollectionMode`], which determines //! whether we are visiting a used item or merely a mentioned item. //! //! The collector and "mentioned items" gathering (which lives in `rustc_mir_transform::mentioned_items`) //! need to stay in sync in the following sense: //! //! - For every item that the collector gather that could eventually lead to build failure (most //! likely due to containing a constant that fails to evaluate), a corresponding mentioned item //! must be added. This should use the exact same strategy as the ecollector to make sure they are //! in sync. However, while the collector works on monomorphized types, mentioned items are //! collected on generic MIR -- so any time the collector checks for a particular type (such as //! `ty::FnDef`), we have to just onconditionally add this as a mentioned item. //! - In `visit_mentioned_item`, we then do with that mentioned item exactly what the collector //! would have done during regular MIR visiting. Basically you can think of the collector having //! two stages, a pre-monomorphization stage and a post-monomorphization stage (usually quite //! literally separated by a call to `self.monomorphize`); the pre-monomorphizationn stage is //! duplicated in mentioned items gathering and the post-monomorphization stage is duplicated in //! `visit_mentioned_item`. //! - Finally, as a performance optimization, the collector should fill `used_mentioned_item` during //! its MIR traversal with exactly what mentioned item gathering would have added in the same //! situation. This detects mentioned items that have *not* been optimized away and hence don't //! need a dedicated traversal. enum CollectionMode { /// Collect items that are used, i.e., actually needed for codegen. /// /// Which items are used can depend on optimization levels, as MIR optimizations can remove /// uses. UsedItems, /// Collect items that are mentioned. The goal of this mode is that it is independent of /// optimizations: the set of "mentioned" items is computed before optimizations are run. /// /// The exact contents of this set are *not* a stable guarantee. (For instance, it is currently /// computed after drop-elaboration. If we ever do some optimizations even in debug builds, we /// might decide to run them before computing mentioned items.) The key property of this set is /// that it is optimization-independent. MentionedItems, } ``` And the `mentioned_items` MIR body field docs: ```rust /// Further items that were mentioned in this function and hence *may* become monomorphized, /// depending on optimizations. We use this to avoid optimization-dependent compile errors: the /// collector recursively traverses all "mentioned" items and evaluates all their /// `required_consts`. /// /// This is *not* soundness-critical and the contents of this list are *not* a stable guarantee. /// All that's relevant is that this set is optimization-level-independent, and that it includes /// everything that the collector would consider "used". (For example, we currently compute this /// set after drop elaboration, so some drop calls that can never be reached are not considered /// "mentioned".) See the documentation of `CollectionMode` in /// `compiler/rustc_monomorphize/src/collector.rs` for more context. pub mentioned_items: Vec<Spanned<MentionedItem<'tcx>>>, ``` Fixes #107503
I tried this code: [playground]
I expected to see this happen:
Instead, this happened: it compiled without any warnings.
Meta
Playground nightly, 1.69.0-nightly (2023-01-30 001a77f)
Related
if const { expr }
even in opt-level=0 #85836Potential fix:
The text was updated successfully, but these errors were encountered: