Skip to content
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.

Commit ae98528

Browse files
authoredJun 28, 2024
Rollup merge of rust-lang#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](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```
2 parents 6c38c60 + 8db57c2 commit ae98528

File tree

2 files changed

+31
-21
lines changed

2 files changed

+31
-21
lines changed
 

‎core/src/fmt/mod.rs

+6
Original file line numberDiff line numberDiff line change
@@ -459,6 +459,12 @@ impl<'a> Arguments<'a> {
459459
}
460460
}
461461

462+
// Manually implementing these results in better error messages.
463+
#[stable(feature = "rust1", since = "1.0.0")]
464+
impl !Send for Arguments<'_> {}
465+
#[stable(feature = "rust1", since = "1.0.0")]
466+
impl !Sync for Arguments<'_> {}
467+
462468
#[stable(feature = "rust1", since = "1.0.0")]
463469
impl Debug for Arguments<'_> {
464470
fn fmt(&self, fmt: &mut Formatter<'_>) -> Result {

‎core/src/fmt/rt.rs

+25-21
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
66
use super::*;
77
use crate::hint::unreachable_unchecked;
8+
use crate::ptr::NonNull;
89

910
#[lang = "format_placeholder"]
1011
#[derive(Copy, Clone)]
@@ -66,7 +67,13 @@ pub(super) enum Flag {
6667

6768
#[derive(Copy, Clone)]
6869
enum ArgumentType<'a> {
69-
Placeholder { value: &'a Opaque, formatter: fn(&Opaque, &mut Formatter<'_>) -> Result },
70+
Placeholder {
71+
// INVARIANT: `formatter` has type `fn(&T, _) -> _` for some `T`, and `value`
72+
// was derived from a `&'a T`.
73+
value: NonNull<()>,
74+
formatter: unsafe fn(NonNull<()>, &mut Formatter<'_>) -> Result,
75+
_lifetime: PhantomData<&'a ()>,
76+
},
7077
Count(usize),
7178
}
7279

@@ -90,21 +97,15 @@ pub struct Argument<'a> {
9097
impl<'a> Argument<'a> {
9198
#[inline(always)]
9299
fn new<'b, T>(x: &'b T, f: fn(&T, &mut Formatter<'_>) -> Result) -> Argument<'b> {
93-
// SAFETY: `mem::transmute(x)` is safe because
94-
// 1. `&'b T` keeps the lifetime it originated with `'b`
95-
// (so as to not have an unbounded lifetime)
96-
// 2. `&'b T` and `&'b Opaque` have the same memory layout
97-
// (when `T` is `Sized`, as it is here)
98-
// `mem::transmute(f)` is safe since `fn(&T, &mut Formatter<'_>) -> Result`
99-
// and `fn(&Opaque, &mut Formatter<'_>) -> Result` have the same ABI
100-
// (as long as `T` is `Sized`)
101-
unsafe {
102-
Argument {
103-
ty: ArgumentType::Placeholder {
104-
formatter: mem::transmute(f),
105-
value: mem::transmute(x),
106-
},
107-
}
100+
Argument {
101+
// INVARIANT: this creates an `ArgumentType<'b>` from a `&'b T` and
102+
// a `fn(&T, ...)`, so the invariant is maintained.
103+
ty: ArgumentType::Placeholder {
104+
value: NonNull::from(x).cast(),
105+
// SAFETY: function pointers always have the same layout.
106+
formatter: unsafe { mem::transmute(f) },
107+
_lifetime: PhantomData,
108+
},
108109
}
109110
}
110111

@@ -162,7 +163,14 @@ impl<'a> Argument<'a> {
162163
#[inline(always)]
163164
pub(super) unsafe fn fmt(&self, f: &mut Formatter<'_>) -> Result {
164165
match self.ty {
165-
ArgumentType::Placeholder { formatter, value } => formatter(value, f),
166+
// SAFETY:
167+
// Because of the invariant that if `formatter` had the type
168+
// `fn(&T, _) -> _` then `value` has type `&'b T` where `'b` is
169+
// the lifetime of the `ArgumentType`, and because references
170+
// and `NonNull` are ABI-compatible, this is completely equivalent
171+
// to calling the original function passed to `new` with the
172+
// original reference, which is sound.
173+
ArgumentType::Placeholder { formatter, value, .. } => unsafe { formatter(value, f) },
166174
// SAFETY: the caller promised this.
167175
ArgumentType::Count(_) => unsafe { unreachable_unchecked() },
168176
}
@@ -208,7 +216,3 @@ impl UnsafeArg {
208216
Self { _private: () }
209217
}
210218
}
211-
212-
extern "C" {
213-
type Opaque;
214-
}

0 commit comments

Comments
 (0)
Failed to load comments.