Add an intrinsic for `ptr::metadata` #124251

scottmcm · 2024-04-22T08:20:46Z

The follow-up to #123840, so we can remove PtrComponents and PtrRepr from libcore entirely (well, after a bootstrap update).

As discussed in https://rust-lang.zulipchat.com/#narrow/stream/189540-t-compiler.2Fwg-mir-opt/topic/.60ptr_metadata.60.20in.20MIR/near/435637808, this introduces UnOp::PtrMetadata taking a raw pointer and returning the associated metadata value.

By no longer going through a union, this should also help future PRs better optimize pointer operations.

r? @oli-obk

rustbot · 2024-04-22T08:20:56Z

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

This PR changes MIR

cc @oli-obk, @RalfJung, @JakobDegen, @davidtwco, @celinval, @vakaras

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

This PR changes Stable MIR

cc @oli-obk, @celinval, @ouz-a

scottmcm · 2024-04-22T08:40:51Z

tests/codegen/intrinsics/ptr_metadata.rs

+    // CHECK: %[[Q:.+]] = getelementptr inbounds i8, ptr %p.0, i64 %n
+    // CHECK: %[[TEMP1:.+]] = insertvalue { ptr, ptr } poison, ptr %[[Q]], 0
+    // CHECK: %[[TEMP2:.+]] = insertvalue { ptr, ptr } %[[TEMP1]], ptr %p.1, 1
+    // CHECK: ret { ptr, ptr } %[[TEMP2]]


This is a -C no-prepopulate-passes test, so this is actually what we output directly, no optimization needed.

Compare to what you get on nightly 2024-04-20 https://rust.godbolt.org/z/6zP58xoT4

define { ptr, ptr } @dyn_byte_offset(ptr noundef %p.0, ptr noalias noundef readonly align 8 dereferenceable(24) %p.1, i64 noundef %n) unnamed_addr #0 { %_9 = alloca %"core::ptr::metadata::PtrComponents<dyn core::fmt::Debug>", align 8 %_8 = alloca %"core::ptr::metadata::PtrRepr<dyn core::fmt::Debug>", align 8 %_7 = alloca %"core::ptr::metadata::PtrRepr<dyn core::fmt::Debug>", align 8 %_3 = getelementptr inbounds i8, ptr %p.0, i64 %n call void @llvm.lifetime.start.p0(i64 16, ptr %_7) store ptr %p.0, ptr %_7, align 8 %0 = getelementptr inbounds i8, ptr %_7, i64 8 store ptr %p.1, ptr %0, align 8 %1 = getelementptr inbounds i8, ptr %_7, i64 8 %_6 = load ptr, ptr %1, align 8, !nonnull !3, !align !4, !noundef !3 call void @llvm.lifetime.end.p0(i64 16, ptr %_7) call void @llvm.lifetime.start.p0(i64 16, ptr %_8) call void @llvm.lifetime.start.p0(i64 16, ptr %_9) store ptr %_3, ptr %_9, align 8 %2 = getelementptr inbounds i8, ptr %_9, i64 8 store ptr %_6, ptr %2, align 8 %3 = load ptr, ptr %_9, align 8, !noundef !3 %4 = getelementptr inbounds i8, ptr %_9, i64 8 %5 = load ptr, ptr %4, align 8, !nonnull !3, !align !4, !noundef !3 store ptr %3, ptr %_8, align 8 %6 = getelementptr inbounds i8, ptr %_8, i64 8 store ptr %5, ptr %6, align 8 call void @llvm.lifetime.end.p0(i64 16, ptr %_9) %_0.0 = load ptr, ptr %_8, align 8, !noundef !3 %7 = getelementptr inbounds i8, ptr %_8, i64 8 %_0.1 = load ptr, ptr %7, align 8, !nonnull !3, !align !5, !noundef !3 call void @llvm.lifetime.end.p0(i64 16, ptr %_8) %8 = insertvalue { ptr, ptr } poison, ptr %_0.0, 0 %9 = insertvalue { ptr, ptr } %8, ptr %_0.1, 1 ret { ptr, ptr } %9 }

rustbot · 2024-04-22T08:44:19Z

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

oli-obk · 2024-04-22T11:11:14Z

