Auto merge of #123645 - matthiaskrgr:rollup-yd8d7f1, r=matthiaskrgr

Rollup of 9 pull requests

Successful merges:

 - #122781 (Fix argument ABI for overaligned structs on ppc64le)
 - #123367 (Safe Transmute: Compute transmutability from `rustc_target::abi::Layout`)
 - #123518 (Fix `ByMove` coroutine-closure shim (for 2021 precise closure capturing behavior))
 - #123547 (bootstrap: remove unused pub fns)
 - #123564 (Don't emit divide-by-zero panic paths in `StepBy::len`)
 - #123578 (Restore `pred_known_to_hold_modulo_regions`)
 - #123591 (Remove unnecessary cast from `LLVMRustGetInstrProfIncrementIntrinsic`)
 - #123632 (parser: reduce visibility of unnecessary public `UnmatchedDelim`)
 - #123635 (CFI: Fix ICE in KCFI non-associated function pointers)

r? `@ghost`
`@rustbot` modify labels: rollup

Author: bors

compiler/rustc_codegen_llvm/src/abi.rs

@@ -150,7 +150,10 @@ impl LlvmType for CastTarget {
         // Simplify to a single unit or an array if there's no prefix.
         // This produces the same layout, but using a simpler type.
         if self.prefix.iter().all(|x| x.is_none()) {
-            if rest_count == 1 {
+            // We can't do this if is_consecutive is set and the unit would get
+            // split on the target. Currently, this is only relevant for i128
+            // registers.
+            if rest_count == 1 && (!self.rest.is_consecutive || self.rest.unit != Reg::i128()) {
                 return rest_ll_unit;
             }
 

compiler/rustc_llvm/llvm-wrapper/RustWrapper.cpp

@@ -1524,8 +1524,8 @@ extern "C" LLVMValueRef LLVMRustBuildCall(LLVMBuilderRef B, LLVMTypeRef Ty, LLVM
 }
 
 extern "C" LLVMValueRef LLVMRustGetInstrProfIncrementIntrinsic(LLVMModuleRef M) {
-  return wrap(llvm::Intrinsic::getDeclaration(unwrap(M),
-              (llvm::Intrinsic::ID)llvm::Intrinsic::instrprof_increment));
+  return wrap(llvm::Intrinsic::getDeclaration(
+      unwrap(M), llvm::Intrinsic::instrprof_increment));
 }
 
 extern "C" LLVMValueRef LLVMRustBuildMemCpy(LLVMBuilderRef B,

compiler/rustc_middle/src/ty/instance.rs

@@ -520,7 +520,10 @@ impl<'tcx> Instance<'tcx> {
                 // Reify `Trait::method` implementations if KCFI is enabled
                 // FIXME(maurer) only reify it if it is a vtable-safe function
                 _ if tcx.sess.is_sanitizer_kcfi_enabled()
-                    && tcx.associated_item(def_id).trait_item_def_id.is_some() =>
+                    && tcx
+                        .opt_associated_item(def_id)
+                        .and_then(|assoc| assoc.trait_item_def_id)
+                        .is_some() =>
                 {
                     // If this function could also go in a vtable, we need to `ReifyShim` it with
                     // KCFI because it can only attach one type per function.

compiler/rustc_mir_transform/src/coroutine/by_move_body.rs

@@ -58,16 +58,24 @@
 //! borrowing from the outer closure, and we simply peel off a `deref` projection
 //! from them. This second body is stored alongside the first body, and optimized
 //! with it in lockstep. When we need to resolve a body for `FnOnce` or `AsyncFnOnce`,
-//! we use this "by move" body instead.
-
-use itertools::Itertools;
+//! we use this "by-move" body instead.
+//!
+//! ## How does this work?
+//!
+//! This pass essentially remaps the body of the (child) closure of the coroutine-closure
+//! to take the set of upvars of the parent closure by value. This at least requires
+//! changing a by-ref upvar to be by-value in the case that the outer coroutine-closure
+//! captures something by value; however, it may also require renumbering field indices
+//! in case precise captures (edition 2021 closure capture rules) caused the inner coroutine
+//! to split one field capture into two.
 
-use rustc_data_structures::unord::UnordSet;
+use rustc_data_structures::unord::UnordMap;
 use rustc_hir as hir;
+use rustc_middle::hir::place::{PlaceBase, Projection, ProjectionKind};
 use rustc_middle::mir::visit::MutVisitor;
 use rustc_middle::mir::{self, dump_mir, MirPass};
 use rustc_middle::ty::{self, InstanceDef, Ty, TyCtxt, TypeVisitableExt};
-use rustc_target::abi::FieldIdx;
+use rustc_target::abi::{FieldIdx, VariantIdx};
 
 pub struct ByMoveBody;
 
@@ -116,32 +124,116 @@ impl<'tcx> MirPass<'tcx> for ByMoveBody {
             .tuple_fields()
             .len();
 
-        let mut by_ref_fields = UnordSet::default();
-        for (idx, (coroutine_capture, parent_capture)) in tcx
+        let mut field_remapping = UnordMap::default();
+
+        // One parent capture may correspond to several child captures if we end up
+        // refining the set of captures via edition-2021 precise captures. We want to
+        // match up any number of child captures with one parent capture, so we keep
+        // peeking off this `Peekable` until the child doesn't match anymore.
+        let mut parent_captures =
+            tcx.closure_captures(parent_def_id).iter().copied().enumerate().peekable();
+        // Make sure we use every field at least once, b/c why are we capturing something
+        // if it's not used in the inner coroutine.
+        let mut field_used_at_least_once = false;
+
+        for (child_field_idx, child_capture) in tcx
             .closure_captures(coroutine_def_id)
             .iter()
+            .copied()
             // By construction we capture all the args first.
             .skip(num_args)
-            .zip_eq(tcx.closure_captures(parent_def_id))
             .enumerate()
         {
-            // This upvar is captured by-move from the parent closure, but by-ref
-            // from the inner async block. That means that it's being borrowed from
-            // the outer closure body -- we need to change the coroutine to take the
-            // upvar by value.
-            if coroutine_capture.is_by_ref() && !parent_capture.is_by_ref() {
-                assert_ne!(
-                    coroutine_kind,
-                    ty::ClosureKind::FnOnce,
-                    "`FnOnce` coroutine-closures return coroutines that capture from \
-                    their body; it will always result in a borrowck error!"
+            loop {
+                let Some(&(parent_field_idx, parent_capture)) = parent_captures.peek() else {
+                    bug!("we ran out of parent captures!")
+                };
+
+                let PlaceBase::Upvar(parent_base) = parent_capture.place.base else {
+                    bug!("expected capture to be an upvar");
+                };
+                let PlaceBase::Upvar(child_base) = child_capture.place.base else {
+                    bug!("expected capture to be an upvar");
+                };
+
+                assert!(
+                    child_capture.place.projections.len() >= parent_capture.place.projections.len()
                 );
-                by_ref_fields.insert(FieldIdx::from_usize(num_args + idx));
+                // A parent matches a child they share the same prefix of projections.
+                // The child may have more, if it is capturing sub-fields out of
+                // something that is captured by-move in the parent closure.
+                if parent_base.var_path.hir_id != child_base.var_path.hir_id
+                    || !std::iter::zip(
+                        &child_capture.place.projections,
+                        &parent_capture.place.projections,
+                    )
+                    .all(|(child, parent)| child.kind == parent.kind)
+                {
+                    // Make sure the field was used at least once.
+                    assert!(
+                        field_used_at_least_once,
+                        "we captured {parent_capture:#?} but it was not used in the child coroutine?"
+                    );
+                    field_used_at_least_once = false;
+                    // Skip this field.
+                    let _ = parent_captures.next().unwrap();
+                    continue;
+                }
+
+                // Store this set of additional projections (fields and derefs).
+                // We need to re-apply them later.
+                let child_precise_captures =
+                    &child_capture.place.projections[parent_capture.place.projections.len()..];
+
+                // If the parent captures by-move, and the child captures by-ref, then we
+                // need to peel an additional `deref` off of the body of the child.
+                let needs_deref = child_capture.is_by_ref() && !parent_capture.is_by_ref();
+                if needs_deref {
+                    assert_ne!(
+                        coroutine_kind,
+                        ty::ClosureKind::FnOnce,
+                        "`FnOnce` coroutine-closures return coroutines that capture from \
+                        their body; it will always result in a borrowck error!"
+                    );
+                }
+
+                // Finally, store the type of the parent's captured place. We need
+                // this when building the field projection in the MIR body later on.
+                let mut parent_capture_ty = parent_capture.place.ty();
+                parent_capture_ty = match parent_capture.info.capture_kind {
+                    ty::UpvarCapture::ByValue => parent_capture_ty,
+                    ty::UpvarCapture::ByRef(kind) => Ty::new_ref(
+                        tcx,
+                        tcx.lifetimes.re_erased,
+                        parent_capture_ty,
+                        kind.to_mutbl_lossy(),
+                    ),
+                };
+
+                field_remapping.insert(
+                    FieldIdx::from_usize(child_field_idx + num_args),
+                    (
+                        FieldIdx::from_usize(parent_field_idx + num_args),
+                        parent_capture_ty,
+                        needs_deref,
+                        child_precise_captures,
+                    ),
+                );
+
+                field_used_at_least_once = true;
+                break;
             }
+        }
+
+        // Pop the last parent capture
+        if field_used_at_least_once {
+            let _ = parent_captures.next().unwrap();
+        }
+        assert_eq!(parent_captures.next(), None, "leftover parent captures?");
 
-            // Make sure we're actually talking about the same capture.
-            // FIXME(async_closures): We could look at the `hir::Upvar` instead?
-            assert_eq!(coroutine_capture.place.ty(), parent_capture.place.ty());
+        if coroutine_kind == ty::ClosureKind::FnOnce {
+            assert_eq!(field_remapping.len(), tcx.closure_captures(parent_def_id).len());
+            return;
         }
 
         let by_move_coroutine_ty = tcx
@@ -157,7 +249,7 @@ impl<'tcx> MirPass<'tcx> for ByMoveBody {
             );
 
         let mut by_move_body = body.clone();
-        MakeByMoveBody { tcx, by_ref_fields, by_move_coroutine_ty }.visit_body(&mut by_move_body);
+        MakeByMoveBody { tcx, field_remapping, by_move_coroutine_ty }.visit_body(&mut by_move_body);
         dump_mir(tcx, false, "coroutine_by_move", &0, &by_move_body, |_, _| Ok(()));
         by_move_body.source = mir::MirSource::from_instance(InstanceDef::CoroutineKindShim {
             coroutine_def_id: coroutine_def_id.to_def_id(),
@@ -168,7 +260,7 @@ impl<'tcx> MirPass<'tcx> for ByMoveBody {
 
 struct MakeByMoveBody<'tcx> {
     tcx: TyCtxt<'tcx>,
-    by_ref_fields: UnordSet<FieldIdx>,
+    field_remapping: UnordMap<FieldIdx, (FieldIdx, Ty<'tcx>, bool, &'tcx [Projection<'tcx>])>,
     by_move_coroutine_ty: Ty<'tcx>,
 }
 
@@ -183,24 +275,59 @@ impl<'tcx> MutVisitor<'tcx> for MakeByMoveBody<'tcx> {
         context: mir::visit::PlaceContext,
         location: mir::Location,
     ) {
+        // Initializing an upvar local always starts with `CAPTURE_STRUCT_LOCAL` and a
+        // field projection. If this is in `field_remapping`, then it must not be an
+        // arg from calling the closure, but instead an upvar.
         if place.local == ty::CAPTURE_STRUCT_LOCAL
-            && let Some((&mir::ProjectionElem::Field(idx, ty), projection)) =
+            && let Some((&mir::ProjectionElem::Field(idx, _), projection)) =
                 place.projection.split_first()
-            && self.by_ref_fields.contains(&idx)
+            && let Some(&(remapped_idx, remapped_ty, needs_deref, additional_projections)) =
+                self.field_remapping.get(&idx)
         {
-            let (begin, end) = projection.split_first().unwrap();
-            // FIXME(async_closures): I'm actually a bit surprised to see that we always
-            // initially deref the by-ref upvars. If this is not actually true, then we
-            // will at least get an ICE that explains why this isn't true :^)
-            assert_eq!(*begin, mir::ProjectionElem::Deref);
-            // Peel one ref off of the ty.
-            let peeled_ty = ty.builtin_deref(true).unwrap().ty;
+            // As noted before, if the parent closure captures a field by value, and
+            // the child captures a field by ref, then for the by-move body we're
+            // generating, we also are taking that field by value. Peel off a deref,
+            // since a layer of reffing has now become redundant.
+            let final_deref = if needs_deref {
+                let Some((mir::ProjectionElem::Deref, projection)) = projection.split_first()
+                else {
+                    bug!(
+                        "There should be at least a single deref for an upvar local initialization, found {projection:#?}"
+                    );
+                };
+                // There may be more derefs, since we may also implicitly reborrow
+                // a captured mut pointer.
+                projection
+            } else {
+                projection
+            };
+
+            // The only thing that should be left is a deref, if the parent captured
+            // an upvar by-ref.
+            std::assert_matches::assert_matches!(final_deref, [] | [mir::ProjectionElem::Deref]);
+
+            // For all of the additional projections that come out of precise capturing,
+            // re-apply these projections.
+            let additional_projections =
+                additional_projections.iter().map(|elem| match elem.kind {
+                    ProjectionKind::Deref => mir::ProjectionElem::Deref,
+                    ProjectionKind::Field(idx, VariantIdx::ZERO) => {
+                        mir::ProjectionElem::Field(idx, elem.ty)
+                    }
+                    _ => unreachable!("precise captures only through fields and derefs"),
+                });
+
+            // We start out with an adjusted field index (and ty), representing the
+            // upvar that we get from our parent closure. We apply any of the additional
+            // projections to make sure that to the rest of the body of the closure, the
+            // place looks the same, and then apply that final deref if necessary.
             *place = mir::Place {
                 local: place.local,
                 projection: self.tcx.mk_place_elems_from_iter(
-                    [mir::ProjectionElem::Field(idx, peeled_ty)]
+                    [mir::ProjectionElem::Field(remapped_idx, remapped_ty)]
                         .into_iter()
-                        .chain(end.iter().copied()),
+                        .chain(additional_projections)
+                        .chain(final_deref.iter().copied()),
                 ),
             };
         }

compiler/rustc_parse/src/lexer/mod.rs

@@ -34,8 +34,7 @@ use unescape_error_reporting::{emit_unescape_error, escaped_char};
 rustc_data_structures::static_assert_size!(rustc_lexer::Token, 12);
 
 #[derive(Clone, Debug)]
-pub struct UnmatchedDelim {
-    pub expected_delim: Delimiter,
+pub(crate) struct UnmatchedDelim {
     pub found_delim: Option<Delimiter>,
     pub found_span: Span,
     pub unclosed_span: Option<Span>,

compiler/rustc_parse/src/lexer/tokentrees.rs

@@ -77,7 +77,6 @@ impl<'psess, 'src> TokenTreesReader<'psess, 'src> {
         for &(_, sp) in &self.diag_info.open_braces {
             err.span_label(sp, "unclosed delimiter");
             self.diag_info.unmatched_delims.push(UnmatchedDelim {
-                expected_delim: Delimiter::Brace,
                 found_delim: None,
                 found_span: self.token.span,
                 unclosed_span: Some(sp),
@@ -163,9 +162,8 @@ impl<'psess, 'src> TokenTreesReader<'psess, 'src> {
                             candidate = Some(*brace_span);
                         }
                     }
-                    let (tok, _) = self.diag_info.open_braces.pop().unwrap();
+                    let (_, _) = self.diag_info.open_braces.pop().unwrap();
                     self.diag_info.unmatched_delims.push(UnmatchedDelim {
-                        expected_delim: tok,
                         found_delim: Some(close_delim),
                         found_span: self.token.span,
                         unclosed_span: unclosed_delimiter,

compiler/rustc_target/src/abi/call/aarch64.rs

@@ -31,7 +31,7 @@ where
             RegKind::Vector => size.bits() == 64 || size.bits() == 128,
         };
 
-        valid_unit.then_some(Uniform { unit, total: size })
+        valid_unit.then_some(Uniform::consecutive(unit, size))
     })
 }
 
@@ -60,7 +60,7 @@ where
     let size = ret.layout.size;
     let bits = size.bits();
     if bits <= 128 {
-        ret.cast_to(Uniform { unit: Reg::i64(), total: size });
+        ret.cast_to(Uniform::new(Reg::i64(), size));
         return;
     }
     ret.make_indirect();
@@ -100,9 +100,9 @@ where
     };
     if size.bits() <= 128 {
         if align.bits() == 128 {
-            arg.cast_to(Uniform { unit: Reg::i128(), total: size });
+            arg.cast_to(Uniform::new(Reg::i128(), size));
         } else {
-            arg.cast_to(Uniform { unit: Reg::i64(), total: size });
+            arg.cast_to(Uniform::new(Reg::i64(), size));
         }
         return;
     }

compiler/rustc_target/src/abi/call/arm.rs

@@ -21,7 +21,7 @@ where
             RegKind::Vector => size.bits() == 64 || size.bits() == 128,
         };
 
-        valid_unit.then_some(Uniform { unit, total: size })
+        valid_unit.then_some(Uniform::consecutive(unit, size))
     })
 }
 
@@ -49,7 +49,7 @@ where
     let size = ret.layout.size;
     let bits = size.bits();
     if bits <= 32 {
-        ret.cast_to(Uniform { unit: Reg::i32(), total: size });
+        ret.cast_to(Uniform::new(Reg::i32(), size));
         return;
     }
     ret.make_indirect();
@@ -78,7 +78,7 @@ where
 
     let align = arg.layout.align.abi.bytes();
     let total = arg.layout.size;
-    arg.cast_to(Uniform { unit: if align <= 4 { Reg::i32() } else { Reg::i64() }, total });
+    arg.cast_to(Uniform::consecutive(if align <= 4 { Reg::i32() } else { Reg::i64() }, total));
 }
 
 pub fn compute_abi_info<'a, Ty, C>(cx: &C, fn_abi: &mut FnAbi<'a, Ty>)

compiler/rustc_target/src/abi/call/csky.rs

@@ -18,7 +18,7 @@ fn classify_ret<Ty>(arg: &mut ArgAbi<'_, Ty>) {
         if total.bits() > 64 {
             arg.make_indirect();
         } else if total.bits() > 32 {
-            arg.cast_to(Uniform { unit: Reg::i32(), total });
+            arg.cast_to(Uniform::new(Reg::i32(), total));
         } else {
             arg.cast_to(Reg::i32());
         }
@@ -38,7 +38,7 @@ fn classify_arg<Ty>(arg: &mut ArgAbi<'_, Ty>) {
     if arg.layout.is_aggregate() {
         let total = arg.layout.size;
         if total.bits() > 32 {
-            arg.cast_to(Uniform { unit: Reg::i32(), total });
+            arg.cast_to(Uniform::new(Reg::i32(), total));
         } else {
             arg.cast_to(Reg::i32());
         }