Skip to content

Commit

Permalink
Rollup merge of rust-lang#126956 - joboet:fmt_no_extern_ty, r=RalfJung
Browse files Browse the repository at this point in the history
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](rust-lang@34ef8f5). However, it's also [not really compatible with Stacked Borrows](rust-lang/unsafe-code-guidelines#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](rust-lang#126814 (comment)) 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```
  • Loading branch information
matthiaskrgr authored Jun 28, 2024
2 parents 6c38c60 + 8db57c2 commit ae98528
Show file tree
Hide file tree
Showing 2 changed files with 31 additions and 21 deletions.
6 changes: 6 additions & 0 deletions core/src/fmt/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
46 changes: 25 additions & 21 deletions core/src/fmt/rt.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
use super::*;
use crate::hint::unreachable_unchecked;
use crate::ptr::NonNull;

#[lang = "format_placeholder"]
#[derive(Copy, Clone)]
Expand Down Expand Up @@ -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),
}

Expand All @@ -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,
},
}
}

Expand Down Expand Up @@ -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() },
}
Expand Down Expand Up @@ -208,7 +216,3 @@ impl UnsafeArg {
Self { _private: () }
}
}

extern "C" {
type Opaque;
}

0 comments on commit ae98528

Please sign in to comment.