From 8057586d3dca2f7bdda2096f4284c0323ed46b0b Mon Sep 17 00:00:00 2001 From: Tobias Bucher Date: Wed, 18 Oct 2023 14:59:53 +0200 Subject: [PATCH 01/14] Add discussion that concurrent access to the environment is unsafe The bug report #27970 has existed for 8 years, the actual bug dates back to Rust pre-1.0. I documented it since it's in the interest of the user to be aware of it. The note can be removed once #27970 is fixed. --- library/std/src/env.rs | 38 ++++++++++++++++++++++++++++---------- 1 file changed, 28 insertions(+), 10 deletions(-) diff --git a/library/std/src/env.rs b/library/std/src/env.rs index f67f6034d3412..53cbb80d3c042 100644 --- a/library/std/src/env.rs +++ b/library/std/src/env.rs @@ -313,17 +313,26 @@ impl Error for VarError { /// Sets the environment variable `key` to the value `value` for the currently running /// process. /// -/// Note that while concurrent access to environment variables is safe in Rust, -/// some platforms only expose inherently unsafe non-threadsafe APIs for -/// inspecting the environment. As a result, extra care needs to be taken when -/// auditing calls to unsafe external FFI functions to ensure that any external -/// environment accesses are properly synchronized with accesses in Rust. +/// Note that while concurrent access to environment variables ought to be safe +/// in Rust, some platforms only expose inherently unsafe non-threadsafe APIs +/// for inspecting the environment. As a result, using `set_var` or +/// `remove_var` in a multi-threaded Rust program can lead to undefined +/// behavior, for example in combination with DNS lookups from +/// [`std::net::ToSocketAddrs`]. This is a bug +/// ([rust#27970](https://github.com/rust-lang/rust/issues/27970)) and will be +/// fixed in a future version of Rust. Additionally, extra care needs to be +/// taken when auditing calls to unsafe external FFI functions to ensure that +/// any external environment accesses are properly synchronized with accesses +/// in Rust. Since Rust does not expose its environment lock directly, this +/// means that all accesses to the environment must go through Rust's [`var`]. /// /// Discussion of this unsafety on Unix may be found in: /// /// - [Austin Group Bugzilla](https://austingroupbugs.net/view.php?id=188) /// - [GNU C library Bugzilla](https://sourceware.org/bugzilla/show_bug.cgi?id=15607#c2) /// +/// [`std::net::ToSocketAddrs`]: crate::net::ToSocketAddrs +/// /// # Panics /// /// This function may panic if `key` is empty, contains an ASCII equals sign `'='` @@ -351,17 +360,26 @@ fn _set_var(key: &OsStr, value: &OsStr) { /// Removes an environment variable from the environment of the currently running process. /// -/// Note that while concurrent access to environment variables is safe in Rust, -/// some platforms only expose inherently unsafe non-threadsafe APIs for -/// inspecting the environment. As a result extra care needs to be taken when -/// auditing calls to unsafe external FFI functions to ensure that any external -/// environment accesses are properly synchronized with accesses in Rust. +/// Note that while concurrent access to environment variables ought to be safe +/// in Rust, some platforms only expose inherently unsafe non-threadsafe APIs +/// for inspecting the environment. As a result, using `set_var` or +/// `remove_var` in a multi-threaded Rust program can lead to undefined +/// behavior, for example in combination with DNS lookups from +/// [`std::net::ToSocketAddrs`]. This is a bug +/// ([rust#27970](https://github.com/rust-lang/rust/issues/27970)) and will be +/// fixed in a future version of Rust. Additionally, extra care needs to be +/// taken when auditing calls to unsafe external FFI functions to ensure that +/// any external environment accesses are properly synchronized with accesses +/// in Rust. Since Rust does not expose its environment lock directly, this +/// means that all accesses to the environment must go through Rust's [`var`]. /// /// Discussion of this unsafety on Unix may be found in: /// /// - [Austin Group Bugzilla](https://austingroupbugs.net/view.php?id=188) /// - [GNU C library Bugzilla](https://sourceware.org/bugzilla/show_bug.cgi?id=15607#c2) /// +/// [`std::net::ToSocketAddrs`]: crate::net::ToSocketAddrs +/// /// # Panics /// /// This function may panic if `key` is empty, contains an ASCII equals sign From b8ea6e686f996a158838e39c7515df449dcc55be Mon Sep 17 00:00:00 2001 From: Michael Goulet Date: Tue, 12 Dec 2023 18:54:49 +0000 Subject: [PATCH 02/14] Uplift ClosureKind --- compiler/rustc_errors/src/diagnostic_impls.rs | 6 ++ compiler/rustc_middle/src/ty/closure.rs | 73 +------------------ compiler/rustc_middle/src/ty/context.rs | 24 ++++++ compiler/rustc_middle/src/ty/mod.rs | 2 +- compiler/rustc_middle/src/ty/print/pretty.rs | 6 +- compiler/rustc_type_ir/src/interner.rs | 9 +++ compiler/rustc_type_ir/src/lib.rs | 63 ++++++++++++++++ 7 files changed, 107 insertions(+), 76 deletions(-) diff --git a/compiler/rustc_errors/src/diagnostic_impls.rs b/compiler/rustc_errors/src/diagnostic_impls.rs index 4f77f09b26ec4..558262460c755 100644 --- a/compiler/rustc_errors/src/diagnostic_impls.rs +++ b/compiler/rustc_errors/src/diagnostic_impls.rs @@ -378,3 +378,9 @@ pub struct IndicateAnonymousLifetime { pub count: usize, pub suggestion: String, } + +impl IntoDiagnosticArg for type_ir::ClosureKind { + fn into_diagnostic_arg(self) -> DiagnosticArgValue<'static> { + DiagnosticArgValue::Str(self.as_str().into()) + } +} diff --git a/compiler/rustc_middle/src/ty/closure.rs b/compiler/rustc_middle/src/ty/closure.rs index eaf5da130dd9c..8c29bc5a42865 100644 --- a/compiler/rustc_middle/src/ty/closure.rs +++ b/compiler/rustc_middle/src/ty/closure.rs @@ -7,14 +7,13 @@ use std::fmt::Write; use crate::query::Providers; use rustc_data_structures::fx::FxIndexMap; -use rustc_errors::{DiagnosticArgValue, IntoDiagnosticArg}; -use rustc_hir::def_id::{DefId, LocalDefId}; -use rustc_hir::{self as hir, LangItem}; +use rustc_hir as hir; +use rustc_hir::def_id::LocalDefId; use rustc_span::def_id::LocalDefIdMap; use rustc_span::symbol::Ident; use rustc_span::{Span, Symbol}; -use super::{Ty, TyCtxt}; +use super::TyCtxt; use self::BorrowKind::*; @@ -73,72 +72,6 @@ pub type RootVariableMinCaptureList<'tcx> = FxIndexMap = Vec>; -/// Represents the various closure traits in the language. This -/// will determine the type of the environment (`self`, in the -/// desugaring) argument that the closure expects. -/// -/// You can get the environment type of a closure using -/// `tcx.closure_env_ty()`. -#[derive(Clone, Copy, PartialOrd, Ord, PartialEq, Eq, Hash, Debug, TyEncodable, TyDecodable)] -#[derive(HashStable)] -pub enum ClosureKind { - // Warning: Ordering is significant here! The ordering is chosen - // because the trait Fn is a subtrait of FnMut and so in turn, and - // hence we order it so that Fn < FnMut < FnOnce. - Fn, - FnMut, - FnOnce, -} - -impl ClosureKind { - /// This is the initial value used when doing upvar inference. - pub const LATTICE_BOTTOM: ClosureKind = ClosureKind::Fn; - - pub const fn as_str(self) -> &'static str { - match self { - ClosureKind::Fn => "Fn", - ClosureKind::FnMut => "FnMut", - ClosureKind::FnOnce => "FnOnce", - } - } - - /// Returns `true` if a type that impls this closure kind - /// must also implement `other`. - pub fn extends(self, other: ty::ClosureKind) -> bool { - self <= other - } - - /// Converts `self` to a [`DefId`] of the corresponding trait. - /// - /// Note: the inverse of this function is [`TyCtxt::fn_trait_kind_from_def_id`]. - pub fn to_def_id(&self, tcx: TyCtxt<'_>) -> DefId { - tcx.require_lang_item( - match self { - ClosureKind::Fn => LangItem::Fn, - ClosureKind::FnMut => LangItem::FnMut, - ClosureKind::FnOnce => LangItem::FnOnce, - }, - None, - ) - } - - /// Returns the representative scalar type for this closure kind. - /// See `Ty::to_opt_closure_kind` for more details. - pub fn to_ty<'tcx>(self, tcx: TyCtxt<'tcx>) -> Ty<'tcx> { - match self { - ClosureKind::Fn => tcx.types.i8, - ClosureKind::FnMut => tcx.types.i16, - ClosureKind::FnOnce => tcx.types.i32, - } - } -} - -impl IntoDiagnosticArg for ClosureKind { - fn into_diagnostic_arg(self) -> DiagnosticArgValue<'static> { - DiagnosticArgValue::Str(self.as_str().into()) - } -} - /// A composite describing a `Place` that is captured by a closure. #[derive(PartialEq, Clone, Debug, TyEncodable, TyDecodable, HashStable)] #[derive(TypeFoldable, TypeVisitable)] diff --git a/compiler/rustc_middle/src/ty/context.rs b/compiler/rustc_middle/src/ty/context.rs index eb6fde83fcc8f..62aab72b1df1e 100644 --- a/compiler/rustc_middle/src/ty/context.rs +++ b/compiler/rustc_middle/src/ty/context.rs @@ -158,6 +158,30 @@ impl<'tcx> Interner for TyCtxt<'tcx> { ) -> Self::Const { Const::new_bound(self, debruijn, var, ty) } + + fn fn_def_id(self) -> Self::DefId { + self.require_lang_item(hir::LangItem::Fn, None) + } + + fn fn_mut_def_id(self) -> Self::DefId { + self.require_lang_item(hir::LangItem::FnMut, None) + } + + fn fn_once_def_id(self) -> Self::DefId { + self.require_lang_item(hir::LangItem::FnOnce, None) + } + + fn i8_type(self) -> Self::Ty { + self.types.i8 + } + + fn i16_type(self) -> Self::Ty { + self.types.i16 + } + + fn i32_type(self) -> Self::Ty { + self.types.i32 + } } type InternedSet<'tcx, T> = ShardedHashMap, ()>; diff --git a/compiler/rustc_middle/src/ty/mod.rs b/compiler/rustc_middle/src/ty/mod.rs index 71ff7021ca5de..a09a511538626 100644 --- a/compiler/rustc_middle/src/ty/mod.rs +++ b/compiler/rustc_middle/src/ty/mod.rs @@ -75,7 +75,7 @@ pub use self::binding::BindingMode; pub use self::binding::BindingMode::*; pub use self::closure::{ is_ancestor_or_same_capture, place_to_string_for_capture, BorrowKind, CaptureInfo, - CapturedPlace, ClosureKind, ClosureTypeInfo, MinCaptureInformationMap, MinCaptureList, + CapturedPlace, ClosureTypeInfo, MinCaptureInformationMap, MinCaptureList, RootVariableMinCaptureList, UpvarCapture, UpvarId, UpvarPath, CAPTURE_STRUCT_LOCAL, }; pub use self::consts::{Const, ConstData, ConstInt, Expr, ScalarInt, UnevaluatedConst, ValTree}; diff --git a/compiler/rustc_middle/src/ty/print/pretty.rs b/compiler/rustc_middle/src/ty/print/pretty.rs index 39adfac55cef3..39d031b5be70d 100644 --- a/compiler/rustc_middle/src/ty/print/pretty.rs +++ b/compiler/rustc_middle/src/ty/print/pretty.rs @@ -1680,7 +1680,7 @@ pub trait PrettyPrinter<'tcx>: Printer<'tcx> + fmt::Write { self.wrap_binder(&sig, |sig, cx| { define_scoped_cx!(cx); - p!(print(kind), "("); + p!(write("{kind}(")); for (i, arg) in sig.inputs()[0].tuple_fields().iter().enumerate() { if i > 0 { p!(", "); @@ -2943,10 +2943,6 @@ define_print_and_forward_display! { } } - ty::ClosureKind { - p!(write("{}", self.as_str())) - } - ty::Predicate<'tcx> { p!(print(self.kind())) } diff --git a/compiler/rustc_type_ir/src/interner.rs b/compiler/rustc_type_ir/src/interner.rs index c262302133bee..2e424fa11ec4a 100644 --- a/compiler/rustc_type_ir/src/interner.rs +++ b/compiler/rustc_type_ir/src/interner.rs @@ -89,6 +89,15 @@ pub trait Interner: Sized { fn mk_bound_ty(self, debruijn: DebruijnIndex, var: BoundVar) -> Self::Ty; fn mk_bound_region(self, debruijn: DebruijnIndex, var: BoundVar) -> Self::Region; fn mk_bound_const(self, debruijn: DebruijnIndex, var: BoundVar, ty: Self::Ty) -> Self::Const; + + // FIXME: these could be consolidated into some "WellKnownTraits" thing like chalk does. + fn fn_def_id(self) -> Self::DefId; + fn fn_mut_def_id(self) -> Self::DefId; + fn fn_once_def_id(self) -> Self::DefId; + + fn i8_type(self) -> Self::Ty; + fn i16_type(self) -> Self::Ty; + fn i32_type(self) -> Self::Ty; } /// Common capabilities of placeholder kinds diff --git a/compiler/rustc_type_ir/src/lib.rs b/compiler/rustc_type_ir/src/lib.rs index 200963ff7c5c8..f827969fb766a 100644 --- a/compiler/rustc_type_ir/src/lib.rs +++ b/compiler/rustc_type_ir/src/lib.rs @@ -359,3 +359,66 @@ rustc_index::newtype_index! { #[gate_rustc_only] pub struct BoundVar {} } + +/// Represents the various closure traits in the language. This +/// will determine the type of the environment (`self`, in the +/// desugaring) argument that the closure expects. +/// +/// You can get the environment type of a closure using +/// `tcx.closure_env_ty()`. +#[derive(Clone, Copy, PartialOrd, Ord, PartialEq, Eq, Hash, Debug)] +#[cfg_attr(feature = "nightly", derive(Encodable, Decodable, HashStable_NoContext))] +pub enum ClosureKind { + // Warning: Ordering is significant here! The ordering is chosen + // because the trait Fn is a subtrait of FnMut and so in turn, and + // hence we order it so that Fn < FnMut < FnOnce. + Fn, + FnMut, + FnOnce, +} + +impl ClosureKind { + /// This is the initial value used when doing upvar inference. + pub const LATTICE_BOTTOM: ClosureKind = ClosureKind::Fn; + + pub const fn as_str(self) -> &'static str { + match self { + ClosureKind::Fn => "Fn", + ClosureKind::FnMut => "FnMut", + ClosureKind::FnOnce => "FnOnce", + } + } + + /// Returns `true` if a type that impls this closure kind + /// must also implement `other`. + pub fn extends(self, other: ClosureKind) -> bool { + self <= other + } + + /// Converts `self` to a `DefId` of the corresponding trait. + /// + /// Note: the inverse of this function is `TyCtxt::fn_trait_kind_from_def_id`. + pub fn to_def_id(self, interner: I) -> I::DefId { + match self { + ClosureKind::Fn => interner.fn_def_id(), + ClosureKind::FnMut => interner.fn_mut_def_id(), + ClosureKind::FnOnce => interner.fn_once_def_id(), + } + } + + /// Returns the representative scalar type for this closure kind. + /// See `Ty::to_opt_closure_kind` for more details. + pub fn to_ty(self, interner: I) -> I::Ty { + match self { + ClosureKind::Fn => interner.i8_type(), + ClosureKind::FnMut => interner.i16_type(), + ClosureKind::FnOnce => interner.i32_type(), + } + } +} + +impl fmt::Display for ClosureKind { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + self.as_str().fmt(f) + } +} From 9f0849f9e059648ce18b996110276306863c5a6d Mon Sep 17 00:00:00 2001 From: Michael Goulet Date: Tue, 12 Dec 2023 19:07:19 +0000 Subject: [PATCH 03/14] Uplift TypeAndMut --- .../rustc_middle/src/mir/type_foldable.rs | 1 - compiler/rustc_middle/src/ty/context.rs | 9 +--- compiler/rustc_middle/src/ty/print/pretty.rs | 8 ++-- .../rustc_middle/src/ty/structural_impls.rs | 1 - compiler/rustc_middle/src/ty/sty.rs | 11 ++--- compiler/rustc_type_ir/src/interner.rs | 7 +-- compiler/rustc_type_ir/src/macros.rs | 2 + compiler/rustc_type_ir/src/ty_kind.rs | 46 +++++++++++++++++-- .../clippy_lints/src/casts/as_ptr_cast_mut.rs | 6 +-- 9 files changed, 58 insertions(+), 33 deletions(-) diff --git a/compiler/rustc_middle/src/mir/type_foldable.rs b/compiler/rustc_middle/src/mir/type_foldable.rs index ea6b44d62b0fe..93a9bbf64c926 100644 --- a/compiler/rustc_middle/src/mir/type_foldable.rs +++ b/compiler/rustc_middle/src/mir/type_foldable.rs @@ -15,7 +15,6 @@ TrivialTypeTraversalImpls! { UserTypeAnnotationIndex, BorrowKind, CastKind, - hir::Movability, BasicBlock, SwitchTargets, CoroutineKind, diff --git a/compiler/rustc_middle/src/ty/context.rs b/compiler/rustc_middle/src/ty/context.rs index 62aab72b1df1e..ea9dc76d3cb69 100644 --- a/compiler/rustc_middle/src/ty/context.rs +++ b/compiler/rustc_middle/src/ty/context.rs @@ -28,7 +28,7 @@ use crate::ty::{ self, AdtDef, AdtDefData, AdtKind, Binder, Clause, Const, ConstData, GenericParamDefKind, ImplPolarity, List, ParamConst, ParamTy, PolyExistentialPredicate, PolyFnSig, Predicate, PredicateKind, Region, RegionKind, ReprOptions, TraitObjectVisitor, Ty, TyKind, TyVid, - TypeAndMut, Visibility, + Visibility, }; use crate::ty::{GenericArg, GenericArgs, GenericArgsRef}; use rustc_ast::{self as ast, attr}; @@ -88,7 +88,6 @@ impl<'tcx> Interner for TyCtxt<'tcx> { type Term = ty::Term<'tcx>; type Binder = Binder<'tcx, T>; - type TypeAndMut = TypeAndMut<'tcx>; type CanonicalVars = CanonicalVarInfos<'tcx>; type Ty = Ty<'tcx>; @@ -128,12 +127,6 @@ impl<'tcx> Interner for TyCtxt<'tcx> { type CoercePredicate = ty::CoercePredicate<'tcx>; type ClosureKind = ty::ClosureKind; - fn ty_and_mut_to_parts( - TypeAndMut { ty, mutbl }: TypeAndMut<'tcx>, - ) -> (Self::Ty, ty::Mutability) { - (ty, mutbl) - } - fn mk_canonical_var_infos(self, infos: &[ty::CanonicalVarInfo]) -> Self::CanonicalVars { self.mk_canonical_var_infos(infos) } diff --git a/compiler/rustc_middle/src/ty/print/pretty.rs b/compiler/rustc_middle/src/ty/print/pretty.rs index 39d031b5be70d..8e045397b0ffa 100644 --- a/compiler/rustc_middle/src/ty/print/pretty.rs +++ b/compiler/rustc_middle/src/ty/print/pretty.rs @@ -2754,6 +2754,10 @@ forward_display_to_print! { define_print! { (self, cx): + ty::TypeAndMut<'tcx> { + p!(write("{}", self.mutbl.prefix_str()), print(self.ty)) + } + ty::ClauseKind<'tcx> { match *self { ty::ClauseKind::Trait(ref data) => { @@ -2799,10 +2803,6 @@ define_print_and_forward_display! { p!("{{", comma_sep(self.iter()), "}}") } - ty::TypeAndMut<'tcx> { - p!(write("{}", self.mutbl.prefix_str()), print(self.ty)) - } - ty::ExistentialTraitRef<'tcx> { // Use a type that can't appear in defaults of type parameters. let dummy_self = Ty::new_fresh(cx.tcx(),0); diff --git a/compiler/rustc_middle/src/ty/structural_impls.rs b/compiler/rustc_middle/src/ty/structural_impls.rs index 0cff6b77eb606..1c75d73e5528a 100644 --- a/compiler/rustc_middle/src/ty/structural_impls.rs +++ b/compiler/rustc_middle/src/ty/structural_impls.rs @@ -440,7 +440,6 @@ TrivialTypeTraversalImpls! { // interners). TrivialTypeTraversalAndLiftImpls! { ::rustc_hir::def_id::DefId, - ::rustc_hir::Mutability, ::rustc_hir::Unsafety, ::rustc_target::spec::abi::Abi, crate::ty::ClosureKind, diff --git a/compiler/rustc_middle/src/ty/sty.rs b/compiler/rustc_middle/src/ty/sty.rs index 50a1b85b16918..42753f53c7b06 100644 --- a/compiler/rustc_middle/src/ty/sty.rs +++ b/compiler/rustc_middle/src/ty/sty.rs @@ -42,23 +42,18 @@ use rustc_type_ir::PredicateKind as IrPredicateKind; use rustc_type_ir::RegionKind as IrRegionKind; use rustc_type_ir::TyKind as IrTyKind; use rustc_type_ir::TyKind::*; +use rustc_type_ir::TypeAndMut as IrTypeAndMut; use super::GenericParamDefKind; -// Re-export the `TyKind` from `rustc_type_ir` here for convenience +// Re-export and re-parameterize some `I = TyCtxt<'tcx>` types here #[rustc_diagnostic_item = "TyKind"] pub type TyKind<'tcx> = IrTyKind>; pub type RegionKind<'tcx> = IrRegionKind>; pub type ConstKind<'tcx> = IrConstKind>; pub type PredicateKind<'tcx> = IrPredicateKind>; pub type ClauseKind<'tcx> = IrClauseKind>; - -#[derive(Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash, Debug, TyEncodable, TyDecodable)] -#[derive(HashStable, TypeFoldable, TypeVisitable, Lift)] -pub struct TypeAndMut<'tcx> { - pub ty: Ty<'tcx>, - pub mutbl: hir::Mutability, -} +pub type TypeAndMut<'tcx> = IrTypeAndMut>; #[derive(Clone, PartialEq, PartialOrd, Eq, Ord, Hash, TyEncodable, TyDecodable, Copy)] #[derive(HashStable)] diff --git a/compiler/rustc_type_ir/src/interner.rs b/compiler/rustc_type_ir/src/interner.rs index 2e424fa11ec4a..0eccd2b4cf337 100644 --- a/compiler/rustc_type_ir/src/interner.rs +++ b/compiler/rustc_type_ir/src/interner.rs @@ -3,8 +3,8 @@ use std::fmt::Debug; use std::hash::Hash; use crate::{ - BoundVar, CanonicalVarInfo, ConstKind, DebruijnIndex, DebugWithInfcx, Mutability, RegionKind, - TyKind, UniverseIndex, + BoundVar, CanonicalVarInfo, ConstKind, DebruijnIndex, DebugWithInfcx, RegionKind, TyKind, + UniverseIndex, }; pub trait Interner: Sized { @@ -20,7 +20,6 @@ pub trait Interner: Sized { type Term: Copy + Debug + Hash + Ord; type Binder; - type TypeAndMut: Copy + Debug + Hash + Ord; type CanonicalVars: Copy + Debug + Hash + Eq + IntoIterator>; // Kinds of tys @@ -81,8 +80,6 @@ pub trait Interner: Sized { type CoercePredicate: Copy + Debug + Hash + Eq; type ClosureKind: Copy + Debug + Hash + Eq; - fn ty_and_mut_to_parts(ty_and_mut: Self::TypeAndMut) -> (Self::Ty, Mutability); - fn mk_canonical_var_infos(self, infos: &[CanonicalVarInfo]) -> Self::CanonicalVars; // FIXME: We should not have all these constructors on `Interner`, but as functions on some trait. diff --git a/compiler/rustc_type_ir/src/macros.rs b/compiler/rustc_type_ir/src/macros.rs index cfed84a35c671..82bb1bf291667 100644 --- a/compiler/rustc_type_ir/src/macros.rs +++ b/compiler/rustc_type_ir/src/macros.rs @@ -51,4 +51,6 @@ TrivialTypeTraversalImpls! { crate::DebruijnIndex, crate::AliasRelationDirection, crate::UniverseIndex, + crate::Mutability, + crate::Movability, } diff --git a/compiler/rustc_type_ir/src/ty_kind.rs b/compiler/rustc_type_ir/src/ty_kind.rs index 72ca9199a533b..70adfbee2edc6 100644 --- a/compiler/rustc_type_ir/src/ty_kind.rs +++ b/compiler/rustc_type_ir/src/ty_kind.rs @@ -4,6 +4,8 @@ use rustc_data_structures::stable_hasher::{HashStable, StableHasher}; use rustc_data_structures::unify::{EqUnifyValue, UnifyKey}; use std::fmt; +use crate::fold::{FallibleTypeFolder, TypeFoldable}; +use crate::visit::{TypeVisitable, TypeVisitor}; use crate::Interner; use crate::{DebruijnIndex, DebugWithInfcx, InferCtxtLike, WithInfcx}; @@ -158,7 +160,7 @@ pub enum TyKind { Slice(I::Ty), /// A raw pointer. Written as `*mut T` or `*const T` - RawPtr(I::TypeAndMut), + RawPtr(TypeAndMut), /// A reference; a pointer with an associated lifetime. Written as /// `&'a mut T` or `&'a T`. @@ -410,8 +412,7 @@ impl DebugWithInfcx for TyKind { Str => write!(f, "str"), Array(t, c) => write!(f, "[{:?}; {:?}]", &this.wrap(t), &this.wrap(c)), Slice(t) => write!(f, "[{:?}]", &this.wrap(t)), - RawPtr(p) => { - let (ty, mutbl) = I::ty_and_mut_to_parts(*p); + RawPtr(TypeAndMut { ty, mutbl }) => { match mutbl { Mutability::Mut => write!(f, "*mut "), Mutability::Not => write!(f, "*const "), @@ -831,3 +832,42 @@ impl DebugWithInfcx for InferTy { } } } + +#[derive(derivative::Derivative)] +#[derivative( + Clone(bound = ""), + Copy(bound = ""), + PartialOrd(bound = ""), + Ord(bound = ""), + PartialEq(bound = ""), + Eq(bound = ""), + Hash(bound = ""), + Debug(bound = "") +)] +#[cfg_attr(feature = "nightly", derive(TyEncodable, TyDecodable, HashStable_NoContext))] +pub struct TypeAndMut { + pub ty: I::Ty, + pub mutbl: Mutability, +} + +impl TypeFoldable for TypeAndMut +where + I::Ty: TypeFoldable, +{ + fn try_fold_with>(self, folder: &mut F) -> Result { + Ok(TypeAndMut { + ty: self.ty.try_fold_with(folder)?, + mutbl: self.mutbl.try_fold_with(folder)?, + }) + } +} + +impl TypeVisitable for TypeAndMut +where + I::Ty: TypeVisitable, +{ + fn visit_with>(&self, visitor: &mut V) -> std::ops::ControlFlow { + self.ty.visit_with(visitor)?; + self.mutbl.visit_with(visitor) + } +} diff --git a/src/tools/clippy/clippy_lints/src/casts/as_ptr_cast_mut.rs b/src/tools/clippy/clippy_lints/src/casts/as_ptr_cast_mut.rs index 55294f5f38646..b55cd8833b730 100644 --- a/src/tools/clippy/clippy_lints/src/casts/as_ptr_cast_mut.rs +++ b/src/tools/clippy/clippy_lints/src/casts/as_ptr_cast_mut.rs @@ -10,8 +10,8 @@ use super::AS_PTR_CAST_MUT; pub(super) fn check(cx: &LateContext<'_>, expr: &Expr<'_>, cast_expr: &Expr<'_>, cast_to: Ty<'_>) { if let ty::RawPtr( - ptrty @ TypeAndMut { - mutbl: Mutability::Mut, .. + TypeAndMut { + mutbl: Mutability::Mut, ty: ptrty, }, ) = cast_to.kind() && let ty::RawPtr(TypeAndMut { @@ -34,7 +34,7 @@ pub(super) fn check(cx: &LateContext<'_>, expr: &Expr<'_>, cast_expr: &Expr<'_>, cx, AS_PTR_CAST_MUT, expr.span, - &format!("casting the result of `as_ptr` to *{ptrty}"), + &format!("casting the result of `as_ptr` to *mut {ptrty}"), "replace with", format!("{recv}.as_mut_ptr()"), applicability, From 2093d0c58e4508012439a55d6c5a52929d16748f Mon Sep 17 00:00:00 2001 From: Tobias Bucher Date: Wed, 13 Dec 2023 12:49:38 +0100 Subject: [PATCH 04/14] Reformulate `std::env::{set,remove}_env` as safety note --- library/std/src/env.rs | 60 +++++++++++++++++++++++++----------------- 1 file changed, 36 insertions(+), 24 deletions(-) diff --git a/library/std/src/env.rs b/library/std/src/env.rs index 53cbb80d3c042..30ac0512348ca 100644 --- a/library/std/src/env.rs +++ b/library/std/src/env.rs @@ -313,18 +313,24 @@ impl Error for VarError { /// Sets the environment variable `key` to the value `value` for the currently running /// process. /// -/// Note that while concurrent access to environment variables ought to be safe -/// in Rust, some platforms only expose inherently unsafe non-threadsafe APIs -/// for inspecting the environment. As a result, using `set_var` or -/// `remove_var` in a multi-threaded Rust program can lead to undefined -/// behavior, for example in combination with DNS lookups from -/// [`std::net::ToSocketAddrs`]. This is a bug -/// ([rust#27970](https://github.com/rust-lang/rust/issues/27970)) and will be -/// fixed in a future version of Rust. Additionally, extra care needs to be -/// taken when auditing calls to unsafe external FFI functions to ensure that -/// any external environment accesses are properly synchronized with accesses -/// in Rust. Since Rust does not expose its environment lock directly, this -/// means that all accesses to the environment must go through Rust's [`var`]. +/// # Safety +/// +/// Even though this function is currently not marked as `unsafe`, it needs to +/// be because invoking it can cause undefined behaviour. The function will be +/// marked `unsafe` in a future version of Rust. This is tracked in +/// [rust#27970](https://github.com/rust-lang/rust/issues/27970). +/// +/// This function is safe to call in a single-threaded program. +/// +/// In multi-threaded programs, you must ensure that are no other threads +/// concurrently writing or *reading*(!) from the environment through functions +/// other than the ones in this module. You are responsible for figuring out +/// how to achieve this, but we strongly suggest not using `set_var` or +/// `remove_var` in multi-threaded programs at all. +/// +/// Most C libraries, including libc itself do not advertise which functions +/// read from the environment. Even functions from the Rust standard library do +/// that, e.g. for DNS lookups from [`std::net::ToSocketAddrs`]. /// /// Discussion of this unsafety on Unix may be found in: /// @@ -360,18 +366,24 @@ fn _set_var(key: &OsStr, value: &OsStr) { /// Removes an environment variable from the environment of the currently running process. /// -/// Note that while concurrent access to environment variables ought to be safe -/// in Rust, some platforms only expose inherently unsafe non-threadsafe APIs -/// for inspecting the environment. As a result, using `set_var` or -/// `remove_var` in a multi-threaded Rust program can lead to undefined -/// behavior, for example in combination with DNS lookups from -/// [`std::net::ToSocketAddrs`]. This is a bug -/// ([rust#27970](https://github.com/rust-lang/rust/issues/27970)) and will be -/// fixed in a future version of Rust. Additionally, extra care needs to be -/// taken when auditing calls to unsafe external FFI functions to ensure that -/// any external environment accesses are properly synchronized with accesses -/// in Rust. Since Rust does not expose its environment lock directly, this -/// means that all accesses to the environment must go through Rust's [`var`]. +/// # Safety +/// +/// Even though this function is currently not marked as `unsafe`, it needs to +/// be because invoking it can cause undefined behaviour. The function will be +/// marked `unsafe` in a future version of Rust. This is tracked in +/// [rust#27970](https://github.com/rust-lang/rust/issues/27970). +/// +/// This function is safe to call in a single-threaded program. +/// +/// In multi-threaded programs, you must ensure that are no other threads +/// concurrently writing or *reading*(!) from the environment through functions +/// other than the ones in this module. You are responsible for figuring out +/// how to achieve this, but we strongly suggest not using `set_var` or +/// `remove_var` in multi-threaded programs at all. +/// +/// Most C libraries, including libc itself do not advertise which functions +/// read from the environment. Even functions from the Rust standard library do +/// that, e.g. for DNS lookups from [`std::net::ToSocketAddrs`]. /// /// Discussion of this unsafety on Unix may be found in: /// From 742f193ef871aff89a5d818cdb1087f41c66f75d Mon Sep 17 00:00:00 2001 From: Michael Goulet Date: Thu, 14 Dec 2023 19:10:03 +0000 Subject: [PATCH 05/14] Move special methods from ClosureKind back into rustc --- compiler/rustc_hir_typeck/src/closure.rs | 2 +- .../rustc_hir_typeck/src/fn_ctxt/checks.rs | 2 +- compiler/rustc_hir_typeck/src/upvar.rs | 6 ++++- .../rustc_middle/src/middle/lang_items.rs | 11 +++++++++ compiler/rustc_middle/src/ty/context.rs | 24 ------------------- compiler/rustc_middle/src/ty/sty.rs | 9 +++++++ compiler/rustc_type_ir/src/interner.rs | 9 ------- compiler/rustc_type_ir/src/lib.rs | 21 ---------------- 8 files changed, 27 insertions(+), 57 deletions(-) diff --git a/compiler/rustc_hir_typeck/src/closure.rs b/compiler/rustc_hir_typeck/src/closure.rs index f0bb18df48c94..4662ee5825e2e 100644 --- a/compiler/rustc_hir_typeck/src/closure.rs +++ b/compiler/rustc_hir_typeck/src/closure.rs @@ -141,7 +141,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { debug!(?sig, ?opt_kind); let closure_kind_ty = match opt_kind { - Some(kind) => kind.to_ty(self.tcx), + Some(kind) => Ty::from_closure_kind(self.tcx, kind), // Create a type variable (for now) to represent the closure kind. // It will be unified during the upvar inference phase (`upvar.rs`) diff --git a/compiler/rustc_hir_typeck/src/fn_ctxt/checks.rs b/compiler/rustc_hir_typeck/src/fn_ctxt/checks.rs index bb9b849f03bc7..9ba61335e6817 100644 --- a/compiler/rustc_hir_typeck/src/fn_ctxt/checks.rs +++ b/compiler/rustc_hir_typeck/src/fn_ctxt/checks.rs @@ -2018,7 +2018,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { let new_def_id = self.probe(|_| { let trait_ref = ty::TraitRef::new( self.tcx, - call_kind.to_def_id(self.tcx), + self.tcx.fn_trait_kind_to_def_id(call_kind)?, [ callee_ty, self.next_ty_var(TypeVariableOrigin { diff --git a/compiler/rustc_hir_typeck/src/upvar.rs b/compiler/rustc_hir_typeck/src/upvar.rs index c0a5818b9e5f9..ebdd74fbc388c 100644 --- a/compiler/rustc_hir_typeck/src/upvar.rs +++ b/compiler/rustc_hir_typeck/src/upvar.rs @@ -261,7 +261,11 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { // Unify the (as yet unbound) type variable in the closure // args with the kind we inferred. let closure_kind_ty = closure_args.as_closure().kind_ty(); - self.demand_eqtype(span, closure_kind.to_ty(self.tcx), closure_kind_ty); + self.demand_eqtype( + span, + Ty::from_closure_kind(self.tcx, closure_kind), + closure_kind_ty, + ); // If we have an origin, store it. if let Some(mut origin) = origin { diff --git a/compiler/rustc_middle/src/middle/lang_items.rs b/compiler/rustc_middle/src/middle/lang_items.rs index 9a633e04ce70d..2899e629d7914 100644 --- a/compiler/rustc_middle/src/middle/lang_items.rs +++ b/compiler/rustc_middle/src/middle/lang_items.rs @@ -36,6 +36,17 @@ impl<'tcx> TyCtxt<'tcx> { } } + /// Given a [`ty::ClosureKind`], get the [`DefId`] of its corresponding `Fn`-family + /// trait, if it is defined. + pub fn fn_trait_kind_to_def_id(self, kind: ty::ClosureKind) -> Option { + let items = self.lang_items(); + match kind { + ty::ClosureKind::Fn => items.fn_trait(), + ty::ClosureKind::FnMut => items.fn_mut_trait(), + ty::ClosureKind::FnOnce => items.fn_once_trait(), + } + } + /// Returns `true` if `id` is a `DefId` of [`Fn`], [`FnMut`] or [`FnOnce`] traits. pub fn is_fn_trait(self, id: DefId) -> bool { self.fn_trait_kind_from_def_id(id).is_some() diff --git a/compiler/rustc_middle/src/ty/context.rs b/compiler/rustc_middle/src/ty/context.rs index ea9dc76d3cb69..fd3058472bb54 100644 --- a/compiler/rustc_middle/src/ty/context.rs +++ b/compiler/rustc_middle/src/ty/context.rs @@ -151,30 +151,6 @@ impl<'tcx> Interner for TyCtxt<'tcx> { ) -> Self::Const { Const::new_bound(self, debruijn, var, ty) } - - fn fn_def_id(self) -> Self::DefId { - self.require_lang_item(hir::LangItem::Fn, None) - } - - fn fn_mut_def_id(self) -> Self::DefId { - self.require_lang_item(hir::LangItem::FnMut, None) - } - - fn fn_once_def_id(self) -> Self::DefId { - self.require_lang_item(hir::LangItem::FnOnce, None) - } - - fn i8_type(self) -> Self::Ty { - self.types.i8 - } - - fn i16_type(self) -> Self::Ty { - self.types.i16 - } - - fn i32_type(self) -> Self::Ty { - self.types.i32 - } } type InternedSet<'tcx, T> = ShardedHashMap, ()>; diff --git a/compiler/rustc_middle/src/ty/sty.rs b/compiler/rustc_middle/src/ty/sty.rs index 42753f53c7b06..09c90d80e975e 100644 --- a/compiler/rustc_middle/src/ty/sty.rs +++ b/compiler/rustc_middle/src/ty/sty.rs @@ -2811,6 +2811,15 @@ impl<'tcx> Ty<'tcx> { } } + /// Inverse of [`Ty::to_opt_closure_kind`]. + pub fn from_closure_kind(tcx: TyCtxt<'tcx>, kind: ty::ClosureKind) -> Ty<'tcx> { + match kind { + ty::ClosureKind::Fn => tcx.types.i8, + ty::ClosureKind::FnMut => tcx.types.i16, + ty::ClosureKind::FnOnce => tcx.types.i32, + } + } + /// Fast path helper for testing if a type is `Sized`. /// /// Returning true means the type is known to be sized. Returning diff --git a/compiler/rustc_type_ir/src/interner.rs b/compiler/rustc_type_ir/src/interner.rs index 0eccd2b4cf337..188910ecc52d1 100644 --- a/compiler/rustc_type_ir/src/interner.rs +++ b/compiler/rustc_type_ir/src/interner.rs @@ -86,15 +86,6 @@ pub trait Interner: Sized { fn mk_bound_ty(self, debruijn: DebruijnIndex, var: BoundVar) -> Self::Ty; fn mk_bound_region(self, debruijn: DebruijnIndex, var: BoundVar) -> Self::Region; fn mk_bound_const(self, debruijn: DebruijnIndex, var: BoundVar, ty: Self::Ty) -> Self::Const; - - // FIXME: these could be consolidated into some "WellKnownTraits" thing like chalk does. - fn fn_def_id(self) -> Self::DefId; - fn fn_mut_def_id(self) -> Self::DefId; - fn fn_once_def_id(self) -> Self::DefId; - - fn i8_type(self) -> Self::Ty; - fn i16_type(self) -> Self::Ty; - fn i32_type(self) -> Self::Ty; } /// Common capabilities of placeholder kinds diff --git a/compiler/rustc_type_ir/src/lib.rs b/compiler/rustc_type_ir/src/lib.rs index f827969fb766a..bff938596458a 100644 --- a/compiler/rustc_type_ir/src/lib.rs +++ b/compiler/rustc_type_ir/src/lib.rs @@ -394,27 +394,6 @@ impl ClosureKind { pub fn extends(self, other: ClosureKind) -> bool { self <= other } - - /// Converts `self` to a `DefId` of the corresponding trait. - /// - /// Note: the inverse of this function is `TyCtxt::fn_trait_kind_from_def_id`. - pub fn to_def_id(self, interner: I) -> I::DefId { - match self { - ClosureKind::Fn => interner.fn_def_id(), - ClosureKind::FnMut => interner.fn_mut_def_id(), - ClosureKind::FnOnce => interner.fn_once_def_id(), - } - } - - /// Returns the representative scalar type for this closure kind. - /// See `Ty::to_opt_closure_kind` for more details. - pub fn to_ty(self, interner: I) -> I::Ty { - match self { - ClosureKind::Fn => interner.i8_type(), - ClosureKind::FnMut => interner.i16_type(), - ClosureKind::FnOnce => interner.i32_type(), - } - } } impl fmt::Display for ClosureKind { From 87cffb237774010e5761a07d884419e8993b2f45 Mon Sep 17 00:00:00 2001 From: Zalathar Date: Sat, 18 Nov 2023 17:51:17 +1100 Subject: [PATCH 06/14] coverage: Assert that the instrumentor never sees promoted MIR --- compiler/rustc_mir_transform/src/coverage/mod.rs | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/compiler/rustc_mir_transform/src/coverage/mod.rs b/compiler/rustc_mir_transform/src/coverage/mod.rs index 580cbf7a3f80a..d3a36a9511e21 100644 --- a/compiler/rustc_mir_transform/src/coverage/mod.rs +++ b/compiler/rustc_mir_transform/src/coverage/mod.rs @@ -39,15 +39,9 @@ impl<'tcx> MirPass<'tcx> for InstrumentCoverage { fn run_pass(&self, tcx: TyCtxt<'tcx>, mir_body: &mut mir::Body<'tcx>) { let mir_source = mir_body.source; - // If the InstrumentCoverage pass is called on promoted MIRs, skip them. - // See: https://github.com/rust-lang/rust/pull/73011#discussion_r438317601 - if mir_source.promoted.is_some() { - trace!( - "InstrumentCoverage skipped for {:?} (already promoted for Miri evaluation)", - mir_source.def_id() - ); - return; - } + // This pass runs after MIR promotion, but before promoted MIR starts to + // be transformed, so it should never see promoted MIR. + assert!(mir_source.promoted.is_none()); let is_fn_like = tcx.hir_node_by_def_id(mir_source.def_id().expect_local()).fn_kind().is_some(); From 315c0cf358b69fb2dd799025f682404a9a083d62 Mon Sep 17 00:00:00 2001 From: Zalathar Date: Thu, 14 Dec 2023 10:47:10 +1100 Subject: [PATCH 07/14] coverage: Simplify parts of `InstrumentCoverage::run_pass` Changes in this patch: - Extract local variable `def_id` - Check `is_fn_like` without retrieving HIR - Inline some locals that are used once and aren't needed for clarity --- compiler/rustc_mir_transform/src/coverage/mod.rs | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/compiler/rustc_mir_transform/src/coverage/mod.rs b/compiler/rustc_mir_transform/src/coverage/mod.rs index d3a36a9511e21..fbd9adeb2a41d 100644 --- a/compiler/rustc_mir_transform/src/coverage/mod.rs +++ b/compiler/rustc_mir_transform/src/coverage/mod.rs @@ -43,8 +43,7 @@ impl<'tcx> MirPass<'tcx> for InstrumentCoverage { // be transformed, so it should never see promoted MIR. assert!(mir_source.promoted.is_none()); - let is_fn_like = - tcx.hir_node_by_def_id(mir_source.def_id().expect_local()).fn_kind().is_some(); + let def_id = mir_source.def_id().expect_local(); // Only instrument functions, methods, and closures (not constants since they are evaluated // at compile time by Miri). @@ -53,8 +52,8 @@ impl<'tcx> MirPass<'tcx> for InstrumentCoverage { // expressions from coverage spans in enclosing MIR's, like we do for closures. (That might // be tricky if const expressions have no corresponding statements in the enclosing MIR. // Closures are carved out by their initial `Assign` statement.) - if !is_fn_like { - trace!("InstrumentCoverage skipped for {:?} (not an fn-like)", mir_source.def_id()); + if !tcx.def_kind(def_id).is_fn_like() { + trace!("InstrumentCoverage skipped for {def_id:?} (not an fn-like)"); return; } @@ -66,14 +65,13 @@ impl<'tcx> MirPass<'tcx> for InstrumentCoverage { _ => {} } - let codegen_fn_attrs = tcx.codegen_fn_attrs(mir_source.def_id()); - if codegen_fn_attrs.flags.contains(CodegenFnAttrFlags::NO_COVERAGE) { + if tcx.codegen_fn_attrs(def_id).flags.contains(CodegenFnAttrFlags::NO_COVERAGE) { return; } - trace!("InstrumentCoverage starting for {:?}", mir_source.def_id()); + trace!("InstrumentCoverage starting for {def_id:?}"); Instrumentor::new(tcx, mir_body).inject_counters(); - trace!("InstrumentCoverage done for {:?}", mir_source.def_id()); + trace!("InstrumentCoverage done for {def_id:?}"); } } From 3d5d5b7ef89da99bebb976f14cba226246f10306 Mon Sep 17 00:00:00 2001 From: Zalathar Date: Thu, 14 Dec 2023 11:02:55 +1100 Subject: [PATCH 08/14] coverage: Extract `is_eligible_for_coverage` --- .../rustc_mir_transform/src/coverage/mod.rs | 39 ++++++++++++------- 1 file changed, 25 insertions(+), 14 deletions(-) diff --git a/compiler/rustc_mir_transform/src/coverage/mod.rs b/compiler/rustc_mir_transform/src/coverage/mod.rs index fbd9adeb2a41d..75e3f40689c17 100644 --- a/compiler/rustc_mir_transform/src/coverage/mod.rs +++ b/compiler/rustc_mir_transform/src/coverage/mod.rs @@ -22,7 +22,7 @@ use rustc_middle::mir::{ TerminatorKind, }; use rustc_middle::ty::TyCtxt; -use rustc_span::def_id::DefId; +use rustc_span::def_id::{DefId, LocalDefId}; use rustc_span::source_map::SourceMap; use rustc_span::{ExpnKind, SourceFile, Span, Symbol}; @@ -45,18 +45,13 @@ impl<'tcx> MirPass<'tcx> for InstrumentCoverage { let def_id = mir_source.def_id().expect_local(); - // Only instrument functions, methods, and closures (not constants since they are evaluated - // at compile time by Miri). - // FIXME(#73156): Handle source code coverage in const eval, but note, if and when const - // expressions get coverage spans, we will probably have to "carve out" space for const - // expressions from coverage spans in enclosing MIR's, like we do for closures. (That might - // be tricky if const expressions have no corresponding statements in the enclosing MIR. - // Closures are carved out by their initial `Assign` statement.) - if !tcx.def_kind(def_id).is_fn_like() { - trace!("InstrumentCoverage skipped for {def_id:?} (not an fn-like)"); + if !is_eligible_for_coverage(tcx, def_id) { + trace!("InstrumentCoverage skipped for {def_id:?} (not eligible)"); return; } + // An otherwise-eligible function is still skipped if its start block + // is known to be unreachable. match mir_body.basic_blocks[mir::START_BLOCK].terminator().kind { TerminatorKind::Unreachable => { trace!("InstrumentCoverage skipped for unreachable `START_BLOCK`"); @@ -65,10 +60,6 @@ impl<'tcx> MirPass<'tcx> for InstrumentCoverage { _ => {} } - if tcx.codegen_fn_attrs(def_id).flags.contains(CodegenFnAttrFlags::NO_COVERAGE) { - return; - } - trace!("InstrumentCoverage starting for {def_id:?}"); Instrumentor::new(tcx, mir_body).inject_counters(); trace!("InstrumentCoverage done for {def_id:?}"); @@ -317,6 +308,26 @@ fn make_code_region( } } +fn is_eligible_for_coverage(tcx: TyCtxt<'_>, def_id: LocalDefId) -> bool { + // Only instrument functions, methods, and closures (not constants since they are evaluated + // at compile time by Miri). + // FIXME(#73156): Handle source code coverage in const eval, but note, if and when const + // expressions get coverage spans, we will probably have to "carve out" space for const + // expressions from coverage spans in enclosing MIR's, like we do for closures. (That might + // be tricky if const expressions have no corresponding statements in the enclosing MIR. + // Closures are carved out by their initial `Assign` statement.) + if !tcx.def_kind(def_id).is_fn_like() { + trace!("InstrumentCoverage skipped for {def_id:?} (not an fn-like)"); + return false; + } + + if tcx.codegen_fn_attrs(def_id).flags.contains(CodegenFnAttrFlags::NO_COVERAGE) { + return false; + } + + true +} + fn fn_sig_and_body( tcx: TyCtxt<'_>, def_id: DefId, From bf424c28d2fd9aa7480479c48b6414d6b07c8ebc Mon Sep 17 00:00:00 2001 From: Zalathar Date: Thu, 14 Dec 2023 11:55:35 +1100 Subject: [PATCH 09/14] coverage: Don't bother storing the source file in `Instrumentor` We can just as easily look it up again from the source map and body span when needed. --- compiler/rustc_mir_transform/src/coverage/mod.rs | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/compiler/rustc_mir_transform/src/coverage/mod.rs b/compiler/rustc_mir_transform/src/coverage/mod.rs index 75e3f40689c17..6c1dc243dea4b 100644 --- a/compiler/rustc_mir_transform/src/coverage/mod.rs +++ b/compiler/rustc_mir_transform/src/coverage/mod.rs @@ -24,7 +24,7 @@ use rustc_middle::mir::{ use rustc_middle::ty::TyCtxt; use rustc_span::def_id::{DefId, LocalDefId}; use rustc_span::source_map::SourceMap; -use rustc_span::{ExpnKind, SourceFile, Span, Symbol}; +use rustc_span::{ExpnKind, Span, Symbol}; /// Inserts `StatementKind::Coverage` statements that either instrument the binary with injected /// counters, via intrinsic `llvm.instrprof.increment`, and/or inject metadata used during codegen @@ -69,7 +69,6 @@ impl<'tcx> MirPass<'tcx> for InstrumentCoverage { struct Instrumentor<'a, 'tcx> { tcx: TyCtxt<'tcx>, mir_body: &'a mut mir::Body<'tcx>, - source_file: Lrc, fn_sig_span: Span, body_span: Span, function_source_hash: u64, @@ -109,7 +108,6 @@ impl<'a, 'tcx> Instrumentor<'a, 'tcx> { Self { tcx, mir_body, - source_file, fn_sig_span, body_span, function_source_hash, @@ -160,9 +158,10 @@ impl<'a, 'tcx> Instrumentor<'a, 'tcx> { let source_map = self.tcx.sess.source_map(); let body_span = self.body_span; + let source_file = source_map.lookup_source_file(body_span.lo()); use rustc_session::RemapFileNameExt; let file_name = - Symbol::intern(&self.source_file.name.for_codegen(self.tcx.sess).to_string_lossy()); + Symbol::intern(&source_file.name.for_codegen(self.tcx.sess).to_string_lossy()); let mut mappings = Vec::new(); From b9955fb340455fdd20244e5e783b88fec8150b5e Mon Sep 17 00:00:00 2001 From: Zalathar Date: Thu, 14 Dec 2023 12:20:34 +1100 Subject: [PATCH 10/14] coverage: Extract helper for getting HIR info for coverage --- .../rustc_mir_transform/src/coverage/mod.rs | 63 ++++++++++--------- 1 file changed, 35 insertions(+), 28 deletions(-) diff --git a/compiler/rustc_mir_transform/src/coverage/mod.rs b/compiler/rustc_mir_transform/src/coverage/mod.rs index 6c1dc243dea4b..d543d608740a4 100644 --- a/compiler/rustc_mir_transform/src/coverage/mod.rs +++ b/compiler/rustc_mir_transform/src/coverage/mod.rs @@ -78,30 +78,11 @@ struct Instrumentor<'a, 'tcx> { impl<'a, 'tcx> Instrumentor<'a, 'tcx> { fn new(tcx: TyCtxt<'tcx>, mir_body: &'a mut mir::Body<'tcx>) -> Self { - let source_map = tcx.sess.source_map(); - let def_id = mir_body.source.def_id(); - let (some_fn_sig, hir_body) = fn_sig_and_body(tcx, def_id); + let hir_info @ ExtractedHirInfo { function_source_hash, fn_sig_span, body_span } = + extract_hir_info(tcx, mir_body); - let body_span = get_body_span(tcx, hir_body, mir_body); + debug!(?hir_info, "instrumenting {:?}", mir_body.source.def_id()); - let source_file = source_map.lookup_source_file(body_span.lo()); - let fn_sig_span = match some_fn_sig.filter(|fn_sig| { - fn_sig.span.eq_ctxt(body_span) - && Lrc::ptr_eq(&source_file, &source_map.lookup_source_file(fn_sig.span.lo())) - }) { - Some(fn_sig) => fn_sig.span.with_hi(body_span.lo()), - None => body_span.shrink_to_lo(), - }; - - debug!( - "instrumenting {}: {:?}, fn sig span: {:?}, body span: {:?}", - if tcx.is_closure(def_id) { "closure" } else { "function" }, - def_id, - fn_sig_span, - body_span - ); - - let function_source_hash = hash_mir_source(tcx, hir_body); let basic_coverage_blocks = CoverageGraph::from_mir(mir_body); let coverage_counters = CoverageCounters::new(&basic_coverage_blocks); @@ -117,15 +98,12 @@ impl<'a, 'tcx> Instrumentor<'a, 'tcx> { } fn inject_counters(&'a mut self) { - let fn_sig_span = self.fn_sig_span; - let body_span = self.body_span; - //////////////////////////////////////////////////// // Compute coverage spans from the `CoverageGraph`. let coverage_spans = CoverageSpans::generate_coverage_spans( self.mir_body, - fn_sig_span, - body_span, + self.fn_sig_span, + self.body_span, &self.basic_coverage_blocks, ); @@ -327,6 +305,35 @@ fn is_eligible_for_coverage(tcx: TyCtxt<'_>, def_id: LocalDefId) -> bool { true } +/// Function information extracted from HIR by the coverage instrumentor. +#[derive(Debug)] +struct ExtractedHirInfo { + function_source_hash: u64, + fn_sig_span: Span, + body_span: Span, +} + +fn extract_hir_info<'tcx>(tcx: TyCtxt<'tcx>, mir_body: &mir::Body<'tcx>) -> ExtractedHirInfo { + let source_map = tcx.sess.source_map(); + let def_id = mir_body.source.def_id(); + let (some_fn_sig, hir_body) = fn_sig_and_body(tcx, def_id); + + let body_span = get_body_span(tcx, hir_body, mir_body); + + let source_file = source_map.lookup_source_file(body_span.lo()); + let fn_sig_span = match some_fn_sig.filter(|fn_sig| { + fn_sig.span.eq_ctxt(body_span) + && Lrc::ptr_eq(&source_file, &source_map.lookup_source_file(fn_sig.span.lo())) + }) { + Some(fn_sig) => fn_sig.span.with_hi(body_span.lo()), + None => body_span.shrink_to_lo(), + }; + + let function_source_hash = hash_mir_source(tcx, hir_body); + + ExtractedHirInfo { function_source_hash, fn_sig_span, body_span } +} + fn fn_sig_and_body( tcx: TyCtxt<'_>, def_id: DefId, @@ -342,7 +349,7 @@ fn fn_sig_and_body( fn get_body_span<'tcx>( tcx: TyCtxt<'tcx>, hir_body: &rustc_hir::Body<'tcx>, - mir_body: &mut mir::Body<'tcx>, + mir_body: &mir::Body<'tcx>, ) -> Span { let mut body_span = hir_body.value.span; let def_id = mir_body.source.def_id(); From e2f449bcc975464d8cabf24ea6fe3219037ea361 Mon Sep 17 00:00:00 2001 From: Zalathar Date: Thu, 14 Dec 2023 12:30:39 +1100 Subject: [PATCH 11/14] coverage: Use `LocalDefId` in `extract_hir_info` --- .../rustc_mir_transform/src/coverage/mod.rs | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/compiler/rustc_mir_transform/src/coverage/mod.rs b/compiler/rustc_mir_transform/src/coverage/mod.rs index d543d608740a4..d7fdde209bdca 100644 --- a/compiler/rustc_mir_transform/src/coverage/mod.rs +++ b/compiler/rustc_mir_transform/src/coverage/mod.rs @@ -22,7 +22,7 @@ use rustc_middle::mir::{ TerminatorKind, }; use rustc_middle::ty::TyCtxt; -use rustc_span::def_id::{DefId, LocalDefId}; +use rustc_span::def_id::LocalDefId; use rustc_span::source_map::SourceMap; use rustc_span::{ExpnKind, Span, Symbol}; @@ -79,7 +79,7 @@ struct Instrumentor<'a, 'tcx> { impl<'a, 'tcx> Instrumentor<'a, 'tcx> { fn new(tcx: TyCtxt<'tcx>, mir_body: &'a mut mir::Body<'tcx>) -> Self { let hir_info @ ExtractedHirInfo { function_source_hash, fn_sig_span, body_span } = - extract_hir_info(tcx, mir_body); + extract_hir_info(tcx, mir_body.source.def_id().expect_local()); debug!(?hir_info, "instrumenting {:?}", mir_body.source.def_id()); @@ -313,12 +313,11 @@ struct ExtractedHirInfo { body_span: Span, } -fn extract_hir_info<'tcx>(tcx: TyCtxt<'tcx>, mir_body: &mir::Body<'tcx>) -> ExtractedHirInfo { +fn extract_hir_info<'tcx>(tcx: TyCtxt<'tcx>, def_id: LocalDefId) -> ExtractedHirInfo { let source_map = tcx.sess.source_map(); - let def_id = mir_body.source.def_id(); let (some_fn_sig, hir_body) = fn_sig_and_body(tcx, def_id); - let body_span = get_body_span(tcx, hir_body, mir_body); + let body_span = get_body_span(tcx, hir_body, def_id); let source_file = source_map.lookup_source_file(body_span.lo()); let fn_sig_span = match some_fn_sig.filter(|fn_sig| { @@ -336,11 +335,11 @@ fn extract_hir_info<'tcx>(tcx: TyCtxt<'tcx>, mir_body: &mir::Body<'tcx>) -> Extr fn fn_sig_and_body( tcx: TyCtxt<'_>, - def_id: DefId, + def_id: LocalDefId, ) -> (Option<&rustc_hir::FnSig<'_>>, &rustc_hir::Body<'_>) { // FIXME(#79625): Consider improving MIR to provide the information needed, to avoid going back // to HIR for it. - let hir_node = tcx.hir().get_if_local(def_id).expect("expected DefId is local"); + let hir_node = tcx.hir_node_by_def_id(def_id); let (_, fn_body_id) = hir::map::associated_body(hir_node).expect("HIR node is a function with body"); (hir_node.fn_sig(), tcx.hir().body(fn_body_id)) @@ -349,12 +348,11 @@ fn fn_sig_and_body( fn get_body_span<'tcx>( tcx: TyCtxt<'tcx>, hir_body: &rustc_hir::Body<'tcx>, - mir_body: &mir::Body<'tcx>, + def_id: LocalDefId, ) -> Span { let mut body_span = hir_body.value.span; - let def_id = mir_body.source.def_id(); - if tcx.is_closure(def_id) { + if tcx.is_closure(def_id.to_def_id()) { // If the MIR function is a closure, and if the closure body span // starts from a macro, but it's content is not in that macro, try // to find a non-macro callsite, and instrument the spans there From 7de2156bfda6a2c443486a7d469a56c75284e320 Mon Sep 17 00:00:00 2001 From: Zalathar Date: Thu, 14 Dec 2023 12:58:10 +1100 Subject: [PATCH 12/14] coverage: Inline and simplify `fn_sig_and_body` --- .../rustc_mir_transform/src/coverage/mod.rs | 23 ++++++++----------- 1 file changed, 9 insertions(+), 14 deletions(-) diff --git a/compiler/rustc_mir_transform/src/coverage/mod.rs b/compiler/rustc_mir_transform/src/coverage/mod.rs index d7fdde209bdca..76a23fdccd4cc 100644 --- a/compiler/rustc_mir_transform/src/coverage/mod.rs +++ b/compiler/rustc_mir_transform/src/coverage/mod.rs @@ -314,13 +314,20 @@ struct ExtractedHirInfo { } fn extract_hir_info<'tcx>(tcx: TyCtxt<'tcx>, def_id: LocalDefId) -> ExtractedHirInfo { + // FIXME(#79625): Consider improving MIR to provide the information needed, to avoid going back + // to HIR for it. + let source_map = tcx.sess.source_map(); - let (some_fn_sig, hir_body) = fn_sig_and_body(tcx, def_id); + + let hir_node = tcx.hir_node_by_def_id(def_id); + let (_, fn_body_id) = + hir::map::associated_body(hir_node).expect("HIR node is a function with body"); + let hir_body = tcx.hir().body(fn_body_id); let body_span = get_body_span(tcx, hir_body, def_id); let source_file = source_map.lookup_source_file(body_span.lo()); - let fn_sig_span = match some_fn_sig.filter(|fn_sig| { + let fn_sig_span = match hir_node.fn_sig().filter(|fn_sig| { fn_sig.span.eq_ctxt(body_span) && Lrc::ptr_eq(&source_file, &source_map.lookup_source_file(fn_sig.span.lo())) }) { @@ -333,18 +340,6 @@ fn extract_hir_info<'tcx>(tcx: TyCtxt<'tcx>, def_id: LocalDefId) -> ExtractedHir ExtractedHirInfo { function_source_hash, fn_sig_span, body_span } } -fn fn_sig_and_body( - tcx: TyCtxt<'_>, - def_id: LocalDefId, -) -> (Option<&rustc_hir::FnSig<'_>>, &rustc_hir::Body<'_>) { - // FIXME(#79625): Consider improving MIR to provide the information needed, to avoid going back - // to HIR for it. - let hir_node = tcx.hir_node_by_def_id(def_id); - let (_, fn_body_id) = - hir::map::associated_body(hir_node).expect("HIR node is a function with body"); - (hir_node.fn_sig(), tcx.hir().body(fn_body_id)) -} - fn get_body_span<'tcx>( tcx: TyCtxt<'tcx>, hir_body: &rustc_hir::Body<'tcx>, From 3b610c764d1c515699fe6cb681c316a4bf772c7a Mon Sep 17 00:00:00 2001 From: Zalathar Date: Thu, 14 Dec 2023 13:30:31 +1100 Subject: [PATCH 13/14] coverage: Compare span source files without involving `Lrc` If we want to know whether two byte positions are in the same file, we don't need to clone and compare `Lrc`; we can just get their indices and compare those instead. --- .../rustc_mir_transform/src/coverage/mod.rs | 25 +++++++++++-------- 1 file changed, 14 insertions(+), 11 deletions(-) diff --git a/compiler/rustc_mir_transform/src/coverage/mod.rs b/compiler/rustc_mir_transform/src/coverage/mod.rs index 76a23fdccd4cc..5944282527ff4 100644 --- a/compiler/rustc_mir_transform/src/coverage/mod.rs +++ b/compiler/rustc_mir_transform/src/coverage/mod.rs @@ -13,7 +13,6 @@ use self::spans::CoverageSpans; use crate::MirPass; -use rustc_data_structures::sync::Lrc; use rustc_middle::hir; use rustc_middle::middle::codegen_fn_attrs::CodegenFnAttrFlags; use rustc_middle::mir::coverage::*; @@ -317,8 +316,6 @@ fn extract_hir_info<'tcx>(tcx: TyCtxt<'tcx>, def_id: LocalDefId) -> ExtractedHir // FIXME(#79625): Consider improving MIR to provide the information needed, to avoid going back // to HIR for it. - let source_map = tcx.sess.source_map(); - let hir_node = tcx.hir_node_by_def_id(def_id); let (_, fn_body_id) = hir::map::associated_body(hir_node).expect("HIR node is a function with body"); @@ -326,14 +323,20 @@ fn extract_hir_info<'tcx>(tcx: TyCtxt<'tcx>, def_id: LocalDefId) -> ExtractedHir let body_span = get_body_span(tcx, hir_body, def_id); - let source_file = source_map.lookup_source_file(body_span.lo()); - let fn_sig_span = match hir_node.fn_sig().filter(|fn_sig| { - fn_sig.span.eq_ctxt(body_span) - && Lrc::ptr_eq(&source_file, &source_map.lookup_source_file(fn_sig.span.lo())) - }) { - Some(fn_sig) => fn_sig.span.with_hi(body_span.lo()), - None => body_span.shrink_to_lo(), - }; + // The actual signature span is only used if it has the same context and + // filename as the body. + let maybe_fn_sig_span = hir_node.fn_sig().map(|fn_sig| fn_sig.span); + let fn_sig_span = maybe_fn_sig_span + .filter(|&fn_sig_span| { + let source_map = tcx.sess.source_map(); + let file_idx = |span: Span| source_map.lookup_source_file_idx(span.lo()); + + fn_sig_span.eq_ctxt(body_span) && file_idx(fn_sig_span) == file_idx(body_span) + }) + // If so, extend it to the start of the body span. + .map(|fn_sig_span| fn_sig_span.with_hi(body_span.lo())) + // Otherwise, create a dummy signature span at the start of the body. + .unwrap_or_else(|| body_span.shrink_to_lo()); let function_source_hash = hash_mir_source(tcx, hir_body); From 684b9ea408afee07d254113d49c2426bfd2a1f8b Mon Sep 17 00:00:00 2001 From: Zalathar Date: Thu, 14 Dec 2023 13:47:26 +1100 Subject: [PATCH 14/14] coverage: Check that the function signature span precedes the body This will normally be true, but in cases where it's not true we're better off not making any assumptions about the signature. --- compiler/rustc_mir_transform/src/coverage/mod.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/compiler/rustc_mir_transform/src/coverage/mod.rs b/compiler/rustc_mir_transform/src/coverage/mod.rs index 5944282527ff4..65a0924f1c9fb 100644 --- a/compiler/rustc_mir_transform/src/coverage/mod.rs +++ b/compiler/rustc_mir_transform/src/coverage/mod.rs @@ -324,14 +324,16 @@ fn extract_hir_info<'tcx>(tcx: TyCtxt<'tcx>, def_id: LocalDefId) -> ExtractedHir let body_span = get_body_span(tcx, hir_body, def_id); // The actual signature span is only used if it has the same context and - // filename as the body. + // filename as the body, and precedes the body. let maybe_fn_sig_span = hir_node.fn_sig().map(|fn_sig| fn_sig.span); let fn_sig_span = maybe_fn_sig_span .filter(|&fn_sig_span| { let source_map = tcx.sess.source_map(); let file_idx = |span: Span| source_map.lookup_source_file_idx(span.lo()); - fn_sig_span.eq_ctxt(body_span) && file_idx(fn_sig_span) == file_idx(body_span) + fn_sig_span.eq_ctxt(body_span) + && fn_sig_span.hi() <= body_span.lo() + && file_idx(fn_sig_span) == file_idx(body_span) }) // If so, extend it to the start of the body span. .map(|fn_sig_span| fn_sig_span.with_hi(body_span.lo()))