Skip to content

Validate the special layout restriction on DynMetadata #125479

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 1 commit into from
May 24, 2024

Conversation

scottmcm
Copy link
Member

If you look at https://stdrs.dev/nightly/x86_64-unknown-linux-gnu/std/ptr/struct.DynMetadata.html, you'd think that DynMetadata is a struct with fields.

But it's actually not, because the lang item is special-cased in rustc_middle layout:

// Map `Metadata = DynMetadata<dyn Trait>` back to a vtable, since it
// offers better information than `std::ptr::metadata::VTable`,
// and we rely on this layout information to trigger a panic in
// `std::mem::uninitialized::<&dyn Trait>()`, for example.

That explains the very confusing codegen ICEs I was getting in #124251 (comment)

Tried to extract_field 0 from primitive OperandRef(Immediate((ptr: %5 = load ptr, ptr %4, align 8, !nonnull !3, !align !5, !noundef !3)) @ TyAndLayout { ty: DynMetadata, layout: Layout { size: Size(8 bytes), align: AbiAndPrefAlign { abi: Align(8 bytes), pref: Align(8 bytes) }, abi: Scalar(Initialized { value: Pointer(AddressSpace(0)), valid_range: 1..=18446744073709551615 }), fields: Primitive, largest_niche: Some(Niche { offset: Size(0 bytes), value: Pointer(AddressSpace(0)), valid_range: 1..=18446744073709551615 }), variants: Single { index: 0 }, max_repr_align: None, unadjusted_abi_align: Align(8 bytes) } })

because there was a Field projection despite the layout clearly saying it's Primitive.

Thus this PR updates the MIR validator to check for such a projection, and changes libcore to not ever emit any projections into DynMetadata, just to transmute the whole thing when it wants a pointer.

@rustbot
Copy link
Collaborator

rustbot commented May 24, 2024

r? @workingjubilee

rustbot has assigned @workingjubilee.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot 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 May 24, 2024
@workingjubilee
Copy link
Member

Library changes look fine to me but I think I want to ask

r? compiler

@rustbot rustbot assigned lcnr and unassigned workingjubilee May 24, 2024
@Noratrieb
Copy link
Member

@bors r+ rollup

@bors
Copy link
Collaborator

bors commented May 24, 2024

📌 Commit d83f3ca has been approved by Nilstrieb

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 24, 2024
@bors
Copy link
Collaborator

bors commented May 24, 2024

⌛ Testing commit d83f3ca with merge 4649877...

@bors
Copy link
Collaborator

bors commented May 24, 2024

☀️ Test successful - checks-actions
Approved by: Nilstrieb
Pushing 4649877 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label May 24, 2024
@bors bors merged commit 4649877 into rust-lang:master May 24, 2024
7 checks passed
@rustbot rustbot added this to the 1.80.0 milestone May 24, 2024
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (4649877): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

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

Max RSS (memory usage)

Results (secondary -3.4%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-3.4% [-3.4%, -3.4%] 1
All ❌✅ (primary) - - 0

Cycles

Results (primary 2.4%, secondary 4.9%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
2.4% [2.0%, 2.7%] 5
Regressions ❌
(secondary)
4.9% [2.1%, 9.3%] 11
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 2.4% [2.0%, 2.7%] 5

Binary size

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

Bootstrap: 673.551s -> 673.01s (-0.08%)
Artifact size: 315.70 MiB -> 315.73 MiB (0.01%)

@scottmcm scottmcm deleted the validate-vtable-projections branch May 24, 2024 16:40
tgross35 added a commit to tgross35/rust that referenced this pull request Jul 18, 2024
ptr::metadata: avoid references to extern types

References to `extern types` are somewhat dubious entities, since generally we say that references must be dereferenceable for their size as determined via `size_of_val`, but with `extern type` that is an ill-defined statement. I'd like to make Miri warn for such cases since it interacts poorly with Stacked Borrows. To avoid warnings people can't fix, this requires not using references to `extern type` in the standard library, and I think `DynMetadata` is the only currently remaining use. so this changes `DynMetadata` to use a NonNull raw pointer instead. Given that the alignment was 1, this shouldn't really change anything meaningful.

I also updated a comment added by `@scottmcm` in rust-lang#125479, since I think the old comment is wrong. The `DynMetadata` type itself is not special, it is a normal aggregate. But computing field types for wide pointers (including references) is special.
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jul 18, 2024
Rollup merge of rust-lang#127859 - RalfJung:ptr-dyn-metadata, r=scottmcm

ptr::metadata: avoid references to extern types

References to `extern types` are somewhat dubious entities, since generally we say that references must be dereferenceable for their size as determined via `size_of_val`, but with `extern type` that is an ill-defined statement. I'd like to make Miri warn for such cases since it interacts poorly with Stacked Borrows. To avoid warnings people can't fix, this requires not using references to `extern type` in the standard library, and I think `DynMetadata` is the only currently remaining use. so this changes `DynMetadata` to use a NonNull raw pointer instead. Given that the alignment was 1, this shouldn't really change anything meaningful.

I also updated a comment added by `@scottmcm` in rust-lang#125479, since I think the old comment is wrong. The `DynMetadata` type itself is not special, it is a normal aggregate. But computing field types for wide pointers (including references) is special.
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants