Make sure we handle `backwards_incompatible_lint` drops appropriately in drop elaboration #134486

compiler-errors · 2024-12-18T22:03:05Z

In #131326, a new kind of scheduled drop (drop_kind: DropKind::Value + backwards_incompatible_lint: true) was added so that we could insert a new kind of no-op MIR statement (backward incompatible drop) for linting purposes.

These drops were intended to have no side-effects, but drop elaboration code forgot to handle these drops specially and they were handled otherwise as normal drops in most of the code. This ends up being unsound since we insert more than one drop call for some values, which means that Drop::drop could be called more than once.

This PR fixes this by splitting out the DropKind::ForLint and adjusting the code. I'm not totally certain if all of the places I've adjusted are either reachable or correct, but I'm pretty certain that it's more correct than it was previously.

cc @dingxiangfei2009
r? nikomatsakis

Fixes #134482

compiler-errors · 2024-12-18T22:04:43Z

compiler/rustc_mir_build/src/builder/scope.rs

@@ -411,6 +405,27 @@ impl DropTree {
                    };
                    cfg.terminate(block, drop_node.data.source_info, terminator);
                }
+                DropKind::ForLint => {


I ended up adjusting this to act like DropKind::Storage, but inserting a BackwardIncompatibleDropHint instead of a StorageDead. Previously we'd insert a drop terminator here.

Makes sense.

compiler-errors · 2024-12-18T22:05:24Z

compiler/rustc_mir_build/src/builder/scope.rs

@@ -822,6 +832,15 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
                        });
                        block = next;
                    }
