diff options
| author | Matthias Krüger <matthias.krueger@famsik.de> | 2024-06-28 08:34:08 +0200 |
|---|---|---|
| committer | GitHub <noreply@github.com> | 2024-06-28 08:34:08 +0200 |
| commit | c4d0c0892532af122da046d6103f65255ac09690 (patch) | |
| tree | 8495004090234d3453be5ae0c9da66db4d54cb02 /library/core | |
| parent | d5ab93174929284ddd1b818b8ad1b45daeade3b0 (diff) | |
| parent | 7e7d0a959d9a0450ac3596e842dc49e8e6618bc6 (diff) | |
| download | rust-c4d0c0892532af122da046d6103f65255ac09690.tar.gz rust-c4d0c0892532af122da046d6103f65255ac09690.zip | |
Rollup merge of #126956 - joboet:fmt_no_extern_ty, r=RalfJung
core: avoid `extern type`s in formatting infrastructure ```@RalfJung``` [said](https://rust-lang.zulipchat.com/#narrow/stream/219381-t-libs/topic/Use.20of.20.60extern.20type.60.20in.20formatting.20machinery/near/446552837): >How attached are y'all to using `extern type` in the formatting machinery? Seems like this was introduced a [long time ago](https://github.com/rust-lang/rust/commit/34ef8f5441d5335c4177abd622383ed34a6e9315). However, it's also [not really compatible with Stacked Borrows](https://github.com/rust-lang/unsafe-code-guidelines/issues/256), and only works currently because we effectively treat references-to-extern-type almost like raw pointers in Stacked Borrows -- which of course is unsound, it's not how LLVM works. I was planning to make Miri emit a warning when this happens to avoid cases like [this](https://github.com/rust-lang/rust/issues/126814#issuecomment-2183816373) where people use extern type specifically to silence Miri without realizing what happens. but with the formatting machinery using extern type, this warning would just show up everywhere... > > The "proper" way to do this in Stacked Borrows is to use raw pointers (or `NonNull`). This PR does just that. r? ```@RalfJung```
Diffstat (limited to 'library/core')
| -rw-r--r-- | library/core/src/fmt/mod.rs | 6 | ||||
| -rw-r--r-- | library/core/src/fmt/rt.rs | 46 |
2 files changed, 31 insertions, 21 deletions
diff --git a/library/core/src/fmt/mod.rs b/library/core/src/fmt/mod.rs index c25bc5a1b13..3bcd4be1834 100644 --- a/library/core/src/fmt/mod.rs +++ b/library/core/src/fmt/mod.rs @@ -459,6 +459,12 @@ impl<'a> Arguments<'a> { } } +// Manually implementing these results in better error messages. +#[stable(feature = "rust1", since = "1.0.0")] +impl !Send for Arguments<'_> {} +#[stable(feature = "rust1", since = "1.0.0")] +impl !Sync for Arguments<'_> {} + #[stable(feature = "rust1", since = "1.0.0")] impl Debug for Arguments<'_> { fn fmt(&self, fmt: &mut Formatter<'_>) -> Result { diff --git a/library/core/src/fmt/rt.rs b/library/core/src/fmt/rt.rs index 92626feabf3..65a4d537cc7 100644 --- a/library/core/src/fmt/rt.rs +++ b/library/core/src/fmt/rt.rs @@ -5,6 +5,7 @@ use super::*; use crate::hint::unreachable_unchecked; +use crate::ptr::NonNull; #[lang = "format_placeholder"] #[derive(Copy, Clone)] @@ -66,7 +67,13 @@ pub(super) enum Flag { #[derive(Copy, Clone)] enum ArgumentType<'a> { - Placeholder { value: &'a Opaque, formatter: fn(&Opaque, &mut Formatter<'_>) -> Result }, + Placeholder { + // INVARIANT: `formatter` has type `fn(&T, _) -> _` for some `T`, and `value` + // was derived from a `&'a T`. + value: NonNull<()>, + formatter: unsafe fn(NonNull<()>, &mut Formatter<'_>) -> Result, + _lifetime: PhantomData<&'a ()>, + }, Count(usize), } @@ -90,21 +97,15 @@ pub struct Argument<'a> { impl<'a> Argument<'a> { #[inline(always)] fn new<'b, T>(x: &'b T, f: fn(&T, &mut Formatter<'_>) -> Result) -> Argument<'b> { - // SAFETY: `mem::transmute(x)` is safe because - // 1. `&'b T` keeps the lifetime it originated with `'b` - // (so as to not have an unbounded lifetime) - // 2. `&'b T` and `&'b Opaque` have the same memory layout - // (when `T` is `Sized`, as it is here) - // `mem::transmute(f)` is safe since `fn(&T, &mut Formatter<'_>) -> Result` - // and `fn(&Opaque, &mut Formatter<'_>) -> Result` have the same ABI - // (as long as `T` is `Sized`) - unsafe { - Argument { - ty: ArgumentType::Placeholder { - formatter: mem::transmute(f), - value: mem::transmute(x), - }, - } + Argument { + // INVARIANT: this creates an `ArgumentType<'b>` from a `&'b T` and + // a `fn(&T, ...)`, so the invariant is maintained. + ty: ArgumentType::Placeholder { + value: NonNull::from(x).cast(), + // SAFETY: function pointers always have the same layout. + formatter: unsafe { mem::transmute(f) }, + _lifetime: PhantomData, + }, } } @@ -162,7 +163,14 @@ impl<'a> Argument<'a> { #[inline(always)] pub(super) unsafe fn fmt(&self, f: &mut Formatter<'_>) -> Result { match self.ty { - ArgumentType::Placeholder { formatter, value } => formatter(value, f), + // SAFETY: + // Because of the invariant that if `formatter` had the type + // `fn(&T, _) -> _` then `value` has type `&'b T` where `'b` is + // the lifetime of the `ArgumentType`, and because references + // and `NonNull` are ABI-compatible, this is completely equivalent + // to calling the original function passed to `new` with the + // original reference, which is sound. + ArgumentType::Placeholder { formatter, value, .. } => unsafe { formatter(value, f) }, // SAFETY: the caller promised this. ArgumentType::Count(_) => unsafe { unreachable_unchecked() }, } @@ -208,7 +216,3 @@ impl UnsafeArg { Self { _private: () } } } - -extern "C" { - type Opaque; -} |