compiler/rustc_const_eval/src/interpret/cast.rs

+                ImmTy::uninit(cast_to)
+            }
+            Immediate::ScalarPair(_, meta) => ImmTy::from_scalar(meta, cast_to),
+            Immediate::Uninit => throw_ub!(InvalidUninitBytes(None)),


Unsure whether we want to raise UB here. We don't raise UB for offsetting an immediate to get a sub-immeidate

either way, needs const-eval and miri tests

We generally raise UB for by-val operations on uninit immediates. This is a cast, not a projection, so I'd say it's a by-val operation and as such should be UB when the input is uninit.

Hmm, what about for reading the metadata of an uninit thin pointer? Should that be UB because of an uninit pointer, or defined because the metadata of a thin pointer is trivially () without even looking at the input?

This would affect whether removing the unit StatementKind::Assign could hide UB...

An uninit thin pointer violates the thin pointer validity invariant. I don't think we have any case here that we want to allow -- all our pointers are noundef on both the data and metadata component.

oli-obk · 2024-04-22T11:15:39Z

compiler/rustc_middle/src/mir/syntax.rs

+    /// Convert a raw pointer to an `impl Pointee<Metadata = M>` into `M`.
+    ///
+    /// For example, this will give a `()` from `*const i32`, a `usize` from
+    /// `*mut [u8]`, or a vtable from a `*const dyn Foo`.


Suggested change

/// `*mut [u8]`, or a vtable from a `*const dyn Foo`.

/// `*mut [u8]`, or a pointer to a vtable from a `*const dyn Foo`.

the8472 · 2024-04-22T18:37:19Z

Wouldn't adding support for the metadata to offset_of! be a more general solution? It'd allow both read and write and also let users build their own transmutable PtrComponents as long as they check the offsets.

saethlin · 2024-04-22T19:39:17Z

I'm not sure how that would fix the size of the MIR.

the8472 · 2024-04-22T20:57:41Z

Oh I thought it was just about removing layout assumptions from the library.

compiler/rustc_middle/src/mir/syntax.rs

RalfJung · 2024-04-23T11:40:42Z

compiler/rustc_middle/src/mir/syntax.rs

+    /// Convert a raw pointer to an `impl Pointee<Metadata = M>` into `M`.
+    ///
+    /// For example, this will give a `()` from `*const i32`, a `usize` from
+    /// `*mut [u8]`, or a vtable from a `*const dyn Foo`.


What will the type of the vtable be?

compiler/rustc_middle/src/mir/syntax.rs

compiler/rustc_const_eval/src/interpret/cast.rs

RalfJung · 2024-04-23T11:45:18Z

compiler/rustc_const_eval/src/interpret/cast.rs

@@ -64,6 +64,12 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
                self.write_immediate(*res, dest)?;
            }

