about summary refs log tree commit diff
path: root/library/core
diff options
context:
space:
mode:
authorMatthias Krüger <matthias.krueger@famsik.de>2024-06-28 08:34:08 +0200
committerGitHub <noreply@github.com>2024-06-28 08:34:08 +0200
commitc4d0c0892532af122da046d6103f65255ac09690 (patch)
tree8495004090234d3453be5ae0c9da66db4d54cb02 /library/core
parentd5ab93174929284ddd1b818b8ad1b45daeade3b0 (diff)
parent7e7d0a959d9a0450ac3596e842dc49e8e6618bc6 (diff)
downloadrust-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.rs6
-rw-r--r--library/core/src/fmt/rt.rs46
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;
-}