+                    DropKind::ForLint => {


Previously we were inserting a drop terminator for tail calls. Not certain if this is ever reachable, but it's more correct.

compiler-errors · 2024-12-18T22:05:59Z

compiler/rustc_mir_build/src/builder/scope.rs

+                    continue;
+                }
+
+                if storage_dead_on_unwind {


I'm not certain if we need to handle this unwind block here.

compiler-errors · 2024-12-18T22:29:23Z

tests/mir-opt/tail_expr_drop_order_unwind.method_1.ElaborateDrops.after.panic-unwind.mir

-    }
-
-    bb29 (cleanup): {
-        drop(_5) -> [return: bb16, unwind terminate(cleanup)];


notably, this is the drop terminator that is bad. previously we were unwinding to bb14, which would drop _5, then which branched to bb15 -> bb30 which tested a drop flag for _5 (which was never set to false), which branched to bb29 where we would drop _5 yet again.

ehuss · 2024-12-19T00:51:26Z

@nikomatsakis I just want to make sure that you are aware of my comment in #132861 (comment). This approach of using MIR to implement the lint means that it does not work with cargo fix --edition, because cargo check does not run far enough to reach this. The only way for a user to see this is if they manually enable the lint and run a full build. I'm concerned that almost nobody will do that, making this ineffective.

I also want to make sure you are aware that there are significant false negatives with this lint (such as the basic example in #132861 (comment)), even when doing a full build. That pattern unfortunately happens quite often due to the introduction of unsafe_op_in_unsafe_fn which adds a lot of inline unsafe {…} expressions.

nikomatsakis · 2024-12-19T02:28:07Z

Hmm! I'll check into that tomorrow morning. Also, doh, the dropck code was indeed an oversight on my part. I'm amazed that didn't trigger test failures! But I guess the drops are rarely inserted.

lqd · 2024-12-19T05:56:11Z

(#131326 is on beta, nominating for backport)

nikomatsakis

r+

wesleywiser · 2024-12-19T15:30:37Z

@bors p=1

Fixes P-critical unsoundness in beta

jieyouxu · 2024-12-19T15:35:13Z

(btw @nikomatsakis, bors doesn't pick up r+ from review comments, only in regular comments)

jieyouxu · 2024-12-19T15:36:56Z

@bors p=10 (scheduling before a rollup)

jieyouxu · 2024-12-19T15:37:15Z

@bors r=@nikomatsakis

bors · 2024-12-19T15:37:18Z

📌 Commit 6564403 has been approved by nikomatsakis

It is now in the queue for this repository.

bors · 2024-12-19T15:37:19Z

🌲 The tree is currently closed for pull requests below priority 100. This pull request will be tested once the tree is reopened.

nikomatsakis · 2024-12-19T15:38:28Z

@jieyouxu thanks!

(re: bors, I was intentionally waiting to see that CI was good first...appreciate you forwarding the r= though!)

apiraino · 2024-12-19T15:43:03Z

beta backport approved on Zulip

@rustbot label +beta-accepted

matthiaskrgr · 2024-12-19T15:43:06Z

Fixes P-critical unsoundness in beta

🤔 this pr is against the master branch

compiler-errors · 2024-12-19T15:44:05Z

@matthiaskrgr: It will get backported.

Clipi-12 · 2024-12-19T15:44:15Z

tests/mir-opt/tail_expr_drop_order_unwind.rs

+
+#![deny(tail_expr_drop_order)]
+
+use std::backtrace::Backtrace;


This was probably unintentionally copy-pasted from my mcve

lol ya, doesn't really matter bc i'm not gonna fix it now, and doesn't affect the test

bors · 2024-12-19T15:58:11Z

⌛ Testing commit 6564403 with merge 11663cd...

bors · 2024-12-19T18:54:07Z

☀️ Test successful - checks-actions
Approved by: nikomatsakis
Pushing 11663cd to master...

rust-timer · 2024-12-19T20:15:46Z

Finished benchmarking commit (11663cd): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results (primary 0.5%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	1.9%	[0.8%, 3.0%]	2
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-2.2%	[-2.2%, -2.2%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.5%	[-2.2%, 3.0%]	3

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 767.394s -> 769.451s (0.27%)
Artifact size: 330.38 MiB -> 330.39 MiB (0.00%)

jieyouxu · 2024-12-20T17:12:59Z

I'm not 100% certain, but the changes to the drop handling might be able to trigger an ICE related to

error: internal compiler error: compiler/rustc_mir_build/src/builder/scope.rs:1724:17: cannot unwind from goto -> bb198

cf. #134541.

Clipi-12 · 2024-12-20T17:43:54Z

I'm not at home atm but earlier I tried to use nightly-2024-12-20 in the rust-toolchain file of the project from which I created the MCVE and it indeed ICEd with a very similar message

(again, not at home right now so I cannot confirm if it has to do something with this patch)

[beta] backports - Do not call `extern_crate` on current trait on crate mismatch errors rust-lang#133585 - Correctly handle comments in attributes in doctests source code rust-lang#134260 - Correctly document CTFE behavior of is_null and methods that call is_null. rust-lang#134325 - Make sure we handle `backwards_incompatible_lint` drops appropriately in drop elaboration rust-lang#134486 - Bump compiler `cc` to 1.2.5 rust-lang#134505 - Handle `DropKind::ForLint` in coroutines correctly rust-lang#134575 - docs: inline `std::ffi::c_str` types to `std::ffi` rust-lang#134791 - docs: inline `alloc::ffi::c_str` types to `alloc::ffi` rust-lang#134851 r? cuviper

compiler-errors added 2 commits December 18, 2024 21:57

Add a failing test

2e57394

Separate DropKind::ForLint

5e07901

rustbot assigned nikomatsakis Dec 18, 2024

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Dec 18, 2024

compiler-errors commented Dec 18, 2024

View reviewed changes

lqd added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Dec 19, 2024

explain how build_scope_drops works

b535061

nikomatsakis approved these changes Dec 19, 2024

View reviewed changes

This comment has been minimized.

Sign in to view

pacify merciless fmt

6564403

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 Dec 19, 2024

rustbot added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Dec 19, 2024

Clipi-12 reviewed Dec 19, 2024

View reviewed changes

bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 19, 2024

bors merged commit 11663cd into rust-lang:master Dec 19, 2024
7 checks passed

rustbot added this to the 1.85.0 milestone Dec 19, 2024

compiler-errors deleted the drop-for-lint branch December 19, 2024 20:19

traviscross added the L-tail_expr_drop_order Lint: tail_expr_drop_order label Dec 19, 2024

jieyouxu mentioned this pull request Dec 20, 2024

Internal Compiler Error in rustc with axum-client-ip-0.6.1 #134541

Closed

compiler-errors mentioned this pull request Dec 22, 2024

Handle DropKind::ForLint in coroutines correctly #134575

Merged

cuviper mentioned this pull request Jan 2, 2025

[beta] backports #135048

Merged

cuviper modified the milestones: 1.85.0, 1.84.0 Jan 2, 2025

cuviper removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Jan 2, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make sure we handle `backwards_incompatible_lint` drops appropriately in drop elaboration #134486

Make sure we handle `backwards_incompatible_lint` drops appropriately in drop elaboration #134486

compiler-errors commented Dec 18, 2024

compiler-errors Dec 18, 2024

nikomatsakis Dec 19, 2024

compiler-errors Dec 18, 2024

compiler-errors Dec 18, 2024

compiler-errors Dec 18, 2024

ehuss commented Dec 19, 2024

nikomatsakis commented Dec 19, 2024

lqd commented Dec 19, 2024

nikomatsakis left a comment

This comment has been minimized.

wesleywiser commented Dec 19, 2024

jieyouxu commented Dec 19, 2024

jieyouxu commented Dec 19, 2024

jieyouxu commented Dec 19, 2024

bors commented Dec 19, 2024

bors commented Dec 19, 2024

nikomatsakis commented Dec 19, 2024 •

edited

Loading

apiraino commented Dec 19, 2024

matthiaskrgr commented Dec 19, 2024 •

edited

Loading

compiler-errors commented Dec 19, 2024

Clipi-12 Dec 19, 2024

compiler-errors Dec 19, 2024

bors commented Dec 19, 2024

bors commented Dec 19, 2024

rust-timer commented Dec 19, 2024

jieyouxu commented Dec 20, 2024 •

edited

Loading

Clipi-12 commented Dec 20, 2024 •

edited

Loading


		#![deny(tail_expr_drop_order)]

		use std::backtrace::Backtrace;

Make sure we handle backwards_incompatible_lint drops appropriately in drop elaboration #134486

Make sure we handle backwards_incompatible_lint drops appropriately in drop elaboration #134486

Conversation

compiler-errors commented Dec 18, 2024

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

ehuss commented Dec 19, 2024

nikomatsakis commented Dec 19, 2024

lqd commented Dec 19, 2024

nikomatsakis left a comment

Choose a reason for hiding this comment

This comment has been minimized.

wesleywiser commented Dec 19, 2024

jieyouxu commented Dec 19, 2024

jieyouxu commented Dec 19, 2024

jieyouxu commented Dec 19, 2024

bors commented Dec 19, 2024

bors commented Dec 19, 2024

nikomatsakis commented Dec 19, 2024 • edited Loading

apiraino commented Dec 19, 2024

matthiaskrgr commented Dec 19, 2024 • edited Loading

compiler-errors commented Dec 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Dec 19, 2024

bors commented Dec 19, 2024

rust-timer commented Dec 19, 2024

Overall result: no relevant changes - no action needed

jieyouxu commented Dec 20, 2024 • edited Loading

Clipi-12 commented Dec 20, 2024 • edited Loading

Make sure we handle `backwards_incompatible_lint` drops appropriately in drop elaboration #134486

Make sure we handle `backwards_incompatible_lint` drops appropriately in drop elaboration #134486

nikomatsakis commented Dec 19, 2024 •

edited

Loading

matthiaskrgr commented Dec 19, 2024 •

edited

Loading

jieyouxu commented Dec 20, 2024 •

edited

Loading

Clipi-12 commented Dec 20, 2024 •

edited

Loading