+            CastKind::PtrToMetadata => {


Is this covered by Miri tests? Not sure how well we are covering the ptr::metadata API surface.

scottmcm · 2024-04-23T22:49:40Z

compiler/rustc_codegen_cranelift/src/base.rs

+                Rvalue::Cast(CastKind::PtrToMetadata, ref operand, _to_ty) => {
+                    let operand = codegen_operand(fx, operand);
+                    let meta = match operand.layout().abi {
+                        Abi::Scalar(_) => CValue::zst(dest_layout),
+                        Abi::ScalarPair(_, _) => {
+                            CValue::by_val(operand.load_scalar_pair(fx).1, dest_layout)
+                        }
+                        _ => bug!("Unexpected `PtrToMetadata` operand: {operand:?}"),
+                    };
+                    lval.write_cvalue(fx, meta);
+                }


@bjorn3 Since I broke stuff in my last PR, please take a look at this one and let me know whether this is the right approach for getting metadata in cg_clif.

This looks correct to me.

bors · 2024-05-02T12:13:12Z

☔ The latest upstream changes (presumably #124521) made this pull request unmergeable. Please resolve the merge conflicts.

oli-obk · 2024-05-28T09:52:00Z

@bors r+

bors · 2024-05-28T09:52:03Z

📌 Commit 713ddc2 has been approved by oli-obk

It is now in the queue for this repository.

Add an intrinsic for `ptr::metadata` The follow-up to rust-lang#123840, so we can remove `PtrComponents` and `PtrRepr` from libcore entirely (well, after a bootstrap update). As discussed in <https://rust-lang.zulipchat.com/#narrow/stream/189540-t-compiler.2Fwg-mir-opt/topic/.60ptr_metadata.60.20in.20MIR/near/435637808>, this introduces `UnOp::PtrMetadata` taking a raw pointer and returning the associated metadata value. By no longer going through a `union`, this should also help future PRs better optimize pointer operations. r? `@oli-obk`

…iaskrgr Rollup of 6 pull requests Successful merges: - rust-lang#117671 (NVPTX: Avoid PassMode::Direct for args in C abi) - rust-lang#124251 (Add an intrinsic for `ptr::metadata`) - rust-lang#125573 (Migrate `run-make/allow-warnings-cmdline-stability` to `rmake.rs`) - rust-lang#125590 (Add a "Setup Python" action for github-hosted runners and remove unnecessary `CUSTOM_MINGW` environment variable) - rust-lang#125598 (Make `ProofTreeBuilder` actually generic over `Interner`) - rust-lang#125637 (rustfmt fixes) r? `@ghost` `@rustbot` modify labels: rollup

matthiaskrgr · 2024-05-28T16:04:06Z

@bors r-
#125663 (comment)

scottmcm · 2024-05-28T16:29:54Z

Oh, ./x fmt doesn't fix cg_cranelift. Fixed. (I'll wait for ci before r=ing.)

scottmcm · 2024-05-28T17:40:18Z

@bors r=oli-obk

bors · 2024-05-28T17:40:20Z

📌 Commit 57948c8 has been approved by oli-obk

It is now in the queue for this repository.

Add an intrinsic for `ptr::metadata` The follow-up to rust-lang#123840, so we can remove `PtrComponents` and `PtrRepr` from libcore entirely (well, after a bootstrap update). As discussed in <https://rust-lang.zulipchat.com/#narrow/stream/189540-t-compiler.2Fwg-mir-opt/topic/.60ptr_metadata.60.20in.20MIR/near/435637808>, this introduces `UnOp::PtrMetadata` taking a raw pointer and returning the associated metadata value. By no longer going through a `union`, this should also help future PRs better optimize pointer operations. r? `@oli-obk`

Rollup of 8 pull requests Successful merges: - rust-lang#124251 (Add an intrinsic for `ptr::metadata`) - rust-lang#124320 (Add `--print=check-cfg` to get the expected configs) - rust-lang#125226 (Make more of the test suite run on Mac Catalyst) - rust-lang#125381 (Silence some resolve errors when there have been glob import errors) - rust-lang#125633 (miri: avoid making a full copy of all new allocations) - rust-lang#125638 (Rewrite `lto-smoke`, `simple-rlib` and `mixing-deps` `run-make` tests in `rmake.rs` format) - rust-lang#125639 (Support `./x doc run-make-support --open`) - rust-lang#125664 (Tweak relations to no longer rely on `TypeTrace`) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#124251 - scottmcm:unop-ptr-metadata, r=oli-obk Add an intrinsic for `ptr::metadata` The follow-up to rust-lang#123840, so we can remove `PtrComponents` and `PtrRepr` from libcore entirely (well, after a bootstrap update). As discussed in <https://rust-lang.zulipchat.com/#narrow/stream/189540-t-compiler.2Fwg-mir-opt/topic/.60ptr_metadata.60.20in.20MIR/near/435637808>, this introduces `UnOp::PtrMetadata` taking a raw pointer and returning the associated metadata value. By no longer going through a `union`, this should also help future PRs better optimize pointer operations. r? ``@oli-obk``

rustbot assigned oli-obk Apr 22, 2024

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Apr 22, 2024

This comment has been minimized.

Sign in to view

scottmcm force-pushed the unop-ptr-metadata branch from f670fb1 to 809cbb1 Compare April 22, 2024 08:27

This comment has been minimized.

Sign in to view

scottmcm commented Apr 22, 2024

View reviewed changes

oli-obk reviewed Apr 22, 2024

View reviewed changes