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`
matthiaskrgr · Jun 28, 2024 · 9352e7c · 9352e7c
2 parents a100d94 + 7e7d0a9
commit 9352e7c
Show file tree

Hide file tree

Showing 5 changed files with 47 additions and 55 deletions.
diff --git 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
@@ -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;
-}
diff --git a/tests/coverage/issue-83601.cov-map b/tests/coverage/issue-83601.cov-map
@@ -1,12 +1,12 @@
 Function name: issue_83601::main
-Raw bytes (21): 0x[01, 01, 01, 05, 00, 03, 01, 06, 01, 02, 1c, 05, 03, 09, 01, 1c, 02, 02, 05, 03, 02]
+Raw bytes (21): 0x[01, 01, 01, 05, 09, 03, 01, 06, 01, 02, 1c, 05, 03, 09, 01, 1c, 02, 02, 05, 03, 02]
 Number of files: 1
 - file 0 => global file 1
 Number of expressions: 1
-- expression 0 operands: lhs = Counter(1), rhs = Zero
+- expression 0 operands: lhs = Counter(1), rhs = Counter(2)
 Number of file 0 mappings: 3
 - Code(Counter(0)) at (prev + 6, 1) to (start + 2, 28)
 - Code(Counter(1)) at (prev + 3, 9) to (start + 1, 28)
 - Code(Expression(0, Sub)) at (prev + 2, 5) to (start + 3, 2)
-    = (c1 - Zero)
+    = (c1 - c2)
 
diff --git a/tests/coverage/issue-84561.cov-map b/tests/coverage/issue-84561.cov-map
@@ -54,15 +54,15 @@ Number of file 0 mappings: 1
 - Code(Counter(0)) at (prev + 167, 9) to (start + 2, 10)
 
 Function name: issue_84561::test3
-Raw bytes (375): 0x[01, 01, 31, 05, 00, 0d, 00, 15, 00, 12, 00, 15, 00, 21, 00, 1e, 00, 21, 00, 31, 00, 3d, 00, 2e, 45, 3d, 00, 42, 49, 45, 00, 3f, 51, 42, 49, 45, 00, 7a, 55, 51, 00, 7a, 55, 51, 00, 77, 5d, 7a, 55, 51, 00, 77, 61, 7a, 55, 51, 00, 72, 65, 77, 61, 7a, 55, 51, 00, 75, be, 01, c2, 01, 79, 69, 6d, 69, 6d, 69, 6d, c2, 01, 00, 69, 6d, c2, 01, 79, 69, 6d, bb, 01, 7d, 75, be, 01, c2, 01, 79, 69, 6d, b6, 01, 00, bb, 01, 7d, 75, be, 01, c2, 01, 79, 69, 6d, 33, 01, 08, 01, 03, 1c, 05, 04, 09, 01, 1c, 02, 02, 05, 04, 1f, 0d, 05, 05, 00, 1f, 06, 01, 05, 00, 1f, 15, 01, 09, 01, 1c, 12, 02, 05, 00, 1f, 0e, 01, 05, 00, 0f, 00, 00, 20, 00, 30, 21, 01, 05, 03, 0f, 00, 03, 20, 00, 30, 00, 00, 33, 00, 41, 00, 00, 4b, 00, 5a, 1e, 01, 05, 00, 0f, 00, 05, 09, 03, 10, 00, 05, 0d, 00, 1b, 00, 02, 0d, 00, 1c, 1a, 04, 09, 05, 06, 31, 06, 05, 03, 06, 22, 04, 05, 03, 06, 3d, 04, 09, 04, 06, 2e, 05, 08, 00, 0f, 45, 01, 09, 03, 0a, 2a, 05, 09, 03, 0a, 3f, 05, 08, 00, 0f, 51, 01, 09, 00, 13, 00, 03, 0d, 00, 1d, 3a, 03, 09, 00, 13, 00, 03, 0d, 00, 1d, 77, 03, 05, 00, 0f, 77, 01, 0c, 00, 13, 5d, 01, 0d, 00, 13, 56, 02, 0d, 00, 13, 72, 04, 05, 02, 13, 65, 03, 0d, 00, 13, 6e, 02, 0d, 00, 13, bb, 01, 03, 05, 00, 0f, 69, 01, 0c, 00, 13, 6d, 01, 0d, 03, 0e, 75, 04, 0d, 00, 13, c2, 01, 02, 0d, 00, 17, c2, 01, 01, 14, 00, 1b, 00, 01, 15, 00, 1b, 92, 01, 02, 15, 00, 1b, be, 01, 04, 0d, 00, 13, 7d, 03, 09, 00, 19, b6, 01, 02, 05, 00, 0f, b2, 01, 03, 09, 00, 22, 00, 02, 05, 00, 0f, 00, 03, 09, 00, 2c, 00, 02, 01, 00, 02]
+Raw bytes (375): 0x[01, 01, 31, 05, 09, 0d, 00, 15, 19, 12, 00, 15, 19, 21, 00, 1e, 00, 21, 00, 31, 00, 3d, 00, 2e, 45, 3d, 00, 42, 49, 45, 00, 3f, 51, 42, 49, 45, 00, 7a, 55, 51, 00, 7a, 55, 51, 00, 77, 5d, 7a, 55, 51, 00, 77, 61, 7a, 55, 51, 00, 72, 65, 77, 61, 7a, 55, 51, 00, 75, be, 01, c2, 01, 79, 69, 6d, 69, 6d, 69, 6d, c2, 01, 00, 69, 6d, c2, 01, 79, 69, 6d, bb, 01, 7d, 75, be, 01, c2, 01, 79, 69, 6d, b6, 01, 00, bb, 01, 7d, 75, be, 01, c2, 01, 79, 69, 6d, 33, 01, 08, 01, 03, 1c, 05, 04, 09, 01, 1c, 02, 02, 05, 04, 1f, 0d, 05, 05, 00, 1f, 06, 01, 05, 00, 1f, 15, 01, 09, 01, 1c, 12, 02, 05, 00, 1f, 0e, 01, 05, 00, 0f, 00, 00, 20, 00, 30, 21, 01, 05, 03, 0f, 00, 03, 20, 00, 30, 00, 00, 33, 00, 41, 00, 00, 4b, 00, 5a, 1e, 01, 05, 00, 0f, 00, 05, 09, 03, 10, 00, 05, 0d, 00, 1b, 00, 02, 0d, 00, 1c, 1a, 04, 09, 05, 06, 31, 06, 05, 03, 06, 22, 04, 05, 03, 06, 3d, 04, 09, 04, 06, 2e, 05, 08, 00, 0f, 45, 01, 09, 03, 0a, 2a, 05, 09, 03, 0a, 3f, 05, 08, 00, 0f, 51, 01, 09, 00, 13, 00, 03, 0d, 00, 1d, 3a, 03, 09, 00, 13, 00, 03, 0d, 00, 1d, 77, 03, 05, 00, 0f, 77, 01, 0c, 00, 13, 5d, 01, 0d, 00, 13, 56, 02, 0d, 00, 13, 72, 04, 05, 02, 13, 65, 03, 0d, 00, 13, 6e, 02, 0d, 00, 13, bb, 01, 03, 05, 00, 0f, 69, 01, 0c, 00, 13, 6d, 01, 0d, 03, 0e, 75, 04, 0d, 00, 13, c2, 01, 02, 0d, 00, 17, c2, 01, 01, 14, 00, 1b, 00, 01, 15, 00, 1b, 92, 01, 02, 15, 00, 1b, be, 01, 04, 0d, 00, 13, 7d, 03, 09, 00, 19, b6, 01, 02, 05, 00, 0f, b2, 01, 03, 09, 00, 22, 00, 02, 05, 00, 0f, 00, 03, 09, 00, 2c, 00, 02, 01, 00, 02]
 Number of files: 1
 - file 0 => global file 1
 Number of expressions: 49
-- expression 0 operands: lhs = Counter(1), rhs = Zero
+- expression 0 operands: lhs = Counter(1), rhs = Counter(2)
 - expression 1 operands: lhs = Counter(3), rhs = Zero
-- expression 2 operands: lhs = Counter(5), rhs = Zero
+- expression 2 operands: lhs = Counter(5), rhs = Counter(6)
 - expression 3 operands: lhs = Expression(4, Sub), rhs = Zero
-- expression 4 operands: lhs = Counter(5), rhs = Zero
+- expression 4 operands: lhs = Counter(5), rhs = Counter(6)
 - expression 5 operands: lhs = Counter(8), rhs = Zero
 - expression 6 operands: lhs = Expression(7, Sub), rhs = Zero
 - expression 7 operands: lhs = Counter(8), rhs = Zero
@@ -111,15 +111,15 @@ Number of file 0 mappings: 51
 - Code(Counter(0)) at (prev + 8, 1) to (start + 3, 28)
 - Code(Counter(1)) at (prev + 4, 9) to (start + 1, 28)
 - Code(Expression(0, Sub)) at (prev + 2, 5) to (start + 4, 31)
-    = (c1 - Zero)
+    = (c1 - c2)
 - Code(Counter(3)) at (prev + 5, 5) to (start + 0, 31)
 - Code(Expression(1, Sub)) at (prev + 1, 5) to (start + 0, 31)
     = (c3 - Zero)
 - Code(Counter(5)) at (prev + 1, 9) to (start + 1, 28)
 - Code(Expression(4, Sub)) at (prev + 2, 5) to (start + 0, 31)
-    = (c5 - Zero)
+    = (c5 - c6)
 - Code(Expression(3, Sub)) at (prev + 1, 5) to (start + 0, 15)
-    = ((c5 - Zero) - Zero)
+    = ((c5 - c6) - Zero)
 - Code(Zero) at (prev + 0, 32) to (start + 0, 48)
 - Code(Counter(8)) at (prev + 1, 5) to (start + 3, 15)
 - Code(Zero) at (prev + 3, 32) to (start + 0, 48)

diff --git a/tests/ui/fmt/send-sync.stderr b/tests/ui/fmt/send-sync.stderr
@@ -1,45 +1,27 @@
-error[E0277]: `core::fmt::rt::Opaque` cannot be shared between threads safely
+error[E0277]: `Arguments<'_>` cannot be sent between threads safely
   --> $DIR/send-sync.rs:8:10
    |
 LL |     send(format_args!("{:?}", c));
