From eb3cc5f824d74eb50ad1bdb3105045bd8eb135ef Mon Sep 17 00:00:00 2001 From: Scott McMurray Date: Sun, 14 Jul 2024 22:02:13 -0700 Subject: [PATCH 1/6] Use Option's discriminant as its size hint --- library/core/src/option.rs | 24 +++++++++++++++------- tests/codegen/option-as-slice.rs | 35 +++++++++++++++++++++++++++++++- 2 files changed, 51 insertions(+), 8 deletions(-) diff --git a/library/core/src/option.rs b/library/core/src/option.rs index 8ec7716012f59..1a8fe1e6051ad 100644 --- a/library/core/src/option.rs +++ b/library/core/src/option.rs @@ -770,6 +770,13 @@ impl Option { } } + #[inline] + const fn len(&self) -> usize { + // Using the intrinsic avoids emitting a branch to get the 0 or 1. + let discriminant: isize = crate::intrinsics::discriminant_value(self); + discriminant as usize + } + /// Returns a slice of the contained value, if any. If this is `None`, an /// empty slice is returned. This can be useful to have a single type of /// iterator over an `Option` or slice. @@ -812,7 +819,7 @@ impl Option { unsafe { slice::from_raw_parts( (self as *const Self).byte_add(core::mem::offset_of!(Self, Some.0)).cast(), - self.is_some() as usize, + self.len(), ) } } @@ -869,7 +876,7 @@ impl Option { unsafe { slice::from_raw_parts_mut( (self as *mut Self).byte_add(core::mem::offset_of!(Self, Some.0)).cast(), - self.is_some() as usize, + self.len(), ) } } @@ -2242,10 +2249,8 @@ impl Iterator for Item { #[inline] fn size_hint(&self) -> (usize, Option) { - match self.opt { - Some(_) => (1, Some(1)), - None => (0, Some(0)), - } + let len = self.len(); + (len, Some(len)) } } @@ -2256,7 +2261,12 @@ impl DoubleEndedIterator for Item { } } -impl ExactSizeIterator for Item {} +impl ExactSizeIterator for Item { + #[inline] + fn len(&self) -> usize { + self.opt.len() + } +} impl FusedIterator for Item {} unsafe impl TrustedLen for Item {} diff --git a/tests/codegen/option-as-slice.rs b/tests/codegen/option-as-slice.rs index 65637a2495d09..cfa479aa4b2f2 100644 --- a/tests/codegen/option-as-slice.rs +++ b/tests/codegen/option-as-slice.rs @@ -14,6 +14,14 @@ pub fn u64_opt_as_slice(o: &Option) -> &[u64] { // CHECK-NOT: br // CHECK-NOT: switch // CHECK-NOT: icmp + // CHECK: %[[LEN:.+]] = load i64,{{.+}} !range ![[META_U64:.+]], !noundef + // CHECK-NOT: select + // CHECK-NOT: br + // CHECK-NOT: switch + // CHECK-NOT: icmp + // CHECK: %[[T0:.+]] = insertvalue { ptr, i64 } poison, ptr %{{.+}}, 0 + // CHECK-NEXT: %[[T1:.+]] = insertvalue { ptr, i64 } %[[T0]], i64 %[[LEN]], 1 + // CHECK-NEXT: ret { ptr, i64 } %[[T1]] o.as_slice() } @@ -25,10 +33,35 @@ pub fn nonzero_u64_opt_as_slice(o: &Option>) -> &[NonZero] { // CHECK-NOT: switch // CHECK-NOT: icmp // CHECK: %[[NZ:.+]] = icmp ne i64 %{{.+}}, 0 - // CHECK-NEXT: zext i1 %[[NZ]] to i64 + // CHECK-NEXT: %[[LEN:.+]] = zext i1 %[[NZ]] to i64 // CHECK-NOT: select // CHECK-NOT: br // CHECK-NOT: switch // CHECK-NOT: icmp + // CHECK: %[[T0:.+]] = insertvalue { ptr, i64 } poison, ptr %o, 0 + // CHECK-NEXT: %[[T1:.+]] = insertvalue { ptr, i64 } %[[T0]], i64 %[[LEN]], 1 + // CHECK-NEXT: ret { ptr, i64 } %[[T1]] o.as_slice() } + +// CHECK-LABEL: @u8_opt_as_slice +#[no_mangle] +pub fn u8_opt_as_slice(o: &Option) -> &[u8] { + // CHECK-NOT: select + // CHECK-NOT: br + // CHECK-NOT: switch + // CHECK-NOT: icmp + // CHECK: %[[TAG:.+]] = load i8,{{.+}} !range ![[META_U8:.+]], !noundef + // CHECK: %[[LEN:.+]] = zext{{.*}} i8 %[[TAG]] to i64 + // CHECK-NOT: select + // CHECK-NOT: br + // CHECK-NOT: switch + // CHECK-NOT: icmp + // CHECK: %[[T0:.+]] = insertvalue { ptr, i64 } poison, ptr %{{.+}}, 0 + // CHECK-NEXT: %[[T1:.+]] = insertvalue { ptr, i64 } %[[T0]], i64 %[[LEN]], 1 + // CHECK-NEXT: ret { ptr, i64 } %[[T1]] + o.as_slice() +} + +// CHECK: ![[META_U64]] = !{i64 0, i64 2} +// CHECK: ![[META_U8]] = !{i8 0, i8 2} From d3303b02b5d11fe84152c454c42a314d78144d9b Mon Sep 17 00:00:00 2001 From: Amos Wenger Date: Thu, 18 Jul 2024 12:39:42 +0200 Subject: [PATCH 2/6] Update extern linking documentation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In particular, remove the note saying cdylibs can't link against dylibs — that hasn't been true for over four years. * 2019-11-07: note is written: https://github.com/rust-lang/rust/commit/b54e8ecc2e0eec9ab9d0b1c1d9cb55f7602800c4 * 2020-01-23: restriction is lifted (without updating docs): https://github.com/rust-lang/rust/commit/72aaa3a414d17aa0c4f19feafa5bab5f84b60e63 --- src/doc/rustc/src/codegen-options/index.md | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/src/doc/rustc/src/codegen-options/index.md b/src/doc/rustc/src/codegen-options/index.md index cd10168bc1cf3..cb43feca758df 100644 --- a/src/doc/rustc/src/codegen-options/index.md +++ b/src/doc/rustc/src/codegen-options/index.md @@ -410,13 +410,16 @@ See also the [`no-prepopulate-passes`](#no-prepopulate-passes) flag. By default, `rustc` prefers to statically link dependencies. This option will indicate that dynamic linking should be used if possible if both a static and -dynamic versions of a library are available. There is an internal algorithm -for determining whether or not it is possible to statically or dynamically -link with a dependency. For example, `cdylib` crate types may only use static -linkage. This flag takes one of the following values: +dynamic versions of a library are available. -* `y`, `yes`, `on`, `true` or no value: use dynamic linking. -* `n`, `no`, `off` or `false`: use static linking (the default). +There is [an internal algorithm](https://github.com/rust-lang/rust/blob/master/compiler/rustc_metadata/src/dependency_format.rs) +for determining whether or not it is possible to statically or dynamically link +with a dependency. + +This flag takes one of the following values: + +* `y`, `yes`, `on`, `true` or no value: prefer dynamic linking. +* `n`, `no`, `off` or `false`: prefer static linking (the default). ## profile-generate From 2507301de0814406175fdb0bbe9da40a348b1f30 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Le=C3=B3n=20Orell=20Valerian=20Liehr?= Date: Wed, 17 Jul 2024 12:35:50 +0200 Subject: [PATCH 3/6] Add internal lint for detecting non-glob imports of `rustc_type_ir::inherent` --- compiler/rustc_lint/messages.ftl | 3 + compiler/rustc_lint/src/internal.rs | 46 ++++++++++++- compiler/rustc_lint/src/lib.rs | 3 + compiler/rustc_lint/src/lints.rs | 8 +++ compiler/rustc_span/src/symbol.rs | 1 + compiler/rustc_type_ir/src/effects.rs | 2 +- compiler/rustc_type_ir/src/lib.rs | 1 + .../non_glob_import_of_type_ir_inherent.rs | 38 +++++++++++ ...non_glob_import_of_type_ir_inherent.stderr | 68 +++++++++++++++++++ 9 files changed, 168 insertions(+), 2 deletions(-) create mode 100644 tests/ui-fulldeps/internal-lints/non_glob_import_of_type_ir_inherent.rs create mode 100644 tests/ui-fulldeps/internal-lints/non_glob_import_of_type_ir_inherent.stderr diff --git a/compiler/rustc_lint/messages.ftl b/compiler/rustc_lint/messages.ftl index de04d882f5163..79d529143811e 100644 --- a/compiler/rustc_lint/messages.ftl +++ b/compiler/rustc_lint/messages.ftl @@ -556,6 +556,9 @@ lint_non_fmt_panic_unused = } .add_fmt_suggestion = or add a "{"{"}{"}"}" format string to use the message literally +lint_non_glob_import_type_ir_inherent = non-glob import of `rustc_type_ir::inherent` + .suggestion = try using a glob import instead + lint_non_local_definitions_cargo_update = the {$macro_kind} `{$macro_name}` may come from an old version of the `{$crate_name}` crate, try updating your dependency with `cargo update -p {$crate_name}` lint_non_local_definitions_deprecation = this lint may become deny-by-default in the edition 2024 and higher, see the tracking issue diff --git a/compiler/rustc_lint/src/internal.rs b/compiler/rustc_lint/src/internal.rs index 772cc2ff8b997..e15eb90f82708 100644 --- a/compiler/rustc_lint/src/internal.rs +++ b/compiler/rustc_lint/src/internal.rs @@ -3,7 +3,8 @@ use crate::lints::{ BadOptAccessDiag, DefaultHashTypesDiag, DiagOutOfImpl, LintPassByHand, NonExistentDocKeyword, - QueryInstability, SpanUseEqCtxtDiag, TyQualified, TykindDiag, TykindKind, UntranslatableDiag, + NonGlobImportTypeIrInherent, QueryInstability, SpanUseEqCtxtDiag, TyQualified, TykindDiag, + TykindKind, UntranslatableDiag, }; use crate::{EarlyContext, EarlyLintPass, LateContext, LateLintPass, LintContext}; use rustc_ast as ast; @@ -263,6 +264,49 @@ fn gen_args(segment: &PathSegment<'_>) -> String { String::new() } +declare_tool_lint! { + /// The `non_glob_import_of_type_ir_inherent_item` lint detects + /// non-glob imports of module `rustc_type_ir::inherent`. + pub rustc::NON_GLOB_IMPORT_OF_TYPE_IR_INHERENT, + Allow, + "non-glob import of `rustc_type_ir::inherent`", + report_in_external_macro: true +} + +declare_lint_pass!(TypeIr => [NON_GLOB_IMPORT_OF_TYPE_IR_INHERENT]); + +impl<'tcx> LateLintPass<'tcx> for TypeIr { + fn check_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx Item<'tcx>) { + let rustc_hir::ItemKind::Use(path, kind) = item.kind else { return }; + + let is_mod_inherent = |def_id| cx.tcx.is_diagnostic_item(sym::type_ir_inherent, def_id); + let (lo, hi, snippet) = match path.segments { + [.., penultimate, segment] + if penultimate.res.opt_def_id().is_some_and(is_mod_inherent) => + { + (segment.ident.span, item.ident.span, "*") + } + [.., segment] + if path.res.iter().flat_map(Res::opt_def_id).any(is_mod_inherent) + && let rustc_hir::UseKind::Single = kind => + { + let (lo, snippet) = + match cx.tcx.sess.source_map().span_to_snippet(path.span).as_deref() { + Ok("self") => (path.span, "*"), + _ => (segment.ident.span.shrink_to_hi(), "::*"), + }; + (lo, if segment.ident == item.ident { lo } else { item.ident.span }, snippet) + } + _ => return, + }; + cx.emit_span_lint( + NON_GLOB_IMPORT_OF_TYPE_IR_INHERENT, + path.span, + NonGlobImportTypeIrInherent { suggestion: lo.eq_ctxt(hi).then(|| lo.to(hi)), snippet }, + ); + } +} + declare_tool_lint! { /// The `lint_pass_impl_without_macro` detects manual implementations of a lint /// pass, without using [`declare_lint_pass`] or [`impl_lint_pass`]. diff --git a/compiler/rustc_lint/src/lib.rs b/compiler/rustc_lint/src/lib.rs index 8be8996e4c8f0..68f66ca5c0d87 100644 --- a/compiler/rustc_lint/src/lib.rs +++ b/compiler/rustc_lint/src/lib.rs @@ -573,6 +573,8 @@ fn register_internals(store: &mut LintStore) { store.register_late_mod_pass(|_| Box::new(ExistingDocKeyword)); store.register_lints(&TyTyKind::get_lints()); store.register_late_mod_pass(|_| Box::new(TyTyKind)); + store.register_lints(&TypeIr::get_lints()); + store.register_late_mod_pass(|_| Box::new(TypeIr)); store.register_lints(&Diagnostics::get_lints()); store.register_late_mod_pass(|_| Box::new(Diagnostics)); store.register_lints(&BadOptAccess::get_lints()); @@ -596,6 +598,7 @@ fn register_internals(store: &mut LintStore) { LintId::of(PASS_BY_VALUE), LintId::of(LINT_PASS_IMPL_WITHOUT_MACRO), LintId::of(USAGE_OF_QUALIFIED_TY), + LintId::of(NON_GLOB_IMPORT_OF_TYPE_IR_INHERENT), LintId::of(EXISTING_DOC_KEYWORD), LintId::of(BAD_OPT_ACCESS), LintId::of(SPAN_USE_EQ_CTXT), diff --git a/compiler/rustc_lint/src/lints.rs b/compiler/rustc_lint/src/lints.rs index 308bb73f4cea6..ac5511faf1961 100644 --- a/compiler/rustc_lint/src/lints.rs +++ b/compiler/rustc_lint/src/lints.rs @@ -926,6 +926,14 @@ pub struct TyQualified { pub suggestion: Span, } +#[derive(LintDiagnostic)] +#[diag(lint_non_glob_import_type_ir_inherent)] +pub struct NonGlobImportTypeIrInherent { + #[suggestion(code = "{snippet}", applicability = "maybe-incorrect")] + pub suggestion: Option, + pub snippet: &'static str, +} + #[derive(LintDiagnostic)] #[diag(lint_lintpass_by_hand)] #[help] diff --git a/compiler/rustc_span/src/symbol.rs b/compiler/rustc_span/src/symbol.rs index 2fe7c951793f7..b64efadb2619e 100644 --- a/compiler/rustc_span/src/symbol.rs +++ b/compiler/rustc_span/src/symbol.rs @@ -1926,6 +1926,7 @@ symbols! { type_ascription, type_changing_struct_update, type_id, + type_ir_inherent, type_length_limit, type_macros, type_name, diff --git a/compiler/rustc_type_ir/src/effects.rs b/compiler/rustc_type_ir/src/effects.rs index f7942f2f982f5..0f28355c4b88e 100644 --- a/compiler/rustc_type_ir/src/effects.rs +++ b/compiler/rustc_type_ir/src/effects.rs @@ -1,4 +1,4 @@ -use crate::inherent::{AdtDef, IntoKind, Ty}; +use crate::inherent::*; use crate::lang_items::TraitSolverLangItem::{EffectsMaybe, EffectsNoRuntime, EffectsRuntime}; use crate::Interner; diff --git a/compiler/rustc_type_ir/src/lib.rs b/compiler/rustc_type_ir/src/lib.rs index 37ee66fa222ae..80e970a23a9d1 100644 --- a/compiler/rustc_type_ir/src/lib.rs +++ b/compiler/rustc_type_ir/src/lib.rs @@ -24,6 +24,7 @@ pub mod elaborate; pub mod error; pub mod fast_reject; pub mod fold; +#[cfg_attr(feature = "nightly", rustc_diagnostic_item = "type_ir_inherent")] pub mod inherent; pub mod ir_print; pub mod lang_items; diff --git a/tests/ui-fulldeps/internal-lints/non_glob_import_of_type_ir_inherent.rs b/tests/ui-fulldeps/internal-lints/non_glob_import_of_type_ir_inherent.rs new file mode 100644 index 0000000000000..33e10a0071330 --- /dev/null +++ b/tests/ui-fulldeps/internal-lints/non_glob_import_of_type_ir_inherent.rs @@ -0,0 +1,38 @@ +//@ compile-flags: -Z unstable-options +//@ ignore-stage1 (can be removed after beta bump, #[cfg(bootstrap)]) +#![feature(rustc_private)] +#![deny(rustc::non_glob_import_of_type_ir_inherent)] + +extern crate rustc_type_ir; + +mod ok { + use rustc_type_ir::inherent::*; // OK + use rustc_type_ir::inherent::{}; // OK + use rustc_type_ir::inherent::{*}; // OK + + fn usage() {} // OK +} + +mod direct { + use rustc_type_ir::inherent::Predicate; //~ ERROR non-glob import of `rustc_type_ir::inherent` + use rustc_type_ir::inherent::{AdtDef, Ty}; + //~^ ERROR non-glob import of `rustc_type_ir::inherent` + //~| ERROR non-glob import of `rustc_type_ir::inherent` + use rustc_type_ir::inherent::ParamEnv as _; //~ ERROR non-glob import of `rustc_type_ir::inherent` +} + +mod indirect0 { + use rustc_type_ir::inherent; //~ ERROR non-glob import of `rustc_type_ir::inherent` + use rustc_type_ir::inherent as inh; //~ ERROR non-glob import of `rustc_type_ir::inherent` + use rustc_type_ir::{inherent as _}; //~ ERROR non-glob import of `rustc_type_ir::inherent` + + fn usage0() {} + fn usage1() {} +} + +mod indirect1 { + use rustc_type_ir::inherent::{self}; //~ ERROR non-glob import of `rustc_type_ir::inherent` + use rustc_type_ir::inherent::{self as innate}; //~ ERROR non-glob import of `rustc_type_ir::inherent` +} + +fn main() {} diff --git a/tests/ui-fulldeps/internal-lints/non_glob_import_of_type_ir_inherent.stderr b/tests/ui-fulldeps/internal-lints/non_glob_import_of_type_ir_inherent.stderr new file mode 100644 index 0000000000000..84e3867c95bc1 --- /dev/null +++ b/tests/ui-fulldeps/internal-lints/non_glob_import_of_type_ir_inherent.stderr @@ -0,0 +1,68 @@ +error: non-glob import of `rustc_type_ir::inherent` + --> $DIR/non_glob_import_of_type_ir_inherent.rs:17:9 + | +LL | use rustc_type_ir::inherent::Predicate; + | ^^^^^^^^^^^^^^^^^^^^^^^^^--------- + | | + | help: try using a glob import instead: `*` + | +note: the lint level is defined here + --> $DIR/non_glob_import_of_type_ir_inherent.rs:4:9 + | +LL | #![deny(rustc::non_glob_import_of_type_ir_inherent)] + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: non-glob import of `rustc_type_ir::inherent` + --> $DIR/non_glob_import_of_type_ir_inherent.rs:18:35 + | +LL | use rustc_type_ir::inherent::{AdtDef, Ty}; + | ^^^^^^ help: try using a glob import instead: `*` + +error: non-glob import of `rustc_type_ir::inherent` + --> $DIR/non_glob_import_of_type_ir_inherent.rs:18:43 + | +LL | use rustc_type_ir::inherent::{AdtDef, Ty}; + | ^^ help: try using a glob import instead: `*` + +error: non-glob import of `rustc_type_ir::inherent` + --> $DIR/non_glob_import_of_type_ir_inherent.rs:21:9 + | +LL | use rustc_type_ir::inherent::ParamEnv as _; + | ^^^^^^^^^^^^^^^^^^^^^^^^^------------- + | | + | help: try using a glob import instead: `*` + +error: non-glob import of `rustc_type_ir::inherent` + --> $DIR/non_glob_import_of_type_ir_inherent.rs:25:9 + | +LL | use rustc_type_ir::inherent; + | ^^^^^^^^^^^^^^^^^^^^^^^- help: try using a glob import instead: `::*` + +error: non-glob import of `rustc_type_ir::inherent` + --> $DIR/non_glob_import_of_type_ir_inherent.rs:26:9 + | +LL | use rustc_type_ir::inherent as inh; + | ^^^^^^^^^^^^^^^^^^^^^^^------- help: try using a glob import instead: `::*` + +error: non-glob import of `rustc_type_ir::inherent` + --> $DIR/non_glob_import_of_type_ir_inherent.rs:27:25 + | +LL | use rustc_type_ir::{inherent as _}; + | ^^^^^^^^----- help: try using a glob import instead: `::*` + +error: non-glob import of `rustc_type_ir::inherent` + --> $DIR/non_glob_import_of_type_ir_inherent.rs:34:35 + | +LL | use rustc_type_ir::inherent::{self}; + | ^^^^ help: try using a glob import instead: `*` + +error: non-glob import of `rustc_type_ir::inherent` + --> $DIR/non_glob_import_of_type_ir_inherent.rs:35:35 + | +LL | use rustc_type_ir::inherent::{self as innate}; + | ^^^^---------- + | | + | help: try using a glob import instead: `*` + +error: aborting due to 9 previous errors + From 0871175a4da35e2f21f1b0c03670a265fd64f465 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Thu, 18 Jul 2024 12:06:03 +0200 Subject: [PATCH 4/6] make pub_use_of_private_extern_crate show up in future breakage reports --- compiler/rustc_lint_defs/src/builtin.rs | 8 ++++---- .../pub/pub-reexport-priv-extern-crate.stderr | 17 ++++++++++++++++- 2 files changed, 20 insertions(+), 5 deletions(-) diff --git a/compiler/rustc_lint_defs/src/builtin.rs b/compiler/rustc_lint_defs/src/builtin.rs index aa7844f40121b..04764b71b1002 100644 --- a/compiler/rustc_lint_defs/src/builtin.rs +++ b/compiler/rustc_lint_defs/src/builtin.rs @@ -1203,16 +1203,16 @@ declare_lint! { /// This was historically allowed, but is not the intended behavior /// according to the visibility rules. This is a [future-incompatible] /// lint to transition this to a hard error in the future. See [issue - /// #34537] for more details. + /// #127909] for more details. /// - /// [issue #34537]: https://github.com/rust-lang/rust/issues/34537 + /// [issue #127909]: https://github.com/rust-lang/rust/issues/127909 /// [future-incompatible]: ../index.md#future-incompatible-lints pub PUB_USE_OF_PRIVATE_EXTERN_CRATE, Deny, "detect public re-exports of private extern crates", @future_incompatible = FutureIncompatibleInfo { - reason: FutureIncompatibilityReason::FutureReleaseErrorDontReportInDeps, - reference: "issue #34537 ", + reason: FutureIncompatibilityReason::FutureReleaseErrorReportInDeps, + reference: "issue #127909 ", }; } diff --git a/tests/ui/pub/pub-reexport-priv-extern-crate.stderr b/tests/ui/pub/pub-reexport-priv-extern-crate.stderr index 915d07fd08a6c..8ab6e83641d3a 100644 --- a/tests/ui/pub/pub-reexport-priv-extern-crate.stderr +++ b/tests/ui/pub/pub-reexport-priv-extern-crate.stderr @@ -29,7 +29,7 @@ LL | pub use core as reexported_core; | ^^^^^^^^^^^^^^^^^^^^^^^ | = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release! - = note: for more information, see issue #34537 + = note: for more information, see issue #127909 = note: `#[deny(pub_use_of_private_extern_crate)]` on by default help: consider making the `extern crate` item publicly accessible | @@ -40,3 +40,18 @@ error: aborting due to 3 previous errors Some errors have detailed explanations: E0365, E0603. For more information about an error, try `rustc --explain E0365`. +Future incompatibility report: Future breakage diagnostic: +error[E0365]: extern crate `core` is private and cannot be re-exported + --> $DIR/pub-reexport-priv-extern-crate.rs:2:9 + | +LL | pub use core as reexported_core; + | ^^^^^^^^^^^^^^^^^^^^^^^ + | + = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release! + = note: for more information, see issue #127909 + = note: `#[deny(pub_use_of_private_extern_crate)]` on by default +help: consider making the `extern crate` item publicly accessible + | +LL | pub extern crate core; + | +++ + From 4dc8e66074ac25ebbaffbdd1872febff9ac444c5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Ber=C3=A1nek?= Date: Thu, 18 Jul 2024 15:49:10 +0200 Subject: [PATCH 5/6] Allow a git command for getting the current branch in bootstrap to fail It can fail when in git is in detached HEAD mode. --- src/bootstrap/src/lib.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/bootstrap/src/lib.rs b/src/bootstrap/src/lib.rs index db1fa05a82cb6..a77c20067e6bc 100644 --- a/src/bootstrap/src/lib.rs +++ b/src/bootstrap/src/lib.rs @@ -535,6 +535,7 @@ impl Build { // even though that has no relation to the upstream for the submodule. let current_branch = helpers::git(Some(&self.src)) .capture_stdout() + .allow_failure() .run_always() .args(["symbolic-ref", "--short", "HEAD"]) .run(self) From fe89962237e425f661ca36dd36113340188ac057 Mon Sep 17 00:00:00 2001 From: Markus Everling Date: Wed, 8 May 2024 06:48:07 +0000 Subject: [PATCH 6/6] Update `ReentrantLock` implementation, add `CURRENT_ID` thread local. This changes `ReentrantLock` to use `ThreadId` for the thread ownership check instead of the address of a thread local. Unlike TLS blocks, `ThreadId` is guaranteed to be unique across the lifetime of the process, so if any thread ever terminates while holding a `ReentrantLockGuard`, no other thread may ever acquire that lock again. On platforms with 64-bit atomics, this is a very simple change. On other platforms, the approach used is slightly more involved, as explained in the module comment. This also adds a `CURRENT_ID` thread local in addition to the already existing `CURRENT`. This allows us to access the current `ThreadId` without the relatively heavy machinery used by `thread::current().id()`. --- library/std/src/sync/reentrant_lock.rs | 138 ++++++++++++++++++++----- library/std/src/thread/mod.rs | 32 +++++- 2 files changed, 144 insertions(+), 26 deletions(-) diff --git a/library/std/src/sync/reentrant_lock.rs b/library/std/src/sync/reentrant_lock.rs index 80b9e0cf15214..3cf2af2801105 100644 --- a/library/std/src/sync/reentrant_lock.rs +++ b/library/std/src/sync/reentrant_lock.rs @@ -1,12 +1,14 @@ #[cfg(all(test, not(target_os = "emscripten")))] mod tests; +use cfg_if::cfg_if; + use crate::cell::UnsafeCell; use crate::fmt; use crate::ops::Deref; use crate::panic::{RefUnwindSafe, UnwindSafe}; -use crate::sync::atomic::{AtomicUsize, Ordering::Relaxed}; use crate::sys::sync as sys; +use crate::thread::{current_id, ThreadId}; /// A re-entrant mutual exclusion lock /// @@ -53,8 +55,8 @@ use crate::sys::sync as sys; // // The 'owner' field tracks which thread has locked the mutex. // -// We use current_thread_unique_ptr() as the thread identifier, -// which is just the address of a thread local variable. +// We use thread::current_id() as the thread identifier, which is just the +// current thread's ThreadId, so it's unique across the process lifetime. // // If `owner` is set to the identifier of the current thread, // we assume the mutex is already locked and instead of locking it again, @@ -72,14 +74,109 @@ use crate::sys::sync as sys; // since we're not dealing with multiple threads. If it's not equal, // synchronization is left to the mutex, making relaxed memory ordering for // the `owner` field fine in all cases. +// +// On systems without 64 bit atomics we also store the address of a TLS variable +// along the 64-bit TID. We then first check that address against the address +// of that variable on the current thread, and only if they compare equal do we +// compare the actual TIDs. Because we only ever read the TID on the same thread +// that it was written on (or a thread sharing the TLS block with that writer thread), +// we don't need to further synchronize the TID accesses, so they can be regular 64-bit +// non-atomic accesses. #[unstable(feature = "reentrant_lock", issue = "121440")] pub struct ReentrantLock { mutex: sys::Mutex, - owner: AtomicUsize, + owner: Tid, lock_count: UnsafeCell, data: T, } +cfg_if!( + if #[cfg(target_has_atomic = "64")] { + use crate::sync::atomic::{AtomicU64, Ordering::Relaxed}; + + struct Tid(AtomicU64); + + impl Tid { + const fn new() -> Self { + Self(AtomicU64::new(0)) + } + + #[inline] + fn contains(&self, owner: ThreadId) -> bool { + owner.as_u64().get() == self.0.load(Relaxed) + } + + #[inline] + // This is just unsafe to match the API of the Tid type below. + unsafe fn set(&self, tid: Option) { + let value = tid.map_or(0, |tid| tid.as_u64().get()); + self.0.store(value, Relaxed); + } + } + } else { + /// Returns the address of a TLS variable. This is guaranteed to + /// be unique across all currently alive threads. + fn tls_addr() -> usize { + thread_local! { static X: u8 = const { 0u8 } }; + + X.with(|p| <*const u8>::addr(p)) + } + + use crate::sync::atomic::{ + AtomicUsize, + Ordering, + }; + + struct Tid { + // When a thread calls `set()`, this value gets updated to + // the address of a thread local on that thread. This is + // used as a first check in `contains()`; if the `tls_addr` + // doesn't match the TLS address of the current thread, then + // the ThreadId also can't match. Only if the TLS addresses do + // match do we read out the actual TID. + // Note also that we can use relaxed atomic operations here, because + // we only ever read from the tid if `tls_addr` matches the current + // TLS address. In that case, either the the tid has been set by + // the current thread, or by a thread that has terminated before + // the current thread was created. In either case, no further + // synchronization is needed (as per ) + tls_addr: AtomicUsize, + tid: UnsafeCell, + } + + unsafe impl Send for Tid {} + unsafe impl Sync for Tid {} + + impl Tid { + const fn new() -> Self { + Self { tls_addr: AtomicUsize::new(0), tid: UnsafeCell::new(0) } + } + + #[inline] + // NOTE: This assumes that `owner` is the ID of the current + // thread, and may spuriously return `false` if that's not the case. + fn contains(&self, owner: ThreadId) -> bool { + // SAFETY: See the comments in the struct definition. + self.tls_addr.load(Ordering::Relaxed) == tls_addr() + && unsafe { *self.tid.get() } == owner.as_u64().get() + } + + #[inline] + // This may only be called by one thread at a time, and can lead to + // race conditions otherwise. + unsafe fn set(&self, tid: Option) { + // It's important that we set `self.tls_addr` to 0 if the tid is + // cleared. Otherwise, there might be race conditions between + // `set()` and `get()`. + let tls_addr = if tid.is_some() { tls_addr() } else { 0 }; + let value = tid.map_or(0, |tid| tid.as_u64().get()); + self.tls_addr.store(tls_addr, Ordering::Relaxed); + unsafe { *self.tid.get() = value }; + } + } + } +); + #[unstable(feature = "reentrant_lock", issue = "121440")] unsafe impl Send for ReentrantLock {} #[unstable(feature = "reentrant_lock", issue = "121440")] @@ -131,7 +228,7 @@ impl ReentrantLock { pub const fn new(t: T) -> ReentrantLock { ReentrantLock { mutex: sys::Mutex::new(), - owner: AtomicUsize::new(0), + owner: Tid::new(), lock_count: UnsafeCell::new(0), data: t, } @@ -181,14 +278,16 @@ impl ReentrantLock { /// assert_eq!(lock.lock().get(), 10); /// ``` pub fn lock(&self) -> ReentrantLockGuard<'_, T> { - let this_thread = current_thread_unique_ptr(); - // Safety: We only touch lock_count when we own the lock. + let this_thread = current_id(); + // Safety: We only touch lock_count when we own the inner mutex. + // Additionally, we only call `self.owner.set()` while holding + // the inner mutex, so no two threads can call it concurrently. unsafe { - if self.owner.load(Relaxed) == this_thread { + if self.owner.contains(this_thread) { self.increment_lock_count().expect("lock count overflow in reentrant mutex"); } else { self.mutex.lock(); - self.owner.store(this_thread, Relaxed); + self.owner.set(Some(this_thread)); debug_assert_eq!(*self.lock_count.get(), 0); *self.lock_count.get() = 1; } @@ -223,14 +322,16 @@ impl ReentrantLock { /// /// This function does not block. pub(crate) fn try_lock(&self) -> Option> { - let this_thread = current_thread_unique_ptr(); - // Safety: We only touch lock_count when we own the lock. + let this_thread = current_id(); + // Safety: We only touch lock_count when we own the inner mutex. + // Additionally, we only call `self.owner.set()` while holding + // the inner mutex, so no two threads can call it concurrently. unsafe { - if self.owner.load(Relaxed) == this_thread { + if self.owner.contains(this_thread) { self.increment_lock_count()?; Some(ReentrantLockGuard { lock: self }) } else if self.mutex.try_lock() { - self.owner.store(this_thread, Relaxed); + self.owner.set(Some(this_thread)); debug_assert_eq!(*self.lock_count.get(), 0); *self.lock_count.get() = 1; Some(ReentrantLockGuard { lock: self }) @@ -303,18 +404,9 @@ impl Drop for ReentrantLockGuard<'_, T> { unsafe { *self.lock.lock_count.get() -= 1; if *self.lock.lock_count.get() == 0 { - self.lock.owner.store(0, Relaxed); + self.lock.owner.set(None); self.lock.mutex.unlock(); } } } } - -/// Get an address that is unique per running thread. -/// -/// This can be used as a non-null usize-sized ID. -pub(crate) fn current_thread_unique_ptr() -> usize { - // Use a non-drop type to make sure it's still available during thread destruction. - thread_local! { static X: u8 = const { 0 } } - X.with(|x| <*const _>::addr(x)) -} diff --git a/library/std/src/thread/mod.rs b/library/std/src/thread/mod.rs index 22215873933d6..d78b896cd78cd 100644 --- a/library/std/src/thread/mod.rs +++ b/library/std/src/thread/mod.rs @@ -159,7 +159,7 @@ mod tests; use crate::any::Any; -use crate::cell::{OnceCell, UnsafeCell}; +use crate::cell::{Cell, OnceCell, UnsafeCell}; use crate::env; use crate::ffi::{CStr, CString}; use crate::fmt; @@ -698,17 +698,22 @@ where } thread_local! { + // Invariant: `CURRENT` and `CURRENT_ID` will always be initialized together. + // If `CURRENT` is initialized, then `CURRENT_ID` will hold the same value + // as `CURRENT.id()`. static CURRENT: OnceCell = const { OnceCell::new() }; + static CURRENT_ID: Cell> = const { Cell::new(None) }; } /// Sets the thread handle for the current thread. /// /// Aborts if the handle has been set already to reduce code size. pub(crate) fn set_current(thread: Thread) { + let tid = thread.id(); // Using `unwrap` here can add ~3kB to the binary size. We have complete // control over where this is called, so just abort if there is a bug. CURRENT.with(|current| match current.set(thread) { - Ok(()) => {} + Ok(()) => CURRENT_ID.set(Some(tid)), Err(_) => rtabort!("thread::set_current should only be called once per thread"), }); } @@ -718,7 +723,28 @@ pub(crate) fn set_current(thread: Thread) { /// In contrast to the public `current` function, this will not panic if called /// from inside a TLS destructor. pub(crate) fn try_current() -> Option { - CURRENT.try_with(|current| current.get_or_init(|| Thread::new_unnamed()).clone()).ok() + CURRENT + .try_with(|current| { + current + .get_or_init(|| { + let thread = Thread::new_unnamed(); + CURRENT_ID.set(Some(thread.id())); + thread + }) + .clone() + }) + .ok() +} + +/// Gets the id of the thread that invokes it. +#[inline] +pub(crate) fn current_id() -> ThreadId { + CURRENT_ID.get().unwrap_or_else(|| { + // If `CURRENT_ID` isn't initialized yet, then `CURRENT` must also not be initialized. + // `current()` will initialize both `CURRENT` and `CURRENT_ID` so subsequent calls to + // `current_id()` will succeed immediately. + current().id() + }) } /// Gets a handle to the thread that invokes it.