Fix drop tracking ICEs and re-enable generator drop tracking #93180

eholk · 2022-01-21T23:39:39Z

Generator drop tracking recently was merged (#91032) but unfortunately caused several ICEs. The code is currently disabled (#93165, #93284). This PR fixes the underlying issues and re-enables drop range tracking.

There are several changes involved:

Allow uninhabited types in generator interiors
Support block-target break and continue expressions
Enable drop tracking

Allow uninhabited types in generator interiors

Drop tracking in the generator interior type checking pass would count all values in unreachable code as dropped (e.g. code after a call to a function with an uninhabited return type), which would lead to those values not being included in the witness type. This resulted in the type checker being more precise than the corresponding sanity check in the MIR transform.

This patch changes the check in the MIR code to match the results of typeck by skipping uninhabited types.

Fixes #93161

Support block-target `break` and `continue` expressions

The target of a break or continue expression is usually an expression, but can also be a block. This situation arises, for example, in try blocks. Drop tracking did not previously handle this case. To fix it, we look up the target of the expression and if it's a block we use the hir_id of the tail expression of the block rather than the block itself. This is because the drop tracking analysis only gives PostOrderIds to expressions, but not to blocks.

An alternative would be to give blocks a PostOrderId as well, but this needs to be kept in sync with the scope analysis in region.rs, so it seemed better not to have another pair of code blocks that need to be kept in sync.

Re-enable drop tracking

Changes the ENABLE_DROP_TRACKING flag to true in generator_interior.rs and re-enables all the tests that are enabled by it.

r? @nikomatsakis

eholk · 2022-01-21T23:40:45Z

Note that since this fixes the underlying issue worked around by #93165, we do have the option of aborting that PR and merging this instead. Otherwise, I'll rebase this one once #93165 lands.

lqd · 2022-01-22T11:54:35Z

I'd like to have PR artifacts so that people can check whether this fixes their ICEs

@bors try

Note that since this fixes the underlying issue worked around by #93165, we do have the option of aborting that PR and merging this instead. Otherwise, I'll rebase this one once #93165 lands.

There are ICEs being reported on nightly, so it would probably be safer to:

still go with the "soft revert" in Disable drop range tracking in generators #93165 to avoid the ICEs (the PR is in the process of being merged in rollups as I'm typing this)
temporarily enable drop range tracking in this PR, with the expected fix
check this PR on crater

bors · 2022-01-22T11:54:48Z

⌛ Trying commit 197640adea324c9fdedd3162286fdbb84f50805a with merge 12c8eb434c751050bfbc9b7e270af75a2ef98b4c...

bors · 2022-01-22T13:09:59Z

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

lqd · 2022-01-22T13:46:46Z

@eholk this may fix issue #93161 but doesn't seem to fix some of the other ICEs (thanks to @Eijebong for the help). You can try to build tonic locally with this PR and it should fail with a similar ICE.

Locally (with rustup-toolchain-install-master 12c8eb434c751050bfbc9b7e270af75a2ef98b4c -c rustfmt && git clone https://github.com/hyperium/tonic && cd tonic && RUST_BACKTRACE=1 cargo +12c8eb434c751050bfbc9b7e270af75a2ef98b4c build) ICEs:

error: internal compiler error: compiler/rustc_mir_transform/src/generator.rs:761:13: Broken MIR: generator contains type http::uri::Parts in MIR, but typeck only knows about {ResumeTy, &mut client::grpc::Grpc<T>, C, http::Request<UnsyncBoxBody<bytes::Bytes, status::Status>>, client::grpc::Grpc<T>, <T as GrpcService<UnsyncBoxBody<bytes::Bytes, status::Status>>>::Future, ()} and [&mut client::grpc::Grpc<T>, request::Request<S>, http::uri::PathAndQuery, C]

eholk · 2022-01-23T21:13:06Z

There are ICEs being reported on nightly, so it would probably be safer to:

I agree that this is the right course of action. I will continue working on this PR to fix any known PRs and make sure to do a crater run before merging it.

nikomatsakis · 2022-01-25T19:49:11Z

compiler/rustc_mir_transform/src/generator.rs

@@ -749,9 +749,15 @@ fn sanitize_witness<'tcx>(
        }
        let decl_ty = tcx.normalize_erasing_regions(param_env, decl.ty);

+        let is_uninhabited = tcx.is_ty_uninhabited_from(


Suggested change

let is_uninhabited = tcx.is_ty_uninhabited_from(

let is_inhabited = !tcx.is_ty_uninhabited_from(

nikomatsakis · 2022-01-25T19:51:35Z

compiler/rustc_mir_transform/src/generator.rs

        // Sanity check that typeck knows about the type of locals which are
        // live across a suspension point
-        if !allowed.contains(&decl_ty) && !allowed_upvars.contains(&decl_ty) {
+        if !is_uninhabited && !allowed.contains(&decl_ty) && !allowed_upvars.contains(&decl_ty) {


Suggested change

if !is_uninhabited && !allowed.contains(&decl_ty) && !allowed_upvars.contains(&decl_ty) {

if is_inhabited && !allowed.contains(&decl_ty) && !allowed_upvars.contains(&decl_ty) {

This if condition is sufficiently complex that, to me at least, it is easier to read this way.

Alternatively, I would consider:

if is_uninhabited || allowed.contains(&decl_ty) || allowed_upvars.contains(&decl_ty) { // This type which appears in the generator either... // - is uninhabited, in which case it can't actually be captured at runtime // - appears in the approximation from thje static type (`allowed`) // - appears in the lsit of upvars ... } else { span_bug!(...); }

(only those comments are wrong)

That said, maybe we should move the uninhabited check somewhere else so that we just don't capture the type at the MIR level?

I looked at that a little at first and gave up. The right place to do this is probably in either locals_live_across_suspend_points or maybe in the MaybeLiveLocals MIR pass that locals_live_across_suspend_points uses. In MaybeLiveLocals, I didn't see a great way to get access to the tcx to ask if a type is uninhabited, but I wouldn't be surprised if there's an obvious way I don't know about.

I like your rephrasing of the conditional, so I appliked your suggestion.

#93313 exposes information about uninhabited types in call expressions to analyses running on MIR, and would be an another way to close the precision gap.

@tmiasko - do you know if you change fixes this same issue as well? If so, I think yours is the more principled fix.

It does fix the test case included here.

I understood the difference to be caused by modelling of calls with uninhabited calls as not returning in handle_uninhabited_return, so #93313 should provide a MIR equivalent.

Awesome, thanks! I'll leave my version here for now, but if yours merges before this one I'll pull it out and rely on yours since it's better.

eholk · 2022-01-26T00:38:48Z

I've updated the PR to fix the known ICEs introduced by drop tracking. This means the PR now does three separate things (description is updated to match). Would folks prefer I split it into three PRs?

It would be good to do a crater run before merging this PR (or at least part 3) to uncover any other ICEs we may not have discovered yet.

pnkfelix · 2022-01-26T03:49:33Z

I've updated the PR to fix the known ICEs introduced by drop tracking. This means the PR now does three separate things (description is updated to match). Would folks prefer I split it into three PRs?

Are the three separate things reflected as individual commits? From my reading of your description and of the commit series, that seems to be the case, and I think we are fine with having them being collected in one PR here in that case. (If for some reason we want to backport one commit and not another, we can wrestle with that then.)

It would be good to do a crater run before merging this PR (or at least part 3) to uncover any other ICEs we may not have discovered yet.

Sounds like a good idea.

wesleywiser · 2022-01-26T17:22:14Z

@bors try

bors · 2022-01-26T17:22:24Z

⌛ Trying commit 487ba6534c7d4259446d3861977fbbb053153e22 with merge 2cd6942e35a7765b6869d3a149ca54f2598b77ad...

eholk · 2022-01-26T18:57:58Z

Are the three separate things reflected as individual commits? From my reading of your description and of the commit series, that seems to be the case, and I think we are fine with having them being collected in one PR here in that case.

It's not exactly one commit per thing, but I don't think there's any overlap between them. I can rebase into three commits if that's helpful.

bors · 2022-01-26T18:58:05Z

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

wesleywiser · 2022-01-26T19:06:07Z

@craterbot run mode=build-only

craterbot · 2022-01-26T19:06:14Z

👌 Experiment pr-93180 created and queued.
🤖 Automatically detected try build 2cd6942e35a7765b6869d3a149ca54f2598b77ad
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

craterbot · 2022-01-28T22:23:36Z

🚧 Experiment pr-93180 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

nikomatsakis · 2022-01-31T15:18:44Z

compiler/rustc_typeck/src/check/generator_interior/drop_ranges/cfg_build.rs

+    /// but this analysis only looks at expressions. In case a break has a block as a
+    /// target, this will find the last expression in the block and return its HirId
+    /// instead.
+    fn find_target_expression(&self, hir_id: HirId) -> HirId {


Comment/name could be improved: the fn seems to only find the target for breaks, but the name sounds generic, and the comment mentions both break and continue. Also, what is hir_id, is it the hir-id of the break? etc.

nikomatsakis · 2022-01-31T15:20:30Z

compiler/rustc_typeck/src/check/generator_interior/drop_ranges/cfg_build.rs

@@ -334,7 +373,8 @@ impl<'a, 'tcx> Visitor<'tcx> for DropRangeVisitor<'a, 'tcx> {
            }
            ExprKind::Break(hir::Destination { target_id: Ok(target), .. }, ..)
            | ExprKind::Continue(hir::Destination { target_id: Ok(target), .. }, ..) => {
-                self.drop_ranges.add_control_edge_hir_id(self.expr_index, target);
+                self.drop_ranges
+                    .add_control_edge_hir_id(self.expr_index, self.find_target_expression(target));


oh, I guess it is used for both break/continue-- that confuses me a bit. It would be good to give an example

'a: { ... break 'a } 'a: loop { ... continue 'a }

It's probably confusing because it's wrong. Here's an example that fails:

let mut arr = [Ptr]; let mut count = 0; drop(arr); while count < 3 { count += 1; yield; if b { arr = [Ptr]; continue; } }

I mistakenly assumed that Break and Continue worked like goto where the Destination field pointed at the expression where control flow transferred to. It actually points to the block being broken or continued. It mostly worked because of the way post-order numbering worked.

I just uploaded a new commit that handles continue correctly.

nikomatsakis · 2022-01-31T15:24:40Z

@eholk r=me with function comment improved :)

nikomatsakis · 2022-01-31T15:24:49Z

@rustbot ready

nikomatsakis · 2022-01-31T15:25:06Z

@rustbot author

eholk · 2022-01-31T22:14:25Z

Thanks for the feedback! It turns out I wasn't handling Continue correctly, but I've uploaded a new commit that fixes that. The comments should be better now too.

The crater report isn't finished, but it looks like there were 739 ICEs. I think we should wait to see the report before merging this. Hopefully they were all due to the issue with continue.

bors · 2022-02-01T17:02:23Z

☔ The latest upstream changes (presumably #93284) made this pull request unmergeable. Please resolve the merge conflicts.

Drop tracking in the generator interior type checking pass would count all values in unreachable code as dropped (e.g. code after a call to a function with an uninhabited return type), which would lead to those values not being included in the witness type. This resulted in the type checker being more precise than the corresponding sanity check in the MIR transform. This patch changes the check in the MIR code to match the results of typeck by skipping uninhabited types.

Issue rust-lang#93197

Mark-Simulacrum · 2022-02-02T00:19:27Z

@craterbot retry-report

craterbot · 2022-02-02T00:19:28Z

🛠️ Generation of the report for pr-93180 queued again.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

craterbot · 2022-02-02T04:32:54Z

🎉 Experiment pr-93180 is completed!
📊 2897 regressed and 13 fixed (208641 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

tmiasko · 2022-02-05T15:05:25Z

Two review comments in the form of bug reports #93674, #93648, I don't think those are fixed here yet.

eholk · 2022-02-08T00:38:04Z

As we talked about in the async triage meeting today, I don't think it makes sense to do this as a mega-PR that fixes all the ICEs we can find. Instead I'd like to break out the individual fixes and then do a PR that just re-enables drop tracking.

I propose instead of the patches from this one that deal with the Never type we use @tmiasko's #93313 instead.

For the break and continue handling, I've pulled these patches into #93752.

I'm going to start spot-testing these patches against different crates that failed the crater run and see if those look we can kick off another one that retries just the failed crates.

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

rust-highfive assigned nikomatsakis Jan 21, 2022

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

lqd mentioned this pull request Jan 22, 2022

[Internal Compiler Error] occurred when compiling eww with latest nightly build (2022-01-21) #93197

Closed

eholk force-pushed the drop-tracking-never-type branch from 197640a to e1bf8bc Compare January 24, 2022 19:27

nikomatsakis requested changes Jan 25, 2022

View reviewed changes

eholk changed the title ~~Allow uninhabited types in generator interiors~~ Fix drop tracking ICEs and re-enable generator drop tracking Jan 26, 2022

This comment has been minimized.

Sign in to view

craterbot added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 26, 2022

nikomatsakis approved these changes Jan 31, 2022

View reviewed changes

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 31, 2022

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

eholk mentioned this pull request Jan 31, 2022

Disable drop range analysis #93284

Merged

eholk added 7 commits February 1, 2022 14:44

Reduce regression test

cc9f16b

Support break and continue with block targets

06d5a96

Issue rust-lang#93197

Apply code review comments

0c7f1f9

Apply code review suggestion better

44ca6bc

Re-enable drop tracking

f462bca

Handle Continue correctly

e5b2270

eholk force-pushed the drop-tracking-never-type branch from 1109fa3 to e5b2270 Compare February 1, 2022 22:56

craterbot removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Feb 2, 2022

craterbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Feb 2, 2022

eholk closed this Feb 8, 2022

eholk mentioned this pull request Mar 8, 2022

[Drop Tracking] Allow uninhabited types in generator interiors #94752

Closed

	let is_uninhabited = tcx.is_ty_uninhabited_from(
	let is_inhabited = !tcx.is_ty_uninhabited_from(

	if !is_uninhabited && !allowed.contains(&decl_ty) && !allowed_upvars.contains(&decl_ty) {
	if is_inhabited && !allowed.contains(&decl_ty) && !allowed_upvars.contains(&decl_ty) {

Fix drop tracking ICEs and re-enable generator drop tracking #93180

Fix drop tracking ICEs and re-enable generator drop tracking #93180

Conversation

eholk commented Jan 21, 2022 • edited Loading

Allow uninhabited types in generator interiors

Support block-target break and continue expressions

Re-enable drop tracking

eholk commented Jan 21, 2022

lqd commented Jan 22, 2022 • edited Loading

bors commented Jan 22, 2022

bors commented Jan 22, 2022

lqd commented Jan 22, 2022

eholk commented Jan 23, 2022

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

eholk commented Jan 26, 2022

pnkfelix commented Jan 26, 2022

wesleywiser commented Jan 26, 2022

bors commented Jan 26, 2022

eholk commented Jan 26, 2022

bors commented Jan 26, 2022

This comment has been minimized.

This comment has been minimized.

wesleywiser commented Jan 26, 2022

craterbot commented Jan 26, 2022

craterbot commented Jan 28, 2022

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

nikomatsakis commented Jan 31, 2022

nikomatsakis commented Jan 31, 2022

nikomatsakis commented Jan 31, 2022

eholk commented Jan 31, 2022

bors commented Feb 1, 2022

Mark-Simulacrum commented Feb 2, 2022

craterbot commented Feb 2, 2022

craterbot commented Feb 2, 2022

tmiasko commented Feb 5, 2022

eholk commented Feb 8, 2022

eholk commented Jan 21, 2022 •

edited

Loading

Support block-target `break` and `continue` expressions

lqd commented Jan 22, 2022 •

edited

Loading