-   |     ---- ^^^^^^^^^^^^^^^^^^^^^^^ `core::fmt::rt::Opaque` cannot be shared between threads safely
+   |     ---- ^^^^^^^^^^^^^^^^^^^^^^^ `Arguments<'_>` cannot be sent between threads safely
    |     |
    |     required by a bound introduced by this call
    |
-   = help: within `[core::fmt::rt::Argument<'_>]`, the trait `Sync` is not implemented for `core::fmt::rt::Opaque`, which is required by `Arguments<'_>: Send`
-   = note: required because it appears within the type `&core::fmt::rt::Opaque`
-note: required because it appears within the type `core::fmt::rt::ArgumentType<'_>`
-  --> $SRC_DIR/core/src/fmt/rt.rs:LL:COL
-note: required because it appears within the type `core::fmt::rt::Argument<'_>`
-  --> $SRC_DIR/core/src/fmt/rt.rs:LL:COL
-   = note: required because it appears within the type `[core::fmt::rt::Argument<'_>]`
-   = note: required for `&[core::fmt::rt::Argument<'_>]` to implement `Send`
-note: required because it appears within the type `Arguments<'_>`
-  --> $SRC_DIR/core/src/fmt/mod.rs:LL:COL
+   = help: the trait `Send` is not implemented for `Arguments<'_>`
 note: required by a bound in `send`
   --> $DIR/send-sync.rs:1:12
    |
 LL | fn send<T: Send>(_: T) {}
    |            ^^^^ required by this bound in `send`
 
