From 18f8657415da5ebc480e63abe03f87d4b4ce4157 Mon Sep 17 00:00:00 2001 From: Kai Luo Date: Thu, 8 Aug 2024 22:08:57 -0400 Subject: [PATCH 01/14] Pass -bnoipath when adding rust upstream dynamic crates --- compiler/rustc_codegen_ssa/src/back/link.rs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/compiler/rustc_codegen_ssa/src/back/link.rs b/compiler/rustc_codegen_ssa/src/back/link.rs index ad81ff272c62f..b3bb32bedf4b3 100644 --- a/compiler/rustc_codegen_ssa/src/back/link.rs +++ b/compiler/rustc_codegen_ssa/src/back/link.rs @@ -2753,6 +2753,13 @@ fn add_upstream_rust_crates( .find(|(ty, _)| *ty == crate_type) .expect("failed to find crate type in dependency format list"); + if sess.target.is_like_aix { + // Unlike GNU's ld, AIX linker doesn't feature `-soname=...` when output + // a shared library. Instead, AIX linker offers `(no)ipath`. See + // https://www.ibm.com/docs/en/aix/7.3?topic=l-ld-command. + cmd.link_or_cc_arg("-bnoipath"); + } + for &cnum in &codegen_results.crate_info.used_crates { // We may not pass all crates through to the linker. Some crates may appear statically in // an existing dylib, meaning we'll pick up all the symbols from the dylib. From 1ae1f8ce9cb327eaa22454bbf5a8e25123091ffb Mon Sep 17 00:00:00 2001 From: David Tenty Date: Fri, 6 Dec 2024 10:53:26 -0500 Subject: [PATCH 02/14] Clarify comment --- compiler/rustc_codegen_ssa/src/back/link.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/compiler/rustc_codegen_ssa/src/back/link.rs b/compiler/rustc_codegen_ssa/src/back/link.rs index b3bb32bedf4b3..eb8f7cf6d407c 100644 --- a/compiler/rustc_codegen_ssa/src/back/link.rs +++ b/compiler/rustc_codegen_ssa/src/back/link.rs @@ -2754,8 +2754,10 @@ fn add_upstream_rust_crates( .expect("failed to find crate type in dependency format list"); if sess.target.is_like_aix { - // Unlike GNU's ld, AIX linker doesn't feature `-soname=...` when output - // a shared library. Instead, AIX linker offers `(no)ipath`. See + // Unlike ELF linkers, AIX doesn't feature `DT_SONAME` to override + // the dependency name when outputing a shared library. Thus, `ld` will + // use the full path to shared libraries as the dependency if passed it + // by default unless `noipath` is passed. // https://www.ibm.com/docs/en/aix/7.3?topic=l-ld-command. cmd.link_or_cc_arg("-bnoipath"); } From 3109f07d855cd758aec574b009d339fe3e54cdb3 Mon Sep 17 00:00:00 2001 From: Xing Xue Date: Fri, 6 Dec 2024 12:51:59 -0500 Subject: [PATCH 03/14] Replace sa_sigaction with sa_union.__su_sigaction for AIX. --- .../auxiliary/assert-sigpipe-disposition.rs | 9 ++++++++- .../ui/runtime/on-broken-pipe/auxiliary/sigpipe-utils.rs | 9 ++++++++- tests/ui/runtime/signal-alternate-stack-cleanup.rs | 9 ++++++++- 3 files changed, 24 insertions(+), 3 deletions(-) diff --git a/tests/ui/runtime/on-broken-pipe/auxiliary/assert-sigpipe-disposition.rs b/tests/ui/runtime/on-broken-pipe/auxiliary/assert-sigpipe-disposition.rs index 117f6134b4e03..229408fb72474 100644 --- a/tests/ui/runtime/on-broken-pipe/auxiliary/assert-sigpipe-disposition.rs +++ b/tests/ui/runtime/on-broken-pipe/auxiliary/assert-sigpipe-disposition.rs @@ -25,7 +25,14 @@ fn start(argc: isize, argv: *const *const u8) -> isize { let actual = unsafe { let mut actual: libc::sigaction = std::mem::zeroed(); libc::sigaction(libc::SIGPIPE, std::ptr::null(), &mut actual); - actual.sa_sigaction + #[cfg(not(target_os = "aix"))] + { + actual.sa_sigaction + } + #[cfg(target_os = "aix")] + { + actual.sa_union.__su_sigaction as libc::sighandler_t + } }; assert_eq!(actual, expected, "actual and expected SIGPIPE disposition in child differs"); diff --git a/tests/ui/runtime/on-broken-pipe/auxiliary/sigpipe-utils.rs b/tests/ui/runtime/on-broken-pipe/auxiliary/sigpipe-utils.rs index 3d93d50ca3fbb..d16a2b4d8c8ab 100644 --- a/tests/ui/runtime/on-broken-pipe/auxiliary/sigpipe-utils.rs +++ b/tests/ui/runtime/on-broken-pipe/auxiliary/sigpipe-utils.rs @@ -20,7 +20,14 @@ pub fn assert_sigpipe_handler(expected_handler: SignalHandler) { let actual = unsafe { let mut actual: libc::sigaction = std::mem::zeroed(); libc::sigaction(libc::SIGPIPE, std::ptr::null(), &mut actual); - actual.sa_sigaction + #[cfg(not(target_os = "aix"))] + { + actual.sa_sigaction + } + #[cfg(target_os = "aix")] + { + actual.sa_union.__su_sigaction as libc::sighandler_t + } }; let expected = match expected_handler { diff --git a/tests/ui/runtime/signal-alternate-stack-cleanup.rs b/tests/ui/runtime/signal-alternate-stack-cleanup.rs index f2af86be0a5f5..8fce092827318 100644 --- a/tests/ui/runtime/signal-alternate-stack-cleanup.rs +++ b/tests/ui/runtime/signal-alternate-stack-cleanup.rs @@ -29,7 +29,14 @@ fn main() { // Install signal handler that runs on alternate signal stack. let mut action: sigaction = std::mem::zeroed(); action.sa_flags = (SA_ONSTACK | SA_SIGINFO) as _; - action.sa_sigaction = signal_handler as sighandler_t; + #[cfg(not(target_os = "aix"))] + { + action.sa_sigaction = signal_handler as sighandler_t; + } + #[cfg(target_os = "aix")] + { + action.sa_union.__su_sigaction = signal_handler as sighandler_t; + } sigaction(SIGWINCH, &action, std::ptr::null_mut()); // Send SIGWINCH on exit. From ab2ee7aa2f0eb8906b5d04104c93050e703b3936 Mon Sep 17 00:00:00 2001 From: Xing Xue Date: Fri, 6 Dec 2024 15:22:46 -0500 Subject: [PATCH 04/14] Use option "-sf" for the AIX "ln" command. --- tests/run-make/libs-through-symlinks/Makefile | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/tests/run-make/libs-through-symlinks/Makefile b/tests/run-make/libs-through-symlinks/Makefile index 592eae663a49a..c6ff566a0e86e 100644 --- a/tests/run-make/libs-through-symlinks/Makefile +++ b/tests/run-make/libs-through-symlinks/Makefile @@ -3,10 +3,20 @@ include ../tools.mk # ignore-windows +# The option -n for the AIX ln command has a different purpose than it does +# on Linux. On Linux, the -n option is used to treat the destination path as +# normal file if it is a symbolic link to a directory, which is the default +# behavior of the AIX ln command. +ifeq ($(UNAME),AIX) +LN_FLAGS := -sf +else +LN_FLAGS := -nsf +endif + NAME := $(shell $(RUSTC) --print file-names foo.rs) all: mkdir -p $(TMPDIR)/outdir $(RUSTC) foo.rs -o $(TMPDIR)/outdir/$(NAME) - ln -nsf outdir/$(NAME) $(TMPDIR) + ln $(LN_FLAGS) outdir/$(NAME) $(TMPDIR) RUSTC_LOG=rustc_metadata::loader $(RUSTC) bar.rs From 3ce35a4ec5f06828f908a018da083af5eb54301a Mon Sep 17 00:00:00 2001 From: Jack Wrenn Date: Fri, 6 Dec 2024 17:11:36 +0000 Subject: [PATCH 05/14] Make `Copy` unsafe to implement for ADTs with `unsafe` fields As a rule, the application of `unsafe` to a declaration requires that use-sites of that declaration also require `unsafe`. For example, a field declared `unsafe` may only be read in the lexical context of an `unsafe` block. For nearly all safe traits, the safety obligations of fields are explicitly discharged when they are mentioned in method definitions. For example, idiomatically implementing `Clone` (a safe trait) for a type with unsafe fields will require `unsafe` to clone those fields. Prior to this commit, `Copy` violated this rule. The trait is marked safe, and although it has no explicit methods, its implementation permits reads of `Self`. This commit resolves this by making `Copy` conditionally safe to implement. It remains safe to implement for ADTs without unsafe fields, but unsafe to implement for ADTs with unsafe fields. Tracking: #132922 --- .../example/mini_core.rs | 40 +++++++++--------- .../rustc_codegen_gcc/example/mini_core.rs | 36 ++++++++-------- .../src/coherence/builtin.rs | 8 +++- .../src/coherence/unsafety.rs | 38 +++++++++++++---- compiler/rustc_lint/src/builtin.rs | 1 + compiler/rustc_middle/src/ty/sty.rs | 6 +-- compiler/rustc_middle/src/ty/util.rs | 9 ++++ .../rustc_trait_selection/src/traits/misc.rs | 10 +++++ .../src/traits/select/candidate_assembly.rs | 2 - .../src/needless_pass_by_value.rs | 1 + tests/ui/unsafe-fields/copy-trait.rs | 41 +++++++++++++++++++ tests/ui/unsafe-fields/copy-trait.stderr | 28 +++++++++++++ 12 files changed, 166 insertions(+), 54 deletions(-) create mode 100644 tests/ui/unsafe-fields/copy-trait.rs create mode 100644 tests/ui/unsafe-fields/copy-trait.stderr diff --git a/compiler/rustc_codegen_cranelift/example/mini_core.rs b/compiler/rustc_codegen_cranelift/example/mini_core.rs index 3da215fe6c019..a0a381638c061 100644 --- a/compiler/rustc_codegen_cranelift/example/mini_core.rs +++ b/compiler/rustc_codegen_cranelift/example/mini_core.rs @@ -55,26 +55,26 @@ impl LegacyReceiver for &mut T {} impl LegacyReceiver for Box {} #[lang = "copy"] -pub unsafe trait Copy {} - -unsafe impl Copy for bool {} -unsafe impl Copy for u8 {} -unsafe impl Copy for u16 {} -unsafe impl Copy for u32 {} -unsafe impl Copy for u64 {} -unsafe impl Copy for u128 {} -unsafe impl Copy for usize {} -unsafe impl Copy for i8 {} -unsafe impl Copy for i16 {} -unsafe impl Copy for i32 {} -unsafe impl Copy for isize {} -unsafe impl Copy for f32 {} -unsafe impl Copy for f64 {} -unsafe impl Copy for char {} -unsafe impl<'a, T: ?Sized> Copy for &'a T {} -unsafe impl Copy for *const T {} -unsafe impl Copy for *mut T {} -unsafe impl Copy for Option {} +pub trait Copy {} + +impl Copy for bool {} +impl Copy for u8 {} +impl Copy for u16 {} +impl Copy for u32 {} +impl Copy for u64 {} +impl Copy for u128 {} +impl Copy for usize {} +impl Copy for i8 {} +impl Copy for i16 {} +impl Copy for i32 {} +impl Copy for isize {} +impl Copy for f32 {} +impl Copy for f64 {} +impl Copy for char {} +impl<'a, T: ?Sized> Copy for &'a T {} +impl Copy for *const T {} +impl Copy for *mut T {} +impl Copy for Option {} #[lang = "sync"] pub unsafe trait Sync {} diff --git a/compiler/rustc_codegen_gcc/example/mini_core.rs b/compiler/rustc_codegen_gcc/example/mini_core.rs index c887598f6e902..cdd151613df84 100644 --- a/compiler/rustc_codegen_gcc/example/mini_core.rs +++ b/compiler/rustc_codegen_gcc/example/mini_core.rs @@ -52,24 +52,24 @@ impl LegacyReceiver for &mut T {} impl LegacyReceiver for Box {} #[lang = "copy"] -pub unsafe trait Copy {} - -unsafe impl Copy for bool {} -unsafe impl Copy for u8 {} -unsafe impl Copy for u16 {} -unsafe impl Copy for u32 {} -unsafe impl Copy for u64 {} -unsafe impl Copy for usize {} -unsafe impl Copy for i8 {} -unsafe impl Copy for i16 {} -unsafe impl Copy for i32 {} -unsafe impl Copy for isize {} -unsafe impl Copy for f32 {} -unsafe impl Copy for f64 {} -unsafe impl Copy for char {} -unsafe impl<'a, T: ?Sized> Copy for &'a T {} -unsafe impl Copy for *const T {} -unsafe impl Copy for *mut T {} +pub trait Copy {} + +impl Copy for bool {} +impl Copy for u8 {} +impl Copy for u16 {} +impl Copy for u32 {} +impl Copy for u64 {} +impl Copy for usize {} +impl Copy for i8 {} +impl Copy for i16 {} +impl Copy for i32 {} +impl Copy for isize {} +impl Copy for f32 {} +impl Copy for f64 {} +impl Copy for char {} +impl<'a, T: ?Sized> Copy for &'a T {} +impl Copy for *const T {} +impl Copy for *mut T {} #[lang = "sync"] pub unsafe trait Sync {} diff --git a/compiler/rustc_hir_analysis/src/coherence/builtin.rs b/compiler/rustc_hir_analysis/src/coherence/builtin.rs index 3b49bc41ffe43..2eea65125b038 100644 --- a/compiler/rustc_hir_analysis/src/coherence/builtin.rs +++ b/compiler/rustc_hir_analysis/src/coherence/builtin.rs @@ -103,7 +103,7 @@ fn visit_implementation_of_copy(checker: &Checker<'_>) -> Result<(), ErrorGuaran } let cause = traits::ObligationCause::misc(DUMMY_SP, impl_did); - match type_allowed_to_implement_copy(tcx, param_env, self_type, cause) { + match type_allowed_to_implement_copy(tcx, param_env, self_type, cause, impl_header.safety) { Ok(()) => Ok(()), Err(CopyImplementationError::InfringingFields(fields)) => { let span = tcx.hir().expect_item(impl_did).expect_impl().self_ty.span; @@ -123,6 +123,12 @@ fn visit_implementation_of_copy(checker: &Checker<'_>) -> Result<(), ErrorGuaran let span = tcx.hir().expect_item(impl_did).expect_impl().self_ty.span; Err(tcx.dcx().emit_err(errors::CopyImplOnTypeWithDtor { span })) } + Err(CopyImplementationError::HasUnsafeFields) => { + let span = tcx.hir().expect_item(impl_did).expect_impl().self_ty.span; + Err(tcx + .dcx() + .span_delayed_bug(span, format!("cannot implement `Copy` for `{}`", self_type))) + } } } diff --git a/compiler/rustc_hir_analysis/src/coherence/unsafety.rs b/compiler/rustc_hir_analysis/src/coherence/unsafety.rs index d66114a50d78b..86839e4033034 100644 --- a/compiler/rustc_hir_analysis/src/coherence/unsafety.rs +++ b/compiler/rustc_hir_analysis/src/coherence/unsafety.rs @@ -3,7 +3,7 @@ use rustc_errors::codes::*; use rustc_errors::struct_span_code_err; -use rustc_hir::Safety; +use rustc_hir::{LangItem, Safety}; use rustc_middle::ty::ImplPolarity::*; use rustc_middle::ty::print::PrintTraitRefExt as _; use rustc_middle::ty::{ImplTraitHeader, TraitDef, TyCtxt}; @@ -20,7 +20,19 @@ pub(super) fn check_item( tcx.generics_of(def_id).own_params.iter().find(|p| p.pure_wrt_drop).map(|_| "may_dangle"); let trait_ref = trait_header.trait_ref.instantiate_identity(); - match (trait_def.safety, unsafe_attr, trait_header.safety, trait_header.polarity) { + let is_copy = tcx.is_lang_item(trait_def.def_id, LangItem::Copy); + let trait_def_safety = if is_copy { + // If `Self` has unsafe fields, `Copy` is unsafe to implement. + if trait_header.trait_ref.skip_binder().self_ty().has_unsafe_fields() { + rustc_hir::Safety::Unsafe + } else { + rustc_hir::Safety::Safe + } + } else { + trait_def.safety + }; + + match (trait_def_safety, unsafe_attr, trait_header.safety, trait_header.polarity) { (Safety::Safe, None, Safety::Unsafe, Positive | Reservation) => { let span = tcx.def_span(def_id); return Err(struct_span_code_err!( @@ -48,12 +60,22 @@ pub(super) fn check_item( "the trait `{}` requires an `unsafe impl` declaration", trait_ref.print_trait_sugared() ) - .with_note(format!( - "the trait `{}` enforces invariants that the compiler can't check. \ - Review the trait documentation and make sure this implementation \ - upholds those invariants before adding the `unsafe` keyword", - trait_ref.print_trait_sugared() - )) + .with_note(if is_copy { + format!( + "the trait `{}` cannot be safely implemented for `{}` \ + because it has unsafe fields. Review the invariants \ + of those fields before adding an `unsafe impl`", + trait_ref.print_trait_sugared(), + trait_ref.self_ty(), + ) + } else { + format!( + "the trait `{}` enforces invariants that the compiler can't check. \ + Review the trait documentation and make sure this implementation \ + upholds those invariants before adding the `unsafe` keyword", + trait_ref.print_trait_sugared() + ) + }) .with_span_suggestion_verbose( span.shrink_to_lo(), "add `unsafe` to this trait implementation", diff --git a/compiler/rustc_lint/src/builtin.rs b/compiler/rustc_lint/src/builtin.rs index ec08519892203..093cc16fb4c18 100644 --- a/compiler/rustc_lint/src/builtin.rs +++ b/compiler/rustc_lint/src/builtin.rs @@ -625,6 +625,7 @@ impl<'tcx> LateLintPass<'tcx> for MissingCopyImplementations { cx.param_env, ty, traits::ObligationCause::misc(item.span, item.owner_id.def_id), + hir::Safety::Safe, ) .is_ok() { diff --git a/compiler/rustc_middle/src/ty/sty.rs b/compiler/rustc_middle/src/ty/sty.rs index 474062218c97a..3fbc23924f59a 100644 --- a/compiler/rustc_middle/src/ty/sty.rs +++ b/compiler/rustc_middle/src/ty/sty.rs @@ -980,11 +980,7 @@ impl<'tcx> rustc_type_ir::inherent::Ty> for Ty<'tcx> { } fn has_unsafe_fields(self) -> bool { - if let ty::Adt(adt_def, ..) = self.kind() { - adt_def.all_fields().any(|x| x.safety == hir::Safety::Unsafe) - } else { - false - } + Ty::has_unsafe_fields(self) } } diff --git a/compiler/rustc_middle/src/ty/util.rs b/compiler/rustc_middle/src/ty/util.rs index 57054bd1a0b24..b9a45ea3c2c5a 100644 --- a/compiler/rustc_middle/src/ty/util.rs +++ b/compiler/rustc_middle/src/ty/util.rs @@ -1288,6 +1288,15 @@ impl<'tcx> Ty<'tcx> { } } + /// Checks whether this type is an ADT that has unsafe fields. + pub fn has_unsafe_fields(self) -> bool { + if let ty::Adt(adt_def, ..) = self.kind() { + adt_def.all_fields().any(|x| x.safety == hir::Safety::Unsafe) + } else { + false + } + } + /// Get morphology of the async drop glue, needed for types which do not /// use async drop. To get async drop glue morphology for a definition see /// [`TyCtxt::async_drop_glue_morphology`]. Used for `AsyncDestruct::Destructor` diff --git a/compiler/rustc_trait_selection/src/traits/misc.rs b/compiler/rustc_trait_selection/src/traits/misc.rs index 3b17fa6b03285..d216ae7291337 100644 --- a/compiler/rustc_trait_selection/src/traits/misc.rs +++ b/compiler/rustc_trait_selection/src/traits/misc.rs @@ -18,6 +18,7 @@ pub enum CopyImplementationError<'tcx> { InfringingFields(Vec<(&'tcx ty::FieldDef, Ty<'tcx>, InfringingFieldsReason<'tcx>)>), NotAnAdt, HasDestructor, + HasUnsafeFields, } pub enum ConstParamTyImplementationError<'tcx> { @@ -39,11 +40,16 @@ pub enum InfringingFieldsReason<'tcx> { /// /// If it's not an ADT, int ty, `bool`, float ty, `char`, raw pointer, `!`, /// a reference or an array returns `Err(NotAnAdt)`. +/// +/// If the impl is `Safe`, `self_type` must not have unsafe fields. When used to +/// generate suggestions in lints, `Safe` should be supplied so as to not +/// suggest implementing `Copy` for types with unsafe fields. pub fn type_allowed_to_implement_copy<'tcx>( tcx: TyCtxt<'tcx>, param_env: ty::ParamEnv<'tcx>, self_type: Ty<'tcx>, parent_cause: ObligationCause<'tcx>, + impl_safety: hir::Safety, ) -> Result<(), CopyImplementationError<'tcx>> { let (adt, args) = match self_type.kind() { // These types used to have a builtin impl. @@ -78,6 +84,10 @@ pub fn type_allowed_to_implement_copy<'tcx>( return Err(CopyImplementationError::HasDestructor); } + if impl_safety == hir::Safety::Safe && self_type.has_unsafe_fields() { + return Err(CopyImplementationError::HasUnsafeFields); + } + Ok(()) } diff --git a/compiler/rustc_trait_selection/src/traits/select/candidate_assembly.rs b/compiler/rustc_trait_selection/src/traits/select/candidate_assembly.rs index 3e2c8467d3220..5e27fd4378976 100644 --- a/compiler/rustc_trait_selection/src/traits/select/candidate_assembly.rs +++ b/compiler/rustc_trait_selection/src/traits/select/candidate_assembly.rs @@ -795,8 +795,6 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> { | ty::Never | ty::Tuple(_) | ty::CoroutineWitness(..) => { - use rustc_type_ir::inherent::*; - // Only consider auto impls of unsafe traits when there are // no unsafe fields. if self.tcx().trait_is_unsafe(def_id) && self_ty.has_unsafe_fields() { diff --git a/src/tools/clippy/clippy_lints/src/needless_pass_by_value.rs b/src/tools/clippy/clippy_lints/src/needless_pass_by_value.rs index cd90d2f90f77b..6f3f371a68d6b 100644 --- a/src/tools/clippy/clippy_lints/src/needless_pass_by_value.rs +++ b/src/tools/clippy/clippy_lints/src/needless_pass_by_value.rs @@ -200,6 +200,7 @@ impl<'tcx> LateLintPass<'tcx> for NeedlessPassByValue { cx.param_env, ty, traits::ObligationCause::dummy_with_span(span), + rustc_hir::Safety::Safe, ) .is_ok() { diff --git a/tests/ui/unsafe-fields/copy-trait.rs b/tests/ui/unsafe-fields/copy-trait.rs new file mode 100644 index 0000000000000..fb09ed02e3ffe --- /dev/null +++ b/tests/ui/unsafe-fields/copy-trait.rs @@ -0,0 +1,41 @@ +//@ compile-flags: --crate-type=lib + +#![feature(unsafe_fields)] +#![allow(incomplete_features)] +#![deny(missing_copy_implementations)] + +mod good_safe_impl { + enum SafeEnum { + Safe(u8), + } + + impl Copy for SafeEnum {} +} + +mod bad_safe_impl { + enum UnsafeEnum { + Safe(u8), + Unsafe { unsafe field: u8 }, + } + + impl Copy for UnsafeEnum {} + //~^ ERROR the trait `Copy` requires an `unsafe impl` declaration +} + +mod good_unsafe_impl { + enum UnsafeEnum { + Safe(u8), + Unsafe { unsafe field: u8 }, + } + + unsafe impl Copy for UnsafeEnum {} +} + +mod bad_unsafe_impl { + enum SafeEnum { + Safe(u8), + } + + unsafe impl Copy for SafeEnum {} + //~^ ERROR implementing the trait `Copy` is not unsafe +} diff --git a/tests/ui/unsafe-fields/copy-trait.stderr b/tests/ui/unsafe-fields/copy-trait.stderr new file mode 100644 index 0000000000000..5952f8c89c146 --- /dev/null +++ b/tests/ui/unsafe-fields/copy-trait.stderr @@ -0,0 +1,28 @@ +error[E0200]: the trait `Copy` requires an `unsafe impl` declaration + --> $DIR/copy-trait.rs:21:5 + | +LL | impl Copy for UnsafeEnum {} + | ^^^^^^^^^^^^^^^^^^^^^^^^ + | + = note: the trait `Copy` cannot be safely implemented for `bad_safe_impl::UnsafeEnum` because it has unsafe fields. Review the invariants of those fields before adding an `unsafe impl` +help: add `unsafe` to this trait implementation + | +LL | unsafe impl Copy for UnsafeEnum {} + | ++++++ + +error[E0199]: implementing the trait `Copy` is not unsafe + --> $DIR/copy-trait.rs:39:5 + | +LL | unsafe impl Copy for SafeEnum {} + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: remove `unsafe` from this trait implementation + | +LL - unsafe impl Copy for SafeEnum {} +LL + impl Copy for SafeEnum {} + | + +error: aborting due to 2 previous errors + +Some errors have detailed explanations: E0199, E0200. +For more information about an error, try `rustc --explain E0199`. From 88669aed22aeeef5fb7ecdb7f43ed33e674f8fcb Mon Sep 17 00:00:00 2001 From: Michael Goulet Date: Sat, 7 Dec 2024 23:54:10 +0000 Subject: [PATCH 06/14] Don't use AsyncFnOnce::CallOnceFuture bounds for signature deduction --- compiler/rustc_hir_typeck/src/closure.rs | 12 ++++-------- .../async-closures/call-once-deduction.rs | 14 ++++++++++++++ 2 files changed, 18 insertions(+), 8 deletions(-) create mode 100644 tests/ui/async-await/async-closures/call-once-deduction.rs diff --git a/compiler/rustc_hir_typeck/src/closure.rs b/compiler/rustc_hir_typeck/src/closure.rs index e715a7f7e1588..9037caf0066fe 100644 --- a/compiler/rustc_hir_typeck/src/closure.rs +++ b/compiler/rustc_hir_typeck/src/closure.rs @@ -454,20 +454,16 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { closure_kind: hir::ClosureKind, projection: ty::PolyProjectionPredicate<'tcx>, ) -> Option> { - let tcx = self.tcx; - - let trait_def_id = projection.trait_def_id(tcx); + let def_id = projection.projection_def_id(); // For now, we only do signature deduction based off of the `Fn` and `AsyncFn` traits, // for closures and async closures, respectively. match closure_kind { - hir::ClosureKind::Closure - if self.tcx.fn_trait_kind_from_def_id(trait_def_id).is_some() => - { + hir::ClosureKind::Closure if self.tcx.is_lang_item(def_id, LangItem::FnOnceOutput) => { self.extract_sig_from_projection(cause_span, projection) } hir::ClosureKind::CoroutineClosure(hir::CoroutineDesugaring::Async) - if self.tcx.async_fn_trait_kind_from_def_id(trait_def_id).is_some() => + if self.tcx.is_lang_item(def_id, LangItem::AsyncFnOnceOutput) => { self.extract_sig_from_projection(cause_span, projection) } @@ -475,7 +471,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { // `F: FnOnce() -> Fut, Fut: Future` style bound. Let's still // guide inference here, since it's beneficial for the user. hir::ClosureKind::CoroutineClosure(hir::CoroutineDesugaring::Async) - if self.tcx.fn_trait_kind_from_def_id(trait_def_id).is_some() => + if self.tcx.is_lang_item(def_id, LangItem::FnOnceOutput) => { self.extract_sig_from_projection_and_future_bound(cause_span, projection) } diff --git a/tests/ui/async-await/async-closures/call-once-deduction.rs b/tests/ui/async-await/async-closures/call-once-deduction.rs new file mode 100644 index 0000000000000..41d92bc3d786c --- /dev/null +++ b/tests/ui/async-await/async-closures/call-once-deduction.rs @@ -0,0 +1,14 @@ +//@ edition: 2021 +//@ check-pass + +#![feature(async_closure, async_fn_traits, unboxed_closures)] + +fn bar(_: F) +where + F: AsyncFnOnce<(), CallOnceFuture = O>, +{ +} + +fn main() { + bar(async move || {}); +} From c4d7f1d29f05e9655e3827529ba4da3438e3e1fb Mon Sep 17 00:00:00 2001 From: onur-ozkan Date: Sun, 8 Dec 2024 18:27:22 +0300 Subject: [PATCH 07/14] implement `TargetSelection::is_cygwin` function Signed-off-by: onur-ozkan --- src/bootstrap/src/core/config/config.rs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/bootstrap/src/core/config/config.rs b/src/bootstrap/src/core/config/config.rs index b06147055f2a7..a07d1597746c2 100644 --- a/src/bootstrap/src/core/config/config.rs +++ b/src/bootstrap/src/core/config/config.rs @@ -565,6 +565,12 @@ impl TargetSelection { self.ends_with("windows-gnu") } + pub fn is_cygwin(&self) -> bool { + self.is_windows() && + // ref. https://cygwin.com/pipermail/cygwin/2022-February/250802.html + env::var("OSTYPE").is_ok_and(|v| v.to_lowercase().contains("cygwin")) + } + /// Path to the file defining the custom target, if any. pub fn filepath(&self) -> Option<&Path> { self.file.as_ref().map(Path::new) From d3b53408789ad749b027a1932459e22bc2685622 Mon Sep 17 00:00:00 2001 From: onur-ozkan Date: Sun, 8 Dec 2024 18:27:46 +0300 Subject: [PATCH 08/14] handle cygwin environments in `install::sanitize_sh` Signed-off-by: onur-ozkan --- src/bootstrap/src/core/build_steps/install.rs | 25 ++++++++++--------- 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/src/bootstrap/src/core/build_steps/install.rs b/src/bootstrap/src/core/build_steps/install.rs index 0ce86eadbce67..b6862c2d5c4a9 100644 --- a/src/bootstrap/src/core/build_steps/install.rs +++ b/src/bootstrap/src/core/build_steps/install.rs @@ -21,9 +21,9 @@ const SHELL: &str = "sh"; /// We have to run a few shell scripts, which choke quite a bit on both `\` /// characters and on `C:\` paths, so normalize both of them away. -fn sanitize_sh(path: &Path) -> String { +fn sanitize_sh(path: &Path, is_cygwin: bool) -> String { let path = path.to_str().unwrap().replace('\\', "/"); - return change_drive(unc_to_lfs(&path)).unwrap_or(path); + return if is_cygwin { path } else { change_drive(unc_to_lfs(&path)).unwrap_or(path) }; fn unc_to_lfs(s: &str) -> &str { s.strip_prefix("//?/").unwrap_or(s) @@ -71,6 +71,7 @@ fn install_sh( let prefix = default_path(&builder.config.prefix, "/usr/local"); let sysconfdir = prefix.join(default_path(&builder.config.sysconfdir, "/etc")); let destdir_env = env::var_os("DESTDIR").map(PathBuf::from); + let is_cygwin = builder.config.build.is_cygwin(); // Sanity checks on the write access of user. // @@ -103,14 +104,14 @@ fn install_sh( let mut cmd = command(SHELL); cmd.current_dir(&empty_dir) - .arg(sanitize_sh(&tarball.decompressed_output().join("install.sh"))) - .arg(format!("--prefix={}", prepare_dir(&destdir_env, prefix))) - .arg(format!("--sysconfdir={}", prepare_dir(&destdir_env, sysconfdir))) - .arg(format!("--datadir={}", prepare_dir(&destdir_env, datadir))) - .arg(format!("--docdir={}", prepare_dir(&destdir_env, docdir))) - .arg(format!("--bindir={}", prepare_dir(&destdir_env, bindir))) - .arg(format!("--libdir={}", prepare_dir(&destdir_env, libdir))) - .arg(format!("--mandir={}", prepare_dir(&destdir_env, mandir))) + .arg(sanitize_sh(&tarball.decompressed_output().join("install.sh"), is_cygwin)) + .arg(format!("--prefix={}", prepare_dir(&destdir_env, prefix, is_cygwin))) + .arg(format!("--sysconfdir={}", prepare_dir(&destdir_env, sysconfdir, is_cygwin))) + .arg(format!("--datadir={}", prepare_dir(&destdir_env, datadir, is_cygwin))) + .arg(format!("--docdir={}", prepare_dir(&destdir_env, docdir, is_cygwin))) + .arg(format!("--bindir={}", prepare_dir(&destdir_env, bindir, is_cygwin))) + .arg(format!("--libdir={}", prepare_dir(&destdir_env, libdir, is_cygwin))) + .arg(format!("--mandir={}", prepare_dir(&destdir_env, mandir, is_cygwin))) .arg("--disable-ldconfig"); cmd.run(builder); t!(fs::remove_dir_all(&empty_dir)); @@ -120,7 +121,7 @@ fn default_path(config: &Option, default: &str) -> PathBuf { config.as_ref().cloned().unwrap_or_else(|| PathBuf::from(default)) } -fn prepare_dir(destdir_env: &Option, mut path: PathBuf) -> String { +fn prepare_dir(destdir_env: &Option, mut path: PathBuf, is_cygwin: bool) -> String { // The DESTDIR environment variable is a standard way to install software in a subdirectory // while keeping the original directory structure, even if the prefix or other directories // contain absolute paths. @@ -146,7 +147,7 @@ fn prepare_dir(destdir_env: &Option, mut path: PathBuf) -> String { assert!(path.is_absolute(), "could not make the path relative"); } - sanitize_sh(&path) + sanitize_sh(&path, is_cygwin) } macro_rules! install { From c199c49aef61be2b483399b62e01fd0736035ccb Mon Sep 17 00:00:00 2001 From: clubby789 Date: Sun, 8 Dec 2024 18:57:04 +0000 Subject: [PATCH 09/14] Use SourceMap to load debugger visualizer files --- compiler/rustc_passes/src/debugger_visualizer.rs | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/compiler/rustc_passes/src/debugger_visualizer.rs b/compiler/rustc_passes/src/debugger_visualizer.rs index fec149c8c4324..062d56a79a0b4 100644 --- a/compiler/rustc_passes/src/debugger_visualizer.rs +++ b/compiler/rustc_passes/src/debugger_visualizer.rs @@ -1,7 +1,6 @@ //! Detecting usage of the `#[debugger_visualizer]` attribute. use rustc_ast::Attribute; -use rustc_data_structures::sync::Lrc; use rustc_expand::base::resolve_path; use rustc_middle::middle::debugger_visualizer::{DebuggerVisualizerFile, DebuggerVisualizerType}; use rustc_middle::query::{LocalCrate, Providers}; @@ -49,10 +48,10 @@ impl DebuggerVisualizerCollector<'_> { } }; - match std::fs::read(&file) { - Ok(contents) => { + match self.sess.source_map().load_binary_file(&file) { + Ok((source, _)) => { self.visualizers.push(DebuggerVisualizerFile::new( - Lrc::from(contents), + source, visualizer_type, file, )); From db760e27fd05e88bd1e1d2e48ce15c113e6e1c96 Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Mon, 9 Dec 2024 10:02:03 +1100 Subject: [PATCH 10/14] Move `write_graphviz_results` from `results.rs` to `graphviz.rs`. It's more graphviz-y than it is results-y. This lets us reduce visibility of several types in `graphviz.rs`. --- .../src/framework/graphviz.rs | 183 ++++++++++++++++- .../rustc_mir_dataflow/src/framework/mod.rs | 2 +- .../src/framework/results.rs | 184 +----------------- 3 files changed, 182 insertions(+), 187 deletions(-) diff --git a/compiler/rustc_mir_dataflow/src/framework/graphviz.rs b/compiler/rustc_mir_dataflow/src/framework/graphviz.rs index 561229cf7255b..f844e8fbe0304 100644 --- a/compiler/rustc_mir_dataflow/src/framework/graphviz.rs +++ b/compiler/rustc_mir_dataflow/src/framework/graphviz.rs @@ -2,19 +2,186 @@ use std::borrow::Cow; use std::cell::RefCell; +use std::ffi::OsString; +use std::path::PathBuf; use std::sync::OnceLock; use std::{io, ops, str}; use regex::Regex; -use rustc_graphviz as dot; +use rustc_hir::def_id::DefId; use rustc_index::bit_set::BitSet; -use rustc_middle::mir::{self, BasicBlock, Body, Location, graphviz_safe_def_name}; +use rustc_middle::mir::{ + self, BasicBlock, Body, Location, create_dump_file, dump_enabled, graphviz_safe_def_name, + traversal, +}; +use rustc_middle::ty::TyCtxt; +use rustc_middle::ty::print::with_no_trimmed_paths; +use rustc_span::symbol::{Symbol, sym}; +use tracing::debug; +use {rustc_ast as ast, rustc_graphviz as dot}; use super::fmt::{DebugDiffWithAdapter, DebugWithAdapter, DebugWithContext}; use super::{Analysis, CallReturnPlaces, Direction, Results, ResultsCursor, ResultsVisitor}; +use crate::errors::{ + DuplicateValuesFor, PathMustEndInFilename, RequiresAnArgument, UnknownFormatter, +}; + +/// Writes a DOT file containing the results of a dataflow analysis if the user requested it via +/// `rustc_mir` attributes and `-Z dump-mir-dataflow`. The `Result` in and the `Results` out are +/// the same. +pub(super) fn write_graphviz_results<'tcx, A>( + tcx: TyCtxt<'tcx>, + body: &Body<'tcx>, + results: &mut Results<'tcx, A>, + pass_name: Option<&'static str>, +) -> std::io::Result<()> +where + A: Analysis<'tcx>, + A::Domain: DebugWithContext, +{ + use std::fs; + use std::io::Write; + + let def_id = body.source.def_id(); + let Ok(attrs) = RustcMirAttrs::parse(tcx, def_id) else { + // Invalid `rustc_mir` attrs are reported in `RustcMirAttrs::parse` + return Ok(()); + }; + + let file = try { + match attrs.output_path(A::NAME) { + Some(path) => { + debug!("printing dataflow results for {:?} to {}", def_id, path.display()); + if let Some(parent) = path.parent() { + fs::create_dir_all(parent)?; + } + fs::File::create_buffered(&path)? + } + + None if dump_enabled(tcx, A::NAME, def_id) => { + create_dump_file(tcx, "dot", false, A::NAME, &pass_name.unwrap_or("-----"), body)? + } + + _ => return Ok(()), + } + }; + let mut file = match file { + Ok(f) => f, + Err(e) => return Err(e), + }; + + let style = match attrs.formatter { + Some(sym::two_phase) => OutputStyle::BeforeAndAfter, + _ => OutputStyle::AfterOnly, + }; + + let mut buf = Vec::new(); + + let graphviz = Formatter::new(body, results, style); + let mut render_opts = + vec![dot::RenderOption::Fontname(tcx.sess.opts.unstable_opts.graphviz_font.clone())]; + if tcx.sess.opts.unstable_opts.graphviz_dark_mode { + render_opts.push(dot::RenderOption::DarkTheme); + } + let r = with_no_trimmed_paths!(dot::render_opts(&graphviz, &mut buf, &render_opts)); + + let lhs = try { + r?; + file.write_all(&buf)?; + }; + + lhs +} + +#[derive(Default)] +struct RustcMirAttrs { + basename_and_suffix: Option, + formatter: Option, +} + +impl RustcMirAttrs { + fn parse(tcx: TyCtxt<'_>, def_id: DefId) -> Result { + let mut result = Ok(()); + let mut ret = RustcMirAttrs::default(); + + let rustc_mir_attrs = tcx + .get_attrs(def_id, sym::rustc_mir) + .flat_map(|attr| attr.meta_item_list().into_iter().flat_map(|v| v.into_iter())); + + for attr in rustc_mir_attrs { + let attr_result = if attr.has_name(sym::borrowck_graphviz_postflow) { + Self::set_field(&mut ret.basename_and_suffix, tcx, &attr, |s| { + let path = PathBuf::from(s.to_string()); + match path.file_name() { + Some(_) => Ok(path), + None => { + tcx.dcx().emit_err(PathMustEndInFilename { span: attr.span() }); + Err(()) + } + } + }) + } else if attr.has_name(sym::borrowck_graphviz_format) { + Self::set_field(&mut ret.formatter, tcx, &attr, |s| match s { + sym::gen_kill | sym::two_phase => Ok(s), + _ => { + tcx.dcx().emit_err(UnknownFormatter { span: attr.span() }); + Err(()) + } + }) + } else { + Ok(()) + }; + + result = result.and(attr_result); + } + + result.map(|()| ret) + } + + fn set_field( + field: &mut Option, + tcx: TyCtxt<'_>, + attr: &ast::MetaItemInner, + mapper: impl FnOnce(Symbol) -> Result, + ) -> Result<(), ()> { + if field.is_some() { + tcx.dcx() + .emit_err(DuplicateValuesFor { span: attr.span(), name: attr.name_or_empty() }); + + return Err(()); + } + + if let Some(s) = attr.value_str() { + *field = Some(mapper(s)?); + Ok(()) + } else { + tcx.dcx() + .emit_err(RequiresAnArgument { span: attr.span(), name: attr.name_or_empty() }); + Err(()) + } + } + + /// Returns the path where dataflow results should be written, or `None` + /// `borrowck_graphviz_postflow` was not specified. + /// + /// This performs the following transformation to the argument of `borrowck_graphviz_postflow`: + /// + /// "path/suffix.dot" -> "path/analysis_name_suffix.dot" + fn output_path(&self, analysis_name: &str) -> Option { + let mut ret = self.basename_and_suffix.as_ref().cloned()?; + let suffix = ret.file_name().unwrap(); // Checked when parsing attrs + + let mut file_name: OsString = analysis_name.into(); + file_name.push("_"); + file_name.push(suffix); + ret.set_file_name(file_name); + + Some(ret) + } +} #[derive(Clone, Copy, Debug, PartialEq, Eq)] -pub(crate) enum OutputStyle { +enum OutputStyle { AfterOnly, BeforeAndAfter, } @@ -28,7 +195,7 @@ impl OutputStyle { } } -pub(crate) struct Formatter<'mir, 'tcx, A> +struct Formatter<'mir, 'tcx, A> where A: Analysis<'tcx>, { @@ -45,12 +212,12 @@ impl<'mir, 'tcx, A> Formatter<'mir, 'tcx, A> where A: Analysis<'tcx>, { - pub(crate) fn new( + fn new( body: &'mir Body<'tcx>, results: &'mir mut Results<'tcx, A>, style: OutputStyle, ) -> Self { - let reachable = mir::traversal::reachable_as_bitset(body); + let reachable = traversal::reachable_as_bitset(body); Formatter { cursor: results.as_results_cursor(body).into(), style, reachable } } @@ -61,7 +228,7 @@ where /// A pair of a basic block and an index into that basic blocks `successors`. #[derive(Copy, Clone, PartialEq, Eq, Debug)] -pub(crate) struct CfgEdge { +struct CfgEdge { source: BasicBlock, index: usize, } @@ -520,7 +687,7 @@ struct StateDiffCollector { impl StateDiffCollector { fn run<'tcx, A>( - body: &mir::Body<'tcx>, + body: &Body<'tcx>, block: BasicBlock, results: &mut Results<'tcx, A>, style: OutputStyle, diff --git a/compiler/rustc_mir_dataflow/src/framework/mod.rs b/compiler/rustc_mir_dataflow/src/framework/mod.rs index b9407882ec5d2..13384d6f28553 100644 --- a/compiler/rustc_mir_dataflow/src/framework/mod.rs +++ b/compiler/rustc_mir_dataflow/src/framework/mod.rs @@ -42,7 +42,7 @@ use rustc_middle::mir::{self, BasicBlock, CallReturnPlaces, Location, Terminator use rustc_middle::ty::TyCtxt; use tracing::error; -use self::results::write_graphviz_results; +use self::graphviz::write_graphviz_results; use super::fmt::DebugWithContext; mod cursor; diff --git a/compiler/rustc_mir_dataflow/src/framework/results.rs b/compiler/rustc_mir_dataflow/src/framework/results.rs index c4321c454e6ef..a7dbd99b8abde 100644 --- a/compiler/rustc_mir_dataflow/src/framework/results.rs +++ b/compiler/rustc_mir_dataflow/src/framework/results.rs @@ -1,22 +1,9 @@ //! Dataflow analysis results. -use std::ffi::OsString; -use std::path::PathBuf; - -use rustc_hir::def_id::DefId; use rustc_index::IndexVec; -use rustc_middle::mir::{self, BasicBlock, create_dump_file, dump_enabled, traversal}; -use rustc_middle::ty::TyCtxt; -use rustc_middle::ty::print::with_no_trimmed_paths; -use rustc_span::symbol::{Symbol, sym}; -use tracing::debug; -use {rustc_ast as ast, rustc_graphviz as dot}; +use rustc_middle::mir::{BasicBlock, Body, traversal}; -use super::fmt::DebugWithContext; -use super::{Analysis, ResultsCursor, ResultsVisitor, graphviz, visit_results}; -use crate::errors::{ - DuplicateValuesFor, PathMustEndInFilename, RequiresAnArgument, UnknownFormatter, -}; +use super::{Analysis, ResultsCursor, ResultsVisitor, visit_results}; use crate::framework::cursor::ResultsHandle; pub type EntrySets<'tcx, A> = IndexVec>::Domain>; @@ -41,16 +28,13 @@ where /// `Results` is also used outside the cursor. pub fn as_results_cursor<'mir>( &'mir mut self, - body: &'mir mir::Body<'tcx>, + body: &'mir Body<'tcx>, ) -> ResultsCursor<'mir, 'tcx, A> { ResultsCursor::new(body, ResultsHandle::BorrowedMut(self)) } /// Creates a `ResultsCursor` that takes ownership of the `Results`. - pub fn into_results_cursor<'mir>( - self, - body: &'mir mir::Body<'tcx>, - ) -> ResultsCursor<'mir, 'tcx, A> { + pub fn into_results_cursor<'mir>(self, body: &'mir Body<'tcx>) -> ResultsCursor<'mir, 'tcx, A> { ResultsCursor::new(body, ResultsHandle::Owned(self)) } @@ -61,7 +45,7 @@ where pub fn visit_with<'mir>( &mut self, - body: &'mir mir::Body<'tcx>, + body: &'mir Body<'tcx>, blocks: impl IntoIterator, vis: &mut impl ResultsVisitor<'mir, 'tcx, A>, ) { @@ -70,166 +54,10 @@ where pub fn visit_reachable_with<'mir>( &mut self, - body: &'mir mir::Body<'tcx>, + body: &'mir Body<'tcx>, vis: &mut impl ResultsVisitor<'mir, 'tcx, A>, ) { let blocks = traversal::reachable(body); visit_results(body, blocks.map(|(bb, _)| bb), self, vis) } } - -// Graphviz - -/// Writes a DOT file containing the results of a dataflow analysis if the user requested it via -/// `rustc_mir` attributes and `-Z dump-mir-dataflow`. The `Result` in and the `Results` out are -/// the same. -pub(super) fn write_graphviz_results<'tcx, A>( - tcx: TyCtxt<'tcx>, - body: &mir::Body<'tcx>, - results: &mut Results<'tcx, A>, - pass_name: Option<&'static str>, -) -> std::io::Result<()> -where - A: Analysis<'tcx>, - A::Domain: DebugWithContext, -{ - use std::fs; - use std::io::Write; - - let def_id = body.source.def_id(); - let Ok(attrs) = RustcMirAttrs::parse(tcx, def_id) else { - // Invalid `rustc_mir` attrs are reported in `RustcMirAttrs::parse` - return Ok(()); - }; - - let file = try { - match attrs.output_path(A::NAME) { - Some(path) => { - debug!("printing dataflow results for {:?} to {}", def_id, path.display()); - if let Some(parent) = path.parent() { - fs::create_dir_all(parent)?; - } - fs::File::create_buffered(&path)? - } - - None if dump_enabled(tcx, A::NAME, def_id) => { - create_dump_file(tcx, "dot", false, A::NAME, &pass_name.unwrap_or("-----"), body)? - } - - _ => return Ok(()), - } - }; - let mut file = match file { - Ok(f) => f, - Err(e) => return Err(e), - }; - - let style = match attrs.formatter { - Some(sym::two_phase) => graphviz::OutputStyle::BeforeAndAfter, - _ => graphviz::OutputStyle::AfterOnly, - }; - - let mut buf = Vec::new(); - - let graphviz = graphviz::Formatter::new(body, results, style); - let mut render_opts = - vec![dot::RenderOption::Fontname(tcx.sess.opts.unstable_opts.graphviz_font.clone())]; - if tcx.sess.opts.unstable_opts.graphviz_dark_mode { - render_opts.push(dot::RenderOption::DarkTheme); - } - let r = with_no_trimmed_paths!(dot::render_opts(&graphviz, &mut buf, &render_opts)); - - let lhs = try { - r?; - file.write_all(&buf)?; - }; - - lhs -} - -#[derive(Default)] -struct RustcMirAttrs { - basename_and_suffix: Option, - formatter: Option, -} - -impl RustcMirAttrs { - fn parse(tcx: TyCtxt<'_>, def_id: DefId) -> Result { - let mut result = Ok(()); - let mut ret = RustcMirAttrs::default(); - - let rustc_mir_attrs = tcx - .get_attrs(def_id, sym::rustc_mir) - .flat_map(|attr| attr.meta_item_list().into_iter().flat_map(|v| v.into_iter())); - - for attr in rustc_mir_attrs { - let attr_result = if attr.has_name(sym::borrowck_graphviz_postflow) { - Self::set_field(&mut ret.basename_and_suffix, tcx, &attr, |s| { - let path = PathBuf::from(s.to_string()); - match path.file_name() { - Some(_) => Ok(path), - None => { - tcx.dcx().emit_err(PathMustEndInFilename { span: attr.span() }); - Err(()) - } - } - }) - } else if attr.has_name(sym::borrowck_graphviz_format) { - Self::set_field(&mut ret.formatter, tcx, &attr, |s| match s { - sym::gen_kill | sym::two_phase => Ok(s), - _ => { - tcx.dcx().emit_err(UnknownFormatter { span: attr.span() }); - Err(()) - } - }) - } else { - Ok(()) - }; - - result = result.and(attr_result); - } - - result.map(|()| ret) - } - - fn set_field( - field: &mut Option, - tcx: TyCtxt<'_>, - attr: &ast::MetaItemInner, - mapper: impl FnOnce(Symbol) -> Result, - ) -> Result<(), ()> { - if field.is_some() { - tcx.dcx() - .emit_err(DuplicateValuesFor { span: attr.span(), name: attr.name_or_empty() }); - - return Err(()); - } - - if let Some(s) = attr.value_str() { - *field = Some(mapper(s)?); - Ok(()) - } else { - tcx.dcx() - .emit_err(RequiresAnArgument { span: attr.span(), name: attr.name_or_empty() }); - Err(()) - } - } - - /// Returns the path where dataflow results should be written, or `None` - /// `borrowck_graphviz_postflow` was not specified. - /// - /// This performs the following transformation to the argument of `borrowck_graphviz_postflow`: - /// - /// "path/suffix.dot" -> "path/analysis_name_suffix.dot" - fn output_path(&self, analysis_name: &str) -> Option { - let mut ret = self.basename_and_suffix.as_ref().cloned()?; - let suffix = ret.file_name().unwrap(); // Checked when parsing attrs - - let mut file_name: OsString = analysis_name.into(); - file_name.push("_"); - file_name.push(suffix); - ret.set_file_name(file_name); - - Some(ret) - } -} From b59c4dc2612865a945ae200385115d7815a6f91f Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Mon, 9 Dec 2024 10:04:27 +1100 Subject: [PATCH 11/14] Remove an out-of-date comment. The part about zero-sized structures is totally wrong. The rest of it has almost no explanatory value; there are better explanations in comments elsewhere. --- compiler/rustc_mir_dataflow/src/impls/mod.rs | 4 ---- 1 file changed, 4 deletions(-) diff --git a/compiler/rustc_mir_dataflow/src/impls/mod.rs b/compiler/rustc_mir_dataflow/src/impls/mod.rs index b9e194b00c54e..d69a8019c8d0e 100644 --- a/compiler/rustc_mir_dataflow/src/impls/mod.rs +++ b/compiler/rustc_mir_dataflow/src/impls/mod.rs @@ -1,7 +1,3 @@ -//! Dataflow analyses are built upon some interpretation of the -//! bitvectors attached to each basic block, represented via a -//! zero-sized structure. - mod borrowed_locals; mod initialized; mod liveness; From fe605b9514c7fa62f621b5189a7a05dd31aed0a3 Mon Sep 17 00:00:00 2001 From: Oli Scherer Date: Mon, 9 Dec 2024 09:51:55 +0000 Subject: [PATCH 12/14] Add regression tests --- tests/ui/asm/generic_const_simd_vec_len.rs | 18 +++++++++++++++++ .../ui/asm/generic_const_simd_vec_len.stderr | 10 ++++++++++ tests/ui/asm/named_const_simd_vec_len.rs | 20 +++++++++++++++++++ tests/ui/asm/named_const_simd_vec_len.stderr | 10 ++++++++++ 4 files changed, 58 insertions(+) create mode 100644 tests/ui/asm/generic_const_simd_vec_len.rs create mode 100644 tests/ui/asm/generic_const_simd_vec_len.stderr create mode 100644 tests/ui/asm/named_const_simd_vec_len.rs create mode 100644 tests/ui/asm/named_const_simd_vec_len.stderr diff --git a/tests/ui/asm/generic_const_simd_vec_len.rs b/tests/ui/asm/generic_const_simd_vec_len.rs new file mode 100644 index 0000000000000..4ef839e381674 --- /dev/null +++ b/tests/ui/asm/generic_const_simd_vec_len.rs @@ -0,0 +1,18 @@ +//! This is a regression test to ensure that we emit a diagnostic pointing to the +//! reason the type was rejected in inline assembly. + +#![feature(repr_simd)] + +#[repr(simd)] +#[derive(Copy, Clone)] +pub struct Foo([u8; C]); + +pub unsafe fn foo(a: Foo) { + std::arch::asm!( + "movaps {src}, {src}", + src = in(xmm_reg) a, + //~^ ERROR: cannot use value of type `Foo` for inline assembly + ); +} + +fn main() {} diff --git a/tests/ui/asm/generic_const_simd_vec_len.stderr b/tests/ui/asm/generic_const_simd_vec_len.stderr new file mode 100644 index 0000000000000..d0044d144adcb --- /dev/null +++ b/tests/ui/asm/generic_const_simd_vec_len.stderr @@ -0,0 +1,10 @@ +error: cannot use value of type `Foo` for inline assembly + --> $DIR/generic_const_simd_vec_len.rs:13:27 + | +LL | src = in(xmm_reg) a, + | ^ + | + = note: only integers, floats, SIMD vectors, pointers and function pointers can be used as arguments for inline assembly + +error: aborting due to 1 previous error + diff --git a/tests/ui/asm/named_const_simd_vec_len.rs b/tests/ui/asm/named_const_simd_vec_len.rs new file mode 100644 index 0000000000000..124f5357290ea --- /dev/null +++ b/tests/ui/asm/named_const_simd_vec_len.rs @@ -0,0 +1,20 @@ +//! This is a regression test to ensure that we evaluate +//! SIMD vector length constants instead of assuming they are literals. + +#![feature(repr_simd)] + +const C: usize = 16; + +#[repr(simd)] +#[derive(Copy, Clone)] +pub struct Foo([u8; C]); + +pub unsafe fn foo(a: Foo) { + std::arch::asm!( + "movaps {src}, {src}", + src = in(xmm_reg) a, + //~^ ERROR: cannot use value of type `Foo` for inline assembly + ); +} + +fn main() {} diff --git a/tests/ui/asm/named_const_simd_vec_len.stderr b/tests/ui/asm/named_const_simd_vec_len.stderr new file mode 100644 index 0000000000000..fb3e6b1738c51 --- /dev/null +++ b/tests/ui/asm/named_const_simd_vec_len.stderr @@ -0,0 +1,10 @@ +error: cannot use value of type `Foo` for inline assembly + --> $DIR/named_const_simd_vec_len.rs:15:27 + | +LL | src = in(xmm_reg) a, + | ^ + | + = note: only integers, floats, SIMD vectors, pointers and function pointers can be used as arguments for inline assembly + +error: aborting due to 1 previous error + From 5370f1243e1d5cb18ede24935fcdcfa14f0584dd Mon Sep 17 00:00:00 2001 From: Oli Scherer Date: Mon, 9 Dec 2024 10:06:51 +0000 Subject: [PATCH 13/14] Evaluate constants in SIMD vec lengths before rejecting them --- compiler/rustc_hir_analysis/src/check/intrinsicck.rs | 1 + tests/ui/asm/named_const_simd_vec_len.rs | 3 ++- tests/ui/asm/named_const_simd_vec_len.stderr | 10 ---------- 3 files changed, 3 insertions(+), 11 deletions(-) delete mode 100644 tests/ui/asm/named_const_simd_vec_len.stderr diff --git a/compiler/rustc_hir_analysis/src/check/intrinsicck.rs b/compiler/rustc_hir_analysis/src/check/intrinsicck.rs index df4da03f0f59d..c4762b1235867 100644 --- a/compiler/rustc_hir_analysis/src/check/intrinsicck.rs +++ b/compiler/rustc_hir_analysis/src/check/intrinsicck.rs @@ -83,6 +83,7 @@ impl<'a, 'tcx> InlineAsmCtxt<'a, 'tcx> { let (size, ty) = match elem_ty.kind() { ty::Array(ty, len) => { + let len = self.tcx.normalize_erasing_regions(self.typing_env, *len); if let Some(len) = len.try_to_target_usize(self.tcx) { (len, *ty) } else { diff --git a/tests/ui/asm/named_const_simd_vec_len.rs b/tests/ui/asm/named_const_simd_vec_len.rs index 124f5357290ea..c7e01de6be15b 100644 --- a/tests/ui/asm/named_const_simd_vec_len.rs +++ b/tests/ui/asm/named_const_simd_vec_len.rs @@ -1,6 +1,8 @@ //! This is a regression test to ensure that we evaluate //! SIMD vector length constants instead of assuming they are literals. +//@check-pass + #![feature(repr_simd)] const C: usize = 16; @@ -13,7 +15,6 @@ pub unsafe fn foo(a: Foo) { std::arch::asm!( "movaps {src}, {src}", src = in(xmm_reg) a, - //~^ ERROR: cannot use value of type `Foo` for inline assembly ); } diff --git a/tests/ui/asm/named_const_simd_vec_len.stderr b/tests/ui/asm/named_const_simd_vec_len.stderr deleted file mode 100644 index fb3e6b1738c51..0000000000000 --- a/tests/ui/asm/named_const_simd_vec_len.stderr +++ /dev/null @@ -1,10 +0,0 @@ -error: cannot use value of type `Foo` for inline assembly - --> $DIR/named_const_simd_vec_len.rs:15:27 - | -LL | src = in(xmm_reg) a, - | ^ - | - = note: only integers, floats, SIMD vectors, pointers and function pointers can be used as arguments for inline assembly - -error: aborting due to 1 previous error - From f167d3ccc839b3d2962a11c8d7d4655c2ef55b5c Mon Sep 17 00:00:00 2001 From: Oli Scherer Date: Mon, 9 Dec 2024 10:37:47 +0000 Subject: [PATCH 14/14] Clarify why a type is rejected for asm! --- .../src/check/intrinsicck.rs | 117 +++++++++++------- tests/ui/asm/generic_const_simd_vec_len.rs | 3 +- .../ui/asm/generic_const_simd_vec_len.stderr | 12 +- 3 files changed, 83 insertions(+), 49 deletions(-) diff --git a/compiler/rustc_hir_analysis/src/check/intrinsicck.rs b/compiler/rustc_hir_analysis/src/check/intrinsicck.rs index c4762b1235867..90e93bdbb5075 100644 --- a/compiler/rustc_hir_analysis/src/check/intrinsicck.rs +++ b/compiler/rustc_hir_analysis/src/check/intrinsicck.rs @@ -3,6 +3,7 @@ use std::assert_matches::debug_assert_matches; use rustc_abi::FieldIdx; use rustc_ast::InlineAsmTemplatePiece; use rustc_data_structures::fx::FxIndexSet; +use rustc_hir::def_id::DefId; use rustc_hir::{self as hir, LangItem}; use rustc_middle::bug; use rustc_middle::ty::{self, FloatTy, IntTy, Ty, TyCtxt, TypeVisitableExt, UintTy}; @@ -21,6 +22,12 @@ pub struct InlineAsmCtxt<'a, 'tcx> { get_operand_ty: Box) -> Ty<'tcx> + 'a>, } +enum NonAsmTypeReason<'tcx> { + UnevaluatedSIMDArrayLength(DefId, ty::Const<'tcx>), + Invalid(Ty<'tcx>), + InvalidElement(DefId, Ty<'tcx>), +} + impl<'a, 'tcx> InlineAsmCtxt<'a, 'tcx> { pub fn new_global_asm(tcx: TyCtxt<'tcx>) -> Self { InlineAsmCtxt { @@ -56,7 +63,7 @@ impl<'a, 'tcx> InlineAsmCtxt<'a, 'tcx> { false } - fn get_asm_ty(&self, ty: Ty<'tcx>) -> Option { + fn get_asm_ty(&self, ty: Ty<'tcx>) -> Result> { let asm_ty_isize = match self.tcx.sess.target.pointer_width { 16 => InlineAsmType::I16, 32 => InlineAsmType::I32, @@ -65,21 +72,22 @@ impl<'a, 'tcx> InlineAsmCtxt<'a, 'tcx> { }; match *ty.kind() { - ty::Int(IntTy::I8) | ty::Uint(UintTy::U8) => Some(InlineAsmType::I8), - ty::Int(IntTy::I16) | ty::Uint(UintTy::U16) => Some(InlineAsmType::I16), - ty::Int(IntTy::I32) | ty::Uint(UintTy::U32) => Some(InlineAsmType::I32), - ty::Int(IntTy::I64) | ty::Uint(UintTy::U64) => Some(InlineAsmType::I64), - ty::Int(IntTy::I128) | ty::Uint(UintTy::U128) => Some(InlineAsmType::I128), - ty::Int(IntTy::Isize) | ty::Uint(UintTy::Usize) => Some(asm_ty_isize), - ty::Float(FloatTy::F16) => Some(InlineAsmType::F16), - ty::Float(FloatTy::F32) => Some(InlineAsmType::F32), - ty::Float(FloatTy::F64) => Some(InlineAsmType::F64), - ty::Float(FloatTy::F128) => Some(InlineAsmType::F128), - ty::FnPtr(..) => Some(asm_ty_isize), - ty::RawPtr(ty, _) if self.is_thin_ptr_ty(ty) => Some(asm_ty_isize), + ty::Int(IntTy::I8) | ty::Uint(UintTy::U8) => Ok(InlineAsmType::I8), + ty::Int(IntTy::I16) | ty::Uint(UintTy::U16) => Ok(InlineAsmType::I16), + ty::Int(IntTy::I32) | ty::Uint(UintTy::U32) => Ok(InlineAsmType::I32), + ty::Int(IntTy::I64) | ty::Uint(UintTy::U64) => Ok(InlineAsmType::I64), + ty::Int(IntTy::I128) | ty::Uint(UintTy::U128) => Ok(InlineAsmType::I128), + ty::Int(IntTy::Isize) | ty::Uint(UintTy::Usize) => Ok(asm_ty_isize), + ty::Float(FloatTy::F16) => Ok(InlineAsmType::F16), + ty::Float(FloatTy::F32) => Ok(InlineAsmType::F32), + ty::Float(FloatTy::F64) => Ok(InlineAsmType::F64), + ty::Float(FloatTy::F128) => Ok(InlineAsmType::F128), + ty::FnPtr(..) => Ok(asm_ty_isize), + ty::RawPtr(ty, _) if self.is_thin_ptr_ty(ty) => Ok(asm_ty_isize), ty::Adt(adt, args) if adt.repr().simd() => { let fields = &adt.non_enum_variant().fields; - let elem_ty = fields[FieldIdx::ZERO].ty(self.tcx, args); + let field = &fields[FieldIdx::ZERO]; + let elem_ty = field.ty(self.tcx, args); let (size, ty) = match elem_ty.kind() { ty::Array(ty, len) => { @@ -87,43 +95,39 @@ impl<'a, 'tcx> InlineAsmCtxt<'a, 'tcx> { if let Some(len) = len.try_to_target_usize(self.tcx) { (len, *ty) } else { - return None; + return Err(NonAsmTypeReason::UnevaluatedSIMDArrayLength( + field.did, len, + )); } } _ => (fields.len() as u64, elem_ty), }; match ty.kind() { - ty::Int(IntTy::I8) | ty::Uint(UintTy::U8) => Some(InlineAsmType::VecI8(size)), - ty::Int(IntTy::I16) | ty::Uint(UintTy::U16) => { - Some(InlineAsmType::VecI16(size)) - } - ty::Int(IntTy::I32) | ty::Uint(UintTy::U32) => { - Some(InlineAsmType::VecI32(size)) - } - ty::Int(IntTy::I64) | ty::Uint(UintTy::U64) => { - Some(InlineAsmType::VecI64(size)) - } + ty::Int(IntTy::I8) | ty::Uint(UintTy::U8) => Ok(InlineAsmType::VecI8(size)), + ty::Int(IntTy::I16) | ty::Uint(UintTy::U16) => Ok(InlineAsmType::VecI16(size)), + ty::Int(IntTy::I32) | ty::Uint(UintTy::U32) => Ok(InlineAsmType::VecI32(size)), + ty::Int(IntTy::I64) | ty::Uint(UintTy::U64) => Ok(InlineAsmType::VecI64(size)), ty::Int(IntTy::I128) | ty::Uint(UintTy::U128) => { - Some(InlineAsmType::VecI128(size)) + Ok(InlineAsmType::VecI128(size)) } ty::Int(IntTy::Isize) | ty::Uint(UintTy::Usize) => { - Some(match self.tcx.sess.target.pointer_width { + Ok(match self.tcx.sess.target.pointer_width { 16 => InlineAsmType::VecI16(size), 32 => InlineAsmType::VecI32(size), 64 => InlineAsmType::VecI64(size), width => bug!("unsupported pointer width: {width}"), }) } - ty::Float(FloatTy::F16) => Some(InlineAsmType::VecF16(size)), - ty::Float(FloatTy::F32) => Some(InlineAsmType::VecF32(size)), - ty::Float(FloatTy::F64) => Some(InlineAsmType::VecF64(size)), - ty::Float(FloatTy::F128) => Some(InlineAsmType::VecF128(size)), - _ => None, + ty::Float(FloatTy::F16) => Ok(InlineAsmType::VecF16(size)), + ty::Float(FloatTy::F32) => Ok(InlineAsmType::VecF32(size)), + ty::Float(FloatTy::F64) => Ok(InlineAsmType::VecF64(size)), + ty::Float(FloatTy::F128) => Ok(InlineAsmType::VecF128(size)), + _ => Err(NonAsmTypeReason::InvalidElement(field.did, ty)), } } ty::Infer(_) => bug!("unexpected infer ty in asm operand"), - _ => None, + _ => Err(NonAsmTypeReason::Invalid(ty)), } } @@ -164,17 +168,42 @@ impl<'a, 'tcx> InlineAsmCtxt<'a, 'tcx> { } _ => self.get_asm_ty(ty), }; - let Some(asm_ty) = asm_ty else { - let msg = format!("cannot use value of type `{ty}` for inline assembly"); - self.tcx - .dcx() - .struct_span_err(expr.span, msg) - .with_note( - "only integers, floats, SIMD vectors, pointers and function pointers \ - can be used as arguments for inline assembly", - ) - .emit(); - return None; + let asm_ty = match asm_ty { + Ok(asm_ty) => asm_ty, + Err(reason) => { + match reason { + NonAsmTypeReason::UnevaluatedSIMDArrayLength(did, len) => { + let msg = format!("cannot evaluate SIMD vector length `{len}`"); + self.tcx + .dcx() + .struct_span_err(self.tcx.def_span(did), msg) + .with_span_note( + expr.span, + "SIMD vector length needs to be known statically for use in `asm!`", + ) + .emit(); + } + NonAsmTypeReason::Invalid(ty) => { + let msg = format!("cannot use value of type `{ty}` for inline assembly"); + self.tcx.dcx().struct_span_err(expr.span, msg).with_note( + "only integers, floats, SIMD vectors, pointers and function pointers \ + can be used as arguments for inline assembly", + ).emit(); + } + NonAsmTypeReason::InvalidElement(did, ty) => { + let msg = format!( + "cannot use SIMD vector with element type `{ty}` for inline assembly" + ); + self.tcx.dcx() + .struct_span_err(self.tcx.def_span(did), msg).with_span_note( + expr.span, + "only integers, floats, SIMD vectors, pointers and function pointers \ + can be used as arguments for inline assembly", + ).emit(); + } + } + return None; + } }; // Check that the type implements Copy. The only case where this can diff --git a/tests/ui/asm/generic_const_simd_vec_len.rs b/tests/ui/asm/generic_const_simd_vec_len.rs index 4ef839e381674..1cb3e0982f6e9 100644 --- a/tests/ui/asm/generic_const_simd_vec_len.rs +++ b/tests/ui/asm/generic_const_simd_vec_len.rs @@ -6,12 +6,13 @@ #[repr(simd)] #[derive(Copy, Clone)] pub struct Foo([u8; C]); +//~^ ERROR: cannot evaluate SIMD vector length pub unsafe fn foo(a: Foo) { std::arch::asm!( "movaps {src}, {src}", src = in(xmm_reg) a, - //~^ ERROR: cannot use value of type `Foo` for inline assembly + //~^ NOTE: SIMD vector length needs to be known statically ); } diff --git a/tests/ui/asm/generic_const_simd_vec_len.stderr b/tests/ui/asm/generic_const_simd_vec_len.stderr index d0044d144adcb..dcdcbc76c3da5 100644 --- a/tests/ui/asm/generic_const_simd_vec_len.stderr +++ b/tests/ui/asm/generic_const_simd_vec_len.stderr @@ -1,10 +1,14 @@ -error: cannot use value of type `Foo` for inline assembly - --> $DIR/generic_const_simd_vec_len.rs:13:27 +error: cannot evaluate SIMD vector length `C` + --> $DIR/generic_const_simd_vec_len.rs:8:32 + | +LL | pub struct Foo([u8; C]); + | ^^^^^^^ + | +note: SIMD vector length needs to be known statically for use in `asm!` + --> $DIR/generic_const_simd_vec_len.rs:14:27 | LL | src = in(xmm_reg) a, | ^ - | - = note: only integers, floats, SIMD vectors, pointers and function pointers can be used as arguments for inline assembly error: aborting due to 1 previous error