Implement `tainted_by_errors` in MIR borrowck, use it to skip CTFE #93691

compiler-errors · 2022-02-06T05:03:46Z

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

compiler-errors · 2022-02-06T05:04:12Z

I wonder what other places we could use this tainted field that we generate during MIR borrowck...

oli-obk

I think cbec6f8 is too invasive and does not fit how most queries handle errors. Maybe we should just add a tainted_by_errors field to mir::Body, too?

What do you think?

oli-obk · 2022-02-06T09:12:21Z

compiler/rustc_borrowck/src/lib.rs

+
+    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.

compiler-errors · 2022-02-06T14:08:46Z

@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...

oli-obk · 2022-02-06T14:39:33Z

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

compiler-errors · 2022-02-06T14:47:12Z

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?

oli-obk · 2022-02-06T14:56:18Z

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?

Oh yes, we need all your borrowck changes , only now the mir queries don't change ^^

compiler-errors · 2022-02-06T14:57:56Z

oh sure, let me see if i can do that then.

compiler-errors · 2022-02-06T20:21:04Z

@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.

oli-obk · 2022-02-07T10:35:20Z

compiler/rustc_middle/src/query/mod.rs

@@ -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

oli-obk · 2022-02-07T10:38:00Z

compiler/rustc_middle/src/mir/query.rs

@@ -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

oli-obk · 2022-02-07T10:46:13Z

compiler/rustc_const_eval/src/const_eval/eval_queries.rs

@@ -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

oli-obk · 2022-02-07T10:48:05Z

@bors try @rust-timer queue

rust-timer · 2022-02-07T10:48:06Z

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

bors · 2022-02-07T10:48:13Z

⌛ Trying commit eb42e801ea203377536cf31ad51caae5cdcd1449 with merge 86a277ecc46223ab296a8b964b834266a79a9d1f...

bors · 2022-02-07T15:03:13Z

💥 Test timed out

oli-obk · 2022-02-07T15:15:58Z

@bors try @rust-timer queue

rust-timer · 2022-02-07T15:16:00Z

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

bors · 2022-02-07T15:16:07Z

⌛ Trying commit eb42e801ea203377536cf31ad51caae5cdcd1449 with merge d106320594d82e721bfc144b18d7f1db27135b68...

bors · 2022-02-07T16:45:50Z

☀️ Try build successful - checks-actions
Build commit: d106320594d82e721bfc144b18d7f1db27135b68 (d106320594d82e721bfc144b18d7f1db27135b68)

compiler-errors · 2022-02-11T21:12:21Z

rebased, an error message went away but it was definitely due to opaque types PR being reverted.

@rustbot ready

oli-obk · 2022-02-11T21:23:22Z

@bors r+

bors · 2022-02-11T21:23:24Z

📌 Commit 67ad0ff has been approved by oli-obk

bors · 2022-02-12T05:19:36Z

⌛ Testing commit 67ad0ff with merge 9cdefd7...

bors · 2022-02-12T07:47:35Z

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing 9cdefd7 to master...

rust-timer · 2022-02-12T09:18:34Z

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-unchanged builds of keccak check
Largest regression in instruction counts: 2.4% on incr-patched: u8 3072 builds of issue-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

oli-obk · 2022-02-12T09:45:17Z

@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.

rust-highfive assigned oli-obk Feb 6, 2022

rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Feb 6, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 6, 2022

This comment has been minimized.

Sign in to view

oli-obk requested changes Feb 6, 2022

View reviewed changes

compiler-errors force-pushed the mir-tainted-by-errors branch 2 times, most recently from 2afbfaf to bc9ea71 Compare February 6, 2022 20:17

compiler-errors 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 Feb 6, 2022

This comment has been minimized.

Sign in to view

oli-obk reviewed Feb 7, 2022

View reviewed changes

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Feb 7, 2022

compiler-errors added 6 commits February 11, 2022 12:45

skip const eval if we have an error in borrowck

77dae2d

fix tests, add new tests checking borrowck CFTE ICE

06eb912

always cache result from mir_borrowck

8b7b0a0

rework borrowck errors so that it's harder to not set tainted

29c2bb5

add tainted_by_errors to mir::Body

a431174

use body.tainted_by_error to skip loading MIR

67ad0ff

compiler-errors force-pushed the mir-tainted-by-errors branch from 72f265e to 67ad0ff Compare February 11, 2022 21:04

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 11, 2022

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 11, 2022

bors added the merged-by-bors This PR was explicitly merged by bors. label Feb 12, 2022

bors merged commit 9cdefd7 into rust-lang:master Feb 12, 2022

rustbot added this to the 1.60.0 milestone Feb 12, 2022

This was referenced Feb 12, 2022

Unstably constify impl<I: Iterator> IntoIterator for I #90602

Merged

More informative error message for E0015 #90532

Merged

rustbot added the perf-regression-triaged The performance regression has been triaged. label Feb 12, 2022

matthiaskrgr mentioned this pull request Feb 13, 2022

ice: SaveContext::typeck_results called outside of body' with -Zsave-analysis #88536

Closed

Alexendoo mentioned this pull request Feb 13, 2022

ICE: const_panic panics in some cases #85907

Closed

eddyb mentioned this pull request Feb 23, 2022

rustc_errors: let DiagnosticBuilder::emit return a "guarantee of emission". #93368

Merged

eddyb mentioned this pull request Mar 3, 2022

regression: ICE -- bad ConstKind after monomorphizing #94502

Closed

compiler-errors deleted the mir-tainted-by-errors branch April 7, 2022 04:32

compiler-errors mentioned this pull request Apr 30, 2023

Leave promoteds untainted by errors when borrowck fails #111038

Merged

		@@ -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 {

Implement tainted_by_errors in MIR borrowck, use it to skip CTFE #93691

Implement tainted_by_errors in MIR borrowck, use it to skip CTFE #93691

Conversation

compiler-errors commented Feb 6, 2022

compiler-errors commented Feb 6, 2022

This comment has been minimized.

oli-obk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

compiler-errors commented Feb 6, 2022

oli-obk commented Feb 6, 2022

compiler-errors commented Feb 6, 2022 • edited Loading

oli-obk commented Feb 6, 2022

compiler-errors commented Feb 6, 2022

compiler-errors commented Feb 6, 2022

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oli-obk commented Feb 7, 2022

rust-timer commented Feb 7, 2022

bors commented Feb 7, 2022

bors commented Feb 7, 2022

oli-obk commented Feb 7, 2022

rust-timer commented Feb 7, 2022

bors commented Feb 7, 2022

bors commented Feb 7, 2022

compiler-errors commented Feb 11, 2022

oli-obk commented Feb 11, 2022

bors commented Feb 11, 2022

bors commented Feb 12, 2022

bors commented Feb 12, 2022

rust-timer commented Feb 12, 2022

oli-obk commented Feb 12, 2022

Implement `tainted_by_errors` in MIR borrowck, use it to skip CTFE #93691

Implement `tainted_by_errors` in MIR borrowck, use it to skip CTFE #93691

compiler-errors commented Feb 6, 2022 •

edited

Loading