Implement `tainted_by_errors` in MIR borrowck, use it to skip CTFE by compiler-e...
source link: https://github.com/rust-lang/rust/pull/93691
Go to the source link to view the article. You can view the picture content, updated content and better typesetting reading experience. If the link is broken, please click the button below to view the snapshot at that time.
Conversation
Putting this up for initial review. The issue that I found is when we're evaluating a const, we're doing borrowck, but doing nothing with the fact that borrowck fails.
This implements a tainted_by_errors field for MIR borrowck like we have in infcx, so we can use that information to return an Err during const eval if our const fails to borrowck.
This PR needs some cleaning up. I should probably just use Result in more places, instead of .expecting in the places I am, but I just wanted it to compile so I could see if it worked!
Fixes #93646
r? @oli-obk
feel free to reassign
I wonder what other places we could use this tainted field that we generate during MIR borrowck...
This comment has been hidden.
compiler/rustc_borrowck/src/lib.rs
Outdated
pub fn buffer_error(&mut self, t: DiagnosticBuilder<'_>) {
self.tainted_by_errors = true;
t.buffer(&mut self.errors_buffer);
Seems a bit fragile to keep exposing the buffer list. Maybe nest it in some structure that contains the tainted field and the errors? Or at least leave a FIXME for it ^^
Not sure what you mean by this.
This method on MirBorrowckCtxt is just convenience to both buffer the diagnostic (which we are already doing before this diff) and set tainted_by_errors to true, since I didn't want to have to duplicate setting that boolean (or calling a method to set that boolean) in like the 20 spots I introduced this helper.
Sorry. What I mean is that the previous way is still possible. So people may add invocations of .buffer on the diagnostic and thus keep forgetting to set the tainted flag. If we hide the buffer as a private field of a struct we can avoid people accessing it directly.
It's more of a concern for careless impls in the future than a problem with the PR.
As an alternative we could check the sites where the buffer is consumed. If it is not empty, taint the borrowck result
oh yeah, good point, I can hide buffer in a harder-to-access field to prevent people accidentally not setting tainted with it too.
I can't just set tainted when we drain the buffer later because DiagnosticBuilder::buffer sometimes just emits the error directly instead of buffering it depending on some settings.
@oli-obk, so I thought about putting it into Body, but it looks like borrowck actually treats Body as immutable in favor of returning a different BorrowCheckResult struct, and I didn't want to do something with interior mutability or reallocating a new Body...
I was thinking of doing it where you now return Err, as we definitely have a mutable body available. But at that point the field on the body becomes fairly useless.
We could instead just check borrowck (and probably typeck, too) in load_mir. Then we can remove the redundant check in the const eval queries. There are other checks in those queries that may be good to move to load_mir, too
We could instead just check borrowck (and probably typeck, too) in
load_mir.
Not opposed to this necessarily, but even if we do borrowck again in load_mir, don't we still need to expose some field like the tainted_by_errors I added that reflects the fact an error occurred during that check?
We could instead just check borrowck (and probably typeck, too) in
load_mir.Not opposed to this necessarily, but even if we do borrowck again in
load_mir, don't we still need to expose some field like thetainted_by_errorsI added that reflects the fact an error occurred during that check?
Oh yes, we need all your borrowck changes , only now the mir queries don't change ^^
oh sure, let me see if i can do that then.
force-pushed the mir-tainted-by-errors
branch
2 times, most recently
from
2afbfaf
to
bc9ea71
11 days ago
changed the title
[WIP] Implement tainted_by_errors in MIR borrowck, use it to skip CTFE
Implement tainted_by_errors in MIR borrowck, use it to skip CTFE
@oli-obk: Yeah, I didn't need to Result-ify all those MIR queries actually. This is much simpler.
There are two places I do a borrowck call (eval_to_allocation_raw_provider and InterpCx::load_mir), but those are also two places we're currently also checking for typeck errors. Not sure if it's significant to have both of them, and I can try to deduplicate those if you want.
This comment has been hidden.
@@ -833,13 +833,14 @@ rustc_queries! {
/// additional requirements that the closure's creator must verify.
query mir_borrowck(key: LocalDefId) -> &'tcx mir::BorrowCheckResult<'tcx> {
desc { |tcx| "borrow-checking `{}`", tcx.def_path_str(key.to_def_id()) }
cache_on_disk_if(tcx) { tcx.is_typeck_child(key.to_def_id()) }
cache_on_disk_if { true }
Why is this necessary?
I'm going to do a perf run,? but maybe we just need to check the tainted flag here, too
I was getting ICEs in incremental tests complaining that the MIR body for a fn was stolen, since we're now calling borrowck which depends on unoptimized MIR which is probably gone if we do const eval late in the compilation process...
Not sure if only caching if the body if it is tainted works, since we're unconditionally calling borrowck before const eval.
heh, yea, this is what @BoxyUwU ran into, too, and we never got as far as to figuring it out.
I guess it makes sense that if the cache rules differ between two queries that work on stealing things, that said caching breaks, but it's really opaque to me. Well... let's see what perf says
Perf does show the additional query result serialization, so we should try to do something here.
It's probably not worth it, but we could try adding a query that returns just an Option<ErrorReported> and does all the processing of typeck, borrowck and const qualifs. Then we can only cache that on disk and it should rarely get recomputed. But: new query so probably overkill, but worth an experiment?
Yeah, I'm not exactly sure what we can do to limit cache_on_disk just to when it's needed, since it seems like we need to cache it for any body that has a promoted const, see the failing query stack trace:
query stack during panic:
#0 [mir_borrowck] borrow-checking `lit_test::lit_test`
#1 [eval_to_allocation_raw] const-evaluating + checking `lit_test::lit_test::promoted[1]`
#2 [eval_to_allocation_raw] const-evaluating + checking `lit_test::lit_test::promoted[1]`
#3 [optimized_mir] optimizing MIR for `lit_test::lit_test`
#4 [collect_and_partition_mono_items] collect_and_partition_mono_items
... but we don't know if a body has any promoted consts until after borrowck.
so I might try the query experiment you suggested. I'll ping you for a perf run when I put it up, since I don't have super powers.
Actually, we do know it at borrowck time, so we could encode that info in the borrowck result.
But maybe we're going about this the wrong way. When we force mir borrowck and then steal the mir, we own the mir, so we can mutate it and change its tainted field. This is essentially the same idea as the merging of the tainted flags into one query only we're putting it right in the data we have to load anyway
Actually, we do know it at borrowck time, so we could encode that info in the borrowck result.
Oh lol, I thought we did borrowck on mir_built, whoopsy.
When we force mir borrowck and then steal the mir, we own the mir, so we can mutate it and change its tainted field.
So you're suggesting we do add the tainted field to mir::Body then, instead of BorrowCheckerResult?
You need it on both, as (as you reminded me) you can't mutate the mir from borrowck. But at the query that steals the mir after forcing the borrowck query, you can just call borrowck instead of forcing it and copy the tainted field over to the mir body you just stole
@@ -214,6 +214,7 @@ pub struct BorrowCheckResult<'tcx> {
pub concrete_opaque_types: VecMap<OpaqueTypeKey<'tcx>, Ty<'tcx>>,
pub closure_requirements: Option<ClosureRegionRequirements<'tcx>>,
pub used_mut_upvars: SmallVec<[Field; 8]>,
pub tainted_by_errors: bool,
Please make it an option like in the typeck results .this will make it easier for the in-flight PRs that make our error handling more solid
@@ -287,6 +287,9 @@ pub fn eval_to_allocation_raw_provider<'tcx>(
if let Some(error_reported) = tcx.typeck_opt_const_arg(def).tainted_by_errors {
A bit more than planned for this PR, but if you want to try doing this cleanup in this PR, I'm not going to complain ;)
Basically remove the entire logic in the is_local if block and move whatever is missing in load_mir to it.
We can also guard the checks in load_mir with the is_local check, as any mir we load from dependencies must be error free
That said, feel free not to do this in this PR .we can merge it soon in any case
Awaiting bors try build completion.
@rustbot label: +S-waiting-on-perf
Trying commit 
Test timed out
Awaiting bors try build completion.
@rustbot label: +S-waiting-on-perf
Try build successful - checks-actions
Build commit: d106320 (d106320594d82e721bfc144b18d7f1db27135b68)
The latest upstream changes (presumably #93893) made this pull request unmergeable. Please resolve the merge conflicts.
rebased, an error message went away but it was definitely due to opaque types PR being reverted.
@rustbot ready
@bors r+
Commit 67ad0ff has been approved by oli-obk
Test successful - checks-actions
Approved by: oli-obk
Pushing 9cdefd7 to master...
Finished benchmarking commit (9cdefd7): comparison url.
Summary: This benchmark run shows 5 relevant improvements 
but 3 relevant regressions 
to instruction counts.
- Average relevant regression: 1.7%
- Average relevant improvement: -0.4%
- Largest improvement in instruction counts: -0.6% on
incr-unchangedbuilds ofkeccak check - Largest regression in instruction counts: 2.4% on
incr-patched: u8 3072builds ofissue-46449 debug
If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.
Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.
@rustbot label: +perf-regression
@rustbot label: +perf-regression-triaged
This fixes a bunch of compiler crashes that occurred because we weren't checking for borrowck errors before doing const eval. The additional work is entirely expected and other avenues did not pan out.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Recommend
About Joyk
Aggregate valuable and interesting links.
Joyk means Joy of geeK