Fix StorageDead and ForLint drops in tail calls and unwind paths

This commit fixes several issues related to StorageDead and ForLint drops:

1. Add StorageDead and ForLint drops to unwind_drops for all functions
   - Updated diverge_cleanup_target to include StorageDead and ForLint drops
     in the unwind_drops tree for all functions (not just coroutines), but only
     when there's a cleanup path (i.e., when there are Value or ForLint drops)
   - This ensures proper drop ordering for borrow-checking on panic paths

2. Fix break_for_tail_call to handle StorageDead and ForLint drops
   - Don't skip StorageDead drops for non-drop types
   - Adjust unwind_to pointer for StorageDead and ForLint drops, matching
     the behavior in build_scope_drops
   - Only adjust unwind_to when it's valid (not DropIdx::MAX)
   - This prevents debug assert failures when processing drops in tail calls

3. Fix index out of bounds panic when unwind_to is DropIdx::MAX
   - Added checks to ensure unwind_to != DropIdx::MAX before accessing
     unwind_drops.drop_nodes[unwind_to]
   - Only emit StorageDead on unwind paths when there's actually an unwind path
   - Only add entry points to unwind_drops when unwind_to is valid
   - This prevents panics when there's no cleanup needed

4. Add test for explicit tail calls with StorageDead drops
   - Tests that tail calls work correctly when StorageDead and ForLint drops
     are present in the unwind path
   - Verifies that unwind_to is correctly adjusted for all drop kinds

These changes make the borrow-checker stricter and more consistent by ensuring
that StorageDead statements are emitted on unwind paths for all functions when
there's a cleanup path, allowing unsafe code to rely on drop order being enforced
consistently.

Author: sladyn98

compiler/rustc_mir_build/src/builder/scope.rs

