From 5a2dbd0f505afdcefb1cc102be9c448ae0c87d21 Mon Sep 17 00:00:00 2001 From: Pietro Albini Date: Mon, 17 May 2021 12:03:49 +0200 Subject: [PATCH 01/11] backport 1.52.1 release notes --- RELEASES.md | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/RELEASES.md b/RELEASES.md index 1f940e6bc2d3b..9767ba33eaa65 100644 --- a/RELEASES.md +++ b/RELEASES.md @@ -1,3 +1,24 @@ +Version 1.52.1 (2021-05-10) +============================ + +This release disables incremental compilation, unless the user has explicitly +opted in via the newly added RUSTC_FORCE_INCREMENTAL=1 environment variable. + +This is due to the widespread, and frequently occuring, breakage encountered by +Rust users due to newly enabled incremental verification in 1.52.0. Notably, +Rust users **should** upgrade to 1.52.0 or 1.52.1: the bugs that are detected by +newly added incremental verification are still present in past stable versions, +and are not yet fixed on any channel. These bugs can lead to miscompilation of +Rust binaries. + +These problems only affect incremental builds, so release builds with Cargo +should not be affected unless the user has explicitly opted into incremental. +Debug and check builds are affected. + +See [84970] for more details. + +[84970]: https://github.com/rust-lang/rust/issues/84970 + Version 1.52.0 (2021-05-06) ============================ From 586b51a78a8d63026d202d51a0735825f38daf74 Mon Sep 17 00:00:00 2001 From: The8472 Date: Sat, 15 May 2021 20:08:09 +0200 Subject: [PATCH 02/11] remove InPlaceIterable marker from Peekable due to unsoundness The unsoundness is not in Peekable per se, it rather is due to the interaction between Peekable being able to hold an extra item and vec::IntoIter's clone implementation shortening the allocation. An alternative solution would be to change IntoIter's clone implementation to keep enough spare capacity available. --- library/alloc/benches/vec.rs | 1 - library/alloc/tests/vec.rs | 1 - library/core/src/iter/adapters/peekable.rs | 5 +---- 3 files changed, 1 insertion(+), 6 deletions(-) diff --git a/library/alloc/benches/vec.rs b/library/alloc/benches/vec.rs index 48709e89823d8..c9bdcaa78f391 100644 --- a/library/alloc/benches/vec.rs +++ b/library/alloc/benches/vec.rs @@ -468,7 +468,6 @@ fn bench_in_place_recycle(b: &mut Bencher) { .enumerate() .map(|(idx, e)| idx.wrapping_add(e)) .fuse() - .peekable() .collect::>(), ); }); diff --git a/library/alloc/tests/vec.rs b/library/alloc/tests/vec.rs index 4dcc5d30debf7..aa606cd23158a 100644 --- a/library/alloc/tests/vec.rs +++ b/library/alloc/tests/vec.rs @@ -1002,7 +1002,6 @@ fn test_from_iter_specialization_with_iterator_adapters() { .zip(std::iter::repeat(1usize)) .map(|(a, b)| a + b) .map_while(Option::Some) - .peekable() .skip(1) .map(|e| if e != usize::MAX { Ok(std::num::NonZeroUsize::new(e)) } else { Err(()) }); assert_in_place_trait(&iter); diff --git a/library/core/src/iter/adapters/peekable.rs b/library/core/src/iter/adapters/peekable.rs index 21386e28a9643..0d860aec49a0e 100644 --- a/library/core/src/iter/adapters/peekable.rs +++ b/library/core/src/iter/adapters/peekable.rs @@ -1,4 +1,4 @@ -use crate::iter::{adapters::SourceIter, FusedIterator, InPlaceIterable, TrustedLen}; +use crate::iter::{adapters::SourceIter, FusedIterator, TrustedLen}; use crate::ops::Try; /// An iterator with a `peek()` that returns an optional reference to the next @@ -333,6 +333,3 @@ where unsafe { SourceIter::as_inner(&mut self.iter) } } } - -#[unstable(issue = "none", feature = "inplace_iteration")] -unsafe impl InPlaceIterable for Peekable {} From c47274b2eb70aae802ed9022f5e4315044abd66e Mon Sep 17 00:00:00 2001 From: The8472 Date: Sat, 15 May 2021 20:41:15 +0200 Subject: [PATCH 03/11] add regression test --- library/alloc/tests/vec.rs | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/library/alloc/tests/vec.rs b/library/alloc/tests/vec.rs index aa606cd23158a..6c71cf924d2a3 100644 --- a/library/alloc/tests/vec.rs +++ b/library/alloc/tests/vec.rs @@ -1094,6 +1094,18 @@ fn test_from_iter_specialization_panic_during_drop_leaks() { } } +// regression test for issue #85322. Peekable previously implemented InPlaceIterable, +// but due to an interaction with IntoIter's current Clone implementation it failed to uphold +// the contract. +#[test] +fn test_collect_after_iterator_clone() { + let v = vec![0; 5]; + let mut i = v.into_iter().peekable(); + i.peek(); + let v = i.clone().collect::>(); + assert!(v.len() <= v.capacity()); +} + #[test] fn test_cow_from() { let borrowed: &[_] = &["borrowed", "(slice)"]; From ff346b98e291d58f3cb2433230d83929410ea1e9 Mon Sep 17 00:00:00 2001 From: the8472 Date: Sun, 16 May 2021 15:35:05 +0200 Subject: [PATCH 04/11] from review: more robust test This also checks the contents and not only the capacity in case IntoIter's clone implementation is changed to add capacity at the end. Extra capacity at the beginning would be needed to make InPlaceIterable work. Co-authored-by: Giacomo Stevanato --- library/alloc/tests/vec.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/library/alloc/tests/vec.rs b/library/alloc/tests/vec.rs index 6c71cf924d2a3..ad69234403b9c 100644 --- a/library/alloc/tests/vec.rs +++ b/library/alloc/tests/vec.rs @@ -1100,12 +1100,12 @@ fn test_from_iter_specialization_panic_during_drop_leaks() { #[test] fn test_collect_after_iterator_clone() { let v = vec![0; 5]; - let mut i = v.into_iter().peekable(); + let mut i = v.into_iter().map(|i| i + 1).peekable(); i.peek(); let v = i.clone().collect::>(); + assert_eq!(v, [1, 1, 1, 1, 1]); assert!(v.len() <= v.capacity()); } - #[test] fn test_cow_from() { let borrowed: &[_] = &["borrowed", "(slice)"]; From 855a573d66aef0adc16be38cde97f1ffca325da1 Mon Sep 17 00:00:00 2001 From: Justus K Date: Fri, 14 May 2021 22:45:00 +0200 Subject: [PATCH 05/11] Call `initSidebarItems` in root module of crate --- src/librustdoc/html/render/mod.rs | 11 ++++++++--- src/librustdoc/html/render/write_shared.rs | 2 ++ src/librustdoc/html/static/sidebar-items.js | 1 + src/librustdoc/html/static_files.rs | 3 +++ 4 files changed, 14 insertions(+), 3 deletions(-) create mode 100644 src/librustdoc/html/static/sidebar-items.js diff --git a/src/librustdoc/html/render/mod.rs b/src/librustdoc/html/render/mod.rs index 15ce3740e05d4..ab500c165e60d 100644 --- a/src/librustdoc/html/render/mod.rs +++ b/src/librustdoc/html/render/mod.rs @@ -1724,12 +1724,17 @@ fn print_sidebar(cx: &Context<'_>, it: &clean::Item, buffer: &mut Buffer) { ty = it.type_(), path = relpath ); + if parentlen == 0 { - // There is no sidebar-items.js beyond the crate root path - // FIXME maybe dynamic crate loading can be merged here + write!( + buffer, + "", + relpath, cx.shared.resource_suffix + ); } else { - write!(buffer, "", path = relpath); + write!(buffer, "", relpath); } + // Closes sidebar-elems div. buffer.write_str(""); } diff --git a/src/librustdoc/html/render/write_shared.rs b/src/librustdoc/html/render/write_shared.rs index 8e10c696df05d..ea19501023a69 100644 --- a/src/librustdoc/html/render/write_shared.rs +++ b/src/librustdoc/html/render/write_shared.rs @@ -224,6 +224,8 @@ pub(super) fn write_shared( )?; write_minify("search.js", static_files::SEARCH_JS)?; write_minify("settings.js", static_files::SETTINGS_JS)?; + write_minify("sidebar-items.js", static_files::sidebar::ITEMS)?; + if cx.shared.include_sources { write_minify("source-script.js", static_files::sidebar::SOURCE_SCRIPT)?; } diff --git a/src/librustdoc/html/static/sidebar-items.js b/src/librustdoc/html/static/sidebar-items.js new file mode 100644 index 0000000000000..81172ba0d9280 --- /dev/null +++ b/src/librustdoc/html/static/sidebar-items.js @@ -0,0 +1 @@ +initSidebarItems({}); diff --git a/src/librustdoc/html/static_files.rs b/src/librustdoc/html/static_files.rs index 2b73bd5d52ee6..6b1e1e9622106 100644 --- a/src/librustdoc/html/static_files.rs +++ b/src/librustdoc/html/static_files.rs @@ -127,4 +127,7 @@ crate mod source_code_pro { crate mod sidebar { /// File script to handle sidebar. crate static SOURCE_SCRIPT: &str = include_str!("static/source-script.js"); + + /// Top Level sidebar items script which will load a sidebar without items. + crate static ITEMS: &str = include_str!("static/sidebar-items.js"); } From 5d1125d0833993679c95c3bb8d6fe37be2295d5d Mon Sep 17 00:00:00 2001 From: Mark Rousskov Date: Sat, 22 May 2021 15:28:47 -0400 Subject: [PATCH 06/11] Synchronize llvm to master commit --- src/llvm-project | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/llvm-project b/src/llvm-project index b61c24f352130..5f67a5715771b 160000 --- a/src/llvm-project +++ b/src/llvm-project @@ -1 +1 @@ -Subproject commit b61c24f3521303d442fa86fe691bc8e6acc15103 +Subproject commit 5f67a5715771b7d29e4713e8d68338602d216dcf From e68af1269106a79cb0e4383872adf7bfb0ad3baf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20K=C3=BCber?= Date: Tue, 4 May 2021 08:41:40 -0700 Subject: [PATCH 07/11] Do not ICE on invalid const param When encountering a path that can't have generics, do not call `generics_of`. This would happen when writing something like `path::this_is_a_mod`. Fix #84831. --- compiler/rustc_typeck/src/collect/type_of.rs | 20 ++++++++++++++- src/test/ui/typeck/issue-84831.rs | 9 +++++++ src/test/ui/typeck/issue-84831.stderr | 26 ++++++++++++++++++++ 3 files changed, 54 insertions(+), 1 deletion(-) create mode 100644 src/test/ui/typeck/issue-84831.rs create mode 100644 src/test/ui/typeck/issue-84831.stderr diff --git a/compiler/rustc_typeck/src/collect/type_of.rs b/compiler/rustc_typeck/src/collect/type_of.rs index 51d5f4ebe2bd2..97b6f5cf41211 100644 --- a/compiler/rustc_typeck/src/collect/type_of.rs +++ b/compiler/rustc_typeck/src/collect/type_of.rs @@ -191,7 +191,25 @@ pub(super) fn opt_const_param_of(tcx: TyCtxt<'_>, def_id: LocalDefId) -> Option< Res::Def(DefKind::Ctor(..), def_id) => { tcx.generics_of(tcx.parent(def_id).unwrap()) } - Res::Def(_, def_id) => tcx.generics_of(def_id), + // Other `DefKind`s don't have generics and would ICE when calling + // `generics_of`. + Res::Def( + DefKind::Struct + | DefKind::Union + | DefKind::Enum + | DefKind::Variant + | DefKind::Trait + | DefKind::OpaqueTy + | DefKind::TyAlias + | DefKind::ForeignTy + | DefKind::TraitAlias + | DefKind::AssocTy + | DefKind::Fn + | DefKind::AssocFn + | DefKind::AssocConst + | DefKind::Impl, + def_id, + ) => tcx.generics_of(def_id), Res::Err => { tcx.sess.delay_span_bug(tcx.def_span(def_id), "anon const with Res::Err"); return None; diff --git a/src/test/ui/typeck/issue-84831.rs b/src/test/ui/typeck/issue-84831.rs new file mode 100644 index 0000000000000..c646f71072532 --- /dev/null +++ b/src/test/ui/typeck/issue-84831.rs @@ -0,0 +1,9 @@ +fn f() { + std::<0>; //~ ERROR expected value +} +fn j() { + std::<_ as _>; //~ ERROR expected value + //~^ ERROR expected one of `,` or `>`, found keyword `as` +} + +fn main () {} diff --git a/src/test/ui/typeck/issue-84831.stderr b/src/test/ui/typeck/issue-84831.stderr new file mode 100644 index 0000000000000..e3cce10a00fd1 --- /dev/null +++ b/src/test/ui/typeck/issue-84831.stderr @@ -0,0 +1,26 @@ +error: expected one of `,` or `>`, found keyword `as` + --> $DIR/issue-84831.rs:5:13 + | +LL | std::<_ as _>; + | ^^ expected one of `,` or `>` + | +help: expressions must be enclosed in braces to be used as const generic arguments + | +LL | std::<{ _ as _ }>; + | ^ ^ + +error[E0423]: expected value, found crate `std` + --> $DIR/issue-84831.rs:2:5 + | +LL | std::<0>; + | ^^^^^^^^ not a value + +error[E0423]: expected value, found crate `std` + --> $DIR/issue-84831.rs:5:5 + | +LL | std::<_ as _>; + | ^^^^^^^^^^^^^ not a value + +error: aborting due to 3 previous errors + +For more information about this error, try `rustc --explain E0423`. From d1e2499b3b5a5e3903c6e3b6d1e2a59f40f0ff00 Mon Sep 17 00:00:00 2001 From: Rich Kadel Date: Mon, 3 May 2021 08:47:43 -0700 Subject: [PATCH 08/11] Disallows `#![feature(no_coverage)]` on stable and beta using allow_internal_unstable (as recommended) Fixes: #84836 ```shell $ ./build/x86_64-unknown-linux-gnu/stage1/bin/rustc src/test/run-make-fulldeps/coverage/no_cov_crate.rs error[E0554]: `#![feature]` may not be used on the dev release channel --> src/test/run-make-fulldeps/coverage/no_cov_crate.rs:2:1 | 2 | #![feature(no_coverage)] | ^^^^^^^^^^^^^^^^^^^^^^^^ error: aborting due to previous error For more information about this error, try `rustc --explain E0554`. ``` --- .../src/deriving/cmp/eq.rs | 14 ++-------- compiler/rustc_feature/src/builtin_attrs.rs | 8 +----- compiler/rustc_typeck/src/collect.rs | 28 +------------------ library/core/src/cmp.rs | 5 ++-- library/core/src/lib.rs | 1 + .../expected_show_coverage.no_cov_func.txt | 19 ------------- .../run-make-fulldeps/coverage/no_cov_func.rs | 18 ------------ .../feature-gates/feature-gate-no_coverage.rs | 13 ++++++--- .../feature-gate-no_coverage.stderr | 3 +- 9 files changed, 18 insertions(+), 91 deletions(-) delete mode 100644 src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.no_cov_func.txt delete mode 100644 src/test/run-make-fulldeps/coverage/no_cov_func.rs diff --git a/compiler/rustc_builtin_macros/src/deriving/cmp/eq.rs b/compiler/rustc_builtin_macros/src/deriving/cmp/eq.rs index 5a4e7fd9d07b4..54ab88dc3ffc9 100644 --- a/compiler/rustc_builtin_macros/src/deriving/cmp/eq.rs +++ b/compiler/rustc_builtin_macros/src/deriving/cmp/eq.rs @@ -15,20 +15,12 @@ pub fn expand_deriving_eq( item: &Annotatable, push: &mut dyn FnMut(Annotatable), ) { + let span = cx.with_def_site_ctxt(span); let inline = cx.meta_word(span, sym::inline); - let no_coverage_ident = - rustc_ast::attr::mk_nested_word_item(Ident::new(sym::no_coverage, span)); - let no_coverage_feature = - rustc_ast::attr::mk_list_item(Ident::new(sym::feature, span), vec![no_coverage_ident]); - let no_coverage = cx.meta_word(span, sym::no_coverage); let hidden = rustc_ast::attr::mk_nested_word_item(Ident::new(sym::hidden, span)); let doc = rustc_ast::attr::mk_list_item(Ident::new(sym::doc, span), vec![hidden]); - let attrs = vec![ - cx.attribute(inline), - cx.attribute(no_coverage_feature), - cx.attribute(no_coverage), - cx.attribute(doc), - ]; + let no_coverage = cx.meta_word(span, sym::no_coverage); + let attrs = vec![cx.attribute(inline), cx.attribute(doc), cx.attribute(no_coverage)]; let trait_def = TraitDef { span, attributes: Vec::new(), diff --git a/compiler/rustc_feature/src/builtin_attrs.rs b/compiler/rustc_feature/src/builtin_attrs.rs index 5474fea9c7857..a8719be84c2a4 100644 --- a/compiler/rustc_feature/src/builtin_attrs.rs +++ b/compiler/rustc_feature/src/builtin_attrs.rs @@ -273,13 +273,7 @@ pub const BUILTIN_ATTRIBUTES: &[BuiltinAttribute] = &[ template!(List: "address, memory, thread"), experimental!(no_sanitize) ), - ungated!( - // Not exclusively gated at the crate level (though crate-level is - // supported). The feature can alternatively be enabled on individual - // functions. - no_coverage, AssumedUsed, - template!(Word), - ), + gated!(no_coverage, AssumedUsed, template!(Word), experimental!(no_coverage)), // FIXME: #14408 assume docs are used since rustdoc looks at them. ungated!(doc, AssumedUsed, template!(List: "hidden|inline|...", NameValueStr: "string")), diff --git a/compiler/rustc_typeck/src/collect.rs b/compiler/rustc_typeck/src/collect.rs index 190c9d35934f9..0528f8812f920 100644 --- a/compiler/rustc_typeck/src/collect.rs +++ b/compiler/rustc_typeck/src/collect.rs @@ -2661,8 +2661,6 @@ fn codegen_fn_attrs(tcx: TyCtxt<'_>, id: DefId) -> CodegenFnAttrs { let mut inline_span = None; let mut link_ordinal_span = None; let mut no_sanitize_span = None; - let mut no_coverage_feature_enabled = false; - let mut no_coverage_attr = None; for attr in attrs.iter() { if tcx.sess.check_name(attr, sym::cold) { codegen_fn_attrs.flags |= CodegenFnAttrFlags::COLD; @@ -2726,15 +2724,8 @@ fn codegen_fn_attrs(tcx: TyCtxt<'_>, id: DefId) -> CodegenFnAttrs { codegen_fn_attrs.flags |= CodegenFnAttrFlags::NAKED; } else if tcx.sess.check_name(attr, sym::no_mangle) { codegen_fn_attrs.flags |= CodegenFnAttrFlags::NO_MANGLE; - } else if attr.has_name(sym::feature) { - if let Some(list) = attr.meta_item_list() { - if list.iter().any(|nested_meta_item| nested_meta_item.has_name(sym::no_coverage)) { - tcx.sess.mark_attr_used(attr); - no_coverage_feature_enabled = true; - } - } } else if tcx.sess.check_name(attr, sym::no_coverage) { - no_coverage_attr = Some(attr); + codegen_fn_attrs.flags |= CodegenFnAttrFlags::NO_COVERAGE; } else if tcx.sess.check_name(attr, sym::rustc_std_internal_symbol) { codegen_fn_attrs.flags |= CodegenFnAttrFlags::RUSTC_STD_INTERNAL_SYMBOL; } else if tcx.sess.check_name(attr, sym::used) { @@ -2945,23 +2936,6 @@ fn codegen_fn_attrs(tcx: TyCtxt<'_>, id: DefId) -> CodegenFnAttrs { } } - if let Some(no_coverage_attr) = no_coverage_attr { - if tcx.sess.features_untracked().no_coverage || no_coverage_feature_enabled { - codegen_fn_attrs.flags |= CodegenFnAttrFlags::NO_COVERAGE - } else { - let mut err = feature_err( - &tcx.sess.parse_sess, - sym::no_coverage, - no_coverage_attr.span, - "the `#[no_coverage]` attribute is an experimental feature", - ); - if tcx.sess.parse_sess.unstable_features.is_nightly_build() { - err.help("or, alternatively, add `#[feature(no_coverage)]` to the function"); - } - err.emit(); - } - } - codegen_fn_attrs.inline = attrs.iter().fold(InlineAttr::None, |ia, attr| { if !attr.has_name(sym::inline) { return ia; diff --git a/library/core/src/cmp.rs b/library/core/src/cmp.rs index 0a3e5789e8bed..f8b16b6f9275c 100644 --- a/library/core/src/cmp.rs +++ b/library/core/src/cmp.rs @@ -274,8 +274,7 @@ pub trait Eq: PartialEq { // // This should never be implemented by hand. #[doc(hidden)] - #[cfg_attr(not(bootstrap), feature(no_coverage))] - #[cfg_attr(not(bootstrap), no_coverage)] + #[cfg_attr(not(bootstrap), no_coverage)] // rust-lang/rust#84605 #[inline] #[stable(feature = "rust1", since = "1.0.0")] fn assert_receiver_is_total_eq(&self) {} @@ -284,7 +283,7 @@ pub trait Eq: PartialEq { /// Derive macro generating an impl of the trait `Eq`. #[rustc_builtin_macro] #[stable(feature = "builtin_macro_prelude", since = "1.38.0")] -#[allow_internal_unstable(core_intrinsics, derive_eq, structural_match)] +#[allow_internal_unstable(core_intrinsics, derive_eq, structural_match, no_coverage)] pub macro Eq($item:item) { /* compiler built-in */ } diff --git a/library/core/src/lib.rs b/library/core/src/lib.rs index 0e2c140c367a9..d1329b8f22116 100644 --- a/library/core/src/lib.rs +++ b/library/core/src/lib.rs @@ -166,6 +166,7 @@ #![feature(const_caller_location)] #![feature(slice_ptr_get)] #![feature(no_niche)] // rust-lang/rust#68303 +#![cfg_attr(not(bootstrap), feature(no_coverage))] // rust-lang/rust#84605 #![feature(int_error_matching)] #![deny(unsafe_op_in_unsafe_fn)] diff --git a/src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.no_cov_func.txt b/src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.no_cov_func.txt deleted file mode 100644 index 16eaf7c858c19..0000000000000 --- a/src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.no_cov_func.txt +++ /dev/null @@ -1,19 +0,0 @@ - 1| |// Enables `no_coverage` on individual functions - 2| | - 3| |#[feature(no_coverage)] - 4| |#[no_coverage] - 5| |fn do_not_add_coverage_1() { - 6| | println!("called but not covered"); - 7| |} - 8| | - 9| |#[no_coverage] - 10| |#[feature(no_coverage)] - 11| |fn do_not_add_coverage_2() { - 12| | println!("called but not covered"); - 13| |} - 14| | - 15| 1|fn main() { - 16| 1| do_not_add_coverage_1(); - 17| 1| do_not_add_coverage_2(); - 18| 1|} - diff --git a/src/test/run-make-fulldeps/coverage/no_cov_func.rs b/src/test/run-make-fulldeps/coverage/no_cov_func.rs deleted file mode 100644 index e19a2c4a87200..0000000000000 --- a/src/test/run-make-fulldeps/coverage/no_cov_func.rs +++ /dev/null @@ -1,18 +0,0 @@ -// Enables `no_coverage` on individual functions - -#[feature(no_coverage)] -#[no_coverage] -fn do_not_add_coverage_1() { - println!("called but not covered"); -} - -#[no_coverage] -#[feature(no_coverage)] -fn do_not_add_coverage_2() { - println!("called but not covered"); -} - -fn main() { - do_not_add_coverage_1(); - do_not_add_coverage_2(); -} diff --git a/src/test/ui/feature-gates/feature-gate-no_coverage.rs b/src/test/ui/feature-gates/feature-gate-no_coverage.rs index c6b79f9a43171..fd4c6f76059aa 100644 --- a/src/test/ui/feature-gates/feature-gate-no_coverage.rs +++ b/src/test/ui/feature-gates/feature-gate-no_coverage.rs @@ -1,8 +1,13 @@ #![crate_type = "lib"] -#[no_coverage] -#[feature(no_coverage)] // does not have to be enabled before `#[no_coverage]` -fn no_coverage_is_enabled_on_this_function() {} +#[derive(PartialEq, Eq)] // ensure deriving `Eq` does not enable `feature(no_coverage)` +struct Foo { + a: u8, + b: u32, +} #[no_coverage] //~ ERROR the `#[no_coverage]` attribute is an experimental feature -fn requires_feature_no_coverage() {} +fn requires_feature_no_coverage() -> bool { + let bar = Foo { a: 0, b: 0 }; + bar == Foo { a: 0, b: 0 } +} diff --git a/src/test/ui/feature-gates/feature-gate-no_coverage.stderr b/src/test/ui/feature-gates/feature-gate-no_coverage.stderr index 04627be4aaf65..f7167e0b771c0 100644 --- a/src/test/ui/feature-gates/feature-gate-no_coverage.stderr +++ b/src/test/ui/feature-gates/feature-gate-no_coverage.stderr @@ -1,12 +1,11 @@ error[E0658]: the `#[no_coverage]` attribute is an experimental feature - --> $DIR/feature-gate-no_coverage.rs:7:1 + --> $DIR/feature-gate-no_coverage.rs:9:1 | LL | #[no_coverage] | ^^^^^^^^^^^^^^ | = note: see issue #84605 for more information = help: add `#![feature(no_coverage)]` to the crate attributes to enable - = help: or, alternatively, add `#[feature(no_coverage)]` to the function error: aborting due to previous error From c297d1e61bde36b840172a44b9fc6b97a37be6d6 Mon Sep 17 00:00:00 2001 From: Mohsen Zohrevandi Date: Wed, 21 Apr 2021 13:45:57 -0700 Subject: [PATCH 09/11] Ensure TLS destructors run before thread joins in SGX --- library/std/src/sys/sgx/abi/mod.rs | 6 ++- library/std/src/sys/sgx/thread.rs | 69 +++++++++++++++++++++++---- library/std/src/thread/local/tests.rs | 55 ++++++++++++++++++++- 3 files changed, 119 insertions(+), 11 deletions(-) diff --git a/library/std/src/sys/sgx/abi/mod.rs b/library/std/src/sys/sgx/abi/mod.rs index a5e453034762c..f9536c4203df2 100644 --- a/library/std/src/sys/sgx/abi/mod.rs +++ b/library/std/src/sys/sgx/abi/mod.rs @@ -62,10 +62,12 @@ unsafe extern "C" fn tcs_init(secondary: bool) { extern "C" fn entry(p1: u64, p2: u64, p3: u64, secondary: bool, p4: u64, p5: u64) -> EntryReturn { // FIXME: how to support TLS in library mode? let tls = Box::new(tls::Tls::new()); - let _tls_guard = unsafe { tls.activate() }; + let tls_guard = unsafe { tls.activate() }; if secondary { - super::thread::Thread::entry(); + let join_notifier = super::thread::Thread::entry(); + drop(tls_guard); + drop(join_notifier); EntryReturn(0, 0) } else { diff --git a/library/std/src/sys/sgx/thread.rs b/library/std/src/sys/sgx/thread.rs index 55ef460cc90c5..67e2e8b59d397 100644 --- a/library/std/src/sys/sgx/thread.rs +++ b/library/std/src/sys/sgx/thread.rs @@ -9,26 +9,37 @@ pub struct Thread(task_queue::JoinHandle); pub const DEFAULT_MIN_STACK_SIZE: usize = 4096; +pub use self::task_queue::JoinNotifier; + mod task_queue { - use crate::sync::mpsc; + use super::wait_notify; use crate::sync::{Mutex, MutexGuard, Once}; - pub type JoinHandle = mpsc::Receiver<()>; + pub type JoinHandle = wait_notify::Waiter; + + pub struct JoinNotifier(Option); + + impl Drop for JoinNotifier { + fn drop(&mut self) { + self.0.take().unwrap().notify(); + } + } pub(super) struct Task { p: Box, - done: mpsc::Sender<()>, + done: JoinNotifier, } impl Task { pub(super) fn new(p: Box) -> (Task, JoinHandle) { - let (done, recv) = mpsc::channel(); + let (done, recv) = wait_notify::new(); + let done = JoinNotifier(Some(done)); (Task { p, done }, recv) } - pub(super) fn run(self) { + pub(super) fn run(self) -> JoinNotifier { (self.p)(); - let _ = self.done.send(()); + self.done } } @@ -47,6 +58,48 @@ mod task_queue { } } +/// This module provides a synchronization primitive that does not use thread +/// local variables. This is needed for signaling that a thread has finished +/// execution. The signal is sent once all TLS destructors have finished at +/// which point no new thread locals should be created. +pub mod wait_notify { + use super::super::waitqueue::{SpinMutex, WaitQueue, WaitVariable}; + use crate::sync::Arc; + + pub struct Notifier(Arc>>); + + impl Notifier { + /// Notify the waiter. The waiter is either notified right away (if + /// currently blocked in `Waiter::wait()`) or later when it calls the + /// `Waiter::wait()` method. + pub fn notify(self) { + let mut guard = self.0.lock(); + *guard.lock_var_mut() = true; + let _ = WaitQueue::notify_one(guard); + } + } + + pub struct Waiter(Arc>>); + + impl Waiter { + /// Wait for a notification. If `Notifier::notify()` has already been + /// called, this will return immediately, otherwise the current thread + /// is blocked until notified. + pub fn wait(self) { + let guard = self.0.lock(); + if *guard.lock_var() { + return; + } + WaitQueue::wait(guard, || {}); + } + } + + pub fn new() -> (Notifier, Waiter) { + let inner = Arc::new(SpinMutex::new(WaitVariable::new(false))); + (Notifier(inner.clone()), Waiter(inner)) + } +} + impl Thread { // unsafe: see thread::Builder::spawn_unchecked for safety requirements pub unsafe fn new(_stack: usize, p: Box) -> io::Result { @@ -57,7 +110,7 @@ impl Thread { Ok(Thread(handle)) } - pub(super) fn entry() { + pub(super) fn entry() -> JoinNotifier { let mut pending_tasks = task_queue::lock(); let task = rtunwrap!(Some, pending_tasks.pop()); drop(pending_tasks); // make sure to not hold the task queue lock longer than necessary @@ -78,7 +131,7 @@ impl Thread { } pub fn join(self) { - let _ = self.0.recv(); + self.0.wait(); } } diff --git a/library/std/src/thread/local/tests.rs b/library/std/src/thread/local/tests.rs index 80e6798d847b1..98f525eafb0f6 100644 --- a/library/std/src/thread/local/tests.rs +++ b/library/std/src/thread/local/tests.rs @@ -1,5 +1,6 @@ use crate::cell::{Cell, UnsafeCell}; -use crate::sync::mpsc::{channel, Sender}; +use crate::sync::atomic::{AtomicBool, Ordering}; +use crate::sync::mpsc::{self, channel, Sender}; use crate::thread::{self, LocalKey}; use crate::thread_local; @@ -207,3 +208,55 @@ fn dtors_in_dtors_in_dtors_const_init() { }); rx.recv().unwrap(); } + +// This test tests that TLS destructors have run before the thread joins. The +// test has no false positives (meaning: if the test fails, there's actually +// an ordering problem). It may have false negatives, where the test passes but +// join is not guaranteed to be after the TLS destructors. However, false +// negatives should be exceedingly rare due to judicious use of +// thread::yield_now and running the test several times. +#[test] +fn join_orders_after_tls_destructors() { + static THREAD2_LAUNCHED: AtomicBool = AtomicBool::new(false); + + for _ in 0..10 { + let (tx, rx) = mpsc::sync_channel(0); + THREAD2_LAUNCHED.store(false, Ordering::SeqCst); + + let jh = thread::spawn(move || { + struct RecvOnDrop(Cell>>); + + impl Drop for RecvOnDrop { + fn drop(&mut self) { + let rx = self.0.take().unwrap(); + while !THREAD2_LAUNCHED.load(Ordering::SeqCst) { + thread::yield_now(); + } + rx.recv().unwrap(); + } + } + + thread_local! { + static TL_RX: RecvOnDrop = RecvOnDrop(Cell::new(None)); + } + + TL_RX.with(|v| v.0.set(Some(rx))) + }); + + let tx_clone = tx.clone(); + let jh2 = thread::spawn(move || { + THREAD2_LAUNCHED.store(true, Ordering::SeqCst); + jh.join().unwrap(); + tx_clone.send(()).expect_err( + "Expecting channel to be closed because thread 1 TLS destructors must've run", + ); + }); + + while !THREAD2_LAUNCHED.load(Ordering::SeqCst) { + thread::yield_now(); + } + thread::yield_now(); + tx.send(()).expect("Expecting channel to be live because thread 2 must block on join"); + jh2.join().unwrap(); + } +} From 5d1fdf44a996a71015a1e086f060e9ef36f987d6 Mon Sep 17 00:00:00 2001 From: Mohsen Zohrevandi Date: Thu, 29 Apr 2021 08:44:45 -0700 Subject: [PATCH 10/11] Use atomics in join_orders_after_tls_destructors test std::sync::mpsc uses thread locals and depending on the order TLS dtors are run `rx.recv()` can panic when used in a TLS dtor. --- library/std/src/thread/local/tests.rs | 122 +++++++++++++++++++------- 1 file changed, 88 insertions(+), 34 deletions(-) diff --git a/library/std/src/thread/local/tests.rs b/library/std/src/thread/local/tests.rs index 98f525eafb0f6..494ad4e5fda9a 100644 --- a/library/std/src/thread/local/tests.rs +++ b/library/std/src/thread/local/tests.rs @@ -1,6 +1,6 @@ use crate::cell::{Cell, UnsafeCell}; -use crate::sync::atomic::{AtomicBool, Ordering}; -use crate::sync::mpsc::{self, channel, Sender}; +use crate::sync::atomic::{AtomicU8, Ordering}; +use crate::sync::mpsc::{channel, Sender}; use crate::thread::{self, LocalKey}; use crate::thread_local; @@ -217,46 +217,100 @@ fn dtors_in_dtors_in_dtors_const_init() { // thread::yield_now and running the test several times. #[test] fn join_orders_after_tls_destructors() { - static THREAD2_LAUNCHED: AtomicBool = AtomicBool::new(false); + // We emulate a synchronous MPSC rendezvous channel using only atomics and + // thread::yield_now. We can't use std::mpsc as the implementation itself + // may rely on thread locals. + // + // The basic state machine for an SPSC rendezvous channel is: + // FRESH -> THREAD1_WAITING -> MAIN_THREAD_RENDEZVOUS + // where the first transition is done by the “receiving” thread and the 2nd + // transition is done by the “sending” thread. + // + // We add an additional state `THREAD2_LAUNCHED` between `FRESH` and + // `THREAD1_WAITING` to block until all threads are actually running. + // + // A thread that joins on the “receiving” thread completion should never + // observe the channel in the `THREAD1_WAITING` state. If this does occur, + // we switch to the “poison” state `THREAD2_JOINED` and panic all around. + // (This is equivalent to “sending” from an alternate producer thread.) + const FRESH: u8 = 0; + const THREAD2_LAUNCHED: u8 = 1; + const THREAD1_WAITING: u8 = 2; + const MAIN_THREAD_RENDEZVOUS: u8 = 3; + const THREAD2_JOINED: u8 = 4; + static SYNC_STATE: AtomicU8 = AtomicU8::new(FRESH); for _ in 0..10 { - let (tx, rx) = mpsc::sync_channel(0); - THREAD2_LAUNCHED.store(false, Ordering::SeqCst); + SYNC_STATE.store(FRESH, Ordering::SeqCst); + + let jh = thread::Builder::new() + .name("thread1".into()) + .spawn(move || { + struct TlDrop; + + impl Drop for TlDrop { + fn drop(&mut self) { + loop { + match SYNC_STATE.load(Ordering::SeqCst) { + FRESH => thread::yield_now(), + THREAD2_LAUNCHED => break, + v => unreachable!("sync state: {}", v), + } + } + let mut sync_state = SYNC_STATE.swap(THREAD1_WAITING, Ordering::SeqCst); + loop { + match sync_state { + THREAD2_LAUNCHED | THREAD1_WAITING => thread::yield_now(), + MAIN_THREAD_RENDEZVOUS => break, + THREAD2_JOINED => panic!( + "Thread 1 still running after thread 2 joined on thread 1" + ), + v => unreachable!("sync state: {}", v), + } + sync_state = SYNC_STATE.load(Ordering::SeqCst); + } + } + } - let jh = thread::spawn(move || { - struct RecvOnDrop(Cell>>); + thread_local! { + static TL_DROP: TlDrop = TlDrop; + } - impl Drop for RecvOnDrop { - fn drop(&mut self) { - let rx = self.0.take().unwrap(); - while !THREAD2_LAUNCHED.load(Ordering::SeqCst) { - thread::yield_now(); + TL_DROP.with(|_| {}) + }) + .unwrap(); + + let jh2 = thread::Builder::new() + .name("thread2".into()) + .spawn(move || { + assert_eq!(SYNC_STATE.swap(THREAD2_LAUNCHED, Ordering::SeqCst), FRESH); + jh.join().unwrap(); + match SYNC_STATE.swap(THREAD2_JOINED, Ordering::SeqCst) { + MAIN_THREAD_RENDEZVOUS => return, + THREAD2_LAUNCHED | THREAD1_WAITING => { + panic!("Thread 2 running after thread 1 join before main thread rendezvous") } - rx.recv().unwrap(); + v => unreachable!("sync state: {:?}", v), } + }) + .unwrap(); + + loop { + match SYNC_STATE.compare_exchange_weak( + THREAD1_WAITING, + MAIN_THREAD_RENDEZVOUS, + Ordering::SeqCst, + Ordering::SeqCst, + ) { + Ok(_) => break, + Err(FRESH) => thread::yield_now(), + Err(THREAD2_LAUNCHED) => thread::yield_now(), + Err(THREAD2_JOINED) => { + panic!("Main thread rendezvous after thread 2 joined thread 1") + } + v => unreachable!("sync state: {:?}", v), } - - thread_local! { - static TL_RX: RecvOnDrop = RecvOnDrop(Cell::new(None)); - } - - TL_RX.with(|v| v.0.set(Some(rx))) - }); - - let tx_clone = tx.clone(); - let jh2 = thread::spawn(move || { - THREAD2_LAUNCHED.store(true, Ordering::SeqCst); - jh.join().unwrap(); - tx_clone.send(()).expect_err( - "Expecting channel to be closed because thread 1 TLS destructors must've run", - ); - }); - - while !THREAD2_LAUNCHED.load(Ordering::SeqCst) { - thread::yield_now(); } - thread::yield_now(); - tx.send(()).expect("Expecting channel to be live because thread 2 must block on join"); jh2.join().unwrap(); } } From 0e49ff0cb041fec36df8b17209b4655424d5e184 Mon Sep 17 00:00:00 2001 From: Mohsen Zohrevandi Date: Thu, 6 May 2021 09:36:26 -0700 Subject: [PATCH 11/11] join_orders_after_tls_destructors: ensure thread 2 is launched before thread 1 enters TLS destructors --- library/std/src/thread/local/tests.rs | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/library/std/src/thread/local/tests.rs b/library/std/src/thread/local/tests.rs index 494ad4e5fda9a..f33d612961931 100644 --- a/library/std/src/thread/local/tests.rs +++ b/library/std/src/thread/local/tests.rs @@ -250,13 +250,6 @@ fn join_orders_after_tls_destructors() { impl Drop for TlDrop { fn drop(&mut self) { - loop { - match SYNC_STATE.load(Ordering::SeqCst) { - FRESH => thread::yield_now(), - THREAD2_LAUNCHED => break, - v => unreachable!("sync state: {}", v), - } - } let mut sync_state = SYNC_STATE.swap(THREAD1_WAITING, Ordering::SeqCst); loop { match sync_state { @@ -276,7 +269,15 @@ fn join_orders_after_tls_destructors() { static TL_DROP: TlDrop = TlDrop; } - TL_DROP.with(|_| {}) + TL_DROP.with(|_| {}); + + loop { + match SYNC_STATE.load(Ordering::SeqCst) { + FRESH => thread::yield_now(), + THREAD2_LAUNCHED => break, + v => unreachable!("sync state: {}", v), + } + } }) .unwrap();