-error[E0277]: `core::fmt::rt::Opaque` cannot be shared between threads safely
+error[E0277]: `Arguments<'_>` cannot be shared between threads safely
   --> $DIR/send-sync.rs:9:10
    |
 LL |     sync(format_args!("{:?}", c));
-   |     ---- ^^^^^^^^^^^^^^^^^^^^^^^ `core::fmt::rt::Opaque` cannot be shared between threads safely
+   |     ---- ^^^^^^^^^^^^^^^^^^^^^^^ `Arguments<'_>` cannot be shared between threads safely
    |     |
    |     required by a bound introduced by this call
    |
-   = help: within `Arguments<'_>`, the trait `Sync` is not implemented for `core::fmt::rt::Opaque`, which is required by `Arguments<'_>: Sync`
-   = note: required because it appears within the type `&core::fmt::rt::Opaque`
-note: required because it appears within the type `core::fmt::rt::ArgumentType<'_>`
-  --> $SRC_DIR/core/src/fmt/rt.rs:LL:COL
-note: required because it appears within the type `core::fmt::rt::Argument<'_>`
-  --> $SRC_DIR/core/src/fmt/rt.rs:LL:COL
-   = note: required because it appears within the type `[core::fmt::rt::Argument<'_>]`
-   = note: required because it appears within the type `&[core::fmt::rt::Argument<'_>]`
-note: required because it appears within the type `Arguments<'_>`
-  --> $SRC_DIR/core/src/fmt/mod.rs:LL:COL
+   = help: the trait `Sync` is not implemented for `Arguments<'_>`
 note: required by a bound in `sync`
   --> $DIR/send-sync.rs:2:12
    |