From 52fa9daa645772603dc170a3f3bed5cab73d646d Mon Sep 17 00:00:00 2001 From: Christiaan Dirkx Date: Wed, 28 Apr 2021 15:51:14 +0200 Subject: [PATCH 01/19] Rework `wasm::thread` to `thread_atomics` --- library/std/src/sys/wasm/mod.rs | 5 ++++- .../sys/wasm/{thread.rs => thread_atomics.rs} | 22 ++++--------------- 2 files changed, 8 insertions(+), 19 deletions(-) rename library/std/src/sys/wasm/{thread.rs => thread_atomics.rs} (83%) diff --git a/library/std/src/sys/wasm/mod.rs b/library/std/src/sys/wasm/mod.rs index 8705910c73a81..71877fce9934e 100644 --- a/library/std/src/sys/wasm/mod.rs +++ b/library/std/src/sys/wasm/mod.rs @@ -37,7 +37,6 @@ pub mod pipe; pub mod process; #[path = "../unsupported/stdio.rs"] pub mod stdio; -pub mod thread; #[path = "../unsupported/thread_local_dtor.rs"] pub mod thread_local_dtor; #[path = "../unsupported/thread_local_key.rs"] @@ -57,6 +56,8 @@ cfg_if::cfg_if! { pub mod rwlock; #[path = "futex_atomics.rs"] pub mod futex; + #[path = "thread_atomics.rs"] + pub mod thread; } else { #[path = "../unsupported/condvar.rs"] pub mod condvar; @@ -64,6 +65,8 @@ cfg_if::cfg_if! { pub mod mutex; #[path = "../unsupported/rwlock.rs"] pub mod rwlock; + #[path = "../unsupported/thread.rs"] + pub mod thread; } } diff --git a/library/std/src/sys/wasm/thread.rs b/library/std/src/sys/wasm/thread_atomics.rs similarity index 83% rename from library/std/src/sys/wasm/thread.rs rename to library/std/src/sys/wasm/thread_atomics.rs index b7bf95c89b482..54bc877aa7de7 100644 --- a/library/std/src/sys/wasm/thread.rs +++ b/library/std/src/sys/wasm/thread_atomics.rs @@ -13,20 +13,10 @@ impl Thread { unsupported() } - pub fn yield_now() { - // do nothing - } + pub fn yield_now() {} - pub fn set_name(_name: &CStr) { - // nope - } + pub fn set_name(_name: &CStr) {} - #[cfg(not(target_feature = "atomics"))] - pub fn sleep(_dur: Duration) { - panic!("can't sleep"); - } - - #[cfg(target_feature = "atomics")] pub fn sleep(dur: Duration) { use crate::arch::wasm32; use crate::cmp; @@ -46,9 +36,7 @@ impl Thread { } } - pub fn join(self) { - self.0 - } + pub fn join(self) {} } pub mod guard { @@ -61,11 +49,9 @@ pub mod guard { } } -// This is only used by atomics primitives when the `atomics` feature is -// enabled. In that mode we currently just use our own thread-local to store our +// We currently just use our own thread-local to store our // current thread's ID, and then we lazily initialize it to something allocated // from a global counter. -#[cfg(target_feature = "atomics")] pub fn my_id() -> u32 { use crate::sync::atomic::{AtomicU32, Ordering::SeqCst}; From fab841080157b0ddc6e0a0d88c4b3fc43d32f147 Mon Sep 17 00:00:00 2001 From: Christiaan Dirkx Date: Wed, 28 Apr 2021 15:54:08 +0200 Subject: [PATCH 02/19] Move `wasm` atomics code to `wasm/atomics` --- .../wasm/{condvar_atomics.rs => atomics/condvar.rs} | 0 .../sys/wasm/{futex_atomics.rs => atomics/futex.rs} | 0 .../sys/wasm/{mutex_atomics.rs => atomics/mutex.rs} | 0 .../sys/wasm/{rwlock_atomics.rs => atomics/rwlock.rs} | 0 .../sys/wasm/{thread_atomics.rs => atomics/thread.rs} | 0 library/std/src/sys/wasm/mod.rs | 10 +++++----- 6 files changed, 5 insertions(+), 5 deletions(-) rename library/std/src/sys/wasm/{condvar_atomics.rs => atomics/condvar.rs} (100%) rename library/std/src/sys/wasm/{futex_atomics.rs => atomics/futex.rs} (100%) rename library/std/src/sys/wasm/{mutex_atomics.rs => atomics/mutex.rs} (100%) rename library/std/src/sys/wasm/{rwlock_atomics.rs => atomics/rwlock.rs} (100%) rename library/std/src/sys/wasm/{thread_atomics.rs => atomics/thread.rs} (100%) diff --git a/library/std/src/sys/wasm/condvar_atomics.rs b/library/std/src/sys/wasm/atomics/condvar.rs similarity index 100% rename from library/std/src/sys/wasm/condvar_atomics.rs rename to library/std/src/sys/wasm/atomics/condvar.rs diff --git a/library/std/src/sys/wasm/futex_atomics.rs b/library/std/src/sys/wasm/atomics/futex.rs similarity index 100% rename from library/std/src/sys/wasm/futex_atomics.rs rename to library/std/src/sys/wasm/atomics/futex.rs diff --git a/library/std/src/sys/wasm/mutex_atomics.rs b/library/std/src/sys/wasm/atomics/mutex.rs similarity index 100% rename from library/std/src/sys/wasm/mutex_atomics.rs rename to library/std/src/sys/wasm/atomics/mutex.rs diff --git a/library/std/src/sys/wasm/rwlock_atomics.rs b/library/std/src/sys/wasm/atomics/rwlock.rs similarity index 100% rename from library/std/src/sys/wasm/rwlock_atomics.rs rename to library/std/src/sys/wasm/atomics/rwlock.rs diff --git a/library/std/src/sys/wasm/thread_atomics.rs b/library/std/src/sys/wasm/atomics/thread.rs similarity index 100% rename from library/std/src/sys/wasm/thread_atomics.rs rename to library/std/src/sys/wasm/atomics/thread.rs diff --git a/library/std/src/sys/wasm/mod.rs b/library/std/src/sys/wasm/mod.rs index 71877fce9934e..0bd2c0d0f9d4c 100644 --- a/library/std/src/sys/wasm/mod.rs +++ b/library/std/src/sys/wasm/mod.rs @@ -48,15 +48,15 @@ pub use crate::sys_common::os_str_bytes as os_str; cfg_if::cfg_if! { if #[cfg(target_feature = "atomics")] { - #[path = "condvar_atomics.rs"] + #[path = "atomics/condvar.rs"] pub mod condvar; - #[path = "mutex_atomics.rs"] + #[path = "atomics/mutex.rs"] pub mod mutex; - #[path = "rwlock_atomics.rs"] + #[path = "atomics/rwlock.rs"] pub mod rwlock; - #[path = "futex_atomics.rs"] + #[path = "atomics/futex.rs"] pub mod futex; - #[path = "thread_atomics.rs"] + #[path = "atomics/thread.rs"] pub mod thread; } else { #[path = "../unsupported/condvar.rs"] From 45bc1930cae22b8b08adc9bb5ed4a494831d6678 Mon Sep 17 00:00:00 2001 From: Christiaan Dirkx Date: Wed, 28 Apr 2021 15:44:13 +0200 Subject: [PATCH 03/19] Reuse `unsupported::args` on `wasm` --- library/std/src/sys/wasm/args.rs | 42 -------------------------------- library/std/src/sys/wasm/mod.rs | 1 + 2 files changed, 1 insertion(+), 42 deletions(-) delete mode 100644 library/std/src/sys/wasm/args.rs diff --git a/library/std/src/sys/wasm/args.rs b/library/std/src/sys/wasm/args.rs deleted file mode 100644 index fde1ab79e1f4b..0000000000000 --- a/library/std/src/sys/wasm/args.rs +++ /dev/null @@ -1,42 +0,0 @@ -use crate::ffi::OsString; -use crate::fmt; -use crate::vec; - -pub fn args() -> Args { - Args { iter: Vec::new().into_iter() } -} - -pub struct Args { - iter: vec::IntoIter, -} - -impl !Send for Args {} -impl !Sync for Args {} - -impl fmt::Debug for Args { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - self.iter.as_slice().fmt(f) - } -} - -impl Iterator for Args { - type Item = OsString; - fn next(&mut self) -> Option { - self.iter.next() - } - fn size_hint(&self) -> (usize, Option) { - self.iter.size_hint() - } -} - -impl ExactSizeIterator for Args { - fn len(&self) -> usize { - self.iter.len() - } -} - -impl DoubleEndedIterator for Args { - fn next_back(&mut self) -> Option { - self.iter.next_back() - } -} diff --git a/library/std/src/sys/wasm/mod.rs b/library/std/src/sys/wasm/mod.rs index 0bd2c0d0f9d4c..18d8ea183f733 100644 --- a/library/std/src/sys/wasm/mod.rs +++ b/library/std/src/sys/wasm/mod.rs @@ -17,6 +17,7 @@ #![deny(unsafe_op_in_unsafe_fn)] pub mod alloc; +#[path = "../unsupported/args.rs"] pub mod args; #[path = "../unsupported/cmath.rs"] pub mod cmath; From cf79c06575f1710f05ee5309fb24bcdfff6f0140 Mon Sep 17 00:00:00 2001 From: Christiaan Dirkx Date: Wed, 28 Apr 2021 16:16:01 +0200 Subject: [PATCH 04/19] Fix missing import in `unsupported::args` --- library/std/src/sys/unsupported/args.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/library/std/src/sys/unsupported/args.rs b/library/std/src/sys/unsupported/args.rs index c924a7d8a2672..a2d75a6197633 100644 --- a/library/std/src/sys/unsupported/args.rs +++ b/library/std/src/sys/unsupported/args.rs @@ -1,4 +1,5 @@ use crate::ffi::OsString; +use crate::fmt; pub struct Args {} From 6eb4735acc50f58e501b42c5d75ae73b3d74b44d Mon Sep 17 00:00:00 2001 From: Joshua Nelson Date: Thu, 22 Apr 2021 13:28:43 -0400 Subject: [PATCH 05/19] Unify rustc and rustdoc parsing of `cfg()` This extracts a new `parse_cfg` function that's used between both. - Treat `#[doc(cfg(x), cfg(y))]` the same as `#[doc(cfg(x)] #[doc(cfg(y))]`. Previously it would be completely ignored. - Treat `#[doc(inline, cfg(x))]` the same as `#[doc(inline)] #[doc(cfg(x))]`. Previously, the cfg would be ignored. - Pass the cfg predicate through to rustc_expand to be validated Co-authored-by: Vadim Petrochenkov --- compiler/rustc_expand/src/config.rs | 54 +++++++++++++----------- src/librustdoc/clean/inline.rs | 4 +- src/librustdoc/clean/mod.rs | 2 +- src/librustdoc/clean/types.rs | 54 +++++++++--------------- src/librustdoc/doctest.rs | 2 +- src/librustdoc/html/render/context.rs | 2 +- src/librustdoc/html/render/print_item.rs | 2 +- src/test/rustdoc-ui/invalid-cfg.rs | 4 ++ src/test/rustdoc-ui/invalid-cfg.stderr | 14 ++++++ src/test/rustdoc/doc-cfg.rs | 8 ++++ 10 files changed, 82 insertions(+), 64 deletions(-) create mode 100644 src/test/rustdoc-ui/invalid-cfg.rs create mode 100644 src/test/rustdoc-ui/invalid-cfg.stderr diff --git a/compiler/rustc_expand/src/config.rs b/compiler/rustc_expand/src/config.rs index 03c83f9c07b5d..f9140609c0f3c 100644 --- a/compiler/rustc_expand/src/config.rs +++ b/compiler/rustc_expand/src/config.rs @@ -464,31 +464,9 @@ impl<'a> StripUnconfigured<'a> { return true; } }; - let error = |span, msg, suggestion: &str| { - let mut err = self.sess.parse_sess.span_diagnostic.struct_span_err(span, msg); - if !suggestion.is_empty() { - err.span_suggestion( - span, - "expected syntax is", - suggestion.into(), - Applicability::MaybeIncorrect, - ); - } - err.emit(); - true - }; - let span = meta_item.span; - match meta_item.meta_item_list() { - None => error(span, "`cfg` is not followed by parentheses", "cfg(/* predicate */)"), - Some([]) => error(span, "`cfg` predicate is not specified", ""), - Some([_, .., l]) => error(l.span(), "multiple `cfg` predicates are specified", ""), - Some([single]) => match single.meta_item() { - Some(meta_item) => { - attr::cfg_matches(meta_item, &self.sess.parse_sess, self.features) - } - None => error(single.span(), "`cfg` predicate key cannot be a literal", ""), - }, - } + parse_cfg(&meta_item, &self.sess).map_or(true, |meta_item| { + attr::cfg_matches(&meta_item, &self.sess.parse_sess, self.features) + }) }) } @@ -532,6 +510,32 @@ impl<'a> StripUnconfigured<'a> { } } +pub fn parse_cfg<'a>(meta_item: &'a MetaItem, sess: &Session) -> Option<&'a MetaItem> { + let error = |span, msg, suggestion: &str| { + let mut err = sess.parse_sess.span_diagnostic.struct_span_err(span, msg); + if !suggestion.is_empty() { + err.span_suggestion( + span, + "expected syntax is", + suggestion.into(), + Applicability::HasPlaceholders, + ); + } + err.emit(); + None + }; + let span = meta_item.span; + match meta_item.meta_item_list() { + None => error(span, "`cfg` is not followed by parentheses", "cfg(/* predicate */)"), + Some([]) => error(span, "`cfg` predicate is not specified", ""), + Some([_, .., l]) => error(l.span(), "multiple `cfg` predicates are specified", ""), + Some([single]) => match single.meta_item() { + Some(meta_item) => Some(meta_item), + None => error(single.span(), "`cfg` predicate key cannot be a literal", ""), + }, + } +} + fn is_cfg(sess: &Session, attr: &Attribute) -> bool { sess.check_name(attr, sym::cfg) } diff --git a/src/librustdoc/clean/inline.rs b/src/librustdoc/clean/inline.rs index 3e89c1ac4c514..4c3b86b2e2b43 100644 --- a/src/librustdoc/clean/inline.rs +++ b/src/librustdoc/clean/inline.rs @@ -307,10 +307,10 @@ fn merge_attrs( } else { Attributes::from_ast(&both, None) }, - both.cfg(cx.sess().diagnostic()), + both.cfg(cx.sess()), ) } else { - (old_attrs.clean(cx), old_attrs.cfg(cx.sess().diagnostic())) + (old_attrs.clean(cx), old_attrs.cfg(cx.sess())) } } diff --git a/src/librustdoc/clean/mod.rs b/src/librustdoc/clean/mod.rs index 6563f398edb6f..a7bc3c20a3d74 100644 --- a/src/librustdoc/clean/mod.rs +++ b/src/librustdoc/clean/mod.rs @@ -2006,7 +2006,7 @@ fn clean_extern_crate( def_id: crate_def_id, visibility: krate.vis.clean(cx), kind: box ExternCrateItem { src: orig_name }, - cfg: attrs.cfg(cx.sess().diagnostic()), + cfg: attrs.cfg(cx.sess()), }] } diff --git a/src/librustdoc/clean/types.rs b/src/librustdoc/clean/types.rs index 5e47144588b3f..a2a03dfd15b7c 100644 --- a/src/librustdoc/clean/types.rs +++ b/src/librustdoc/clean/types.rs @@ -320,7 +320,7 @@ impl Item { kind, box ast_attrs.clean(cx), cx, - ast_attrs.cfg(cx.sess().diagnostic()), + ast_attrs.cfg(cx.sess()), ) } @@ -332,7 +332,7 @@ impl Item { cx: &mut DocContext<'_>, cfg: Option>, ) -> Item { - debug!("name={:?}, def_id={:?}", name, def_id); + trace!("name={:?}, def_id={:?}", name, def_id); Item { def_id, @@ -681,7 +681,7 @@ crate trait AttributesExt { fn other_attrs(&self) -> Vec; - fn cfg(&self, diagnostic: &::rustc_errors::Handler) -> Option>; + fn cfg(&self, sess: &Session) -> Option>; } impl AttributesExt for [ast::Attribute] { @@ -706,17 +706,28 @@ impl AttributesExt for [ast::Attribute] { self.iter().filter(|attr| attr.doc_str().is_none()).cloned().collect() } - fn cfg(&self, diagnostic: &::rustc_errors::Handler) -> Option> { + fn cfg(&self, sess: &Session) -> Option> { let mut cfg = Cfg::True; for attr in self.iter() { + // #[doc] if attr.doc_str().is_none() && attr.has_name(sym::doc) { - if let Some(mi) = attr.meta() { - if let Some(cfg_mi) = Attributes::extract_cfg(&mi) { - // Extracted #[doc(cfg(...))] - match Cfg::parse(cfg_mi) { - Ok(new_cfg) => cfg &= new_cfg, - Err(e) => diagnostic.span_err(e.span, e.msg), + // #[doc(...)] + if let Some(list) = attr.meta().as_ref().and_then(|mi| mi.meta_item_list()) { + for item in list { + // #[doc(include)] + if !item.has_name(sym::cfg) { + continue; + } + // #[doc(cfg(...))] + if let Some(cfg_mi) = item + .meta_item() + .and_then(|item| rustc_expand::config::parse_cfg(&item, sess)) + { + match Cfg::parse(&cfg_mi) { + Ok(new_cfg) => cfg &= new_cfg, + Err(e) => sess.span_err(e.span, e.msg), + } } } } @@ -883,29 +894,6 @@ impl Attributes { self.other_attrs.lists(name) } - /// Extracts the content from an attribute `#[doc(cfg(content))]`. - crate fn extract_cfg(mi: &ast::MetaItem) -> Option<&ast::MetaItem> { - use rustc_ast::NestedMetaItem::MetaItem; - - if let ast::MetaItemKind::List(ref nmis) = mi.kind { - if nmis.len() == 1 { - if let MetaItem(ref cfg_mi) = nmis[0] { - if cfg_mi.has_name(sym::cfg) { - if let ast::MetaItemKind::List(ref cfg_nmis) = cfg_mi.kind { - if cfg_nmis.len() == 1 { - if let MetaItem(ref content_mi) = cfg_nmis[0] { - return Some(content_mi); - } - } - } - } - } - } - } - - None - } - /// Reads a `MetaItem` from within an attribute, looks for whether it is a /// `#[doc(include="file")]`, and returns the filename and contents of the file as loaded from /// its expansion. diff --git a/src/librustdoc/doctest.rs b/src/librustdoc/doctest.rs index 466d1b65406cd..69a47c5b67a7a 100644 --- a/src/librustdoc/doctest.rs +++ b/src/librustdoc/doctest.rs @@ -1095,7 +1095,7 @@ impl<'a, 'hir, 'tcx> HirCollector<'a, 'hir, 'tcx> { let ast_attrs = self.tcx.hir().attrs(hir_id); let mut attrs = Attributes::from_ast(ast_attrs, None); - if let Some(ref cfg) = ast_attrs.cfg(self.sess.diagnostic()) { + if let Some(ref cfg) = ast_attrs.cfg(self.sess) { if !cfg.matches(&self.sess.parse_sess, Some(&self.sess.features_untracked())) { return; } diff --git a/src/librustdoc/html/render/context.rs b/src/librustdoc/html/render/context.rs index 4c8ba0e7b496e..268974169ea85 100644 --- a/src/librustdoc/html/render/context.rs +++ b/src/librustdoc/html/render/context.rs @@ -154,7 +154,7 @@ impl<'tcx> Context<'tcx> { &self.cache } - fn sess(&self) -> &'tcx Session { + pub(super) fn sess(&self) -> &'tcx Session { &self.shared.tcx.sess } diff --git a/src/librustdoc/html/render/print_item.rs b/src/librustdoc/html/render/print_item.rs index 1bb1db00e8825..d33a31ef1ee25 100644 --- a/src/librustdoc/html/render/print_item.rs +++ b/src/librustdoc/html/render/print_item.rs @@ -292,7 +292,7 @@ fn item_module(w: &mut Buffer, cx: &Context<'_>, item: &clean::Item, items: &[cl let import_item = clean::Item { def_id: import_def_id, attrs: import_attrs, - cfg: ast_attrs.cfg(cx.tcx().sess.diagnostic()), + cfg: ast_attrs.cfg(cx.sess()), ..myitem.clone() }; diff --git a/src/test/rustdoc-ui/invalid-cfg.rs b/src/test/rustdoc-ui/invalid-cfg.rs new file mode 100644 index 0000000000000..d237b8605c068 --- /dev/null +++ b/src/test/rustdoc-ui/invalid-cfg.rs @@ -0,0 +1,4 @@ +#![feature(doc_cfg)] +#[doc(cfg = "x")] //~ ERROR not followed by parentheses +#[doc(cfg(x, y))] //~ ERROR multiple `cfg` predicates +struct S {} diff --git a/src/test/rustdoc-ui/invalid-cfg.stderr b/src/test/rustdoc-ui/invalid-cfg.stderr new file mode 100644 index 0000000000000..dae238b052b8a --- /dev/null +++ b/src/test/rustdoc-ui/invalid-cfg.stderr @@ -0,0 +1,14 @@ +error: `cfg` is not followed by parentheses + --> $DIR/invalid-cfg.rs:2:7 + | +LL | #[doc(cfg = "x")] + | ^^^^^^^^^ help: expected syntax is: `cfg(/* predicate */)` + +error: multiple `cfg` predicates are specified + --> $DIR/invalid-cfg.rs:3:14 + | +LL | #[doc(cfg(x, y))] + | ^ + +error: aborting due to 2 previous errors + diff --git a/src/test/rustdoc/doc-cfg.rs b/src/test/rustdoc/doc-cfg.rs index 89a61a289fdeb..1fc80b3e76c53 100644 --- a/src/test/rustdoc/doc-cfg.rs +++ b/src/test/rustdoc/doc-cfg.rs @@ -91,3 +91,11 @@ pub unsafe fn uses_target_feature() { pub fn uses_cfg_target_feature() { uses_target_feature(); } + +// multiple attributes should be allowed +// @has doc_cfg/fn.multiple_attrs.html \ +// '//*[@id="main"]/*[@class="item-info"]/*[@class="stab portability"]' \ +// 'This is supported on x and y and z only.' +#[doc(inline, cfg(x))] +#[doc(cfg(y), cfg(z))] +pub fn multiple_attrs() {} From 97658e58f0f5af5fec694c5c5df143685ee421b0 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Thu, 6 May 2021 08:47:55 -0700 Subject: [PATCH 06/19] rustc: Support Rust-specific features in -Ctarget-feature Since the beginning of time the `-Ctarget-feature` flag on the command line has largely been passed unmodified to LLVM. Afterwards, though, the `#[target_feature]` attribute was stabilized and some of the names in this attribute do not match the corresponding LLVM name. This is because Rust doesn't always want to stabilize the exact feature name in LLVM for the equivalent functionality in Rust. This creates a situation, however, where in Rust you'd write: #[target_feature(enable = "pclmulqdq")] unsafe fn foo() { // ... } but on the command line you would write: RUSTFLAGS="-Ctarget-feature=+pclmul" cargo build --release This difference is somewhat odd to deal with if you're a newcomer and the situation may be made worse with upcoming features like [WebAssembly SIMD](https://github.com/rust-lang/rust/issues/74372) which may be more prevalent. This commit implements a mapping to translate requests via `-Ctarget-feature` through the same name-mapping functionality that's present for attributes in Rust going to LLVM. This means that `+pclmulqdq` will work on x86 targets where as previously it did not. I've attempted to keep this backwards-compatible where the compiler will just opportunistically attempt to remap features found in `-Ctarget-feature`, but if there's something it doesn't understand it gets passed unmodified to LLVM just as it was before. --- compiler/rustc_codegen_llvm/src/llvm_util.rs | 38 +++++++++++-------- .../rust-specific-name-no-warnings.rs | 5 +++ 2 files changed, 28 insertions(+), 15 deletions(-) create mode 100644 src/test/ui/target-feature/rust-specific-name-no-warnings.rs diff --git a/compiler/rustc_codegen_llvm/src/llvm_util.rs b/compiler/rustc_codegen_llvm/src/llvm_util.rs index b44553e4f6d3b..6101b90aea6da 100644 --- a/compiler/rustc_codegen_llvm/src/llvm_util.rs +++ b/compiler/rustc_codegen_llvm/src/llvm_util.rs @@ -339,24 +339,32 @@ pub fn llvm_global_features(sess: &Session) -> Vec { Some(_) | None => {} }; + let filter = |s: &str| { + if s.is_empty() { + return None; + } + let feature = if s.starts_with("+") || s.starts_with("-") { + &s[1..] + } else { + return Some(s.to_string()); + }; + // Rustc-specific feature requests like `+crt-static` or `-crt-static` + // are not passed down to LLVM. + if RUSTC_SPECIFIC_FEATURES.contains(&feature) { + return None; + } + // ... otherwise though we run through `to_llvm_feature` feature when + // passing requests down to LLVM. This means that all in-language + // features also work on the command line instead of having two + // different names when the LLVM name and the Rust name differ. + Some(format!("{}{}", &s[..1], to_llvm_feature(sess, feature))) + }; + // Features implied by an implicit or explicit `--target`. - features.extend( - sess.target - .features - .split(',') - .filter(|f| !f.is_empty() && !RUSTC_SPECIFIC_FEATURES.iter().any(|s| f.contains(s))) - .map(String::from), - ); + features.extend(sess.target.features.split(',').filter_map(&filter)); // -Ctarget-features - features.extend( - sess.opts - .cg - .target_feature - .split(',') - .filter(|f| !f.is_empty() && !RUSTC_SPECIFIC_FEATURES.iter().any(|s| f.contains(s))) - .map(String::from), - ); + features.extend(sess.opts.cg.target_feature.split(',').filter_map(&filter)); features } diff --git a/src/test/ui/target-feature/rust-specific-name-no-warnings.rs b/src/test/ui/target-feature/rust-specific-name-no-warnings.rs new file mode 100644 index 0000000000000..1708a71a9812f --- /dev/null +++ b/src/test/ui/target-feature/rust-specific-name-no-warnings.rs @@ -0,0 +1,5 @@ +// build-pass +// only-x86 +// compile-flags: -C target-feature=+pclmulqdq + +fn main() {} From fb9feb35eda218133e4fb8944e66d41b904fc9f8 Mon Sep 17 00:00:00 2001 From: Vadim Petrochenkov Date: Mon, 3 May 2021 17:04:59 +0300 Subject: [PATCH 07/19] linker: Avoid library duplication with `/WHOLEARCHIVE` --- compiler/rustc_codegen_ssa/src/back/linker.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/compiler/rustc_codegen_ssa/src/back/linker.rs b/compiler/rustc_codegen_ssa/src/back/linker.rs index 401d379b0d161..0e1723b06c5f9 100644 --- a/compiler/rustc_codegen_ssa/src/back/linker.rs +++ b/compiler/rustc_codegen_ssa/src/back/linker.rs @@ -813,11 +813,9 @@ impl<'a> Linker for MsvcLinker<'a> { } fn link_whole_staticlib(&mut self, lib: Symbol, verbatim: bool, _search_path: &[PathBuf]) { - self.link_staticlib(lib, verbatim); self.cmd.arg(format!("/WHOLEARCHIVE:{}{}", lib, if verbatim { "" } else { ".lib" })); } fn link_whole_rlib(&mut self, path: &Path) { - self.link_rlib(path); let mut arg = OsString::from("/WHOLEARCHIVE:"); arg.push(path); self.cmd.arg(arg); From 31c2ad0d4cdd06622353a57b3a678586077b84fe Mon Sep 17 00:00:00 2001 From: "Joshua M. Clulow" Date: Thu, 6 May 2021 17:01:50 -0700 Subject: [PATCH 08/19] illumos should put libc last in library search order Under some conditions, the toolchain will produce a sequence of linker arguments that result in a NEEDED list that puts libc before libgcc_s; e.g., [0] NEEDED 0x2046ba libc.so.1 [1] NEEDED 0x204723 libm.so.2 [2] NEEDED 0x204736 libsocket.so.1 [3] NEEDED 0x20478b libumem.so.1 [4] NEEDED 0x204763 libgcc_s.so.1 Both libc and libgcc_s provide an unwinder implementation, but libgcc_s provides some extra symbols upon which Rust directly depends. If libc is first in the NEEDED list we will find some of those symbols in libc but others in libgcc_s, resulting in undefined behaviour as the two implementations do not use compatible interior data structures. This solution is not perfect, but is the simplest way to produce correct binaries on illumos for now. --- compiler/rustc_codegen_ssa/src/back/linker.rs | 8 ++++++++ compiler/rustc_target/src/spec/illumos_base.rs | 11 +++++++++++ 2 files changed, 19 insertions(+) diff --git a/compiler/rustc_codegen_ssa/src/back/linker.rs b/compiler/rustc_codegen_ssa/src/back/linker.rs index 401d379b0d161..a66de89f68179 100644 --- a/compiler/rustc_codegen_ssa/src/back/linker.rs +++ b/compiler/rustc_codegen_ssa/src/back/linker.rs @@ -340,6 +340,14 @@ impl<'a> Linker for GccLinker<'a> { } fn link_dylib(&mut self, lib: Symbol, verbatim: bool, as_needed: bool) { + if self.sess.target.os == "illumos" && lib.as_str() == "c" { + // libc will be added via late_link_args on illumos so that it will + // appear last in the library search order. + // FIXME: This should be replaced by a more complete and generic + // mechanism for controlling the order of library arguments passed + // to the linker. + return; + } if !as_needed { if self.sess.target.is_like_osx { // FIXME(81490): ld64 doesn't support these flags but macOS 11 diff --git a/compiler/rustc_target/src/spec/illumos_base.rs b/compiler/rustc_target/src/spec/illumos_base.rs index 2e365d210f3f6..2b8e046c46b0e 100644 --- a/compiler/rustc_target/src/spec/illumos_base.rs +++ b/compiler/rustc_target/src/spec/illumos_base.rs @@ -6,6 +6,17 @@ pub fn opts() -> TargetOptions { late_link_args.insert( LinkerFlavor::Gcc, vec![ + // The illumos libc contains a stack unwinding implementation, as + // does libgcc_s. The latter implementation includes several + // additional symbols that are not always in base libc. To force + // the consistent use of just one unwinder, we ensure libc appears + // after libgcc_s in the NEEDED list for the resultant binary by + // ignoring any attempts to add it as a dynamic dependency until the + // very end. + // FIXME: This should be replaced by a more complete and generic + // mechanism for controlling the order of library arguments passed + // to the linker. + "-lc".to_string(), // LLVM will insert calls to the stack protector functions // "__stack_chk_fail" and "__stack_chk_guard" into code in native // object files. Some platforms include these symbols directly in From a9f43a2a8f845ac7be462cc5300da2350995c546 Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Thu, 22 Apr 2021 14:58:05 +0100 Subject: [PATCH 09/19] std panicking: Make decrease() return () Nothing uses the return value. This will make the next changes easier. Signed-off-by: Ian Jackson --- library/std/src/panicking.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/library/std/src/panicking.rs b/library/std/src/panicking.rs index 6cd572cbe87c1..fbcfec7c7a9e9 100644 --- a/library/std/src/panicking.rs +++ b/library/std/src/panicking.rs @@ -252,13 +252,13 @@ pub mod panic_count { }) } - pub fn decrease() -> usize { + pub fn decrease() { GLOBAL_PANIC_COUNT.fetch_sub(1, Ordering::Relaxed); LOCAL_PANIC_COUNT.with(|c| { let next = c.get() - 1; c.set(next); next - }) + }); } pub fn get() -> usize { From 1b1bf2463619e23eba1b36b6d7df276ce73563dd Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Tue, 20 Apr 2021 20:02:49 +0100 Subject: [PATCH 10/19] std panicking: Provide panic::always_abort We must change the atomic read on panic entry to `Acquire`, to pick up a possible an `always_panic` on another thread. We add `count` to the names of panic_count::get and ::is_zaero, because now there is another reason why panic ought to maybe abort. Renaming these ensures that we have checked every call site to ensure that they don't need further adjustment. Signed-off-by: Ian Jackson Co-authored-by: Mara Bos --- library/std/src/panic.rs | 36 ++++++++++++++++++++ library/std/src/panicking.rs | 64 +++++++++++++++++++++++++++--------- 2 files changed, 85 insertions(+), 15 deletions(-) diff --git a/library/std/src/panic.rs b/library/std/src/panic.rs index 3e634239ad301..1c1466492881c 100644 --- a/library/std/src/panic.rs +++ b/library/std/src/panic.rs @@ -461,5 +461,41 @@ pub fn resume_unwind(payload: Box) -> ! { panicking::rust_panic_without_hook(payload) } +/// Make all future panics abort directly without running the panic hook or unwinding. +/// +/// There is no way to undo this; the effect lasts until the process exits or +/// execs (or the equivalent). +/// +/// # Use after fork +/// +/// This function is particularly useful for calling after `libc::fork`. After `fork`, in a +/// multithreaded program it is (on many platforms) not safe to call the allocator. It is also +/// generally highly undesirable for an unwind to unwind past the `fork`, because that results in +/// the unwind propagating to code that was only ever expecting to run in the parent. +/// +/// `panic::always_abort()` helps avoid both of these. It directly avoids any further unwinding, +/// and if there is a panic, the abort will occur without allocating provided that the arguments to +/// panic can be formatted without allocating. +/// +/// Examples +/// +/// ```no_run +/// #![feature(panic_always_abort)] +/// use std::panic; +/// +/// panic::always_abort(); +/// +/// let _ = panic::catch_unwind(|| { +/// panic!("inside the catch"); +/// }); +/// +/// // We will have aborted already, due to the panic. +/// unreachable!(); +/// ``` +#[unstable(feature = "panic_always_abort", issue = "84438")] +pub fn always_abort() { + crate::panicking::panic_count::set_always_abort(); +} + #[cfg(test)] mod tests; diff --git a/library/std/src/panicking.rs b/library/std/src/panicking.rs index fbcfec7c7a9e9..62476581f990f 100644 --- a/library/std/src/panicking.rs +++ b/library/std/src/panicking.rs @@ -180,7 +180,7 @@ pub fn take_hook() -> Box) + 'static + Sync + Send> { fn default_hook(info: &PanicInfo<'_>) { // If this is a double panic, make sure that we print a backtrace // for this panic. Otherwise only print it if logging is enabled. - let backtrace_env = if panic_count::get() >= 2 { + let backtrace_env = if panic_count::get_count() >= 2 { RustBacktrace::Print(crate::backtrace_rs::PrintFmt::Full) } else { backtrace::rust_backtrace_env() @@ -233,6 +233,8 @@ pub mod panic_count { use crate::cell::Cell; use crate::sync::atomic::{AtomicUsize, Ordering}; + pub const ALWAYS_ABORT_FLAG: usize = 1 << (usize::BITS - 1); + // Panic count for the current thread. thread_local! { static LOCAL_PANIC_COUNT: Cell = Cell::new(0) } @@ -241,15 +243,29 @@ pub mod panic_count { // thread, if that thread currently views `GLOBAL_PANIC_COUNT` as being zero, // then `LOCAL_PANIC_COUNT` in that thread is zero. This invariant holds before // and after increase and decrease, but not necessarily during their execution. + // + // Additionally, the top bit of GLOBAL_PANIC_COUNT (GLOBAL_ALWAYS_ABORT_FLAG) + // records whether panic::always_abort() has been called. This can only be + // set, never cleared. + // + // This could be viewed as a struct containing a single bit and an n-1-bit + // value, but if we wrote it like that it would be more than a single word, + // and even a newtype around usize would be clumsy because we need atomics. + // But we use such a tuple for the return type of increase(). + // + // Stealing a bit is fine because it just amounts to assuming that each + // panicking thread consumes at least 2 bytes of address space. static GLOBAL_PANIC_COUNT: AtomicUsize = AtomicUsize::new(0); - pub fn increase() -> usize { - GLOBAL_PANIC_COUNT.fetch_add(1, Ordering::Relaxed); - LOCAL_PANIC_COUNT.with(|c| { - let next = c.get() + 1; - c.set(next); - next - }) + pub fn increase() -> (bool, usize) { + ( + GLOBAL_PANIC_COUNT.fetch_add(1, Ordering::Acquire) & ALWAYS_ABORT_FLAG != 0, + LOCAL_PANIC_COUNT.with(|c| { + let next = c.get() + 1; + c.set(next); + next + }), + ) } pub fn decrease() { @@ -261,13 +277,19 @@ pub mod panic_count { }); } - pub fn get() -> usize { + pub fn set_always_abort() { + GLOBAL_PANIC_COUNT.fetch_or(ALWAYS_ABORT_FLAG, Ordering::Release); + } + + // Disregards ALWAYS_ABORT_FLAG + pub fn get_count() -> usize { LOCAL_PANIC_COUNT.with(|c| c.get()) } + // Disregards ALWAYS_ABORT_FLAG #[inline] - pub fn is_zero() -> bool { - if GLOBAL_PANIC_COUNT.load(Ordering::Relaxed) == 0 { + pub fn count_is_zero() -> bool { + if GLOBAL_PANIC_COUNT.load(Ordering::Relaxed) & !ALWAYS_ABORT_FLAG == 0 { // Fast path: if `GLOBAL_PANIC_COUNT` is zero, all threads // (including the current one) will have `LOCAL_PANIC_COUNT` // equal to zero, so TLS access can be avoided. @@ -410,7 +432,7 @@ pub unsafe fn r#try R>(f: F) -> Result> /// Determines whether the current thread is unwinding because of panic. #[inline] pub fn panicking() -> bool { - !panic_count::is_zero() + !panic_count::count_is_zero() } /// The entry point for panicking with a formatted message. @@ -563,15 +585,27 @@ fn rust_panic_with_hook( message: Option<&fmt::Arguments<'_>>, location: &Location<'_>, ) -> ! { - let panics = panic_count::increase(); + let (must_abort, panics) = panic_count::increase(); // If this is the third nested call (e.g., panics == 2, this is 0-indexed), // the panic hook probably triggered the last panic, otherwise the // double-panic check would have aborted the process. In this case abort the // process real quickly as we don't want to try calling it again as it'll // probably just panic again. - if panics > 2 { - util::dumb_print(format_args!("thread panicked while processing panic. aborting.\n")); + if must_abort || panics > 2 { + if panics > 2 { + // Don't try to print the message in this case + // - perhaps that is causing the recursive panics. + util::dumb_print(format_args!("thread panicked while processing panic. aborting.\n")); + } else { + // Unfortunately, this does not print a backtrace, because creating + // a `Backtrace` will allocate, which we must to avoid here. + let panicinfo = PanicInfo::internal_constructor(message, location); + util::dumb_print(format_args!( + "{}\npanicked after panic::always_abort(), aborting.\n", + panicinfo + )); + } intrinsics::abort() } From 3cba120ba453b5371e53766ec816f016d004ed91 Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Thu, 6 May 2021 15:11:08 +0100 Subject: [PATCH 11/19] std panicking: ALWAYS_ABORT: use Relaxed memory ordering As per https://github.com/rust-lang/rust/pull/81858#discussion_r626507810 Suggested-by: Mara Bos Signed-off-by: Ian Jackson --- library/std/src/panicking.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/library/std/src/panicking.rs b/library/std/src/panicking.rs index 62476581f990f..a8410bea7342b 100644 --- a/library/std/src/panicking.rs +++ b/library/std/src/panicking.rs @@ -259,7 +259,7 @@ pub mod panic_count { pub fn increase() -> (bool, usize) { ( - GLOBAL_PANIC_COUNT.fetch_add(1, Ordering::Acquire) & ALWAYS_ABORT_FLAG != 0, + GLOBAL_PANIC_COUNT.fetch_add(1, Ordering::Relaxed) & ALWAYS_ABORT_FLAG != 0, LOCAL_PANIC_COUNT.with(|c| { let next = c.get() + 1; c.set(next); @@ -278,7 +278,7 @@ pub mod panic_count { } pub fn set_always_abort() { - GLOBAL_PANIC_COUNT.fetch_or(ALWAYS_ABORT_FLAG, Ordering::Release); + GLOBAL_PANIC_COUNT.fetch_or(ALWAYS_ABORT_FLAG, Ordering::Relaxed); } // Disregards ALWAYS_ABORT_FLAG From 820123a949705f404ff080759c32dba4a4d89580 Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Tue, 20 Apr 2021 20:04:31 +0100 Subject: [PATCH 12/19] panic/fork: Command: Do not unwind after fork() in child Unwinding after fork() in the child is UB on some platforms, because on those (including musl) malloc can be UB in the child of a multithreaded program, and unwinding must box for the payload. Even if it's safe, unwinding past fork() in the child causes whatever traps the unwind to return twice. This is very strange and clearly not desirable. With the default behaviour of the thread library, this can even result in a panic in the child being transformed into zero exit status (ie, success) as seen in the parent! Fixes #79740. Signed-off-by: Ian Jackson --- library/std/src/sys/unix/process/process_unix.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/library/std/src/sys/unix/process/process_unix.rs b/library/std/src/sys/unix/process/process_unix.rs index ed9044382a898..08b500b9c825a 100644 --- a/library/std/src/sys/unix/process/process_unix.rs +++ b/library/std/src/sys/unix/process/process_unix.rs @@ -54,6 +54,7 @@ impl Command { let (env_lock, pid) = unsafe { (sys::os::env_read_lock(), cvt(libc::fork())?) }; if pid == 0 { + crate::panic::always_abort(); mem::forget(env_lock); drop(input); let Err(err) = unsafe { self.do_exec(theirs, envp.as_ref()) }; From 9283cdca362065a215e7f8b460719947493ddc54 Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Sun, 7 Feb 2021 13:41:49 +0000 Subject: [PATCH 13/19] unix process: pre_exec: Discuss panic safety Signed-off-by: Ian Jackson Co-authored-by: Mara Bos --- library/std/src/os/unix/process.rs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/library/std/src/os/unix/process.rs b/library/std/src/os/unix/process.rs index 355855bcd10e2..f014a3d7b2539 100644 --- a/library/std/src/os/unix/process.rs +++ b/library/std/src/os/unix/process.rs @@ -75,6 +75,12 @@ pub trait CommandExt: Sealed { /// sure that the closure does not violate library invariants by making /// invalid use of these duplicates. /// + /// Panicking in the closure is safe only if all the format arguments for the + /// panic message can be safely formatted; this is because although + /// `Command` calls [`std::panic::always_abort`](crate::panic::always_abort) + /// before calling the pre_exec hook, panic will still try to format the + /// panic message. + /// /// When this closure is run, aspects such as the stdio file descriptors and /// working directory have successfully been changed, so output to these /// locations may not appear where intended. From f8015061c88ba35f7af09ff68a054ffe6c87990c Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Sun, 7 Feb 2021 11:42:44 +0000 Subject: [PATCH 14/19] panic tests: Command: Test that we do not unwind past fork This is safe (does not involve heap allocation) but we don't yet have a test to ensure that stays true. That will come in a moment. Signed-off-by: Ian Jackson Co-authored-by: Mara Bos --- .../sys/unix/process/process_unix/tests.rs | 23 +++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/library/std/src/sys/unix/process/process_unix/tests.rs b/library/std/src/sys/unix/process/process_unix/tests.rs index 02c469fbcdfd8..61b2e4a145f80 100644 --- a/library/std/src/sys/unix/process/process_unix/tests.rs +++ b/library/std/src/sys/unix/process/process_unix/tests.rs @@ -1,3 +1,7 @@ +use crate::os::unix::process::{CommandExt, ExitStatusExt}; +use crate::panic::catch_unwind; +use crate::process::Command; + #[test] fn exitstatus_display_tests() { // In practice this is the same on every Unix. @@ -28,3 +32,22 @@ fn exitstatus_display_tests() { t(0x000ff, "unrecognised wait status: 255 0xff"); } } + +#[test] +fn test_command_fork_no_unwind() { + let got = catch_unwind(|| { + let mut c = Command::new("echo"); + c.arg("hi"); + unsafe { + c.pre_exec(|| panic!("{}", "crash now!")); + } + let st = c.status().expect("failed to get command status"); + dbg!(st); + st + }); + dbg!(&got); + let status = got.expect("panic unexpectedly propagated"); + dbg!(status); + let signal = status.signal().expect("expected child process to die of signal"); + assert!(signal == libc::SIGABRT || signal == libc::SIGILL); +} From a17eab7beddf87807d9d7fc71b7dfb90b5e2488a Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Sun, 7 Feb 2021 12:16:11 +0000 Subject: [PATCH 15/19] panic ui test: Provide comprehensive test for panic after fork This tests that we can indeed safely panic after fork, both a raw libc::fork and in a Command pre_exec hook. Signed-off-by: Ian Jackson Co-authored-by: Mara Bos --- .../sys/unix/process/process_unix/tests.rs | 3 + src/test/ui/panics/abort-on-panic.rs | 48 +++--- .../ui/process/process-panic-after-fork.rs | 150 ++++++++++++++++++ 3 files changed, 179 insertions(+), 22 deletions(-) create mode 100644 src/test/ui/process/process-panic-after-fork.rs diff --git a/library/std/src/sys/unix/process/process_unix/tests.rs b/library/std/src/sys/unix/process/process_unix/tests.rs index 61b2e4a145f80..59953a2230fce 100644 --- a/library/std/src/sys/unix/process/process_unix/tests.rs +++ b/library/std/src/sys/unix/process/process_unix/tests.rs @@ -2,6 +2,9 @@ use crate::os::unix::process::{CommandExt, ExitStatusExt}; use crate::panic::catch_unwind; use crate::process::Command; +// Many of the other aspects of this situation, including heap alloc concurrency +// safety etc., are tested in src/test/ui/process/process-panic-after-fork.rs + #[test] fn exitstatus_display_tests() { // In practice this is the same on every Unix. diff --git a/src/test/ui/panics/abort-on-panic.rs b/src/test/ui/panics/abort-on-panic.rs index f34cf6a9cbf6d..3ef6d5d187440 100644 --- a/src/test/ui/panics/abort-on-panic.rs +++ b/src/test/ui/panics/abort-on-panic.rs @@ -23,41 +23,45 @@ extern "Rust" fn panic_in_rust_abi() { panic!("TestRust"); } -fn test() { - let _ = panic::catch_unwind(|| { panic_in_ffi(); }); - // The process should have aborted by now. +fn should_have_aborted() { io::stdout().write(b"This should never be printed.\n"); let _ = io::stdout().flush(); } +fn test() { + let _ = panic::catch_unwind(|| { panic_in_ffi(); }); + should_have_aborted(); +} + fn testrust() { let _ = panic::catch_unwind(|| { panic_in_rust_abi(); }); - // The process should have aborted by now. - io::stdout().write(b"This should never be printed.\n"); - let _ = io::stdout().flush(); + should_have_aborted(); } fn main() { + let tests: &[(_, fn())] = &[ + ("test", test), + ("testrust", testrust), + ]; + let args: Vec = env::args().collect(); if args.len() > 1 { // This is inside the self-executed command. - match &*args[1] { - "test" => return test(), - "testrust" => return testrust(), - _ => panic!("bad test"), + for (a,f) in tests { + if &args[1] == a { return f() } } + panic!("bad test"); } - // These end up calling the self-execution branches above. - let mut p = Command::new(&args[0]) - .stdout(Stdio::piped()) - .stdin(Stdio::piped()) - .arg("test").spawn().unwrap(); - assert!(!p.wait().unwrap().success()); - - let mut p = Command::new(&args[0]) - .stdout(Stdio::piped()) - .stdin(Stdio::piped()) - .arg("testrust").spawn().unwrap(); - assert!(!p.wait().unwrap().success()); + let execute_self_expecting_abort = |arg| { + let mut p = Command::new(&args[0]) + .stdout(Stdio::piped()) + .stdin(Stdio::piped()) + .arg(arg).spawn().unwrap(); + assert!(!p.wait().unwrap().success()); + }; + + for (a,_f) in tests { + execute_self_expecting_abort(a); + } } diff --git a/src/test/ui/process/process-panic-after-fork.rs b/src/test/ui/process/process-panic-after-fork.rs new file mode 100644 index 0000000000000..6e07a1611c5c3 --- /dev/null +++ b/src/test/ui/process/process-panic-after-fork.rs @@ -0,0 +1,150 @@ +// run-pass +// no-prefer-dynamic +// ignore-wasm32-bare no libc +// ignore-windows +// ignore-sgx no libc +// ignore-emscripten no processes +// ignore-sgx no processes + +#![feature(bench_black_box)] +#![feature(rustc_private)] +#![feature(never_type)] +#![feature(panic_always_abort)] + +extern crate libc; + +use std::alloc::{GlobalAlloc, Layout}; +use std::fmt; +use std::panic::{self, panic_any}; +use std::os::unix::process::{CommandExt, ExitStatusExt}; +use std::process::{self, Command, ExitStatus}; +use std::sync::atomic::{AtomicU32, Ordering}; + +use libc::c_int; + +/// This stunt allocator allows us to spot heap allocations in the child. +struct PidChecking { + parent: A, + require_pid: AtomicU32, +} + +#[global_allocator] +static ALLOCATOR: PidChecking = PidChecking { + parent: std::alloc::System, + require_pid: AtomicU32::new(0), +}; + +impl PidChecking { + fn engage(&self) { + let parent_pid = process::id(); + eprintln!("engaging allocator trap, parent pid={}", parent_pid); + self.require_pid.store(parent_pid, Ordering::Release); + } + fn check(&self) { + let require_pid = self.require_pid.load(Ordering::Acquire); + if require_pid != 0 { + let actual_pid = process::id(); + if require_pid != actual_pid { + unsafe { + libc::raise(libc::SIGTRAP); + } + } + } + } +} + +unsafe impl GlobalAlloc for PidChecking { + unsafe fn alloc(&self, layout: Layout) -> *mut u8 { + self.check(); + self.parent.alloc(layout) + } + + unsafe fn dealloc(&self, ptr: *mut u8, layout: Layout) { + self.check(); + self.parent.dealloc(ptr, layout) + } + + unsafe fn alloc_zeroed(&self, layout: Layout) -> *mut u8 { + self.check(); + self.parent.alloc_zeroed(layout) + } + + unsafe fn realloc(&self, ptr: *mut u8, layout: Layout, new_size: usize) -> *mut u8 { + self.check(); + self.parent.realloc(ptr, layout, new_size) + } +} + +fn expect_aborted(status: ExitStatus) { + dbg!(status); + let signal = status.signal().expect("expected child process to die of signal"); + assert!(signal == libc::SIGABRT || signal == libc::SIGILL); +} + +fn main() { + ALLOCATOR.engage(); + + fn run(do_panic: &dyn Fn()) -> ExitStatus { + let child = unsafe { libc::fork() }; + assert!(child >= 0); + if child == 0 { + panic::always_abort(); + do_panic(); + process::exit(0); + } + let mut status: c_int = 0; + let got = unsafe { libc::waitpid(child, &mut status, 0) }; + assert_eq!(got, child); + let status = ExitStatus::from_raw(status.into()); + status + } + + fn one(do_panic: &dyn Fn()) { + let status = run(do_panic); + expect_aborted(status); + } + + one(&|| panic!()); + one(&|| panic!("some message")); + one(&|| panic!("message with argument: {}", 42)); + + #[derive(Debug)] + struct Wotsit { } + one(&|| panic_any(Wotsit { })); + + let mut c = Command::new("echo"); + unsafe { + c.pre_exec(|| panic!("{}", "crash now!")); + } + let st = c.status().expect("failed to get command status"); + expect_aborted(st); + + struct DisplayWithHeap; + impl fmt::Display for DisplayWithHeap { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> Result<(), fmt::Error> { + let s = vec![0; 100]; + let s = std::hint::black_box(s); + write!(f, "{:?}", s) + } + } + + // Some panics in the stdlib that we want not to allocate, as + // otherwise these facilities become impossible to use in the + // child after fork, which is really quite awkward. + + one(&|| { None::.unwrap(); }); + one(&|| { None::.expect("unwrapped a none"); }); + one(&|| { std::str::from_utf8(b"\xff").unwrap(); }); + one(&|| { + let x = [0, 1, 2, 3]; + let y = x[std::hint::black_box(4)]; + let _z = std::hint::black_box(y); + }); + + // Finally, check that our stunt allocator can actually catch an allocation after fork. + // ie, that our test is effective. + + let status = run(&|| panic!("allocating to display... {}", DisplayWithHeap)); + dbg!(status); + assert_eq!(status.signal(), Some(libc::SIGTRAP)); +} From 19429ce132d8cc9526ab4fe46d6287f2ad89ef1c Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Thu, 22 Apr 2021 15:05:23 +0100 Subject: [PATCH 16/19] panic ui test: Improve error handling Previoously, if somehow this program got a wrong argument, it would panic in the re-executed child. But that looks like a "success" for this program! We mustn't panic unless everything is great. Signed-off-by: Ian Jackson --- src/test/ui/panics/abort-on-panic.rs | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/src/test/ui/panics/abort-on-panic.rs b/src/test/ui/panics/abort-on-panic.rs index 3ef6d5d187440..7cf60ae960212 100644 --- a/src/test/ui/panics/abort-on-panic.rs +++ b/src/test/ui/panics/abort-on-panic.rs @@ -28,6 +28,11 @@ fn should_have_aborted() { let _ = io::stdout().flush(); } +fn bomb_out_but_not_abort(msg: &str) { + eprintln!("bombing out: {}", msg); + exit(1); +} + fn test() { let _ = panic::catch_unwind(|| { panic_in_ffi(); }); should_have_aborted(); @@ -50,7 +55,7 @@ fn main() { for (a,f) in tests { if &args[1] == a { return f() } } - panic!("bad test"); + bomb_out_but_not_abort("bad test"); } let execute_self_expecting_abort = |arg| { @@ -58,7 +63,11 @@ fn main() { .stdout(Stdio::piped()) .stdin(Stdio::piped()) .arg(arg).spawn().unwrap(); - assert!(!p.wait().unwrap().success()); + let status = p.wait().unwrap(); + assert!(!status.success()); + // Any reasonable platform can distinguish a process which + // called exit(1) from one which panicked. + assert_ne!(status.code(), Some(1)); }; for (a,_f) in tests { From 12fe50010da70abfca84ae9c0d6e798e987fa882 Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Thu, 22 Apr 2021 14:22:42 +0100 Subject: [PATCH 17/19] panic ui test: Add a test for panic::always_abort Our existing tests are only on Unix. We want a general one too. Signed-off-by: Ian Jackson --- src/test/ui/panics/abort-on-panic.rs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/test/ui/panics/abort-on-panic.rs b/src/test/ui/panics/abort-on-panic.rs index 7cf60ae960212..c02552be5192c 100644 --- a/src/test/ui/panics/abort-on-panic.rs +++ b/src/test/ui/panics/abort-on-panic.rs @@ -43,10 +43,17 @@ fn testrust() { should_have_aborted(); } +fn test_always_abort() { + panic::always_abort(); + let _ = panic::catch_unwind(|| { panic!(); }); + should_have_aborted(); +} + fn main() { let tests: &[(_, fn())] = &[ ("test", test), ("testrust", testrust), + ("test_always_abort", test_always_abort), ]; let args: Vec = env::args().collect(); From 756771d54c28c4543d620891e971bf5a95c3ea3e Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Thu, 22 Apr 2021 15:06:53 +0100 Subject: [PATCH 18/19] panic ui test: Test always_abort on one thread, panic on another This test failed on an earlier version of this branch, where this did not work properly, so I know the test works. Signed-off-by: Ian Jackson --- src/test/ui/panics/abort-on-panic.rs | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/src/test/ui/panics/abort-on-panic.rs b/src/test/ui/panics/abort-on-panic.rs index c02552be5192c..2aea607e95489 100644 --- a/src/test/ui/panics/abort-on-panic.rs +++ b/src/test/ui/panics/abort-on-panic.rs @@ -2,6 +2,7 @@ #![allow(unused_must_use)] #![feature(unwind_attributes)] +#![feature(panic_always_abort)] // Since we mark some ABIs as "nounwind" to LLVM, we must make sure that // we never unwind through them. @@ -11,7 +12,9 @@ use std::{env, panic}; use std::io::prelude::*; use std::io; -use std::process::{Command, Stdio}; +use std::process::{exit, Command, Stdio}; +use std::sync::{Arc, Barrier}; +use std::thread; #[unwind(aborts)] // FIXME(#58794) should work even without the attribute extern "C" fn panic_in_ffi() { @@ -49,11 +52,27 @@ fn test_always_abort() { should_have_aborted(); } +fn test_always_abort_thread() { + let barrier = Arc::new(Barrier::new(2)); + let thr = { + let barrier = barrier.clone(); + thread::spawn(move ||{ + barrier.wait(); + panic!("in thread"); + }) + }; + panic::always_abort(); + barrier.wait(); + let _ = thr.join(); + bomb_out_but_not_abort("joined - but we were supposed to panic!"); +} + fn main() { let tests: &[(_, fn())] = &[ ("test", test), ("testrust", testrust), ("test_always_abort", test_always_abort), + ("test_always_abort_thread", test_always_abort_thread), ]; let args: Vec = env::args().collect(); From bfa84842e546a5fb2d1c69ba499ddc94bee1e01a Mon Sep 17 00:00:00 2001 From: Jethro Beekman Date: Fri, 7 May 2021 13:40:43 +0200 Subject: [PATCH 19/19] Rearrange SGX split module files In #75979 several inlined modules were split out into multiple files. This PR keeps the multiple files but moves a few things around to organize things in a coherent way. --- library/std/src/sys/sgx/abi/{tls.rs => tls/mod.rs} | 0 library/std/src/sys/sgx/{waitqueue.rs => waitqueue/mod.rs} | 7 +------ library/std/src/sys/sgx/waitqueue/spin_mutex.rs | 3 +++ library/std/src/sys/sgx/waitqueue/unsafe_list.rs | 3 +++ 4 files changed, 7 insertions(+), 6 deletions(-) rename library/std/src/sys/sgx/abi/{tls.rs => tls/mod.rs} (100%) rename library/std/src/sys/sgx/{waitqueue.rs => waitqueue/mod.rs} (97%) diff --git a/library/std/src/sys/sgx/abi/tls.rs b/library/std/src/sys/sgx/abi/tls/mod.rs similarity index 100% rename from library/std/src/sys/sgx/abi/tls.rs rename to library/std/src/sys/sgx/abi/tls/mod.rs diff --git a/library/std/src/sys/sgx/waitqueue.rs b/library/std/src/sys/sgx/waitqueue/mod.rs similarity index 97% rename from library/std/src/sys/sgx/waitqueue.rs rename to library/std/src/sys/sgx/waitqueue/mod.rs index e464dc3ee9d40..61bb11d9a6fad 100644 --- a/library/std/src/sys/sgx/waitqueue.rs +++ b/library/std/src/sys/sgx/waitqueue/mod.rs @@ -13,13 +13,8 @@ #[cfg(test)] mod tests; -/// A doubly-linked list where callers are in charge of memory allocation -/// of the nodes in the list. -mod unsafe_list; - -/// Trivial spinlock-based implementation of `sync::Mutex`. -// FIXME: Perhaps use Intel TSX to avoid locking? mod spin_mutex; +mod unsafe_list; use crate::num::NonZeroUsize; use crate::ops::{Deref, DerefMut}; diff --git a/library/std/src/sys/sgx/waitqueue/spin_mutex.rs b/library/std/src/sys/sgx/waitqueue/spin_mutex.rs index 7f1a671bab4eb..f6e851ccaddfa 100644 --- a/library/std/src/sys/sgx/waitqueue/spin_mutex.rs +++ b/library/std/src/sys/sgx/waitqueue/spin_mutex.rs @@ -1,3 +1,6 @@ +//! Trivial spinlock-based implementation of `sync::Mutex`. +// FIXME: Perhaps use Intel TSX to avoid locking? + #[cfg(test)] mod tests; diff --git a/library/std/src/sys/sgx/waitqueue/unsafe_list.rs b/library/std/src/sys/sgx/waitqueue/unsafe_list.rs index 0834d2593fc32..cf2f0886c1546 100644 --- a/library/std/src/sys/sgx/waitqueue/unsafe_list.rs +++ b/library/std/src/sys/sgx/waitqueue/unsafe_list.rs @@ -1,3 +1,6 @@ +//! A doubly-linked list where callers are in charge of memory allocation +//! of the nodes in the list. + #[cfg(test)] mod tests;