From 571f7b658964e5781546e8ea4a2d1466e8b0a611 Mon Sep 17 00:00:00 2001 From: Folkert Date: Sun, 28 Jul 2024 15:11:14 +0200 Subject: [PATCH 1/8] improve error message when global asm uses inline asm operands --- compiler/rustc_builtin_macros/messages.ftl | 3 ++ compiler/rustc_builtin_macros/src/asm.rs | 39 +++++++++++++++----- compiler/rustc_builtin_macros/src/errors.rs | 9 +++++ tests/ui/asm/parse-error.rs | 15 ++++++-- tests/ui/asm/parse-error.stderr | 40 ++++++++++++++++++--- 5 files changed, 91 insertions(+), 15 deletions(-) diff --git a/compiler/rustc_builtin_macros/messages.ftl b/compiler/rustc_builtin_macros/messages.ftl index a30ab236213a0..2d9f6b974d2b8 100644 --- a/compiler/rustc_builtin_macros/messages.ftl +++ b/compiler/rustc_builtin_macros/messages.ftl @@ -199,6 +199,9 @@ builtin_macros_format_use_positional = consider using a positional formatting ar builtin_macros_global_asm_clobber_abi = `clobber_abi` cannot be used with `global_asm!` +builtin_macros_global_asm_unsupported_operand = the `{$symbol}` operand cannot be used with `global_asm!` + .label = the `{$symbol}` operand is not meaningful for global-scoped inline assembly, remove it + builtin_macros_global_asm_unsupported_option = the `{$symbol}` option cannot be used with `global_asm!` .label = the `{$symbol}` option is not meaningful for global-scoped inline assembly .suggestion = remove this option diff --git a/compiler/rustc_builtin_macros/src/asm.rs b/compiler/rustc_builtin_macros/src/asm.rs index 62e59f1f4d477..7a5cabfce6ad1 100644 --- a/compiler/rustc_builtin_macros/src/asm.rs +++ b/compiler/rustc_builtin_macros/src/asm.rs @@ -29,6 +29,29 @@ pub struct AsmArgs { pub options_spans: Vec, } +/// Used for better error messages when operand types are used that are not +/// supported by the current macro (e.g. `in` or `out` for `global_asm!`) +/// +/// returns +/// +/// - `Ok(true)` if the current token matches the keyword, and was expected +/// - `Ok(false)` if the current token does not match the keyword +/// - `Err(_)` if the current token matches the keyword, but was not expected +fn eat_operand_keyword<'a>(p: &mut Parser<'a>, symbol: Symbol, expect: bool) -> PResult<'a, bool> { + if expect { + Ok(p.eat_keyword(symbol)) + } else { + let span = p.token.span; + if p.eat_keyword_noexpect(symbol) { + // in gets printed as `r#in` otherwise + let symbol = if symbol == kw::In { "in" } else { symbol.as_str() }; + Err(p.dcx().create_err(errors::GlobalAsmUnsupportedOperand { span, symbol })) + } else { + Ok(false) + } + } +} + fn parse_args<'a>( ecx: &ExtCtxt<'a>, sp: Span, @@ -106,7 +129,7 @@ pub fn parse_asm_args<'a>( }; let mut explicit_reg = false; - let op = if !is_global_asm && p.eat_keyword(kw::In) { + let op = if eat_operand_keyword(p, kw::In, !is_global_asm)? { let reg = parse_reg(p, &mut explicit_reg)?; if p.eat_keyword(kw::Underscore) { let err = dcx.create_err(errors::AsmUnderscoreInput { span: p.token.span }); @@ -114,15 +137,15 @@ pub fn parse_asm_args<'a>( } let expr = p.parse_expr()?; ast::InlineAsmOperand::In { reg, expr } - } else if !is_global_asm && p.eat_keyword(sym::out) { + } else if eat_operand_keyword(p, sym::out, !is_global_asm)? { let reg = parse_reg(p, &mut explicit_reg)?; let expr = if p.eat_keyword(kw::Underscore) { None } else { Some(p.parse_expr()?) }; ast::InlineAsmOperand::Out { reg, expr, late: false } - } else if !is_global_asm && p.eat_keyword(sym::lateout) { + } else if eat_operand_keyword(p, sym::lateout, !is_global_asm)? { let reg = parse_reg(p, &mut explicit_reg)?; let expr = if p.eat_keyword(kw::Underscore) { None } else { Some(p.parse_expr()?) }; ast::InlineAsmOperand::Out { reg, expr, late: true } - } else if !is_global_asm && p.eat_keyword(sym::inout) { + } else if eat_operand_keyword(p, sym::inout, !is_global_asm)? { let reg = parse_reg(p, &mut explicit_reg)?; if p.eat_keyword(kw::Underscore) { let err = dcx.create_err(errors::AsmUnderscoreInput { span: p.token.span }); @@ -136,7 +159,7 @@ pub fn parse_asm_args<'a>( } else { ast::InlineAsmOperand::InOut { reg, expr, late: false } } - } else if !is_global_asm && p.eat_keyword(sym::inlateout) { + } else if eat_operand_keyword(p, sym::inlateout, !is_global_asm)? { let reg = parse_reg(p, &mut explicit_reg)?; if p.eat_keyword(kw::Underscore) { let err = dcx.create_err(errors::AsmUnderscoreInput { span: p.token.span }); @@ -150,6 +173,9 @@ pub fn parse_asm_args<'a>( } else { ast::InlineAsmOperand::InOut { reg, expr, late: true } } + } else if eat_operand_keyword(p, sym::label, !is_global_asm)? { + let block = p.parse_block()?; + ast::InlineAsmOperand::Label { block } } else if p.eat_keyword(kw::Const) { let anon_const = p.parse_expr_anon_const()?; ast::InlineAsmOperand::Const { anon_const } @@ -165,9 +191,6 @@ pub fn parse_asm_args<'a>( path: path.clone(), }; ast::InlineAsmOperand::Sym { sym } - } else if !is_global_asm && p.eat_keyword(sym::label) { - let block = p.parse_block()?; - ast::InlineAsmOperand::Label { block } } else if allow_templates { let template = p.parse_expr()?; // If it can't possibly expand to a string, provide diagnostics here to include other diff --git a/compiler/rustc_builtin_macros/src/errors.rs b/compiler/rustc_builtin_macros/src/errors.rs index f17819474adcd..1c5d98a08970c 100644 --- a/compiler/rustc_builtin_macros/src/errors.rs +++ b/compiler/rustc_builtin_macros/src/errors.rs @@ -856,6 +856,15 @@ pub(crate) struct GlobalAsmUnsupportedOption { pub(crate) full_span: Span, } +#[derive(Diagnostic)] +#[diag(builtin_macros_global_asm_unsupported_operand)] +pub(crate) struct GlobalAsmUnsupportedOperand<'a> { + #[primary_span] + #[label] + pub(crate) span: Span, + pub(crate) symbol: &'a str, +} + #[derive(Diagnostic)] #[diag(builtin_macros_test_runner_invalid)] pub(crate) struct TestRunnerInvalid { diff --git a/tests/ui/asm/parse-error.rs b/tests/ui/asm/parse-error.rs index 9dec3a1c394ef..16ae02828642d 100644 --- a/tests/ui/asm/parse-error.rs +++ b/tests/ui/asm/parse-error.rs @@ -146,5 +146,16 @@ global_asm!(format!("{{{}}}", 0), const FOO); //~^ ERROR asm template must be a string literal global_asm!("{1}", format!("{{{}}}", 0), const FOO, const BAR); //~^ ERROR asm template must be a string literal -global_asm!("{}", label {}); -//~^ ERROR expected operand, options, or additional template string + +global_asm!("{}", in(reg)); +//~^ ERROR the `in` operand cannot be used with `global_asm!` +global_asm!("{}", out(reg)); +//~^ ERROR the `out` operand cannot be used with `global_asm!` +global_asm!("{}", lateout(reg)); +//~^ ERROR the `lateout` operand cannot be used with `global_asm!` +global_asm!("{}", inout(reg)); +//~^ ERROR the `inout` operand cannot be used with `global_asm!` +global_asm!("{}", inlateout(reg)); +//~^ ERROR the `inlateout` operand cannot be used with `global_asm!` +global_asm!("{}", label(reg)); +//~^ ERROR the `label` operand cannot be used with `global_asm!` diff --git a/tests/ui/asm/parse-error.stderr b/tests/ui/asm/parse-error.stderr index e9ecd712bc367..f5f8d537d86b8 100644 --- a/tests/ui/asm/parse-error.stderr +++ b/tests/ui/asm/parse-error.stderr @@ -380,11 +380,41 @@ LL | global_asm!("{1}", format!("{{{}}}", 0), const FOO, const BAR); | = note: this error originates in the macro `format` (in Nightly builds, run with -Z macro-backtrace for more info) -error: expected operand, options, or additional template string - --> $DIR/parse-error.rs:149:19 +error: the `in` operand cannot be used with `global_asm!` + --> $DIR/parse-error.rs:150:19 + | +LL | global_asm!("{}", in(reg)); + | ^^ the `in` operand is not meaningful for global-scoped inline assembly, remove it + +error: the `out` operand cannot be used with `global_asm!` + --> $DIR/parse-error.rs:152:19 + | +LL | global_asm!("{}", out(reg)); + | ^^^ the `out` operand is not meaningful for global-scoped inline assembly, remove it + +error: the `lateout` operand cannot be used with `global_asm!` + --> $DIR/parse-error.rs:154:19 + | +LL | global_asm!("{}", lateout(reg)); + | ^^^^^^^ the `lateout` operand is not meaningful for global-scoped inline assembly, remove it + +error: the `inout` operand cannot be used with `global_asm!` + --> $DIR/parse-error.rs:156:19 + | +LL | global_asm!("{}", inout(reg)); + | ^^^^^ the `inout` operand is not meaningful for global-scoped inline assembly, remove it + +error: the `inlateout` operand cannot be used with `global_asm!` + --> $DIR/parse-error.rs:158:19 + | +LL | global_asm!("{}", inlateout(reg)); + | ^^^^^^^^^ the `inlateout` operand is not meaningful for global-scoped inline assembly, remove it + +error: the `label` operand cannot be used with `global_asm!` + --> $DIR/parse-error.rs:160:19 | -LL | global_asm!("{}", label {}); - | ^^^^^^^^ expected operand, options, or additional template string +LL | global_asm!("{}", label(reg)); + | ^^^^^ the `label` operand is not meaningful for global-scoped inline assembly, remove it error[E0435]: attempt to use a non-constant value in a constant --> $DIR/parse-error.rs:39:37 @@ -441,6 +471,6 @@ help: consider using `const` instead of `let` LL | const bar: /* Type */ = 0; | ~~~~~ ++++++++++++ -error: aborting due to 67 previous errors +error: aborting due to 72 previous errors For more information about this error, try `rustc --explain E0435`. From af79a6396c4175fcdacc18d7448bcde94c3f0363 Mon Sep 17 00:00:00 2001 From: Tshepang Mbambo Date: Fri, 2 Aug 2024 05:16:01 +0200 Subject: [PATCH 2/8] time.rs: remove "Basic usage text" Only one example is given (for each method) --- library/core/src/time.rs | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/library/core/src/time.rs b/library/core/src/time.rs index 179fbabaddeb6..0390bb59a8984 100644 --- a/library/core/src/time.rs +++ b/library/core/src/time.rs @@ -617,8 +617,6 @@ impl Duration { /// /// # Examples /// - /// Basic usage: - /// /// ``` /// use std::time::Duration; /// @@ -640,8 +638,6 @@ impl Duration { /// /// # Examples /// - /// Basic usage: - /// /// ``` /// use std::time::Duration; /// @@ -700,8 +696,6 @@ impl Duration { /// /// # Examples /// - /// Basic usage: - /// /// ``` /// use std::time::Duration; /// @@ -758,8 +752,6 @@ impl Duration { /// /// # Examples /// - /// Basic usage: - /// /// ``` /// use std::time::Duration; /// @@ -814,8 +806,6 @@ impl Duration { /// /// # Examples /// - /// Basic usage: - /// /// ``` /// use std::time::Duration; /// From 5ce554f4ecaffe1161eaecc9111a06da3a95cc12 Mon Sep 17 00:00:00 2001 From: onur-ozkan Date: Sat, 3 Aug 2024 09:31:21 +0300 Subject: [PATCH 3/8] allow setting `link-shared` and `static-libstdcpp` with CI LLVM These options also affect `compiler/rustc_llvm` builds. They should be configurable even when using CI LLVM. Signed-off-by: onur-ozkan --- src/bootstrap/src/core/config/config.rs | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-) diff --git a/src/bootstrap/src/core/config/config.rs b/src/bootstrap/src/core/config/config.rs index 1343e257efe08..39a17754c4f6b 100644 --- a/src/bootstrap/src/core/config/config.rs +++ b/src/bootstrap/src/core/config/config.rs @@ -1840,6 +1840,23 @@ impl Config { config.llvm_from_ci = config.parse_download_ci_llvm(download_ci_llvm, asserts); if config.llvm_from_ci { + let warn = |option: &str| { + println!( + "WARNING: `{option}` will only be used on `compiler/rustc_llvm` build, not for the LLVM build." + ); + println!( + "HELP: To use `{option}` for LLVM builds, set `download-ci-llvm` option to false." + ); + }; + + if static_libstdcpp.is_some() { + warn("static-libstdcpp"); + } + + if link_shared.is_some() { + warn("link-shared"); + } + // None of the LLVM options, except assertions, are supported // when using downloaded LLVM. We could just ignore these but // that's potentially confusing, so force them to not be @@ -1849,9 +1866,6 @@ impl Config { check_ci_llvm!(optimize_toml); check_ci_llvm!(thin_lto); check_ci_llvm!(release_debuginfo); - // CI-built LLVM can be either dynamic or static. We won't know until we download it. - check_ci_llvm!(link_shared); - check_ci_llvm!(static_libstdcpp); check_ci_llvm!(targets); check_ci_llvm!(experimental_targets); check_ci_llvm!(clang_cl); From 21c02517c31e90ec1c001f3997abcea2caa36f0c Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Fri, 2 Aug 2024 10:29:52 +0200 Subject: [PATCH 4/8] Miri: add a flag to do recursive validity checking --- .../src/const_eval/eval_queries.rs | 2 +- .../rustc_const_eval/src/interpret/machine.rs | 7 + .../rustc_const_eval/src/interpret/memory.rs | 5 +- .../rustc_const_eval/src/interpret/place.rs | 15 +- .../src/interpret/validity.rs | 176 +++++++++++------- .../src/util/check_validity_requirement.rs | 2 +- src/tools/miri/README.md | 2 + src/tools/miri/src/bin/miri.rs | 6 +- src/tools/miri/src/eval.rs | 16 +- src/tools/miri/src/intrinsics/mod.rs | 2 +- src/tools/miri/src/lib.rs | 1 + src/tools/miri/src/machine.rs | 36 ++-- .../validity/recursive-validity-ref-bool.rs | 8 + .../recursive-validity-ref-bool.stderr | 15 ++ 14 files changed, 186 insertions(+), 107 deletions(-) create mode 100644 src/tools/miri/tests/fail/validity/recursive-validity-ref-bool.rs create mode 100644 src/tools/miri/tests/fail/validity/recursive-validity-ref-bool.stderr diff --git a/compiler/rustc_const_eval/src/const_eval/eval_queries.rs b/compiler/rustc_const_eval/src/const_eval/eval_queries.rs index ba4b80d102697..6d5bca5731331 100644 --- a/compiler/rustc_const_eval/src/const_eval/eval_queries.rs +++ b/compiler/rustc_const_eval/src/const_eval/eval_queries.rs @@ -396,7 +396,7 @@ fn const_validate_mplace<'tcx>( let alloc_id = mplace.ptr().provenance.unwrap().alloc_id(); let mut ref_tracking = RefTracking::new(mplace.clone()); let mut inner = false; - while let Some((mplace, path)) = ref_tracking.todo.pop() { + while let Some((mplace, path)) = ref_tracking.next() { let mode = match ecx.tcx.static_mutability(cid.instance.def_id()) { _ if cid.promoted.is_some() => CtfeValidationMode::Promoted, Some(mutbl) => CtfeValidationMode::Static { mutbl }, // a `static` diff --git a/compiler/rustc_const_eval/src/interpret/machine.rs b/compiler/rustc_const_eval/src/interpret/machine.rs index 4620b15d8d985..bdce8253b2e76 100644 --- a/compiler/rustc_const_eval/src/interpret/machine.rs +++ b/compiler/rustc_const_eval/src/interpret/machine.rs @@ -165,6 +165,13 @@ pub trait Machine<'tcx>: Sized { /// Whether to enforce the validity invariant for a specific layout. fn enforce_validity(ecx: &InterpCx<'tcx, Self>, layout: TyAndLayout<'tcx>) -> bool; + /// Whether to enforce the validity invariant *recursively*. + fn enforce_validity_recursively( + _ecx: &InterpCx<'tcx, Self>, + _layout: TyAndLayout<'tcx>, + ) -> bool { + false + } /// Whether function calls should be [ABI](CallAbi)-checked. fn enforce_abi(_ecx: &InterpCx<'tcx, Self>) -> bool { diff --git a/compiler/rustc_const_eval/src/interpret/memory.rs b/compiler/rustc_const_eval/src/interpret/memory.rs index b71e6ed8d2b65..2e5d0ae773654 100644 --- a/compiler/rustc_const_eval/src/interpret/memory.rs +++ b/compiler/rustc_const_eval/src/interpret/memory.rs @@ -1006,8 +1006,11 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> { }) } - /// Runs the close in "validation" mode, which means the machine's memory read hooks will be + /// Runs the closure in "validation" mode, which means the machine's memory read hooks will be /// suppressed. Needless to say, this must only be set with great care! Cannot be nested. + /// + /// We do this so Miri's allocation access tracking does not show the validation + /// reads as spurious accesses. pub(super) fn run_for_validation(&self, f: impl FnOnce() -> R) -> R { // This deliberately uses `==` on `bool` to follow the pattern // `assert!(val.replace(new) == old)`. diff --git a/compiler/rustc_const_eval/src/interpret/place.rs b/compiler/rustc_const_eval/src/interpret/place.rs index 242f36363a58e..470a62026b943 100644 --- a/compiler/rustc_const_eval/src/interpret/place.rs +++ b/compiler/rustc_const_eval/src/interpret/place.rs @@ -572,7 +572,10 @@ where if M::enforce_validity(self, dest.layout()) { // Data got changed, better make sure it matches the type! - self.validate_operand(&dest.to_op(self)?)?; + self.validate_operand( + &dest.to_op(self)?, + M::enforce_validity_recursively(self, dest.layout()), + )?; } Ok(()) @@ -811,7 +814,10 @@ where // Generally for transmutation, data must be valid both at the old and new type. // But if the types are the same, the 2nd validation below suffices. if src.layout().ty != dest.layout().ty && M::enforce_validity(self, src.layout()) { - self.validate_operand(&src.to_op(self)?)?; + self.validate_operand( + &src.to_op(self)?, + M::enforce_validity_recursively(self, src.layout()), + )?; } // Do the actual copy. @@ -819,7 +825,10 @@ where if validate_dest && M::enforce_validity(self, dest.layout()) { // Data got changed, better make sure it matches the type! - self.validate_operand(&dest.to_op(self)?)?; + self.validate_operand( + &dest.to_op(self)?, + M::enforce_validity_recursively(self, dest.layout()), + )?; } Ok(()) diff --git a/compiler/rustc_const_eval/src/interpret/validity.rs b/compiler/rustc_const_eval/src/interpret/validity.rs index c8d59c5648da0..460f5448634b5 100644 --- a/compiler/rustc_const_eval/src/interpret/validity.rs +++ b/compiler/rustc_const_eval/src/interpret/validity.rs @@ -155,8 +155,8 @@ impl CtfeValidationMode { /// State for tracking recursive validation of references pub struct RefTracking { - pub seen: FxHashSet, - pub todo: Vec<(T, PATH)>, + seen: FxHashSet, + todo: Vec<(T, PATH)>, } impl RefTracking { @@ -169,8 +169,11 @@ impl RefTracking ref_tracking_for_consts.seen.insert(op); ref_tracking_for_consts } + pub fn next(&mut self) -> Option<(T, PATH)> { + self.todo.pop() + } - pub fn track(&mut self, op: T, path: impl FnOnce() -> PATH) { + fn track(&mut self, op: T, path: impl FnOnce() -> PATH) { if self.seen.insert(op.clone()) { trace!("Recursing below ptr {:#?}", op); let path = path(); @@ -435,88 +438,96 @@ impl<'rt, 'tcx, M: Machine<'tcx>> ValidityVisitor<'rt, 'tcx, M> { if self.ecx.scalar_may_be_null(Scalar::from_maybe_pointer(place.ptr(), self.ecx))? { throw_validation_failure!(self.path, NullPtr { ptr_kind }) } - // Do not allow pointers to uninhabited types. + // Do not allow references to uninhabited types. if place.layout.abi.is_uninhabited() { let ty = place.layout.ty; throw_validation_failure!(self.path, PtrToUninhabited { ptr_kind, ty }) } // Recursive checking if let Some(ref_tracking) = self.ref_tracking.as_deref_mut() { - // Determine whether this pointer expects to be pointing to something mutable. - let ptr_expected_mutbl = match ptr_kind { - PointerKind::Box => Mutability::Mut, - PointerKind::Ref(mutbl) => { - // We do not take into account interior mutability here since we cannot know if - // there really is an `UnsafeCell` inside `Option` -- so we check - // that in the recursive descent behind this reference (controlled by - // `allow_immutable_unsafe_cell`). - mutbl - } - }; // Proceed recursively even for ZST, no reason to skip them! // `!` is a ZST and we want to validate it. - if let Ok((alloc_id, _offset, _prov)) = self.ecx.ptr_try_get_alloc_id(place.ptr(), 0) { + if let Some(ctfe_mode) = self.ctfe_mode { let mut skip_recursive_check = false; - if let Some(GlobalAlloc::Static(did)) = self.ecx.tcx.try_get_global_alloc(alloc_id) + // CTFE imposes restrictions on what references can point to. + if let Ok((alloc_id, _offset, _prov)) = + self.ecx.ptr_try_get_alloc_id(place.ptr(), 0) { - let DefKind::Static { nested, .. } = self.ecx.tcx.def_kind(did) else { bug!() }; - // Special handling for pointers to statics (irrespective of their type). - assert!(!self.ecx.tcx.is_thread_local_static(did)); - assert!(self.ecx.tcx.is_static(did)); - // Mode-specific checks - match self.ctfe_mode { - Some( - CtfeValidationMode::Static { .. } | CtfeValidationMode::Promoted { .. }, - ) => { - // We skip recursively checking other statics. These statics must be sound by - // themselves, and the only way to get broken statics here is by using - // unsafe code. - // The reasons we don't check other statics is twofold. For one, in all - // sound cases, the static was already validated on its own, and second, we - // trigger cycle errors if we try to compute the value of the other static - // and that static refers back to us (potentially through a promoted). - // This could miss some UB, but that's fine. - // We still walk nested allocations, as they are fundamentally part of this validation run. - // This means we will also recurse into nested statics of *other* - // statics, even though we do not recurse into other statics directly. - // That's somewhat inconsistent but harmless. - skip_recursive_check = !nested; - } - Some(CtfeValidationMode::Const { .. }) => { - // We can't recursively validate `extern static`, so we better reject them. - if self.ecx.tcx.is_foreign_item(did) { - throw_validation_failure!(self.path, ConstRefToExtern); + if let Some(GlobalAlloc::Static(did)) = + self.ecx.tcx.try_get_global_alloc(alloc_id) + { + let DefKind::Static { nested, .. } = self.ecx.tcx.def_kind(did) else { + bug!() + }; + // Special handling for pointers to statics (irrespective of their type). + assert!(!self.ecx.tcx.is_thread_local_static(did)); + assert!(self.ecx.tcx.is_static(did)); + // Mode-specific checks + match ctfe_mode { + CtfeValidationMode::Static { .. } + | CtfeValidationMode::Promoted { .. } => { + // We skip recursively checking other statics. These statics must be sound by + // themselves, and the only way to get broken statics here is by using + // unsafe code. + // The reasons we don't check other statics is twofold. For one, in all + // sound cases, the static was already validated on its own, and second, we + // trigger cycle errors if we try to compute the value of the other static + // and that static refers back to us (potentially through a promoted). + // This could miss some UB, but that's fine. + // We still walk nested allocations, as they are fundamentally part of this validation run. + // This means we will also recurse into nested statics of *other* + // statics, even though we do not recurse into other statics directly. + // That's somewhat inconsistent but harmless. + skip_recursive_check = !nested; + } + CtfeValidationMode::Const { .. } => { + // We can't recursively validate `extern static`, so we better reject them. + if self.ecx.tcx.is_foreign_item(did) { + throw_validation_failure!(self.path, ConstRefToExtern); + } } } - None => {} } - } - // Dangling and Mutability check. - let (size, _align, alloc_kind) = self.ecx.get_alloc_info(alloc_id); - if alloc_kind == AllocKind::Dead { - // This can happen for zero-sized references. We can't have *any* references to non-existing - // allocations though, interning rejects them all as the rest of rustc isn't happy with them... - // so we throw an error, even though this isn't really UB. - // A potential future alternative would be to resurrect this as a zero-sized allocation - // (which codegen will then compile to an aligned dummy pointer anyway). - throw_validation_failure!(self.path, DanglingPtrUseAfterFree { ptr_kind }); - } - // If this allocation has size zero, there is no actual mutability here. - if size != Size::ZERO { - let alloc_actual_mutbl = mutability(self.ecx, alloc_id); - // Mutable pointer to immutable memory is no good. - if ptr_expected_mutbl == Mutability::Mut - && alloc_actual_mutbl == Mutability::Not - { - throw_validation_failure!(self.path, MutableRefToImmutable); + // Dangling and Mutability check. + let (size, _align, alloc_kind) = self.ecx.get_alloc_info(alloc_id); + if alloc_kind == AllocKind::Dead { + // This can happen for zero-sized references. We can't have *any* references to + // non-existing allocations in const-eval though, interning rejects them all as + // the rest of rustc isn't happy with them... so we throw an error, even though + // this isn't really UB. + // A potential future alternative would be to resurrect this as a zero-sized allocation + // (which codegen will then compile to an aligned dummy pointer anyway). + throw_validation_failure!(self.path, DanglingPtrUseAfterFree { ptr_kind }); } - // In a const, everything must be completely immutable. - if matches!(self.ctfe_mode, Some(CtfeValidationMode::Const { .. })) { + // If this allocation has size zero, there is no actual mutability here. + if size != Size::ZERO { + // Determine whether this pointer expects to be pointing to something mutable. + let ptr_expected_mutbl = match ptr_kind { + PointerKind::Box => Mutability::Mut, + PointerKind::Ref(mutbl) => { + // We do not take into account interior mutability here since we cannot know if + // there really is an `UnsafeCell` inside `Option` -- so we check + // that in the recursive descent behind this reference (controlled by + // `allow_immutable_unsafe_cell`). + mutbl + } + }; + // Determine what it actually points to. + let alloc_actual_mutbl = mutability(self.ecx, alloc_id); + // Mutable pointer to immutable memory is no good. if ptr_expected_mutbl == Mutability::Mut - || alloc_actual_mutbl == Mutability::Mut + && alloc_actual_mutbl == Mutability::Not { - throw_validation_failure!(self.path, ConstRefToMutable); + throw_validation_failure!(self.path, MutableRefToImmutable); + } + // In a const, everything must be completely immutable. + if matches!(self.ctfe_mode, Some(CtfeValidationMode::Const { .. })) { + if ptr_expected_mutbl == Mutability::Mut + || alloc_actual_mutbl == Mutability::Mut + { + throw_validation_failure!(self.path, ConstRefToMutable); + } } } } @@ -524,6 +535,15 @@ impl<'rt, 'tcx, M: Machine<'tcx>> ValidityVisitor<'rt, 'tcx, M> { if skip_recursive_check { return Ok(()); } + } else { + // This is not CTFE, so it's Miri with recursive checking. + // FIXME: we do *not* check behind boxes, since creating a new box first creates it uninitialized + // and then puts the value in there, so briefly we have a box with uninit contents. + // FIXME: should we also skip `UnsafeCell` behind shared references? Currently that is not + // needed since validation reads bypass Stacked Borrows and data race checks. + if matches!(ptr_kind, PointerKind::Box) { + return Ok(()); + } } let path = &self.path; ref_tracking.track(place, || { @@ -1072,11 +1092,23 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> { /// `op` is assumed to cover valid memory if it is an indirect operand. /// It will error if the bits at the destination do not match the ones described by the layout. #[inline(always)] - pub fn validate_operand(&self, op: &OpTy<'tcx, M::Provenance>) -> InterpResult<'tcx> { + pub fn validate_operand( + &self, + op: &OpTy<'tcx, M::Provenance>, + recursive: bool, + ) -> InterpResult<'tcx> { // Note that we *could* actually be in CTFE here with `-Zextra-const-ub-checks`, but it's // still correct to not use `ctfe_mode`: that mode is for validation of the final constant - // value, it rules out things like `UnsafeCell` in awkward places. It also can make checking - // recurse through references which, for now, we don't want here, either. - self.validate_operand_internal(op, vec![], None, None) + // value, it rules out things like `UnsafeCell` in awkward places. + if !recursive { + return self.validate_operand_internal(op, vec![], None, None); + } + // Do a recursive check. + let mut ref_tracking = RefTracking::empty(); + self.validate_operand_internal(op, vec![], Some(&mut ref_tracking), None)?; + while let Some((mplace, path)) = ref_tracking.todo.pop() { + self.validate_operand_internal(&mplace.into(), path, Some(&mut ref_tracking), None)?; + } + Ok(()) } } diff --git a/compiler/rustc_const_eval/src/util/check_validity_requirement.rs b/compiler/rustc_const_eval/src/util/check_validity_requirement.rs index daf57285ebe6d..4b6b1e453b82e 100644 --- a/compiler/rustc_const_eval/src/util/check_validity_requirement.rs +++ b/compiler/rustc_const_eval/src/util/check_validity_requirement.rs @@ -67,7 +67,7 @@ fn might_permit_raw_init_strict<'tcx>( // This does *not* actually check that references are dereferenceable, but since all types that // require dereferenceability also require non-null, we don't actually get any false negatives // due to this. - Ok(cx.validate_operand(&ot).is_ok()) + Ok(cx.validate_operand(&ot, /*recursive*/ false).is_ok()) } /// Implements the 'lax' (default) version of the `might_permit_raw_init` checks; see that function for diff --git a/src/tools/miri/README.md b/src/tools/miri/README.md index b1be596c00679..ff0f162822c45 100644 --- a/src/tools/miri/README.md +++ b/src/tools/miri/README.md @@ -398,6 +398,8 @@ to Miri failing to detect cases of undefined behavior in a program. application instead of raising an error within the context of Miri (and halting execution). Note that code might not expect these operations to ever panic, so this flag can lead to strange (mis)behavior. +* `-Zmiri-recursive-validation` is a *highly experimental* flag that makes validity checking + recurse below references. * `-Zmiri-retag-fields[=]` controls when Stacked Borrows retagging recurses into fields. `all` means it always recurses (the default, and equivalent to `-Zmiri-retag-fields` without an explicit value), `none` means it never recurses, `scalar` means it only recurses for diff --git a/src/tools/miri/src/bin/miri.rs b/src/tools/miri/src/bin/miri.rs index 25b154a8206c9..4b3c97e248f41 100644 --- a/src/tools/miri/src/bin/miri.rs +++ b/src/tools/miri/src/bin/miri.rs @@ -44,7 +44,7 @@ use rustc_session::config::{CrateType, ErrorOutputType, OptLevel}; use rustc_session::search_paths::PathKind; use rustc_session::{CtfeBacktrace, EarlyDiagCtxt}; -use miri::{BacktraceStyle, BorrowTrackerMethod, ProvenanceMode, RetagFields}; +use miri::{BacktraceStyle, BorrowTrackerMethod, ProvenanceMode, RetagFields, ValidationMode}; struct MiriCompilerCalls { miri_config: miri::MiriConfig, @@ -421,7 +421,9 @@ fn main() { } else if arg == "--" { after_dashdash = true; } else if arg == "-Zmiri-disable-validation" { - miri_config.validate = false; + miri_config.validation = ValidationMode::No; + } else if arg == "-Zmiri-recursive-validation" { + miri_config.validation = ValidationMode::Deep; } else if arg == "-Zmiri-disable-stacked-borrows" { miri_config.borrow_tracker = None; } else if arg == "-Zmiri-tree-borrows" { diff --git a/src/tools/miri/src/eval.rs b/src/tools/miri/src/eval.rs index 2184a4426c8dc..53e877517089c 100644 --- a/src/tools/miri/src/eval.rs +++ b/src/tools/miri/src/eval.rs @@ -68,7 +68,7 @@ pub enum IsolatedOp { Allow, } -#[derive(Copy, Clone, PartialEq, Eq)] +#[derive(Debug, Copy, Clone, PartialEq, Eq)] pub enum BacktraceStyle { /// Prints a terser backtrace which ideally only contains relevant information. Short, @@ -78,6 +78,16 @@ pub enum BacktraceStyle { Off, } +#[derive(Debug, Copy, Clone, PartialEq, Eq)] +pub enum ValidationMode { + /// Do not perform any kind of validation. + No, + /// Validate the interior of the value, but not things behind references. + Shallow, + /// Fully recursively validate references. + Deep, +} + /// Configuration needed to spawn a Miri instance. #[derive(Clone)] pub struct MiriConfig { @@ -85,7 +95,7 @@ pub struct MiriConfig { /// (This is still subject to isolation as well as `forwarded_env_vars`.) pub env: Vec<(OsString, OsString)>, /// Determine if validity checking is enabled. - pub validate: bool, + pub validation: ValidationMode, /// Determines if Stacked Borrows or Tree Borrows is enabled. pub borrow_tracker: Option, /// Whether `core::ptr::Unique` receives special treatment. @@ -162,7 +172,7 @@ impl Default for MiriConfig { fn default() -> MiriConfig { MiriConfig { env: vec![], - validate: true, + validation: ValidationMode::Shallow, borrow_tracker: Some(BorrowTrackerMethod::StackedBorrows), unique_is_unique: false, check_alignment: AlignmentCheck::Int, diff --git a/src/tools/miri/src/intrinsics/mod.rs b/src/tools/miri/src/intrinsics/mod.rs index 9cd776c937101..d60119e75f16d 100644 --- a/src/tools/miri/src/intrinsics/mod.rs +++ b/src/tools/miri/src/intrinsics/mod.rs @@ -153,7 +153,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { // Would not be considered UB, or the other way around (`is_val_statically_known(0)`). "is_val_statically_known" => { let [arg] = check_arg_count(args)?; - this.validate_operand(arg)?; + this.validate_operand(arg, /*recursive*/ false)?; let branch: bool = this.machine.rng.get_mut().gen(); this.write_scalar(Scalar::from_bool(branch), dest)?; } diff --git a/src/tools/miri/src/lib.rs b/src/tools/miri/src/lib.rs index 24909f21eb2b3..2b3ae6df5de8a 100644 --- a/src/tools/miri/src/lib.rs +++ b/src/tools/miri/src/lib.rs @@ -142,6 +142,7 @@ pub use crate::diagnostics::{ }; pub use crate::eval::{ create_ecx, eval_entry, AlignmentCheck, BacktraceStyle, IsolatedOp, MiriConfig, RejectOpWith, + ValidationMode, }; pub use crate::helpers::{AccessKind, EvalContextExt as _}; pub use crate::machine::{ diff --git a/src/tools/miri/src/machine.rs b/src/tools/miri/src/machine.rs index 5f4aa9d2f5d21..fec345c8de1da 100644 --- a/src/tools/miri/src/machine.rs +++ b/src/tools/miri/src/machine.rs @@ -187,7 +187,11 @@ impl fmt::Display for MiriMemoryKind { pub type MemoryKind = interpret::MemoryKind; /// Pointer provenance. -#[derive(Clone, Copy)] +// This needs to be `Eq`+`Hash` because the `Machine` trait needs that because validity checking +// *might* be recursive and then it has to track which places have already been visited. +// These implementations are a bit questionable, and it means we may check the same place multiple +// times with different provenance, but that is in general not wrong. +#[derive(Clone, Copy, PartialEq, Eq, Hash)] pub enum Provenance { /// For pointers with concrete provenance. we exactly know which allocation they are attached to /// and what their borrow tag is. @@ -215,24 +219,6 @@ pub enum Provenance { Wildcard, } -// This needs to be `Eq`+`Hash` because the `Machine` trait needs that because validity checking -// *might* be recursive and then it has to track which places have already been visited. -// However, comparing provenance is meaningless, since `Wildcard` might be any provenance -- and of -// course we don't actually do recursive checking. -// We could change `RefTracking` to strip provenance for its `seen` set but that type is generic so that is quite annoying. -// Instead owe add the required instances but make them panic. -impl PartialEq for Provenance { - fn eq(&self, _other: &Self) -> bool { - panic!("Provenance must not be compared") - } -} -impl Eq for Provenance {} -impl std::hash::Hash for Provenance { - fn hash(&self, _state: &mut H) { - panic!("Provenance must not be hashed") - } -} - /// The "extra" information a pointer has over a regular AllocId. #[derive(Copy, Clone, PartialEq)] pub enum ProvenanceExtra { @@ -460,7 +446,7 @@ pub struct MiriMachine<'tcx> { pub(crate) isolated_op: IsolatedOp, /// Whether to enforce the validity invariant. - pub(crate) validate: bool, + pub(crate) validation: ValidationMode, /// The table of file descriptors. pub(crate) fds: shims::FdTable, @@ -659,7 +645,7 @@ impl<'tcx> MiriMachine<'tcx> { cmd_line: None, tls: TlsData::default(), isolated_op: config.isolated_op, - validate: config.validate, + validation: config.validation, fds: shims::FdTable::init(config.mute_stdout_stderr), dirs: Default::default(), layouts, @@ -801,7 +787,7 @@ impl VisitProvenance for MiriMachine<'_> { fds, tcx: _, isolated_op: _, - validate: _, + validation: _, clock: _, layouts: _, static_roots: _, @@ -943,7 +929,11 @@ impl<'tcx> Machine<'tcx> for MiriMachine<'tcx> { #[inline(always)] fn enforce_validity(ecx: &MiriInterpCx<'tcx>, _layout: TyAndLayout<'tcx>) -> bool { - ecx.machine.validate + ecx.machine.validation != ValidationMode::No + } + #[inline(always)] + fn enforce_validity_recursively(ecx: &InterpCx<'tcx, Self>, _layout: TyAndLayout<'tcx>) -> bool { + ecx.machine.validation == ValidationMode::Deep } #[inline(always)] diff --git a/src/tools/miri/tests/fail/validity/recursive-validity-ref-bool.rs b/src/tools/miri/tests/fail/validity/recursive-validity-ref-bool.rs new file mode 100644 index 0000000000000..17b81f29cfe9a --- /dev/null +++ b/src/tools/miri/tests/fail/validity/recursive-validity-ref-bool.rs @@ -0,0 +1,8 @@ +//@compile-flags: -Zmiri-recursive-validation + +fn main() { + let x = 3u8; + let xref = &x; + let xref_wrong_type: &bool = unsafe { std::mem::transmute(xref) }; //~ERROR: encountered 0x03, but expected a boolean + let _val = *xref_wrong_type; +} diff --git a/src/tools/miri/tests/fail/validity/recursive-validity-ref-bool.stderr b/src/tools/miri/tests/fail/validity/recursive-validity-ref-bool.stderr new file mode 100644 index 0000000000000..2b2fa9b8a206d --- /dev/null +++ b/src/tools/miri/tests/fail/validity/recursive-validity-ref-bool.stderr @@ -0,0 +1,15 @@ +error: Undefined Behavior: constructing invalid value at .: encountered 0x03, but expected a boolean + --> $DIR/recursive-validity-ref-bool.rs:LL:CC + | +LL | let xref_wrong_type: &bool = unsafe { std::mem::transmute(xref) }; + | ^^^^^^^^^^^^^^^^^^^^^^^^^ constructing invalid value at .: encountered 0x03, but expected a boolean + | + = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior + = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information + = note: BACKTRACE: + = note: inside `main` at $DIR/recursive-validity-ref-bool.rs:LL:CC + +note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace + +error: aborting due to 1 previous error + From 9061915fffe5d3f127b7a9286457a285a0df9e93 Mon Sep 17 00:00:00 2001 From: Folkert Date: Mon, 29 Jul 2024 21:54:40 +0200 Subject: [PATCH 5/8] add test for symbol visibility of `#[naked]` functions --- .../naked-symbol-visibility/a_rust_dylib.rs | 74 +++++++++++++++++++ .../run-make/naked-symbol-visibility/rmake.rs | 63 ++++++++++++++++ 2 files changed, 137 insertions(+) create mode 100644 tests/run-make/naked-symbol-visibility/a_rust_dylib.rs create mode 100644 tests/run-make/naked-symbol-visibility/rmake.rs diff --git a/tests/run-make/naked-symbol-visibility/a_rust_dylib.rs b/tests/run-make/naked-symbol-visibility/a_rust_dylib.rs new file mode 100644 index 0000000000000..50cf4943d34c7 --- /dev/null +++ b/tests/run-make/naked-symbol-visibility/a_rust_dylib.rs @@ -0,0 +1,74 @@ +#![feature(naked_functions, asm_const, linkage)] +#![crate_type = "dylib"] + +use std::arch::asm; + +pub trait TraitWithConst { + const COUNT: u32; +} + +struct Test; + +impl TraitWithConst for Test { + const COUNT: u32 = 1; +} + +#[no_mangle] +fn entry() { + private_vanilla_rust_function_from_rust_dylib(); + private_naked_rust_function_from_rust_dylib(); + + public_vanilla_generic_function_from_rust_dylib::(); + public_naked_generic_function_from_rust_dylib::(); +} + +extern "C" fn private_vanilla_rust_function_from_rust_dylib() -> u32 { + 42 +} + +#[no_mangle] +pub extern "C" fn public_vanilla_rust_function_from_rust_dylib() -> u32 { + 42 +} + +pub extern "C" fn public_vanilla_generic_function_from_rust_dylib() -> u32 { + T::COUNT +} + +#[linkage = "weak"] +extern "C" fn vanilla_weak_linkage() -> u32 { + 42 +} + +#[linkage = "external"] +extern "C" fn vanilla_external_linkage() -> u32 { + 42 +} + +#[naked] +extern "C" fn private_naked_rust_function_from_rust_dylib() -> u32 { + unsafe { asm!("mov rax, 42", "ret", options(noreturn)) } +} + +#[naked] +#[no_mangle] +pub extern "C" fn public_naked_rust_function_from_rust_dylib() -> u32 { + unsafe { asm!("mov rax, 42", "ret", options(noreturn)) } +} + +#[naked] +pub extern "C" fn public_naked_generic_function_from_rust_dylib() -> u32 { + unsafe { asm!("mov rax, {}", "ret", const T::COUNT, options(noreturn)) } +} + +#[naked] +#[linkage = "weak"] +extern "C" fn naked_weak_linkage() -> u32 { + unsafe { asm!("mov rax, 42", "ret", options(noreturn)) } +} + +#[naked] +#[linkage = "external"] +extern "C" fn naked_external_linkage() -> u32 { + unsafe { asm!("mov rax, 42", "ret", options(noreturn)) } +} diff --git a/tests/run-make/naked-symbol-visibility/rmake.rs b/tests/run-make/naked-symbol-visibility/rmake.rs new file mode 100644 index 0000000000000..c888e120113ef --- /dev/null +++ b/tests/run-make/naked-symbol-visibility/rmake.rs @@ -0,0 +1,63 @@ +// @only-x86_64 +use run_make_support::{dynamic_lib_name, llvm_readobj, regex, rustc}; + +fn main() { + let rdylib_name = dynamic_lib_name("a_rust_dylib"); + rustc().arg("-Zshare-generics=no").input("a_rust_dylib.rs").run(); + + // check vanilla symbols + not_exported(&rdylib_name, "private_vanilla_rust_function_from_rust_dylib"); + global_function(&rdylib_name, "public_vanilla_rust_function_from_rust_dylib"); + not_exported(&rdylib_name, "public_vanilla_generic_function_from_rust_dylib"); + + weak_function(&rdylib_name, "vanilla_weak_linkage"); + global_function(&rdylib_name, "vanilla_external_linkage"); + + // naked should mirror vanilla + not_exported(&rdylib_name, "private_naked_rust_function_from_rust_dylib"); + global_function(&rdylib_name, "public_naked_rust_function_from_rust_dylib"); + not_exported(&rdylib_name, "public_naked_generic_function_from_rust_dylib"); + + weak_function(&rdylib_name, "naked_weak_linkage"); + global_function(&rdylib_name, "naked_external_linkage"); + + // share generics should expose the generic functions + rustc().arg("-Zshare-generics=yes").input("a_rust_dylib.rs").run(); + global_function(&rdylib_name, "public_vanilla_generic_function_from_rust_dylib"); + global_function(&rdylib_name, "public_naked_generic_function_from_rust_dylib"); +} + +#[track_caller] +fn global_function(path: &str, symbol_name: &str) { + let lines = find_dynamic_symbol(path, symbol_name); + let [line] = lines.as_slice() else { + panic!("symbol {symbol_name} occurs {} times", lines.len()) + }; + + assert!(line.contains("FUNC"), "`{symbol_name}` is not a function"); + assert!(line.contains("GLOBAL"), "`{symbol_name}` is not marked as global"); +} + +#[track_caller] +fn weak_function(path: &str, symbol_name: &str) { + let lines = find_dynamic_symbol(path, symbol_name); + let [line] = lines.as_slice() else { + panic!("symbol {symbol_name} occurs {} times", lines.len()) + }; + + assert!(line.contains("FUNC"), "`{symbol_name}` is not a function"); + assert!(line.contains("WEAK"), "`{symbol_name}` is not marked as weak"); +} + +#[track_caller] +fn not_exported(path: &str, symbol_name: &str) { + assert_eq!(find_dynamic_symbol(path, symbol_name).len(), 0) +} + +fn find_dynamic_symbol<'a>(path: &str, symbol_name: &str) -> Vec { + let out = llvm_readobj().arg("--dyn-symbols").input(path).run().stdout_utf8(); + out.lines() + .filter(|&line| !line.contains("__imp_") && line.contains(symbol_name)) + .map(|line| line.to_string()) + .collect() +} From 940a1090fb36f7b78c9c346a50225cab57639764 Mon Sep 17 00:00:00 2001 From: Folkert Date: Tue, 30 Jul 2024 10:28:41 +0200 Subject: [PATCH 6/8] add test that tracks that functions defined in extern blocks are not exported this maybe should change, but now at least it won't be changed unintentionally --- .../naked-symbol-visibility/a_rust_dylib.rs | 13 +++ .../run-make/naked-symbol-visibility/rmake.rs | 81 +++++++++++-------- 2 files changed, 62 insertions(+), 32 deletions(-) diff --git a/tests/run-make/naked-symbol-visibility/a_rust_dylib.rs b/tests/run-make/naked-symbol-visibility/a_rust_dylib.rs index 50cf4943d34c7..7e81718f76c0e 100644 --- a/tests/run-make/naked-symbol-visibility/a_rust_dylib.rs +++ b/tests/run-make/naked-symbol-visibility/a_rust_dylib.rs @@ -72,3 +72,16 @@ extern "C" fn naked_weak_linkage() -> u32 { extern "C" fn naked_external_linkage() -> u32 { unsafe { asm!("mov rax, 42", "ret", options(noreturn)) } } + +// functions that are declared in an `extern "C"` block are currently not exported +// this maybe should change in the future, this is just tracking the current behavior +// reported in https://github.com/rust-lang/rust/issues/128071 +std::arch::global_asm! { + ".globl function_defined_in_global_asm", + "function_defined_in_global_asm:", + "ret", +} + +extern "C" { + pub fn function_defined_in_global_asm(); +} diff --git a/tests/run-make/naked-symbol-visibility/rmake.rs b/tests/run-make/naked-symbol-visibility/rmake.rs index c888e120113ef..2bc585b840dc5 100644 --- a/tests/run-make/naked-symbol-visibility/rmake.rs +++ b/tests/run-make/naked-symbol-visibility/rmake.rs @@ -1,63 +1,80 @@ // @only-x86_64 -use run_make_support::{dynamic_lib_name, llvm_readobj, regex, rustc}; +use run_make_support::object::read::{File, Object, Symbol}; +use run_make_support::object::ObjectSymbol; +use run_make_support::{dynamic_lib_name, rfs, rustc}; fn main() { let rdylib_name = dynamic_lib_name("a_rust_dylib"); rustc().arg("-Zshare-generics=no").input("a_rust_dylib.rs").run(); + let binary_data = rfs::read(&rdylib_name); + let rdylib = File::parse(&*binary_data).unwrap(); + // check vanilla symbols - not_exported(&rdylib_name, "private_vanilla_rust_function_from_rust_dylib"); - global_function(&rdylib_name, "public_vanilla_rust_function_from_rust_dylib"); - not_exported(&rdylib_name, "public_vanilla_generic_function_from_rust_dylib"); + not_exported(&rdylib, "private_vanilla_rust_function_from_rust_dylib"); + global_function(&rdylib, "public_vanilla_rust_function_from_rust_dylib"); + not_exported(&rdylib, "public_vanilla_generic_function_from_rust_dylib"); - weak_function(&rdylib_name, "vanilla_weak_linkage"); - global_function(&rdylib_name, "vanilla_external_linkage"); + weak_function(&rdylib, "vanilla_weak_linkage"); + global_function(&rdylib, "vanilla_external_linkage"); // naked should mirror vanilla - not_exported(&rdylib_name, "private_naked_rust_function_from_rust_dylib"); - global_function(&rdylib_name, "public_naked_rust_function_from_rust_dylib"); - not_exported(&rdylib_name, "public_naked_generic_function_from_rust_dylib"); + not_exported(&rdylib, "private_naked_rust_function_from_rust_dylib"); + global_function(&rdylib, "public_naked_rust_function_from_rust_dylib"); + not_exported(&rdylib, "public_naked_generic_function_from_rust_dylib"); + + weak_function(&rdylib, "naked_weak_linkage"); + global_function(&rdylib, "naked_external_linkage"); - weak_function(&rdylib_name, "naked_weak_linkage"); - global_function(&rdylib_name, "naked_external_linkage"); + // functions that are declared in an `extern "C"` block are currently not exported + // this maybe should change in the future, this is just tracking the current behavior + // reported in https://github.com/rust-lang/rust/issues/128071 + not_exported(&rdylib, "function_defined_in_global_asm"); // share generics should expose the generic functions rustc().arg("-Zshare-generics=yes").input("a_rust_dylib.rs").run(); - global_function(&rdylib_name, "public_vanilla_generic_function_from_rust_dylib"); - global_function(&rdylib_name, "public_naked_generic_function_from_rust_dylib"); + let binary_data = rfs::read(&rdylib_name); + let rdylib = File::parse(&*binary_data).unwrap(); + + global_function(&rdylib, "public_vanilla_generic_function_from_rust_dylib"); + global_function(&rdylib, "public_naked_generic_function_from_rust_dylib"); } #[track_caller] -fn global_function(path: &str, symbol_name: &str) { - let lines = find_dynamic_symbol(path, symbol_name); - let [line] = lines.as_slice() else { - panic!("symbol {symbol_name} occurs {} times", lines.len()) +fn global_function(file: &File, symbol_name: &str) { + let symbols = find_dynamic_symbol(file, symbol_name); + let [symbol] = symbols.as_slice() else { + panic!("symbol {symbol_name} occurs {} times", symbols.len()) }; - assert!(line.contains("FUNC"), "`{symbol_name}` is not a function"); - assert!(line.contains("GLOBAL"), "`{symbol_name}` is not marked as global"); + assert!(symbol.is_definition(), "`{symbol_name}` is not a function"); + assert!(symbol.is_global(), "`{symbol_name}` is not marked as global"); } #[track_caller] -fn weak_function(path: &str, symbol_name: &str) { - let lines = find_dynamic_symbol(path, symbol_name); - let [line] = lines.as_slice() else { - panic!("symbol {symbol_name} occurs {} times", lines.len()) +fn weak_function(file: &File, symbol_name: &str) { + let symbols = find_dynamic_symbol(file, symbol_name); + let [symbol] = symbols.as_slice() else { + panic!("symbol {symbol_name} occurs {} times", symbols.len()) }; - assert!(line.contains("FUNC"), "`{symbol_name}` is not a function"); - assert!(line.contains("WEAK"), "`{symbol_name}` is not marked as weak"); + assert!(symbol.is_definition(), "`{symbol_name}` is not a function"); + assert!(symbol.is_weak(), "`{symbol_name}` is not marked as weak"); } #[track_caller] -fn not_exported(path: &str, symbol_name: &str) { - assert_eq!(find_dynamic_symbol(path, symbol_name).len(), 0) +fn not_exported(file: &File, symbol_name: &str) { + assert_eq!(find_dynamic_symbol(file, symbol_name).len(), 0) } -fn find_dynamic_symbol<'a>(path: &str, symbol_name: &str) -> Vec { - let out = llvm_readobj().arg("--dyn-symbols").input(path).run().stdout_utf8(); - out.lines() - .filter(|&line| !line.contains("__imp_") && line.contains(symbol_name)) - .map(|line| line.to_string()) +fn find_dynamic_symbol<'file, 'data>( + file: &'file File<'data>, + expected: &str, +) -> Vec> { + file.dynamic_symbols() + .filter(|symbol| { + let name = symbol.name().unwrap(); + !name.contains("__imp_") && name.contains(expected) + }) .collect() } From eb2de64aa1cb0d3eded99458566e8eee8333a751 Mon Sep 17 00:00:00 2001 From: Michael Howell Date: Sat, 3 Aug 2024 12:30:38 -0700 Subject: [PATCH 7/8] rustdoc: make the hover trail for doc anchors a bit bigger https://rust-lang.zulipchat.com/#narrow/stream/266220-t-rustdoc/topic/Weird.20markdown.20heading.20rendering.3F --- src/librustdoc/html/static/css/rustdoc.css | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/librustdoc/html/static/css/rustdoc.css b/src/librustdoc/html/static/css/rustdoc.css index f4e231327a8c2..02a6bb8f5487a 100644 --- a/src/librustdoc/html/static/css/rustdoc.css +++ b/src/librustdoc/html/static/css/rustdoc.css @@ -930,7 +930,7 @@ a.doc-anchor { left: -17px; /* We add this padding so that when the cursor moves from the heading's text to the anchor, the anchor doesn't disappear. */ - padding-right: 5px; + padding-right: 10px; /* And this padding is used to make the anchor larger and easier to click on. */ padding-left: 3px; } From cb7c596681c3c5718c7fd0fbe15c487cd36c65c3 Mon Sep 17 00:00:00 2001 From: Guillaume Gomez Date: Sun, 4 Aug 2024 01:08:10 +0200 Subject: [PATCH 8/8] Update rinja version to 0.3.0 --- Cargo.lock | 15 +++++++++------ src/librustdoc/Cargo.toml | 2 +- 2 files changed, 10 insertions(+), 7 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 0b5aa7db3190c..13b8eaea06804 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3282,20 +3282,22 @@ dependencies = [ [[package]] name = "rinja" -version = "0.2.0" +version = "0.3.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "d2d47a46d7729e891c8accf260e9daa02ae6d570aa2a94fb1fb27eb5364a2323" +checksum = "6d3762e3740cdbf2fd2be465cc2c26d643ad17353cc2e0223d211c1b096118bd" dependencies = [ + "itoa", "rinja_derive", ] [[package]] name = "rinja_derive" -version = "0.2.0" +version = "0.3.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "44dae9afe59d58ed8d988d67d1945f3638125d2fd2104058399382e11bd3ea2a" +checksum = "fd01fd8e15e7d19c8b8052c1d428325131e02ff1633cdcf695190c2e56ab682c" dependencies = [ "basic-toml", + "memchr", "mime", "mime_guess", "once_map", @@ -3308,10 +3310,11 @@ dependencies = [ [[package]] name = "rinja_parser" -version = "0.2.0" +version = "0.3.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "1b1771c78cd5d3b1646ef8d8f2ed100db936e8b291d3cc06e92a339ff346858c" +checksum = "a2f6bf7cef118c6de21206edf0b3f19f5ede60006be674a58ca21b6e003a1b57" dependencies = [ + "memchr", "nom", ] diff --git a/src/librustdoc/Cargo.toml b/src/librustdoc/Cargo.toml index dfd7414652fa7..b3fccbf6456e0 100644 --- a/src/librustdoc/Cargo.toml +++ b/src/librustdoc/Cargo.toml @@ -8,7 +8,7 @@ path = "lib.rs" [dependencies] arrayvec = { version = "0.7", default-features = false } -rinja = { version = "0.2", default-features = false, features = ["config"] } +rinja = { version = "0.3", default-features = false, features = ["config"] } base64 = "0.21.7" itertools = "0.12" indexmap = "2"