make vtable pointers entirely opaque #99420

RalfJung · 2022-07-18T14:22:19Z

This implements the scheme discussed in rust-lang/unsafe-code-guidelines#338: vtable pointers should be considered entirely opaque and not even readable by Rust code, similar to function pointers.

We have a new kind of GlobalAlloc that symbolically refers to a vtable.
Miri uses that kind of allocation when generating a vtable.
The codegen backends, upon encountering such an allocation, call vtable_allocation to obtain an actually dataful allocation for this vtable.
We need new intrinsics to obtain the size and align from a vtable (for some ptr::metadata APIs), since direct accesses are UB now.

I had to touch quite a bit of code that I am not very familiar with, so some of this might not make much sense...
r? @oli-obk

rustbot · 2022-07-18T14:22:23Z

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

Stabilizing library features
Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
Changing public documentation in ways that create new stability guarantees
Changing observable runtime behavior of library APIs

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

RalfJung · 2022-07-18T14:23:11Z

compiler/rustc_codegen_llvm/src/common.rs

@@ -240,6 +240,13 @@ impl<'ll, 'tcx> ConstMethods<'tcx> for CodegenCx<'ll, 'tcx> {
            }
            Scalar::Ptr(ptr, _size) => {
                let (alloc_id, offset) = ptr.into_parts();
+                // For vtables, get the underlying data allocation.
+                let alloc_id = match self.tcx.global_alloc(alloc_id) {


I guess the alternative would be to move the body of the GlobalAlloc::Memory match arm below into a new function, and also call that from the GlobalAlloc::Vtable arm. Not sure which is better.

yea, I was going to ask about that when I read the 3 copies of this snippet in the various codegen backends. Another alternative is to add a codegen_alloc function on TyCtxt that does what this snippet here does. I think I may actually prefer that instead of splitting the logic out into a function

Hm, I think a function to codegen a &Allocation would be nicer. It would make sense to have a "follow the symbolic allocation" operation (which follows statics and vtables), but for statics we don't want to follow them in codegen, so that is just the wrong operation. Following only vtables seems rather ad-hoc.

In fact can we make the vtable_allocation provider return an &Allocation rather than an AllocId? And maybe not even have an AllocId for the "materialized" vtable? That would be best IMO.

compiler/rustc_codegen_llvm/src/intrinsic.rs

compiler/rustc_const_eval/src/interpret/cast.rs

compiler/rustc_const_eval/src/interpret/terminator.rs

compiler/rustc_const_eval/src/interpret/traits.rs

compiler/rustc_middle/src/mir/interpret/mod.rs

compiler/rustc_monomorphize/src/collector.rs

oli-obk

Preliminary review, will dig into it more later this week

compiler/rustc_const_eval/src/interpret/memory.rs

compiler/rustc_const_eval/src/interpret/traits.rs

library/core/src/ptr/metadata.rs

library/core/src/intrinsics.rs

oli-obk · 2022-07-18T17:04:25Z

@bors try @rust-timer queue

rust-timer · 2022-07-18T17:04:27Z

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

bors · 2022-07-18T17:04:34Z

⌛ Trying commit 88c84b2 with merge 02e4d4082990eb37f535b13a7373b6ba6d7f8250...

bors · 2022-07-18T18:55:03Z

☀️ Try build successful - checks-actions
Build commit: 02e4d4082990eb37f535b13a7373b6ba6d7f8250 (02e4d4082990eb37f535b13a7373b6ba6d7f8250)

rust-timer · 2022-07-18T18:55:05Z

Queued 02e4d4082990eb37f535b13a7373b6ba6d7f8250 with parent 144227d, future comparison URL.

rust-timer · 2022-07-18T20:11:52Z

Finished benchmarking commit (02e4d4082990eb37f535b13a7373b6ba6d7f8250): comparison url.

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

Primary benchmarks: no relevant changes found
Secondary benchmarks: mixed results

	mean¹	max	count²
Regressions 😿 (primary)	N/A	N/A	0
Regressions 😿 (secondary)	2.3%	2.5%	4
Improvements 🎉 (primary)	N/A	N/A	0
Improvements 🎉 (secondary)	-2.3%	-3.1%	3
All 😿🎉 (primary)	N/A	N/A	0

Cycles

Results

Primary benchmarks: 🎉 relevant improvement found
Secondary benchmarks: 😿 relevant regression found

	mean¹	max	count²
Regressions 😿 (primary)	N/A	N/A	0
Regressions 😿 (secondary)	4.1%	4.1%	1
Improvements 🎉 (primary)	-2.8%	-2.8%	1
Improvements 🎉 (secondary)	N/A	N/A	0
All 😿🎉 (primary)	-2.8%	-2.8%	1

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf -perf-regression

the arithmetic mean of the percent change ↩ ↩²
number of relevant changes ↩ ↩²

rustbot · 2022-07-20T00:19:28Z

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

compiler/rustc_codegen_llvm/src/intrinsic.rs

oli-obk

r=me with outstanding comments resolved

compiler/rustc_middle/src/mir/interpret/mod.rs

library/core/src/ptr/metadata.rs

bors · 2022-07-21T14:25:06Z

☀️ Try build successful - checks-actions
Build commit: 1e72cb883abe838e32fdd99537aeeda0fe0723b9 (1e72cb883abe838e32fdd99537aeeda0fe0723b9)

RalfJung · 2022-07-22T00:15:57Z

When I download that toolchain, and check out commit 3de6c04269a7d315f7e9864b9013451cd9580a08 of xsv (the thing that failed), everything passes. I also ran a local build of this branch, and ran ./x.py test src/tools/cargotest --stage 0. I cannot reproduce the problem.

So maybe it is random, after all?
@bors r=oli-obk

bors · 2022-07-22T00:15:59Z

📌 Commit d46dfa2 has been approved by oli-obk

It is now in the queue for this repository.

bors · 2022-07-22T01:33:51Z

⌛ Testing commit d46dfa2 with merge aa01891...

bors · 2022-07-22T04:01:19Z

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing aa01891 to master...

rust-highfive · 2022-07-22T04:01:42Z

📣 Toolstate changed by #99420!

Tested on commit aa01891.
Direct link to PR: #99420

💔 miri on windows: test-pass → build-fail (cc @oli-obk @RalfJung).
💔 miri on linux: test-pass → build-fail (cc @oli-obk @RalfJung).

@oli-obk

Tested on commit rust-lang/rust@aa01891. Direct link to PR: <rust-lang/rust#99420> 💔 miri on windows: test-pass → build-fail (cc @oli-obk @RalfJung). 💔 miri on linux: test-pass → build-fail (cc @oli-obk @RalfJung).

rust-timer · 2022-07-22T05:18:38Z

Finished benchmarking commit (aa01891): comparison url.

Instruction count

Primary benchmarks: no relevant changes found
Secondary benchmarks: 😿 relevant regressions found

	mean¹	max	count²
Regressions 😿 (primary)	N/A	N/A	0
Regressions 😿 (secondary)	0.4%	0.5%	2
Improvements 🎉 (primary)	N/A	N/A	0
Improvements 🎉 (secondary)	N/A	N/A	0
All 😿🎉 (primary)	N/A	N/A	0

Max RSS (memory usage)

Results

Primary benchmarks: 😿 relevant regression found
Secondary benchmarks: 😿 relevant regressions found

	mean¹	max	count²
Regressions 😿 (primary)	2.8%	2.8%	1
Regressions 😿 (secondary)	2.5%	2.6%	2
Improvements 🎉 (primary)	N/A	N/A	0
Improvements 🎉 (secondary)	N/A	N/A	0
All 😿🎉 (primary)	2.8%	2.8%	1

Cycles

Results

Primary benchmarks: mixed results
Secondary benchmarks: 😿 relevant regression found

	mean¹	max	count²
Regressions 😿 (primary)	3.5%	3.5%	1
Regressions 😿 (secondary)	2.7%	2.7%	1
Improvements 🎉 (primary)	-2.3%	-2.3%	1
Improvements 🎉 (secondary)	N/A	N/A	0
All 😿🎉 (primary)	0.6%	3.5%	2

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

the arithmetic mean of the percent change ↩ ↩² ↩³
number of relevant changes ↩ ↩² ↩³

adjust for symbolic vtables The Miri side of rust-lang/rust#99420

Regression is still failing. Related changes: - rust-lang/rust#99420 - rust-lang/rust#99495 - rust-lang/rust#99844 - rust-lang/rust#99058

* Fix compilation errors Regression is still failing. Related changes: - rust-lang/rust#99420 - rust-lang/rust#99495 - rust-lang/rust#99844 - rust-lang/rust#99058 * Change test to expect compilation failure The compiler has reverted their fix to Opaque types due to performance degradation. * Fix VTable handling now that it has an Opaque type - Add an implementation for vtable_size and vtable_align intrinsics. - Change how we handled Foreign types. Even though they are unsized, a pointer to foreign types is a thin pointer. Co-authored-by: Daniel Schwartz-Narbonne <danielsn@users.noreply.github.com>

…enkov make NOP dyn casts not require anything about the vtable As suggested [on Zulip](https://rust-lang.zulipchat.com/#narrow/stream/144729-t-types/topic/dyn-upcasting.20stabilization/near/292151439). This matches what the codegen backends already do, and what Miri did do until rust-lang#99420 when I made it super extra paranoid.

make NOP dyn casts not require anything about the vtable As suggested [on Zulip](https://rust-lang.zulipchat.com/#narrow/stream/144729-t-types/topic/dyn-upcasting.20stabilization/near/292151439). This matches what the codegen backends already do, and what Miri did do until rust-lang/rust#99420 when I made it super extra paranoid.

make vtable pointers entirely opaque This implements the scheme discussed in rust-lang/unsafe-code-guidelines#338: vtable pointers should be considered entirely opaque and not even readable by Rust code, similar to function pointers. - We have a new kind of `GlobalAlloc` that symbolically refers to a vtable. - Miri uses that kind of allocation when generating a vtable. - The codegen backends, upon encountering such an allocation, call `vtable_allocation` to obtain an actually dataful allocation for this vtable. - We need new intrinsics to obtain the size and align from a vtable (for some `ptr::metadata` APIs), since direct accesses are UB now. I had to touch quite a bit of code that I am not very familiar with, so some of this might not make much sense... r? `@oli-obk`

rust-highfive assigned oli-obk Jul 18, 2022

rustbot added 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 Jul 18, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 18, 2022