From 847fd88df724280880c705848ba1a120ce15e020 Mon Sep 17 00:00:00 2001 From: Michael Goulet Date: Sun, 24 Mar 2024 20:06:05 -0400 Subject: [PATCH 01/14] Always use tcx.coroutine_layout over calling optimized_mir directly --- .../src/debuginfo/metadata/enums/cpp_like.rs | 2 +- .../rustc_codegen_llvm/src/debuginfo/metadata/enums/native.rs | 3 +-- compiler/rustc_ty_utils/src/layout.rs | 2 +- 3 files changed, 3 insertions(+), 4 deletions(-) diff --git a/compiler/rustc_codegen_llvm/src/debuginfo/metadata/enums/cpp_like.rs b/compiler/rustc_codegen_llvm/src/debuginfo/metadata/enums/cpp_like.rs index 4792b0798dfb8..9a10e1bc1299c 100644 --- a/compiler/rustc_codegen_llvm/src/debuginfo/metadata/enums/cpp_like.rs +++ b/compiler/rustc_codegen_llvm/src/debuginfo/metadata/enums/cpp_like.rs @@ -683,7 +683,7 @@ fn build_union_fields_for_direct_tag_coroutine<'ll, 'tcx>( _ => unreachable!(), }; - let coroutine_layout = cx.tcx.optimized_mir(coroutine_def_id).coroutine_layout().unwrap(); + let coroutine_layout = cx.tcx.coroutine_layout(coroutine_def_id).unwrap(); let common_upvar_names = cx.tcx.closure_saved_names_of_captured_variables(coroutine_def_id); let variant_range = coroutine_args.variant_range(coroutine_def_id, cx.tcx); diff --git a/compiler/rustc_codegen_llvm/src/debuginfo/metadata/enums/native.rs b/compiler/rustc_codegen_llvm/src/debuginfo/metadata/enums/native.rs index 3dbe820b8ff9b..f0f55981760cb 100644 --- a/compiler/rustc_codegen_llvm/src/debuginfo/metadata/enums/native.rs +++ b/compiler/rustc_codegen_llvm/src/debuginfo/metadata/enums/native.rs @@ -158,8 +158,7 @@ pub(super) fn build_coroutine_di_node<'ll, 'tcx>( DIFlags::FlagZero, ), |cx, coroutine_type_di_node| { - let coroutine_layout = - cx.tcx.optimized_mir(coroutine_def_id).coroutine_layout().unwrap(); + let coroutine_layout = cx.tcx.coroutine_layout(coroutine_def_id).unwrap(); let Variants::Multiple { tag_encoding: TagEncoding::Direct, ref variants, .. } = coroutine_type_and_layout.variants diff --git a/compiler/rustc_ty_utils/src/layout.rs b/compiler/rustc_ty_utils/src/layout.rs index 9c3d39307b26f..85ac6071a08cc 100644 --- a/compiler/rustc_ty_utils/src/layout.rs +++ b/compiler/rustc_ty_utils/src/layout.rs @@ -1072,7 +1072,7 @@ fn variant_info_for_coroutine<'tcx>( return (vec![], None); }; - let coroutine = cx.tcx.optimized_mir(def_id).coroutine_layout().unwrap(); + let coroutine = cx.tcx.coroutine_layout(def_id).unwrap(); let upvar_names = cx.tcx.closure_saved_names_of_captured_variables(def_id); let mut upvars_size = Size::ZERO; From b7d67eace78d5e660df93b513326650fe8226a96 Mon Sep 17 00:00:00 2001 From: Michael Goulet Date: Sun, 24 Mar 2024 21:12:49 -0400 Subject: [PATCH 02/14] Require coroutine kind type to be passed to TyCtxt::coroutine_layout --- .../src/debuginfo/metadata/enums/cpp_like.rs | 3 +- .../src/debuginfo/metadata/enums/native.rs | 7 +++- .../src/transform/validate.rs | 13 ++++--- compiler/rustc_middle/src/mir/mod.rs | 3 +- compiler/rustc_middle/src/mir/pretty.rs | 2 +- compiler/rustc_middle/src/ty/mod.rs | 37 ++++++++++++++++++- compiler/rustc_middle/src/ty/sty.rs | 7 +++- compiler/rustc_ty_utils/src/layout.rs | 4 +- 8 files changed, 59 insertions(+), 17 deletions(-) diff --git a/compiler/rustc_codegen_llvm/src/debuginfo/metadata/enums/cpp_like.rs b/compiler/rustc_codegen_llvm/src/debuginfo/metadata/enums/cpp_like.rs index 9a10e1bc1299c..4edef14422e5f 100644 --- a/compiler/rustc_codegen_llvm/src/debuginfo/metadata/enums/cpp_like.rs +++ b/compiler/rustc_codegen_llvm/src/debuginfo/metadata/enums/cpp_like.rs @@ -683,7 +683,8 @@ fn build_union_fields_for_direct_tag_coroutine<'ll, 'tcx>( _ => unreachable!(), }; - let coroutine_layout = cx.tcx.coroutine_layout(coroutine_def_id).unwrap(); + let coroutine_layout = + cx.tcx.coroutine_layout(coroutine_def_id, coroutine_args.kind_ty()).unwrap(); let common_upvar_names = cx.tcx.closure_saved_names_of_captured_variables(coroutine_def_id); let variant_range = coroutine_args.variant_range(coroutine_def_id, cx.tcx); diff --git a/compiler/rustc_codegen_llvm/src/debuginfo/metadata/enums/native.rs b/compiler/rustc_codegen_llvm/src/debuginfo/metadata/enums/native.rs index f0f55981760cb..115d5187eafa8 100644 --- a/compiler/rustc_codegen_llvm/src/debuginfo/metadata/enums/native.rs +++ b/compiler/rustc_codegen_llvm/src/debuginfo/metadata/enums/native.rs @@ -135,7 +135,7 @@ pub(super) fn build_coroutine_di_node<'ll, 'tcx>( unique_type_id: UniqueTypeId<'tcx>, ) -> DINodeCreationResult<'ll> { let coroutine_type = unique_type_id.expect_ty(); - let &ty::Coroutine(coroutine_def_id, _) = coroutine_type.kind() else { + let &ty::Coroutine(coroutine_def_id, coroutine_args) = coroutine_type.kind() else { bug!("build_coroutine_di_node() called with non-coroutine type: `{:?}`", coroutine_type) }; @@ -158,7 +158,10 @@ pub(super) fn build_coroutine_di_node<'ll, 'tcx>( DIFlags::FlagZero, ), |cx, coroutine_type_di_node| { - let coroutine_layout = cx.tcx.coroutine_layout(coroutine_def_id).unwrap(); + let coroutine_layout = cx + .tcx + .coroutine_layout(coroutine_def_id, coroutine_args.as_coroutine().kind_ty()) + .unwrap(); let Variants::Multiple { tag_encoding: TagEncoding::Direct, ref variants, .. } = coroutine_type_and_layout.variants diff --git a/compiler/rustc_const_eval/src/transform/validate.rs b/compiler/rustc_const_eval/src/transform/validate.rs index 08e3e42a82e27..b085e4e76a11e 100644 --- a/compiler/rustc_const_eval/src/transform/validate.rs +++ b/compiler/rustc_const_eval/src/transform/validate.rs @@ -101,9 +101,9 @@ impl<'tcx> MirPass<'tcx> for Validator { } // Enforce that coroutine-closure layouts are identical. - if let Some(layout) = body.coroutine_layout() + if let Some(layout) = body.coroutine_layout_raw() && let Some(by_move_body) = body.coroutine_by_move_body() - && let Some(by_move_layout) = by_move_body.coroutine_layout() + && let Some(by_move_layout) = by_move_body.coroutine_layout_raw() { if layout != by_move_layout { // If this turns out not to be true, please let compiler-errors know. @@ -715,13 +715,14 @@ impl<'a, 'tcx> Visitor<'tcx> for TypeChecker<'a, 'tcx> { // args of the coroutine. Otherwise, we prefer to use this body // since we may be in the process of computing this MIR in the // first place. - let gen_body = if def_id == self.caller_body.source.def_id() { - self.caller_body + let layout = if def_id == self.caller_body.source.def_id() { + // FIXME: This is not right for async closures. + self.caller_body.coroutine_layout_raw() } else { - self.tcx.optimized_mir(def_id) + self.tcx.coroutine_layout(def_id, args.as_coroutine().kind_ty()) }; - let Some(layout) = gen_body.coroutine_layout() else { + let Some(layout) = layout else { self.fail( location, format!("No coroutine layout for {parent_ty:?}"), diff --git a/compiler/rustc_middle/src/mir/mod.rs b/compiler/rustc_middle/src/mir/mod.rs index e4dce2bdc9e80..02af55fbf0e4f 100644 --- a/compiler/rustc_middle/src/mir/mod.rs +++ b/compiler/rustc_middle/src/mir/mod.rs @@ -652,8 +652,9 @@ impl<'tcx> Body<'tcx> { self.coroutine.as_ref().and_then(|coroutine| coroutine.resume_ty) } + /// Prefer going through [`TyCtxt::coroutine_layout`] rather than using this directly. #[inline] - pub fn coroutine_layout(&self) -> Option<&CoroutineLayout<'tcx>> { + pub fn coroutine_layout_raw(&self) -> Option<&CoroutineLayout<'tcx>> { self.coroutine.as_ref().and_then(|coroutine| coroutine.coroutine_layout.as_ref()) } diff --git a/compiler/rustc_middle/src/mir/pretty.rs b/compiler/rustc_middle/src/mir/pretty.rs index f0499cf344fca..41df2e3b5875a 100644 --- a/compiler/rustc_middle/src/mir/pretty.rs +++ b/compiler/rustc_middle/src/mir/pretty.rs @@ -126,7 +126,7 @@ fn dump_matched_mir_node<'tcx, F>( Some(promoted) => write!(file, "::{promoted:?}`")?, } writeln!(file, " {disambiguator} {pass_name}")?; - if let Some(ref layout) = body.coroutine_layout() { + if let Some(ref layout) = body.coroutine_layout_raw() { writeln!(file, "/* coroutine_layout = {layout:#?} */")?; } writeln!(file)?; diff --git a/compiler/rustc_middle/src/ty/mod.rs b/compiler/rustc_middle/src/ty/mod.rs index 6ce53ccc8cd7a..aad2f6a4cf8ac 100644 --- a/compiler/rustc_middle/src/ty/mod.rs +++ b/compiler/rustc_middle/src/ty/mod.rs @@ -60,6 +60,7 @@ pub use rustc_target::abi::{ReprFlags, ReprOptions}; pub use rustc_type_ir::{DebugWithInfcx, InferCtxtLike, WithInfcx}; pub use vtable::*; +use std::assert_matches::assert_matches; use std::fmt::Debug; use std::hash::{Hash, Hasher}; use std::marker::PhantomData; @@ -1826,8 +1827,40 @@ impl<'tcx> TyCtxt<'tcx> { /// Returns layout of a coroutine. Layout might be unavailable if the /// coroutine is tainted by errors. - pub fn coroutine_layout(self, def_id: DefId) -> Option<&'tcx CoroutineLayout<'tcx>> { - self.optimized_mir(def_id).coroutine_layout() + /// + /// Takes `coroutine_kind` which can be acquired from the `CoroutineArgs::kind_ty`, + /// e.g. `args.as_coroutine().kind_ty()`. + pub fn coroutine_layout( + self, + def_id: DefId, + coroutine_kind_ty: Ty<'tcx>, + ) -> Option<&'tcx CoroutineLayout<'tcx>> { + let mir = self.optimized_mir(def_id); + // Regular coroutine + if coroutine_kind_ty.is_unit() { + mir.coroutine_layout_raw() + } else { + // If we have a `Coroutine` that comes from an coroutine-closure, + // then it may be a by-move or by-ref body. + let ty::Coroutine(_, identity_args) = + *self.type_of(def_id).instantiate_identity().kind() + else { + unreachable!(); + }; + let identity_kind_ty = identity_args.as_coroutine().kind_ty(); + // If the types differ, then we must be getting the by-move body of + // a by-ref coroutine. + if identity_kind_ty == coroutine_kind_ty { + mir.coroutine_layout_raw() + } else { + assert_matches!(coroutine_kind_ty.to_opt_closure_kind(), Some(ClosureKind::FnOnce)); + assert_matches!( + identity_kind_ty.to_opt_closure_kind(), + Some(ClosureKind::Fn | ClosureKind::FnMut) + ); + mir.coroutine_by_move_body().unwrap().coroutine_layout_raw() + } + } } /// Given the `DefId` of an impl, returns the `DefId` of the trait it implements. diff --git a/compiler/rustc_middle/src/ty/sty.rs b/compiler/rustc_middle/src/ty/sty.rs index c85ee140fa4ec..82a2423cbc6b0 100644 --- a/compiler/rustc_middle/src/ty/sty.rs +++ b/compiler/rustc_middle/src/ty/sty.rs @@ -694,7 +694,10 @@ impl<'tcx> CoroutineArgs<'tcx> { #[inline] pub fn variant_range(&self, def_id: DefId, tcx: TyCtxt<'tcx>) -> Range { // FIXME requires optimized MIR - FIRST_VARIANT..tcx.coroutine_layout(def_id).unwrap().variant_fields.next_index() + // FIXME(async_closures): We should assert all coroutine layouts have + // the same number of variants. + FIRST_VARIANT + ..tcx.coroutine_layout(def_id, tcx.types.unit).unwrap().variant_fields.next_index() } /// The discriminant for the given variant. Panics if the `variant_index` is @@ -754,7 +757,7 @@ impl<'tcx> CoroutineArgs<'tcx> { def_id: DefId, tcx: TyCtxt<'tcx>, ) -> impl Iterator> + Captures<'tcx>> { - let layout = tcx.coroutine_layout(def_id).unwrap(); + let layout = tcx.coroutine_layout(def_id, self.kind_ty()).unwrap(); layout.variant_fields.iter().map(move |variant| { variant.iter().map(move |field| { ty::EarlyBinder::bind(layout.field_tys[*field].ty).instantiate(tcx, self.args) diff --git a/compiler/rustc_ty_utils/src/layout.rs b/compiler/rustc_ty_utils/src/layout.rs index 85ac6071a08cc..331970ac36233 100644 --- a/compiler/rustc_ty_utils/src/layout.rs +++ b/compiler/rustc_ty_utils/src/layout.rs @@ -745,7 +745,7 @@ fn coroutine_layout<'tcx>( let tcx = cx.tcx; let instantiate_field = |ty: Ty<'tcx>| EarlyBinder::bind(ty).instantiate(tcx, args); - let Some(info) = tcx.coroutine_layout(def_id) else { + let Some(info) = tcx.coroutine_layout(def_id, args.as_coroutine().kind_ty()) else { return Err(error(cx, LayoutError::Unknown(ty))); }; let (ineligible_locals, assignments) = coroutine_saved_local_eligibility(info); @@ -1072,7 +1072,7 @@ fn variant_info_for_coroutine<'tcx>( return (vec![], None); }; - let coroutine = cx.tcx.coroutine_layout(def_id).unwrap(); + let coroutine = cx.tcx.coroutine_layout(def_id, args.as_coroutine().kind_ty()).unwrap(); let upvar_names = cx.tcx.closure_saved_names_of_captured_variables(def_id); let mut upvars_size = Size::ZERO; From 9bda9ac76e8d232c6cf0efde55dace718c1d428c Mon Sep 17 00:00:00 2001 From: Michael Goulet Date: Sun, 24 Mar 2024 21:14:49 -0400 Subject: [PATCH 03/14] Relax validation now --- compiler/rustc_const_eval/src/transform/validate.rs | 9 ++++----- compiler/rustc_middle/src/ty/sty.rs | 2 -- 2 files changed, 4 insertions(+), 7 deletions(-) diff --git a/compiler/rustc_const_eval/src/transform/validate.rs b/compiler/rustc_const_eval/src/transform/validate.rs index b085e4e76a11e..378b168a50c33 100644 --- a/compiler/rustc_const_eval/src/transform/validate.rs +++ b/compiler/rustc_const_eval/src/transform/validate.rs @@ -105,14 +105,13 @@ impl<'tcx> MirPass<'tcx> for Validator { && let Some(by_move_body) = body.coroutine_by_move_body() && let Some(by_move_layout) = by_move_body.coroutine_layout_raw() { - if layout != by_move_layout { - // If this turns out not to be true, please let compiler-errors know. - // It is possible to support, but requires some changes to the layout - // computation code. + // FIXME(async_closures): We could do other validation here? + if layout.variant_fields.len() != by_move_layout.variant_fields.len() { cfg_checker.fail( Location::START, format!( - "Coroutine layout differs from by-move coroutine layout:\n\ + "Coroutine layout has different number of variant fields from \ + by-move coroutine layout:\n\ layout: {layout:#?}\n\ by_move_layout: {by_move_layout:#?}", ), diff --git a/compiler/rustc_middle/src/ty/sty.rs b/compiler/rustc_middle/src/ty/sty.rs index 82a2423cbc6b0..510a4b59520c4 100644 --- a/compiler/rustc_middle/src/ty/sty.rs +++ b/compiler/rustc_middle/src/ty/sty.rs @@ -694,8 +694,6 @@ impl<'tcx> CoroutineArgs<'tcx> { #[inline] pub fn variant_range(&self, def_id: DefId, tcx: TyCtxt<'tcx>) -> Range { // FIXME requires optimized MIR - // FIXME(async_closures): We should assert all coroutine layouts have - // the same number of variants. FIRST_VARIANT ..tcx.coroutine_layout(def_id, tcx.types.unit).unwrap().variant_fields.next_index() } From 8560d01a96f38790299c20b9c34dcc6853a08a00 Mon Sep 17 00:00:00 2001 From: klensy Date: Mon, 25 Mar 2024 23:19:40 +0300 Subject: [PATCH 04/14] lib: fix some unnecessary_cast clippy lint warning: casting raw pointers to the same type and constness is unnecessary (`*mut V` -> `*mut V`) --> library\alloc\src\collections\btree\map\entry.rs:357:31 | 357 | let val_ptr = root.borrow_mut().push(self.key, value) as *mut V; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `root.borrow_mut().push (self.key, value)` | = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_cast warning: casting to the same type is unnecessary (`usize` -> `usize`) --> library\alloc\src\ffi\c_str.rs:411:56 | 411 | let slice = slice::from_raw_parts_mut(ptr, len as usize); | ^^^^^^^^^^^^ help: try: `len` | = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_cast warning: casting raw pointers to the same type and constness is unnecessary (`*mut T` -> `*mut T`) --> library\alloc\src\slice.rs:516:25 | 516 | (buf.as_mut_ptr() as *mut T).add(buf.len()), | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `buf.as_mut_ptr()` | = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_cast warning: casting raw pointers to the same type and constness is unnecessary (`*mut T` -> `*mut T`) --> library\alloc\src\slice.rs:537:21 | 537 | (buf.as_mut_ptr() as *mut T).add(buf.len()), | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `buf.as_mut_ptr()` | = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_cast warning: casting raw pointers to the same type and constness is unnecessary (`*const ()` -> `*const ()`) --> library\alloc\src\task.rs:151:13 | 151 | waker as *const (), | ^^^^^^^^^^^^^^^^^^ help: try: `waker` | = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_cast warning: casting raw pointers to the same type and constness is unnecessary (`*const ()` -> `*const ()`) --> library\alloc\src\task.rs:323:13 | 323 | waker as *const (), | ^^^^^^^^^^^^^^^^^^ help: try: `waker` | = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_cast warning: casting to the same type is unnecessary (`usize` -> `usize`) --> library\std\src\sys_common\net.rs:110:21 | 110 | assert!(len as usize >= mem::size_of::()); | ^^^^^^^^^^^^ help: try: `len` | = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_cast warning: casting to the same type is unnecessary (`usize` -> `usize`) --> library\std\src\sys_common\net.rs:116:21 | 116 | assert!(len as usize >= mem::size_of::()); | ^^^^^^^^^^^^ help: try: `len` | = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_cast --- library/alloc/src/collections/btree/map/entry.rs | 2 +- library/alloc/src/ffi/c_str.rs | 2 +- library/alloc/src/slice.rs | 8 ++++---- library/alloc/src/task.rs | 4 ++-- library/std/src/sys_common/net.rs | 4 ++-- 5 files changed, 10 insertions(+), 10 deletions(-) diff --git a/library/alloc/src/collections/btree/map/entry.rs b/library/alloc/src/collections/btree/map/entry.rs index 0a894258f469f..66eb991c6d4b8 100644 --- a/library/alloc/src/collections/btree/map/entry.rs +++ b/library/alloc/src/collections/btree/map/entry.rs @@ -354,7 +354,7 @@ impl<'a, K: Ord, V, A: Allocator + Clone> VacantEntry<'a, K, V, A> { // SAFETY: There is no tree yet so no reference to it exists. let map = unsafe { self.dormant_map.awaken() }; let mut root = NodeRef::new_leaf(self.alloc.clone()); - let val_ptr = root.borrow_mut().push(self.key, value) as *mut V; + let val_ptr = root.borrow_mut().push(self.key, value); map.root = Some(root.forget_type()); map.length = 1; val_ptr diff --git a/library/alloc/src/ffi/c_str.rs b/library/alloc/src/ffi/c_str.rs index 0c7f94ccceb02..6a64eaf576bb9 100644 --- a/library/alloc/src/ffi/c_str.rs +++ b/library/alloc/src/ffi/c_str.rs @@ -408,7 +408,7 @@ impl CString { fn strlen(s: *const c_char) -> usize; } let len = strlen(ptr) + 1; // Including the NUL byte - let slice = slice::from_raw_parts_mut(ptr, len as usize); + let slice = slice::from_raw_parts_mut(ptr, len); CString { inner: Box::from_raw(slice as *mut [c_char] as *mut [u8]) } } } diff --git a/library/alloc/src/slice.rs b/library/alloc/src/slice.rs index 21d5dce04a0fc..ebe6f7e7caa9b 100644 --- a/library/alloc/src/slice.rs +++ b/library/alloc/src/slice.rs @@ -511,9 +511,9 @@ impl [T] { while m > 0 { // `buf.extend(buf)`: unsafe { - ptr::copy_nonoverlapping( + ptr::copy_nonoverlapping::( buf.as_ptr(), - (buf.as_mut_ptr() as *mut T).add(buf.len()), + (buf.as_mut_ptr()).add(buf.len()), buf.len(), ); // `buf` has capacity of `self.len() * n`. @@ -532,9 +532,9 @@ impl [T] { // `buf.extend(buf[0 .. rem_len])`: unsafe { // This is non-overlapping since `2^expn > rem`. - ptr::copy_nonoverlapping( + ptr::copy_nonoverlapping::( buf.as_ptr(), - (buf.as_mut_ptr() as *mut T).add(buf.len()), + (buf.as_mut_ptr()).add(buf.len()), rem_len, ); // `buf.len() + rem_len` equals to `buf.capacity()` (`= self.len() * n`). diff --git a/library/alloc/src/task.rs b/library/alloc/src/task.rs index b40768a52b6b4..a3fa6585a37f8 100644 --- a/library/alloc/src/task.rs +++ b/library/alloc/src/task.rs @@ -148,7 +148,7 @@ fn raw_waker(waker: Arc) -> RawWaker { unsafe fn clone_waker(waker: *const ()) -> RawWaker { unsafe { Arc::increment_strong_count(waker as *const W) }; RawWaker::new( - waker as *const (), + waker, &RawWakerVTable::new(clone_waker::, wake::, wake_by_ref::, drop_waker::), ) } @@ -320,7 +320,7 @@ fn local_raw_waker(waker: Rc) -> RawWaker { unsafe fn clone_waker(waker: *const ()) -> RawWaker { unsafe { Rc::increment_strong_count(waker as *const W) }; RawWaker::new( - waker as *const (), + waker, &RawWakerVTable::new(clone_waker::, wake::, wake_by_ref::, drop_waker::), ) } diff --git a/library/std/src/sys_common/net.rs b/library/std/src/sys_common/net.rs index 581c46af0eacf..2d785064245dc 100644 --- a/library/std/src/sys_common/net.rs +++ b/library/std/src/sys_common/net.rs @@ -107,13 +107,13 @@ where pub fn sockaddr_to_addr(storage: &c::sockaddr_storage, len: usize) -> io::Result { match storage.ss_family as c_int { c::AF_INET => { - assert!(len as usize >= mem::size_of::()); + assert!(len >= mem::size_of::()); Ok(SocketAddr::V4(FromInner::from_inner(unsafe { *(storage as *const _ as *const c::sockaddr_in) }))) } c::AF_INET6 => { - assert!(len as usize >= mem::size_of::()); + assert!(len >= mem::size_of::()); Ok(SocketAddr::V6(FromInner::from_inner(unsafe { *(storage as *const _ as *const c::sockaddr_in6) }))) From 91547573af82ddac521a5287e2d03a28454a10ce Mon Sep 17 00:00:00 2001 From: Lukas Wirth Date: Thu, 29 Feb 2024 14:16:32 +0100 Subject: [PATCH 05/14] Implement `-L builtin:$path` --- compiler/rustc_interface/src/tests.rs | 55 +++++++++++++--------- compiler/rustc_session/src/config.rs | 10 ++-- compiler/rustc_session/src/search_paths.rs | 29 +++++++++++- src/librustdoc/config.rs | 8 +++- 4 files changed, 71 insertions(+), 31 deletions(-) diff --git a/compiler/rustc_interface/src/tests.rs b/compiler/rustc_interface/src/tests.rs index 8a27e9a6453b2..8e43d6974fe72 100644 --- a/compiler/rustc_interface/src/tests.rs +++ b/compiler/rustc_interface/src/tests.rs @@ -315,30 +315,39 @@ fn test_search_paths_tracking_hash_different_order() { json_rendered: HumanReadableErrorType::Default(ColorConfig::Never), }; + let push = |opts: &mut Options, search_path| { + opts.search_paths.push(SearchPath::from_cli_opt( + None, + &opts.target_triple, + &early_dcx, + search_path, + )); + }; + // Reference - v1.search_paths.push(SearchPath::from_cli_opt(&early_dcx, "native=abc")); - v1.search_paths.push(SearchPath::from_cli_opt(&early_dcx, "crate=def")); - v1.search_paths.push(SearchPath::from_cli_opt(&early_dcx, "dependency=ghi")); - v1.search_paths.push(SearchPath::from_cli_opt(&early_dcx, "framework=jkl")); - v1.search_paths.push(SearchPath::from_cli_opt(&early_dcx, "all=mno")); - - v2.search_paths.push(SearchPath::from_cli_opt(&early_dcx, "native=abc")); - v2.search_paths.push(SearchPath::from_cli_opt(&early_dcx, "dependency=ghi")); - v2.search_paths.push(SearchPath::from_cli_opt(&early_dcx, "crate=def")); - v2.search_paths.push(SearchPath::from_cli_opt(&early_dcx, "framework=jkl")); - v2.search_paths.push(SearchPath::from_cli_opt(&early_dcx, "all=mno")); - - v3.search_paths.push(SearchPath::from_cli_opt(&early_dcx, "crate=def")); - v3.search_paths.push(SearchPath::from_cli_opt(&early_dcx, "framework=jkl")); - v3.search_paths.push(SearchPath::from_cli_opt(&early_dcx, "native=abc")); - v3.search_paths.push(SearchPath::from_cli_opt(&early_dcx, "dependency=ghi")); - v3.search_paths.push(SearchPath::from_cli_opt(&early_dcx, "all=mno")); - - v4.search_paths.push(SearchPath::from_cli_opt(&early_dcx, "all=mno")); - v4.search_paths.push(SearchPath::from_cli_opt(&early_dcx, "native=abc")); - v4.search_paths.push(SearchPath::from_cli_opt(&early_dcx, "crate=def")); - v4.search_paths.push(SearchPath::from_cli_opt(&early_dcx, "dependency=ghi")); - v4.search_paths.push(SearchPath::from_cli_opt(&early_dcx, "framework=jkl")); + push(&mut v1, "native=abc"); + push(&mut v1, "crate=def"); + push(&mut v1, "dependency=ghi"); + push(&mut v1, "framework=jkl"); + push(&mut v1, "all=mno"); + + push(&mut v2, "native=abc"); + push(&mut v2, "dependency=ghi"); + push(&mut v2, "crate=def"); + push(&mut v2, "framework=jkl"); + push(&mut v2, "all=mno"); + + push(&mut v3, "crate=def"); + push(&mut v3, "framework=jkl"); + push(&mut v3, "native=abc"); + push(&mut v3, "dependency=ghi"); + push(&mut v3, "all=mno"); + + push(&mut v4, "all=mno"); + push(&mut v4, "native=abc"); + push(&mut v4, "crate=def"); + push(&mut v4, "dependency=ghi"); + push(&mut v4, "framework=jkl"); assert_same_hash(&v1, &v2); assert_same_hash(&v1, &v3); diff --git a/compiler/rustc_session/src/config.rs b/compiler/rustc_session/src/config.rs index c06fe29c5673f..7932865922776 100644 --- a/compiler/rustc_session/src/config.rs +++ b/compiler/rustc_session/src/config.rs @@ -2795,11 +2795,6 @@ pub fn build_session_options(early_dcx: &mut EarlyDiagCtxt, matches: &getopts::M let debuginfo = select_debuginfo(matches, &cg); let debuginfo_compression = unstable_opts.debuginfo_compression; - let mut search_paths = vec![]; - for s in &matches.opt_strs("L") { - search_paths.push(SearchPath::from_cli_opt(early_dcx, s)); - } - let libs = parse_libs(early_dcx, matches); let test = matches.opt_present("test"); @@ -2848,6 +2843,11 @@ pub fn build_session_options(early_dcx: &mut EarlyDiagCtxt, matches: &getopts::M candidate.join("library/std/src/lib.rs").is_file().then_some(candidate) }; + let mut search_paths = vec![]; + for s in &matches.opt_strs("L") { + search_paths.push(SearchPath::from_cli_opt(Some(&sysroot), &target_triple, early_dcx, s)); + } + let working_dir = std::env::current_dir().unwrap_or_else(|e| { early_dcx.early_fatal(format!("Current directory is invalid: {e}")); }); diff --git a/compiler/rustc_session/src/search_paths.rs b/compiler/rustc_session/src/search_paths.rs index 32d5e430717bd..58b4a08b5b028 100644 --- a/compiler/rustc_session/src/search_paths.rs +++ b/compiler/rustc_session/src/search_paths.rs @@ -1,5 +1,6 @@ use crate::filesearch::make_target_lib_path; use crate::EarlyDiagCtxt; +use rustc_target::spec::TargetTriple; use std::path::{Path, PathBuf}; #[derive(Clone, Debug)] @@ -46,7 +47,12 @@ impl PathKind { } impl SearchPath { - pub fn from_cli_opt(early_dcx: &EarlyDiagCtxt, path: &str) -> Self { + pub fn from_cli_opt( + sysroot: Option<&Path>, + triple: &TargetTriple, + early_dcx: &EarlyDiagCtxt, + path: &str, + ) -> Self { let (kind, path) = if let Some(stripped) = path.strip_prefix("native=") { (PathKind::Native, stripped) } else if let Some(stripped) = path.strip_prefix("crate=") { @@ -57,6 +63,27 @@ impl SearchPath { (PathKind::Framework, stripped) } else if let Some(stripped) = path.strip_prefix("all=") { (PathKind::All, stripped) + } else if let Some(stripped) = path.strip_prefix("builtin:") { + let Some(sysroot) = sysroot else { + early_dcx.early_fatal("`-L builtin:` is not supported without a sysroot present"); + }; + let triple = match triple { + TargetTriple::TargetTriple(triple) => triple, + TargetTriple::TargetJson { .. } => { + early_dcx.early_fatal("`-L builtin:` is not supported with custom targets"); + } + }; + + if stripped.contains(std::path::is_separator) { + early_dcx.early_fatal("`-L builtin:` does not accept paths"); + } + + let path = make_target_lib_path(sysroot, triple).join("builtin").join(stripped); + if !path.is_dir() { + early_dcx.early_fatal(format!("builtin:{stripped} does not exist")); + } + + return Self::new(PathKind::All, path); } else { (PathKind::All, path) }; diff --git a/src/librustdoc/config.rs b/src/librustdoc/config.rs index be7e319bc79f4..ab793bb306214 100644 --- a/src/librustdoc/config.rs +++ b/src/librustdoc/config.rs @@ -460,8 +460,6 @@ impl Options { &matches.free[0] }); - let libs = - matches.opt_strs("L").iter().map(|s| SearchPath::from_cli_opt(early_dcx, s)).collect(); let externs = parse_externs(early_dcx, matches, &unstable_opts); let extern_html_root_urls = match parse_extern_html_roots(matches) { Ok(ex) => ex, @@ -626,6 +624,12 @@ impl Options { let target = parse_target_triple(early_dcx, matches); + let libs = matches + .opt_strs("L") + .iter() + .map(|s| SearchPath::from_cli_opt(None, &target, early_dcx, s)) + .collect(); + let show_coverage = matches.opt_present("show-coverage"); let crate_types = match parse_crate_types_from_list(matches.opt_strs("crate-type")) { From 9ae4e3eb7ca9f9e6c61cedcd8c32f790867a4b37 Mon Sep 17 00:00:00 2001 From: Lukas Wirth Date: Mon, 4 Mar 2024 09:07:02 +0100 Subject: [PATCH 06/14] Make use of sysroot in librustdoc/config.rs for builtin:$path --- src/librustdoc/config.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/librustdoc/config.rs b/src/librustdoc/config.rs index ab793bb306214..fdbbe62c15889 100644 --- a/src/librustdoc/config.rs +++ b/src/librustdoc/config.rs @@ -623,11 +623,12 @@ impl Options { } let target = parse_target_triple(early_dcx, matches); + let maybe_sysroot = matches.opt_str("sysroot").map(PathBuf::from); let libs = matches .opt_strs("L") .iter() - .map(|s| SearchPath::from_cli_opt(None, &target, early_dcx, s)) + .map(|s| SearchPath::from_cli_opt(maybe_sysroot.as_deref(), &target, early_dcx, s)) .collect(); let show_coverage = matches.opt_present("show-coverage"); @@ -657,7 +658,6 @@ impl Options { let bin_crate = crate_types.contains(&CrateType::Executable); let proc_macro_crate = crate_types.contains(&CrateType::ProcMacro); let playground_url = matches.opt_str("playground-url"); - let maybe_sysroot = matches.opt_str("sysroot").map(PathBuf::from); let module_sorting = if matches.opt_present("sort-modules-by-appearance") { ModuleSorting::DeclarationOrder } else { From 2fae4ee92ea9a28722673df442112446f7521079 Mon Sep 17 00:00:00 2001 From: Lukas Wirth Date: Wed, 6 Mar 2024 10:19:34 +0100 Subject: [PATCH 07/14] Make sysroot mandatory for rustdoc --- compiler/rustc_interface/src/tests.rs | 2 +- compiler/rustc_session/src/config.rs | 3 +-- compiler/rustc_session/src/search_paths.rs | 15 +++------------ src/librustdoc/config.rs | 9 ++++++++- 4 files changed, 13 insertions(+), 16 deletions(-) diff --git a/compiler/rustc_interface/src/tests.rs b/compiler/rustc_interface/src/tests.rs index 8e43d6974fe72..3b78e6a43ab33 100644 --- a/compiler/rustc_interface/src/tests.rs +++ b/compiler/rustc_interface/src/tests.rs @@ -317,7 +317,7 @@ fn test_search_paths_tracking_hash_different_order() { let push = |opts: &mut Options, search_path| { opts.search_paths.push(SearchPath::from_cli_opt( - None, + "not-a-sysroot".as_ref(), &opts.target_triple, &early_dcx, search_path, diff --git a/compiler/rustc_session/src/config.rs b/compiler/rustc_session/src/config.rs index 7932865922776..8d186f7c749e8 100644 --- a/compiler/rustc_session/src/config.rs +++ b/compiler/rustc_session/src/config.rs @@ -2824,7 +2824,6 @@ pub fn build_session_options(early_dcx: &mut EarlyDiagCtxt, matches: &getopts::M let logical_env = parse_logical_env(early_dcx, matches); let sysroot = filesearch::materialize_sysroot(sysroot_opt); - let real_rust_source_base_dir = { // This is the location used by the `rust-src` `rustup` component. let mut candidate = sysroot.join("lib/rustlib/src/rust"); @@ -2845,7 +2844,7 @@ pub fn build_session_options(early_dcx: &mut EarlyDiagCtxt, matches: &getopts::M let mut search_paths = vec![]; for s in &matches.opt_strs("L") { - search_paths.push(SearchPath::from_cli_opt(Some(&sysroot), &target_triple, early_dcx, s)); + search_paths.push(SearchPath::from_cli_opt(&sysroot, &target_triple, early_dcx, s)); } let working_dir = std::env::current_dir().unwrap_or_else(|e| { diff --git a/compiler/rustc_session/src/search_paths.rs b/compiler/rustc_session/src/search_paths.rs index 58b4a08b5b028..9cabdec34ae50 100644 --- a/compiler/rustc_session/src/search_paths.rs +++ b/compiler/rustc_session/src/search_paths.rs @@ -48,7 +48,7 @@ impl PathKind { impl SearchPath { pub fn from_cli_opt( - sysroot: Option<&Path>, + sysroot: &Path, triple: &TargetTriple, early_dcx: &EarlyDiagCtxt, path: &str, @@ -64,21 +64,12 @@ impl SearchPath { } else if let Some(stripped) = path.strip_prefix("all=") { (PathKind::All, stripped) } else if let Some(stripped) = path.strip_prefix("builtin:") { - let Some(sysroot) = sysroot else { - early_dcx.early_fatal("`-L builtin:` is not supported without a sysroot present"); - }; - let triple = match triple { - TargetTriple::TargetTriple(triple) => triple, - TargetTriple::TargetJson { .. } => { - early_dcx.early_fatal("`-L builtin:` is not supported with custom targets"); - } - }; - if stripped.contains(std::path::is_separator) { early_dcx.early_fatal("`-L builtin:` does not accept paths"); } - let path = make_target_lib_path(sysroot, triple).join("builtin").join(stripped); + let path = + make_target_lib_path(sysroot, triple.triple()).join("builtin").join(stripped); if !path.is_dir() { early_dcx.early_fatal(format!("builtin:{stripped} does not exist")); } diff --git a/src/librustdoc/config.rs b/src/librustdoc/config.rs index fdbbe62c15889..f078ad2fb5376 100644 --- a/src/librustdoc/config.rs +++ b/src/librustdoc/config.rs @@ -625,10 +625,17 @@ impl Options { let target = parse_target_triple(early_dcx, matches); let maybe_sysroot = matches.opt_str("sysroot").map(PathBuf::from); + let sysroot = match &maybe_sysroot { + Some(s) => s.clone(), + None => { + rustc_session::filesearch::get_or_default_sysroot().expect("Failed finding sysroot") + } + }; + let libs = matches .opt_strs("L") .iter() - .map(|s| SearchPath::from_cli_opt(maybe_sysroot.as_deref(), &target, early_dcx, s)) + .map(|s| SearchPath::from_cli_opt(&sysroot, &target, early_dcx, s)) .collect(); let show_coverage = matches.opt_present("show-coverage"); From 3d65c920404959cc8ef3ba9e963a31fc91983caf Mon Sep 17 00:00:00 2001 From: Lukas Wirth Date: Wed, 6 Mar 2024 13:28:12 +0100 Subject: [PATCH 08/14] Replace implementation with @RUSTC_BUILTIN prefix substitution var --- compiler/rustc_session/src/config.rs | 1 + compiler/rustc_session/src/search_paths.rs | 21 +++++++-------------- 2 files changed, 8 insertions(+), 14 deletions(-) diff --git a/compiler/rustc_session/src/config.rs b/compiler/rustc_session/src/config.rs index 8d186f7c749e8..f612e8b5b1a55 100644 --- a/compiler/rustc_session/src/config.rs +++ b/compiler/rustc_session/src/config.rs @@ -2824,6 +2824,7 @@ pub fn build_session_options(early_dcx: &mut EarlyDiagCtxt, matches: &getopts::M let logical_env = parse_logical_env(early_dcx, matches); let sysroot = filesearch::materialize_sysroot(sysroot_opt); + let real_rust_source_base_dir = { // This is the location used by the `rust-src` `rustup` component. let mut candidate = sysroot.join("lib/rustlib/src/rust"); diff --git a/compiler/rustc_session/src/search_paths.rs b/compiler/rustc_session/src/search_paths.rs index 9cabdec34ae50..16dd40acef047 100644 --- a/compiler/rustc_session/src/search_paths.rs +++ b/compiler/rustc_session/src/search_paths.rs @@ -63,27 +63,20 @@ impl SearchPath { (PathKind::Framework, stripped) } else if let Some(stripped) = path.strip_prefix("all=") { (PathKind::All, stripped) - } else if let Some(stripped) = path.strip_prefix("builtin:") { - if stripped.contains(std::path::is_separator) { - early_dcx.early_fatal("`-L builtin:` does not accept paths"); - } - - let path = - make_target_lib_path(sysroot, triple.triple()).join("builtin").join(stripped); - if !path.is_dir() { - early_dcx.early_fatal(format!("builtin:{stripped} does not exist")); - } - - return Self::new(PathKind::All, path); } else { (PathKind::All, path) }; - if path.is_empty() { + let dir = match path.strip_prefix("@RUSTC_BUILTIN") { + Some(stripped) => { + make_target_lib_path(sysroot, triple.triple()).join("builtin").join(stripped) + } + None => PathBuf::from(path), + }; + if dir.as_os_str().is_empty() { #[allow(rustc::untranslatable_diagnostic)] // FIXME: make this translatable early_dcx.early_fatal("empty search path given via `-L`"); } - let dir = PathBuf::from(path); Self::new(kind, dir) } From 5ddc4f24cc66d02431fcfc6a4cb32c040c0b511c Mon Sep 17 00:00:00 2001 From: Zalathar Date: Fri, 22 Mar 2024 14:33:58 +1100 Subject: [PATCH 09/14] coverage: Inline creating a dummy instance for unused functions --- .../src/coverageinfo/mapgen.rs | 22 +++++++++---------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs b/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs index ee7ea34230168..28ff3eccedac7 100644 --- a/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs +++ b/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs @@ -385,8 +385,7 @@ fn add_unused_functions(cx: &CodegenCx<'_, '_>) { }; debug!("generating unused fn: {def_id:?}"); - let instance = declare_unused_fn(tcx, def_id); - add_unused_function_coverage(cx, instance, function_coverage_info); + add_unused_function_coverage(cx, def_id, function_coverage_info); } } @@ -421,8 +420,15 @@ fn codegenned_and_inlined_items(tcx: TyCtxt<'_>) -> DefIdSet { result } -fn declare_unused_fn<'tcx>(tcx: TyCtxt<'tcx>, def_id: DefId) -> ty::Instance<'tcx> { - ty::Instance::new( +fn add_unused_function_coverage<'tcx>( + cx: &CodegenCx<'_, 'tcx>, + def_id: DefId, + function_coverage_info: &'tcx mir::coverage::FunctionCoverageInfo, +) { + let tcx = cx.tcx; + + // Make a dummy instance that fills in all generics with placeholders. + let instance = ty::Instance::new( def_id, ty::GenericArgs::for_item(tcx, def_id, |param, _| { if let ty::GenericParamDefKind::Lifetime = param.kind { @@ -431,14 +437,8 @@ fn declare_unused_fn<'tcx>(tcx: TyCtxt<'tcx>, def_id: DefId) -> ty::Instance<'tc tcx.mk_param_from_def(param) } }), - ) -} + ); -fn add_unused_function_coverage<'tcx>( - cx: &CodegenCx<'_, 'tcx>, - instance: ty::Instance<'tcx>, - function_coverage_info: &'tcx mir::coverage::FunctionCoverageInfo, -) { // An unused function's mappings will automatically be rewritten to map to // zero, because none of its counters/expressions are marked as seen. let function_coverage = FunctionCoverageCollector::unused(instance, function_coverage_info); From e3f66b24933356e5bda6a7161ab92706cd015dea Mon Sep 17 00:00:00 2001 From: Zalathar Date: Fri, 22 Mar 2024 12:59:49 +1100 Subject: [PATCH 10/14] coverage: Overhaul the search for unused functions --- .../src/coverageinfo/mapgen.rs | 130 +++++++++--------- 1 file changed, 68 insertions(+), 62 deletions(-) diff --git a/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs b/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs index 28ff3eccedac7..95cd4cca4d2ce 100644 --- a/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs +++ b/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs @@ -6,9 +6,8 @@ use crate::llvm; use itertools::Itertools as _; use rustc_codegen_ssa::traits::{BaseTypeMethods, ConstMethods}; -use rustc_data_structures::fx::{FxIndexMap, FxIndexSet}; -use rustc_hir::def::DefKind; -use rustc_hir::def_id::DefId; +use rustc_data_structures::fx::{FxHashSet, FxIndexMap, FxIndexSet}; +use rustc_hir::def_id::{DefId, LocalDefId}; use rustc_index::IndexVec; use rustc_middle::bug; use rustc_middle::mir; @@ -335,16 +334,9 @@ fn save_function_record( ); } -/// When finalizing the coverage map, `FunctionCoverage` only has the `CodeRegion`s and counters for -/// the functions that went through codegen; such as public functions and "used" functions -/// (functions referenced by other "used" or public items). Any other functions considered unused, -/// or "Unreachable", were still parsed and processed through the MIR stage, but were not -/// codegenned. (Note that `-Clink-dead-code` can force some unused code to be codegenned, but -/// that flag is known to cause other errors, when combined with `-C instrument-coverage`; and -/// `-Clink-dead-code` will not generate code for unused generic functions.) -/// -/// We can find the unused functions (including generic functions) by the set difference of all MIR -/// `DefId`s (`tcx` query `mir_keys`) minus the codegenned `DefId`s (`codegenned_and_inlined_items`). +/// Each CGU will normally only emit coverage metadata for the functions that it actually generates. +/// But since we don't want unused functions to disappear from coverage reports, we also scan for +/// functions that were instrumented but are not participating in codegen. /// /// These unused functions don't need to be codegenned, but we do need to add them to the function /// coverage map (in a single designated CGU) so that we still emit coverage mappings for them. @@ -354,78 +346,92 @@ fn add_unused_functions(cx: &CodegenCx<'_, '_>) { assert!(cx.codegen_unit.is_code_coverage_dead_code_cgu()); let tcx = cx.tcx; + let usage = prepare_usage_sets(tcx); + + let is_unused_fn = |def_id: LocalDefId| -> bool { + let def_id = def_id.to_def_id(); + + // To be eligible for "unused function" mappings, a definition must: + // - Be function-like + // - Not participate directly in codegen + // - Not have any coverage statements inlined into codegenned functions + tcx.def_kind(def_id).is_fn_like() + && !usage.all_mono_items.contains(&def_id) + && !usage.used_via_inlining.contains(&def_id) + }; - let eligible_def_ids = tcx.mir_keys(()).iter().filter_map(|local_def_id| { - let def_id = local_def_id.to_def_id(); - let kind = tcx.def_kind(def_id); - // `mir_keys` will give us `DefId`s for all kinds of things, not - // just "functions", like consts, statics, etc. Filter those out. - if !matches!(kind, DefKind::Fn | DefKind::AssocFn | DefKind::Closure) { - return None; - } + // Scan for unused functions that were instrumented for coverage. + for def_id in tcx.mir_keys(()).iter().copied().filter(|&def_id| is_unused_fn(def_id)) { + // Get the coverage info from MIR, skipping functions that were never instrumented. + let body = tcx.optimized_mir(def_id); + let Some(function_coverage_info) = body.function_coverage_info.as_deref() else { continue }; // FIXME(79651): Consider trying to filter out dummy instantiations of // unused generic functions from library crates, because they can produce // "unused instantiation" in coverage reports even when they are actually // used by some downstream crate in the same binary. - Some(local_def_id.to_def_id()) - }); - - let codegenned_def_ids = codegenned_and_inlined_items(tcx); - - // For each `DefId` that should have coverage instrumentation but wasn't - // codegenned, add it to the function coverage map as an unused function. - for def_id in eligible_def_ids.filter(|id| !codegenned_def_ids.contains(id)) { - // Skip any function that didn't have coverage data added to it by the - // coverage instrumentor. - let body = tcx.instance_mir(ty::InstanceDef::Item(def_id)); - let Some(function_coverage_info) = body.function_coverage_info.as_deref() else { - continue; - }; - debug!("generating unused fn: {def_id:?}"); add_unused_function_coverage(cx, def_id, function_coverage_info); } } -/// All items participating in code generation together with (instrumented) -/// items inlined into them. -fn codegenned_and_inlined_items(tcx: TyCtxt<'_>) -> DefIdSet { - let (items, cgus) = tcx.collect_and_partition_mono_items(()); - let mut visited = DefIdSet::default(); - let mut result = items.clone(); - - for cgu in cgus { - for item in cgu.items().keys() { - if let mir::mono::MonoItem::Fn(ref instance) = item { - let did = instance.def_id(); - if !visited.insert(did) { - continue; - } - let body = tcx.instance_mir(instance.def); - for block in body.basic_blocks.iter() { - for statement in &block.statements { - let mir::StatementKind::Coverage(_) = statement.kind else { continue }; - let scope = statement.source_info.scope; - if let Some(inlined) = scope.inlined_instance(&body.source_scopes) { - result.insert(inlined.def_id()); - } - } - } +struct UsageSets<'tcx> { + all_mono_items: &'tcx DefIdSet, + used_via_inlining: FxHashSet, +} + +/// Prepare sets of definitions that are relevant to deciding whether something +/// is an "unused function" for coverage purposes. +fn prepare_usage_sets<'tcx>(tcx: TyCtxt<'tcx>) -> UsageSets<'tcx> { + let (all_mono_items, cgus) = tcx.collect_and_partition_mono_items(()); + + // Obtain a MIR body for each function participating in codegen, via an + // arbitrary instance. + let mut def_ids_seen = FxHashSet::default(); + let def_and_mir_for_all_mono_fns = cgus + .iter() + .flat_map(|cgu| cgu.items().keys()) + .filter_map(|item| match item { + mir::mono::MonoItem::Fn(instance) => Some(instance), + mir::mono::MonoItem::Static(_) | mir::mono::MonoItem::GlobalAsm(_) => None, + }) + // We only need one arbitrary instance per definition. + .filter(move |instance| def_ids_seen.insert(instance.def_id())) + .map(|instance| { + // We don't care about the instance, just its underlying MIR. + let body = tcx.instance_mir(instance.def); + (instance.def_id(), body) + }); + + // Functions whose coverage statments were found inlined into other functions. + let mut used_via_inlining = FxHashSet::default(); + + for (_def_id, body) in def_and_mir_for_all_mono_fns { + // Inspect every coverage statement in the function's MIR. + for stmt in body + .basic_blocks + .iter() + .flat_map(|block| &block.statements) + .filter(|stmt| matches!(stmt.kind, mir::StatementKind::Coverage(_))) + { + if let Some(inlined) = stmt.source_info.scope.inlined_instance(&body.source_scopes) { + // This coverage statement was inlined from another function. + used_via_inlining.insert(inlined.def_id()); } } } - result + UsageSets { all_mono_items, used_via_inlining } } fn add_unused_function_coverage<'tcx>( cx: &CodegenCx<'_, 'tcx>, - def_id: DefId, + def_id: LocalDefId, function_coverage_info: &'tcx mir::coverage::FunctionCoverageInfo, ) { let tcx = cx.tcx; + let def_id = def_id.to_def_id(); // Make a dummy instance that fills in all generics with placeholders. let instance = ty::Instance::new( From 54116c8cae58f1d46f231fbeac5630a81b994a4c Mon Sep 17 00:00:00 2001 From: Zalathar Date: Fri, 22 Mar 2024 13:23:04 +1100 Subject: [PATCH 11/14] coverage: Detect functions that have lost all their coverage statements If a function was instrumented for coverage, but all of its coverage statements have been removed by later MIR transforms, it should be treated as "unused" even if the compiler generates an unreachable stub for it. --- .../src/coverageinfo/mapgen.rs | 22 +++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-) diff --git a/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs b/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs index 95cd4cca4d2ce..0fbc624389bc5 100644 --- a/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs +++ b/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs @@ -353,10 +353,11 @@ fn add_unused_functions(cx: &CodegenCx<'_, '_>) { // To be eligible for "unused function" mappings, a definition must: // - Be function-like - // - Not participate directly in codegen + // - Not participate directly in codegen (or have lost all its coverage statements) // - Not have any coverage statements inlined into codegenned functions tcx.def_kind(def_id).is_fn_like() - && !usage.all_mono_items.contains(&def_id) + && (!usage.all_mono_items.contains(&def_id) + || usage.missing_own_coverage.contains(&def_id)) && !usage.used_via_inlining.contains(&def_id) }; @@ -379,6 +380,7 @@ fn add_unused_functions(cx: &CodegenCx<'_, '_>) { struct UsageSets<'tcx> { all_mono_items: &'tcx DefIdSet, used_via_inlining: FxHashSet, + missing_own_coverage: FxHashSet, } /// Prepare sets of definitions that are relevant to deciding whether something @@ -406,8 +408,13 @@ fn prepare_usage_sets<'tcx>(tcx: TyCtxt<'tcx>) -> UsageSets<'tcx> { // Functions whose coverage statments were found inlined into other functions. let mut used_via_inlining = FxHashSet::default(); + // Functions that were instrumented, but had all of their coverage statements + // removed by later MIR transforms (e.g. UnreachablePropagation). + let mut missing_own_coverage = FxHashSet::default(); + + for (def_id, body) in def_and_mir_for_all_mono_fns { + let mut saw_own_coverage = false; - for (_def_id, body) in def_and_mir_for_all_mono_fns { // Inspect every coverage statement in the function's MIR. for stmt in body .basic_blocks @@ -418,11 +425,18 @@ fn prepare_usage_sets<'tcx>(tcx: TyCtxt<'tcx>) -> UsageSets<'tcx> { if let Some(inlined) = stmt.source_info.scope.inlined_instance(&body.source_scopes) { // This coverage statement was inlined from another function. used_via_inlining.insert(inlined.def_id()); + } else { + // Non-inlined coverage statements belong to the enclosing function. + saw_own_coverage = true; } } + + if !saw_own_coverage && body.function_coverage_info.is_some() { + missing_own_coverage.insert(def_id); + } } - UsageSets { all_mono_items, used_via_inlining } + UsageSets { all_mono_items, used_via_inlining, missing_own_coverage } } fn add_unused_function_coverage<'tcx>( From 1cca2529d1c90458080be6cf271a92eedc49d399 Mon Sep 17 00:00:00 2001 From: Zalathar Date: Fri, 22 Mar 2024 13:34:26 +1100 Subject: [PATCH 12/14] coverage: Re-enable `UnreachablePropagation` for coverage builds --- compiler/rustc_mir_transform/src/unreachable_prop.rs | 6 +----- tests/coverage/unreachable.cov-map | 6 +++--- 2 files changed, 4 insertions(+), 8 deletions(-) diff --git a/compiler/rustc_mir_transform/src/unreachable_prop.rs b/compiler/rustc_mir_transform/src/unreachable_prop.rs index bff59aa602377..8ad7bc394c5ae 100644 --- a/compiler/rustc_mir_transform/src/unreachable_prop.rs +++ b/compiler/rustc_mir_transform/src/unreachable_prop.rs @@ -14,11 +14,7 @@ pub struct UnreachablePropagation; impl MirPass<'_> for UnreachablePropagation { fn is_enabled(&self, sess: &rustc_session::Session) -> bool { // Enable only under -Zmir-opt-level=2 as this can make programs less debuggable. - - // FIXME(#116171) Coverage gets confused by MIR passes that can remove all - // coverage statements from an instrumented function. This pass can be - // re-enabled when coverage codegen is robust against that happening. - sess.mir_opt_level() >= 2 && !sess.instrument_coverage() + sess.mir_opt_level() >= 2 } fn run_pass<'tcx>(&self, tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>) { diff --git a/tests/coverage/unreachable.cov-map b/tests/coverage/unreachable.cov-map index 55d124a16f592..9d4698527d092 100644 --- a/tests/coverage/unreachable.cov-map +++ b/tests/coverage/unreachable.cov-map @@ -14,11 +14,11 @@ Number of expressions: 0 Number of file 0 mappings: 1 - Code(Counter(0)) at (prev + 17, 1) to (start + 1, 37) -Function name: unreachable::unreachable_intrinsic -Raw bytes (9): 0x[01, 01, 00, 01, 01, 16, 01, 01, 2c] +Function name: unreachable::unreachable_intrinsic (unused) +Raw bytes (9): 0x[01, 01, 00, 01, 00, 16, 01, 01, 2c] Number of files: 1 - file 0 => global file 1 Number of expressions: 0 Number of file 0 mappings: 1 -- Code(Counter(0)) at (prev + 22, 1) to (start + 1, 44) +- Code(Zero) at (prev + 22, 1) to (start + 1, 44) From 2c0a8de0b91d0ea9dcab390cbff430329b1342b7 Mon Sep 17 00:00:00 2001 From: Matthew Maurer Date: Mon, 25 Mar 2024 02:42:04 +0000 Subject: [PATCH 13/14] CFI: Enable KCFI testing of run-pass tests This enables KCFI-based testing for all the CFI run-pass tests in the suite today. We can add the test header on top of in-flight CFI tests once they land. It also enables KCFI as a sanitizer for x86_64 and aarch64 Linux to make this possible. The sanitizer should likely be available for all aarch64, x86_64, and riscv targets, but that isn't critical for initial testing. --- .../src/spec/targets/aarch64_unknown_linux_gnu.rs | 1 + .../src/spec/targets/x86_64_unknown_linux_gnu.rs | 1 + src/tools/compiletest/src/header.rs | 1 + tests/ui/sanitizer/cfi-closure-fn-ptr-cast.rs | 10 +++++++--- tests/ui/sanitizer/cfi-complex-receiver.rs | 10 +++++++--- tests/ui/sanitizer/cfi-self-ref.rs | 10 +++++++--- tests/ui/sanitizer/cfi-virtual-auto.rs | 10 +++++++--- 7 files changed, 31 insertions(+), 12 deletions(-) diff --git a/compiler/rustc_target/src/spec/targets/aarch64_unknown_linux_gnu.rs b/compiler/rustc_target/src/spec/targets/aarch64_unknown_linux_gnu.rs index 2169e2971d885..703a3206af21f 100644 --- a/compiler/rustc_target/src/spec/targets/aarch64_unknown_linux_gnu.rs +++ b/compiler/rustc_target/src/spec/targets/aarch64_unknown_linux_gnu.rs @@ -19,6 +19,7 @@ pub fn target() -> Target { stack_probes: StackProbeType::Inline, supported_sanitizers: SanitizerSet::ADDRESS | SanitizerSet::CFI + | SanitizerSet::KCFI | SanitizerSet::LEAK | SanitizerSet::MEMORY | SanitizerSet::MEMTAG diff --git a/compiler/rustc_target/src/spec/targets/x86_64_unknown_linux_gnu.rs b/compiler/rustc_target/src/spec/targets/x86_64_unknown_linux_gnu.rs index 98374023dc57f..11fb28a9aed7f 100644 --- a/compiler/rustc_target/src/spec/targets/x86_64_unknown_linux_gnu.rs +++ b/compiler/rustc_target/src/spec/targets/x86_64_unknown_linux_gnu.rs @@ -10,6 +10,7 @@ pub fn target() -> Target { base.static_position_independent_executables = true; base.supported_sanitizers = SanitizerSet::ADDRESS | SanitizerSet::CFI + | SanitizerSet::KCFI | SanitizerSet::DATAFLOW | SanitizerSet::LEAK | SanitizerSet::MEMORY diff --git a/src/tools/compiletest/src/header.rs b/src/tools/compiletest/src/header.rs index 6ff47dbffbcba..45c330b34e2ca 100644 --- a/src/tools/compiletest/src/header.rs +++ b/src/tools/compiletest/src/header.rs @@ -826,6 +826,7 @@ const KNOWN_DIRECTIVE_NAMES: &[&str] = &[ "needs-sanitizer-cfi", "needs-sanitizer-dataflow", "needs-sanitizer-hwaddress", + "needs-sanitizer-kcfi", "needs-sanitizer-leak", "needs-sanitizer-memory", "needs-sanitizer-memtag", diff --git a/tests/ui/sanitizer/cfi-closure-fn-ptr-cast.rs b/tests/ui/sanitizer/cfi-closure-fn-ptr-cast.rs index a46a3afd7343a..1ae494d87d425 100644 --- a/tests/ui/sanitizer/cfi-closure-fn-ptr-cast.rs +++ b/tests/ui/sanitizer/cfi-closure-fn-ptr-cast.rs @@ -5,11 +5,15 @@ // // This checks that the reified function pointer will have the expected alias set at its call-site. -//@ needs-sanitizer-cfi +//@ revisions: cfi kcfi // FIXME(#122848) Remove only-linux once OSX CFI binaries work //@ only-linux -//@ compile-flags: --crate-type=bin -Cprefer-dynamic=off -Clto -Zsanitizer=cfi -//@ compile-flags: -C target-feature=-crt-static -C codegen-units=1 -C opt-level=0 +//@ [cfi] needs-sanitizer-cfi +//@ [kcfi] needs-sanitizer-kcfi +//@ compile-flags: -C target-feature=-crt-static +//@ [cfi] compile-flags: -C codegen-units=1 -C lto -C prefer-dynamic=off -C opt-level=0 +//@ [cfi] compile-flags: -Z sanitizer=cfi +//@ [kcfi] compile-flags: -Z sanitizer=kcfi //@ run-pass pub fn main() { diff --git a/tests/ui/sanitizer/cfi-complex-receiver.rs b/tests/ui/sanitizer/cfi-complex-receiver.rs index c3e59258db207..52095a384b25d 100644 --- a/tests/ui/sanitizer/cfi-complex-receiver.rs +++ b/tests/ui/sanitizer/cfi-complex-receiver.rs @@ -2,11 +2,15 @@ // * Arc as for custom receivers // * &dyn Bar for type constraints -//@ needs-sanitizer-cfi +//@ revisions: cfi kcfi // FIXME(#122848) Remove only-linux once OSX CFI binaries work //@ only-linux -//@ compile-flags: --crate-type=bin -Cprefer-dynamic=off -Clto -Zsanitizer=cfi -//@ compile-flags: -C target-feature=-crt-static -C codegen-units=1 -C opt-level=0 +//@ [cfi] needs-sanitizer-cfi +//@ [kcfi] needs-sanitizer-kcfi +//@ compile-flags: -C target-feature=-crt-static +//@ [cfi] compile-flags: -C codegen-units=1 -C lto -C prefer-dynamic=off -C opt-level=0 +//@ [cfi] compile-flags: -Z sanitizer=cfi +//@ [kcfi] compile-flags: -Z sanitizer=kcfi //@ run-pass use std::sync::Arc; diff --git a/tests/ui/sanitizer/cfi-self-ref.rs b/tests/ui/sanitizer/cfi-self-ref.rs index 32d0b10070265..f8793aec6e218 100644 --- a/tests/ui/sanitizer/cfi-self-ref.rs +++ b/tests/ui/sanitizer/cfi-self-ref.rs @@ -1,10 +1,14 @@ // Check that encoding self-referential types works with #[repr(transparent)] -//@ needs-sanitizer-cfi +//@ revisions: cfi kcfi // FIXME(#122848) Remove only-linux once OSX CFI binaries work //@ only-linux -//@ compile-flags: --crate-type=bin -Cprefer-dynamic=off -Clto -Zsanitizer=cfi -//@ compile-flags: -C target-feature=-crt-static -C codegen-units=1 -C opt-level=0 +//@ [cfi] needs-sanitizer-cfi +//@ [kcfi] needs-sanitizer-kcfi +//@ compile-flags: -C target-feature=-crt-static +//@ [cfi] compile-flags: -C codegen-units=1 -C lto -C prefer-dynamic=off -C opt-level=0 +//@ [cfi] compile-flags: -Z sanitizer=cfi +//@ [kcfi] compile-flags: -Z sanitizer=kcfi //@ run-pass use std::marker::PhantomData; diff --git a/tests/ui/sanitizer/cfi-virtual-auto.rs b/tests/ui/sanitizer/cfi-virtual-auto.rs index 7a0c246a41221..517c3d49f765a 100644 --- a/tests/ui/sanitizer/cfi-virtual-auto.rs +++ b/tests/ui/sanitizer/cfi-virtual-auto.rs @@ -1,10 +1,14 @@ // Tests that calling a trait object method on a trait object with additional auto traits works. -//@ needs-sanitizer-cfi +//@ revisions: cfi kcfi // FIXME(#122848) Remove only-linux once OSX CFI binaries work //@ only-linux -//@ compile-flags: --crate-type=bin -Cprefer-dynamic=off -Clto -Zsanitizer=cfi -//@ compile-flags: -C target-feature=-crt-static -C codegen-units=1 -C opt-level=0 +//@ [cfi] needs-sanitizer-cfi +//@ [kcfi] needs-sanitizer-kcfi +//@ compile-flags: -C target-feature=-crt-static +//@ [cfi] compile-flags: -C codegen-units=1 -C lto -C prefer-dynamic=off -C opt-level=0 +//@ [cfi] compile-flags: -Z sanitizer=cfi +//@ [kcfi] compile-flags: -Z sanitizer=kcfi //@ run-pass trait Foo { From 1942f956a3d8c92fb33d6841b7410e65c33b60dd Mon Sep 17 00:00:00 2001 From: chloekek Date: Wed, 27 Mar 2024 01:17:33 +0100 Subject: [PATCH 14/14] rustdoc: Swap fields and variant documentations Previously, the documentation for a variant appeared after the documentation for each of its fields. This was inconsistent with structs and unions, and made little sense on its own; fields are subordinate to variants and should therefore appear later in the documentation. --- src/librustdoc/html/render/print_item.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/librustdoc/html/render/print_item.rs b/src/librustdoc/html/render/print_item.rs index 5d4f1acc4b188..fbb521a6188ae 100644 --- a/src/librustdoc/html/render/print_item.rs +++ b/src/librustdoc/html/render/print_item.rs @@ -1728,6 +1728,8 @@ fn item_variants( } w.write_str(""); + write!(w, "{}", document(cx, variant, Some(it), HeadingOffset::H4)); + let heading_and_fields = match &variant_data.kind { clean::VariantKind::Struct(s) => { // If there is no field to display, no need to add the heading. @@ -1789,8 +1791,6 @@ fn item_variants( } w.write_str(""); } - - write!(w, "{}", document(cx, variant, Some(it), HeadingOffset::H4)); } write!(w, ""); }