@@ -256,16 +256,8 @@ struct DropNodeKey {
 
 impl Scope {
     /// Whether there's anything to do for the cleanup path, that is,
-    /// when unwinding through this scope. This includes destructors,
-    /// but not StorageDead statements, which don't get emitted at all
-    /// for unwinding, for several reasons:
-    ///  * clang doesn't emit llvm.lifetime.end for C++ unwinding
-    ///  * LLVM's memory dependency analysis can't handle it atm
-    ///  * polluting the cleanup MIR with StorageDead creates
-    ///    landing pads even though there's no actual destructors
-    ///  * freeing up stack space has no effect during unwinding
-    /// Note that for coroutines we do emit StorageDeads, for the
-    /// use of optimizations in the MIR coroutine transform.
+    /// when unwinding through this scope. This includes destructors
+    /// and StorageDead statements to maintain proper drop ordering.
     fn needs_cleanup(&self) -> bool {
         self.drops.iter().any(|drop| match drop.kind {
             DropKind::Value | DropKind::ForLint => true,
@@ -1109,6 +1101,9 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
         );
         let typing_env = self.typing_env();
         let unwind_drops = &mut self.scopes.unwind_drops;
+        let has_storage_drops = self.scopes.scopes[1..]
+            .iter()
+            .any(|scope| scope.drops.iter().any(|d| d.kind == DropKind::Storage));
 
         // the innermost scope contains only the destructors for the tail call arguments
         // we only want to drop these in case of a panic, so we skip it
@@ -1118,7 +1113,11 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
                 let source_info = drop_data.source_info;
                 let local = drop_data.local;
 
-                if !self.local_decls[local].ty.needs_drop(self.tcx, typing_env) {
+                // Skip Value drops for types that don't need drop, but process
+                // StorageDead and ForLint drops for all locals
+                if drop_data.kind == DropKind::Value
+                    && !self.local_decls[local].ty.needs_drop(self.tcx, typing_env)
+                {
                     continue;
                 }
 
@@ -1127,15 +1126,17 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
                         // `unwind_to` should drop the value that we're about to
                         // schedule. If dropping this value panics, then we continue
                         // with the *next* value on the unwind path.
-                        debug_assert_eq!(
-                            unwind_drops.drop_nodes[unwind_to].data.local,
-                            drop_data.local
-                        );
-                        debug_assert_eq!(
-                            unwind_drops.drop_nodes[unwind_to].data.kind,
-                            drop_data.kind
-                        );
-                        unwind_to = unwind_drops.drop_nodes[unwind_to].next;
+                        if unwind_to != DropIdx::MAX {
+                            debug_assert_eq!(
+                                unwind_drops.drop_nodes[unwind_to].data.local,
+                                drop_data.local
+                            );
+                            debug_assert_eq!(
+                                unwind_drops.drop_nodes[unwind_to].data.kind,
+                                drop_data.kind
+                            );
+                            unwind_to = unwind_drops.drop_nodes[unwind_to].next;
+                        }
 
                         let mut unwind_entry_point = unwind_to;
 
@@ -1162,6 +1163,19 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
                         block = next;
                     }
                     DropKind::ForLint => {
+                        // If this ForLint drop is in unwind_drops, we need to adjust
+                        // unwind_to to match, just like in build_scope_drops
+                        if has_storage_drops && unwind_to != DropIdx::MAX {
+                            debug_assert_eq!(
+                                unwind_drops.drop_nodes[unwind_to].data.local,
+                                drop_data.local
+                            );
+                            debug_assert_eq!(
+                                unwind_drops.drop_nodes[unwind_to].data.kind,
+                                drop_data.kind
+                            );
+                            unwind_to = unwind_drops.drop_nodes[unwind_to].next;
+                        }
                         self.cfg.push(
                             block,
                             Statement::new(
@@ -1174,6 +1188,19 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
                         );
                     }
                     DropKind::Storage => {
+                        // If this StorageDead drop is in unwind_drops, we need to adjust
+                        // unwind_to to match, just like in build_scope_drops
+                        if has_storage_drops && unwind_to != DropIdx::MAX {
+                            debug_assert_eq!(
+                                unwind_drops.drop_nodes[unwind_to].data.local,
+                                drop_data.local
+                            );
+                            debug_assert_eq!(
+                                unwind_drops.drop_nodes[unwind_to].data.kind,
+                                drop_data.kind
+                            );
+                            unwind_to = unwind_drops.drop_nodes[unwind_to].next;
+                        }
                         // Only temps and vars need their storage dead.
                         assert!(local.index() > self.arg_count);
                         self.cfg.push(
@@ -1217,6 +1244,9 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
         let dropline_to = if has_async_drops { Some(self.diverge_dropline()) } else { None };
         let scope = self.scopes.scopes.last().expect("leave_top_scope called with no scopes");
         let typing_env = self.typing_env();
+        let has_storage_drops = scope.drops.iter().any(|d| d.kind == DropKind::Storage);
+        // Only emit StorageDead on unwind paths when there's actually an unwind path
+        let storage_dead_on_unwind = has_storage_drops && unwind_to != DropIdx::MAX;
         build_scope_drops(
             &mut self.cfg,
             &mut self.scopes.unwind_drops,
@@ -1225,7 +1255,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
             block,
             unwind_to,
             dropline_to,
-            is_coroutine && needs_cleanup,
+            storage_dead_on_unwind,
             self.arg_count,
             |v: Local| Self::is_async_drop_impl(self.tcx, &self.local_decls, typing_env, v),
         )
@@ -1609,9 +1639,18 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
         }
 
         let is_coroutine = self.coroutine.is_some();
+        // Check if there's a cleanup path (i.e., Value or ForLint drops that require cleanup)
+        let has_cleanup_path = self.scopes.scopes[uncached_scope..=target]
+            .iter()
+            .any(|scope| scope.drops.iter().any(|d| d.kind == DropKind::Value || d.kind == DropKind::ForLint));
         for scope in &mut self.scopes.scopes[uncached_scope..=target] {
             for drop in &scope.drops {
-                if is_coroutine || drop.kind == DropKind::Value {
+                // Add all drops to unwind_drops for all functions (not just coroutines)
+                // to maintain proper drop ordering for borrow-checking. This matches
+                // the behavior in build_exit_tree.
+                // For coroutines, we always add all drops. For other functions, we now
+                // also add StorageDead and ForLint drops, but only when there's a cleanup path.
+                if is_coroutine || drop.kind == DropKind::Value || drop.kind == DropKind::ForLint || (drop.kind == DropKind::Storage && has_cleanup_path) {
                     cached_drop = self.scopes.unwind_drops.add_drop(*drop, cached_drop);
                 }
             }
@@ -1793,11 +1832,11 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
 /// * `scope`, describes the drops that will occur on exiting the scope in regular execution
 /// * `block`, the block to branch to once drops are complete (assuming no unwind occurs)
 /// * `unwind_to`, describes the drops that would occur at this point in the code if a
-///   panic occurred (a subset of the drops in `scope`, since we sometimes elide StorageDead and other
-///   instructions on unwinding)
+///   panic occurred (a subset of the drops in `scope`)
 /// * `dropline_to`, describes the drops that would occur at this point in the code if a
 ///    coroutine drop occurred.
-/// * `storage_dead_on_unwind`, if true, then we should emit `StorageDead` even when unwinding
+/// * `storage_dead_on_unwind`, if true, then we emit `StorageDead` on the unwind path
+///   and adjust `unwind_to` accordingly (used for all functions with StorageDead drops)
 /// * `arg_count`, number of MIR local variables corresponding to fn arguments (used to assert that we don't drop those)
 fn build_scope_drops<'tcx, F>(
     cfg: &mut CFG<'tcx>,
@@ -1830,10 +1869,10 @@ where
     // drops panic (panicking while unwinding will abort, so there's no need for
     // another set of arrows).
     //
-    // For coroutines, we unwind from a drop on a local to its StorageDead
-    // statement. For other functions we don't worry about StorageDead. The
-    // drops for the unwind path should have already been generated by
-    // `diverge_cleanup_gen`.
+    // We unwind from a drop on a local to its StorageDead statement for all
+    // functions (not just coroutines) to maintain proper drop ordering for
+    // borrow-checking. The drops for the unwind path should have already been
+    // generated by `diverge_cleanup_gen`.
 
     // `unwind_to` indicates what needs to be dropped should unwinding occur.
     // This is a subset of what needs to be dropped when exiting the scope.
@@ -1862,9 +1901,11 @@ where
                 //
                 // We adjust this BEFORE we create the drop (e.g., `drops[n]`)
                 // because `drops[n]` should unwind to `drops[n-1]`.
-                debug_assert_eq!(unwind_drops.drop_nodes[unwind_to].data.local, drop_data.local);
-                debug_assert_eq!(unwind_drops.drop_nodes[unwind_to].data.kind, drop_data.kind);
-                unwind_to = unwind_drops.drop_nodes[unwind_to].next;
+                if unwind_to != DropIdx::MAX {
+                    debug_assert_eq!(unwind_drops.drop_nodes[unwind_to].data.local, drop_data.local);
+                    debug_assert_eq!(unwind_drops.drop_nodes[unwind_to].data.kind, drop_data.kind);
+                    unwind_to = unwind_drops.drop_nodes[unwind_to].next;
+                }
 
                 if let Some(idx) = dropline_to {
                     debug_assert_eq!(coroutine_drops.drop_nodes[idx].data.local, drop_data.local);
@@ -1880,7 +1921,9 @@ where
                     continue;
                 }
 
-                unwind_drops.add_entry_point(block, unwind_to);
+                if unwind_to != DropIdx::MAX {
+                    unwind_drops.add_entry_point(block, unwind_to);
+                }
                 if let Some(to) = dropline_to
                     && is_async_drop(local)
                 {
@@ -1904,11 +1947,9 @@ where
             }
             DropKind::ForLint => {
                 // As in the `DropKind::Storage` case below:
-                // normally lint-related drops are not emitted for unwind,
-                // so we can just leave `unwind_to` unmodified, but in some
-                // cases we emit things ALSO on the unwind path, so we need to adjust
-                // `unwind_to` in that case.
-                if storage_dead_on_unwind {
+                // we emit lint-related drops on the unwind path when `storage_dead_on_unwind`
+                // is true, so we need to adjust `unwind_to` in that case.
+                if storage_dead_on_unwind && unwind_to != DropIdx::MAX {
                     debug_assert_eq!(
                         unwind_drops.drop_nodes[unwind_to].data.local,
                         drop_data.local
@@ -1937,12 +1978,11 @@ where
                 );
             }
             DropKind::Storage => {
-                // Ordinarily, storage-dead nodes are not emitted on unwind, so we don't
-                // need to adjust `unwind_to` on this path. However, in some specific cases
-                // we *do* emit storage-dead nodes on the unwind path, and in that case now that
-                // the storage-dead has completed, we need to adjust the `unwind_to` pointer
-                // so that any future drops we emit will not register storage-dead.
-                if storage_dead_on_unwind {
+                // We emit storage-dead nodes on the unwind path for borrow-checking
+                // purposes. When `storage_dead_on_unwind` is true, we need to adjust
+                // the `unwind_to` pointer now that the storage-dead has completed, so
+                // that any future drops we emit will not register storage-dead.
+                if storage_dead_on_unwind && unwind_to != DropIdx::MAX {
                     debug_assert_eq!(
                         unwind_drops.drop_nodes[unwind_to].data.local,
                         drop_data.local
@@ -1986,15 +2026,11 @@ impl<'a, 'tcx: 'a> Builder<'a, 'tcx> {
             for (drop_idx, drop_node) in drops.drop_nodes.iter_enumerated().skip(1) {
                 match drop_node.data.kind {
                     DropKind::Storage | DropKind::ForLint => {
-                        if is_coroutine {
-                            let unwind_drop = self
-                                .scopes
-                                .unwind_drops
-                                .add_drop(drop_node.data, unwind_indices[drop_node.next]);
-                            unwind_indices.push(unwind_drop);
-                        } else {
-                            unwind_indices.push(unwind_indices[drop_node.next]);
-                        }
+                        let unwind_drop = self
+                            .scopes
+                            .unwind_drops
+                            .add_drop(drop_node.data, unwind_indices[drop_node.next]);
+                        unwind_indices.push(unwind_drop);
                     }
                     DropKind::Value => {
                         let unwind_drop = self

tests/ui/explicit-tail-calls/tail-call-storage-dead-unwind.rs

@@ -0,0 +1,69 @@
+//@ run-pass
+//@ ignore-backends: gcc
+#![expect(incomplete_features)]
+#![feature(explicit_tail_calls)]
+
+// Test that explicit tail calls work correctly when there are StorageDead
+// and ForLint drops in the unwind path. This ensures that `unwind_to` is
+// correctly adjusted in `break_for_tail_call` when encountering StorageDead
+// and ForLint drops, matching the behavior in `build_scope_drops`.
+
+#[allow(dead_code)]
+struct Droppable(i32);
+
+impl Drop for Droppable {
+    fn drop(&mut self) {}
+}
+
+fn tail_call_with_storage_dead() {
+    // These will have StorageDead drops (non-drop types)
+    let _a = 42i32;
+    let _b = true;
+    let _c = 10u8;
+    
+    // This will have a Value drop (drop type)
+    let _d = Droppable(1);
+    
+    // Tail call - if unwind_to isn't adjusted for StorageDead drops,
+    // the debug assert will fail when processing the Value drop
+    become next_function();
+}
+
+fn tail_call_with_mixed_drops() {
+    // StorageDead drop
+    let _storage = 100i32;
+    
+    // Value drop
+    let _value = Droppable(2);
+    
+    // Another StorageDead drop
+    let _storage2 = 200i32;
+    
+    // Another Value drop
+    let _value2 = Droppable(3);
+    
+    // Tail call - tests that unwind_to is adjusted correctly
+    // for both StorageDead and Value drops in sequence
+    become next_function();
+}
+
+fn tail_call_with_storage_before_value() {
+    // Multiple StorageDead drops before a Value drop
+    let _s1 = 1i32;
+    let _s2 = 2i32;
+    let _s3 = 3i32;
+    
+    // Value drop - if StorageDead drops aren't handled,
+    // unwind_to will point to wrong node and assert fails
+    let _v = Droppable(4);
+    
+    become next_function();
+}
+
+fn next_function() {}
+
+fn main() {
+    tail_call_with_storage_dead();
+    tail_call_with_mixed_drops();
+    tail_call_with_storage_before_value();
+}