-
Notifications
You must be signed in to change notification settings - Fork 13.5k
GCI: Don't evaluate the initializer of free const items that have trivially unsatisfied predicates #142293
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
base: master
Are you sure you want to change the base?
GCI: Don't evaluate the initializer of free const items that have trivially unsatisfied predicates #142293
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -238,12 +238,18 @@ pub fn check_crate(tcx: TyCtxt<'_>) { | |
check::maybe_check_static_with_link_section(tcx, item_def_id); | ||
} | ||
DefKind::Const if !tcx.generics_of(item_def_id).own_requires_monomorphization() => { | ||
// FIXME(generic_const_items): Passing empty instead of identity args is fishy but | ||
// seems to be fine for now. Revisit this! | ||
let instance = ty::Instance::new_raw(item_def_id.into(), ty::GenericArgs::empty()); | ||
let cid = GlobalId { instance, promoted: None }; | ||
let typing_env = ty::TypingEnv::fully_monomorphized(); | ||
tcx.ensure_ok().eval_to_const_value_raw(typing_env.as_query_input(cid)); | ||
let predicates = tcx.predicates_of(item_def_id); | ||
let predicates = predicates.instantiate_identity(tcx).predicates; | ||
|
||
if !traits::impossible_predicates(tcx, predicates) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it guaranteed that (I feel I asked @BoxyUwU that same question already earlier this year in a different context and the answer was "yes it's fine", but I'm not sure...) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unfortunately I can't answer that with 100% confidence since I'm not super familiar with the innards and all the little details of const eval. I know that layout computation has been made more robust against that somewhat recently but I'm not sure about the rest. I know that the MIR pass It's certainly a valid question and I need to have researched that before opening the RFC. Tho if my tired brain doesn't trick me right now, my change should in theory eliminate potential ICEs rather than introducing new ones because we eval less. Footnotes
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yeah, definitely. I was just wondering if it evals less enough. ;) |
||
// FIXME(generic_const_items): Passing empty instead of identity args is fishy but | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this should be fixed yeah 🤔 I don't think ctfe should have to handle there being instances with wrong args There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right, we should. If you're okay with it, I'd prefer to address this in a separate PR because this is super perf sensitive. This used to be a call to fn E.g., ea1cdda (perf results: #136429 (comment)) and 6f11489 (perf results: #136429 (comment)). Ofc, they also include unrelated changes to WF-check, so the results are skewed. |
||
// seems to be fine for now. Revisit this! | ||
let instance = | ||
ty::Instance::new_raw(item_def_id.into(), ty::GenericArgs::empty()); | ||
let cid = GlobalId { instance, promoted: None }; | ||
let typing_env = ty::TypingEnv::fully_monomorphized(); | ||
tcx.ensure_ok().eval_to_const_value_raw(typing_env.as_query_input(cid)); | ||
} | ||
} | ||
_ => (), | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,9 +12,11 @@ use crate::ty::{self, ConstToValTreeResult, GenericArgs, TyCtxt, TypeVisitableEx | |
use crate::{error, mir}; | ||
|
||
impl<'tcx> TyCtxt<'tcx> { | ||
/// Evaluates a constant without providing any generic parameters. This is useful to evaluate consts | ||
/// that can't take any generic arguments like const items or enum discriminants. If a | ||
/// generic parameter is used within the constant `ErrorHandled::TooGeneric` will be returned. | ||
/// Evaluates a constant without providing any generic parameters. | ||
/// | ||
/// This is useful to evaluate consts that can't take any generic arguments like enum | ||
/// discriminants. If a non-region generic parameter is used within the constant | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (apart splitting of the synopsis / introductory paragraph, here & below I changed |
||
/// `ErrorHandled::TooGeneric` will be returned. | ||
#[instrument(skip(self), level = "debug")] | ||
pub fn const_eval_poly(self, def_id: DefId) -> EvalToConstValueResult<'tcx> { | ||
// In some situations def_id will have generic parameters within scope, but they aren't allowed | ||
|
@@ -28,9 +30,11 @@ impl<'tcx> TyCtxt<'tcx> { | |
self.const_eval_global_id(typing_env, cid, DUMMY_SP) | ||
} | ||
|
||
/// Evaluates a constant without providing any generic parameters. This is useful to evaluate consts | ||
/// that can't take any generic arguments like const items or enum discriminants. If a | ||
/// generic parameter is used within the constant `ErrorHandled::TooGeneric` will be returned. | ||
/// Evaluates a constant without providing any generic parameters. | ||
/// | ||
/// This is useful to evaluate consts that can't take any generic arguments like enum | ||
/// discriminants. If a non-region generic parameter is used within the constant | ||
/// `ErrorHandled::TooGeneric` will be returned. | ||
#[instrument(skip(self), level = "debug")] | ||
pub fn const_eval_poly_to_alloc(self, def_id: DefId) -> EvalToAllocationRawResult<'tcx> { | ||
// In some situations def_id will have generic parameters within scope, but they aren't allowed | ||
|
fmease marked this conversation as resolved.
Show resolved
Hide resolved
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
As discussed, long term, that would be the way to go. However, it makes me uncomfortable that Caller graph (X called by Y)
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (As an aside, even if we enriched |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -37,6 +37,7 @@ use rustc_middle::query::Providers; | |
use rustc_middle::ty::{self, ExistentialTraitRef, TyCtxt}; | ||
use rustc_privacy::DefIdVisitor; | ||
use rustc_session::config::CrateType; | ||
use rustc_trait_selection::traits; | ||
use tracing::debug; | ||
|
||
/// Determines whether this item is recursive for reachability. See `is_recursively_reachable_local` | ||
|
@@ -205,19 +206,34 @@ impl<'tcx> ReachableContext<'tcx> { | |
} | ||
|
||
hir::ItemKind::Const(_, _, _, init) => { | ||
// Only things actually ending up in the final constant value are reachable | ||
// for codegen. Everything else is only needed during const-eval, so even if | ||
// const-eval happens in a downstream crate, all they need is | ||
// `mir_for_ctfe`. | ||
if self.tcx.generics_of(item.owner_id).own_requires_monomorphization() { | ||
// In this case, we don't want to evaluate the const initializer. | ||
// In lieu of that, we have to consider everything mentioned in it | ||
// as reachable, since it *may* end up in the final value. | ||
self.visit_nested_body(init); | ||
return; | ||
} | ||
|
||
let predicates = self.tcx.predicates_of(item.owner_id); | ||
let predicates = predicates.instantiate_identity(self.tcx).predicates; | ||
if traits::impossible_predicates(self.tcx, predicates) { | ||
// The constant is impossible to reference. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Extend comment: If the usage site also assumes these impossible bounds (so of its ParamEnv also contains these bounds), it's still fine because the surrounding body of the usage site gets replaced with We might want to add a test for all of this. Note to self: However, there are cases where the MIR pass and reachability analysis disagrees about what constitutes an impossible bound (namely if the predicates contains early-bound regions). Check if this is problematic and can be exploited. |
||
// Therefore nothing can be reachable via it. | ||
return; | ||
} | ||
|
||
match self.tcx.const_eval_poly_to_alloc(item.owner_id.def_id.into()) { | ||
Ok(alloc) => { | ||
// Only things actually ending up in the final constant value are | ||
// reachable for codegen. Everything else is only needed during | ||
// const-eval, so even if const-eval happens in a downstream crate, | ||
// all they need is `mir_for_ctfe`. | ||
let alloc = self.tcx.global_alloc(alloc.alloc_id).unwrap_memory(); | ||
self.propagate_from_alloc(alloc); | ||
} | ||
// We can't figure out which value the constant will evaluate to. In | ||
// lieu of that, we have to consider everything mentioned in the const | ||
// initializer reachable, since it *may* end up in the final value. | ||
Err(ErrorHandled::TooGeneric(_)) => self.visit_nested_body(init), | ||
// We've checked at the start that there aren't any non-lifetime params | ||
// in scope. The const initializer can't possibly be too generic. | ||
Err(ErrorHandled::TooGeneric(_)) => bug!(), | ||
// If there was an error evaluating the const, nothing can be reachable | ||
// via it, and anyway compilation will fail. | ||
Err(ErrorHandled::Reported(..)) => {} | ||
|
This file was deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would expect
!own_requires_monomorphization
means there are no generics to begin with? Or what does that function mean? It is sadly undocumented...It does not make sense to me that we'd ever evaluate a generic free const item. How would we even pick the instance for the generic parameters...?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, there was #136429 which I already forgot about. IMO the obvious fix is to just revert that PR...
... but I guess that wouldn't even be enough since even non-generic consts can have where clauses. Right. Uh, we may have to just skip all of those?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
own_requires_monomorphization
is synonymous with "has non-region generic params" meaning we do eval lifetime-generic const exprs (more precisely early-bound-lifetime-generic).Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's obviously fine since we're only dealing with lifetime params since trait resolution etc. happens modulo non-late-bound regions / most if not all regions get fully erased. No problem there.