fix dynamic size/align computation logic for packed types with dyn trait tail #118538

RalfJung · 2023-12-02T13:17:00Z

This logic was never updated to support packed(N) where N > 1, and it turns out to be wrong for that case.

@bjorn3 I have not looked at cranelift; I assume it basically copied the size-of-val logic and hence could use much the same patch.

rustbot · 2023-12-02T13:17:09Z

r? @petrochenkov

(rustbot has picked a reviewer for you, use r? to override)

rustbot · 2023-12-02T13:17:11Z

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

petrochenkov · 2023-12-04T16:41:13Z

r? compiler
I'm overloaded with reviews.

petrochenkov · 2023-12-04T16:46:17Z

r? compiler
b-naber doesn't do reviews.

bors · 2023-12-13T11:22:51Z

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

WaffleLapkin · 2023-12-13T12:23:14Z

compiler/rustc_codegen_ssa/src/glue.rs

 if let ty::Adt(def, _) = t.kind() {
 if def.repr().packed() {
+ let unsized_tail =
+ bx.tcx().struct_tail_with_normalize(field_ty, |ty| ty, || {});
+ assert!(matches!(unsized_tail.kind(), ty::Slice(..) | ty::Str));


I think this is not right. Won't this ICE for code like this:

#[repr(C, packed)] struct Packed<T: ?Sized>(u8, core::mem::ManuallyDrop<T>); let p = Packed(0, core::mem::ManuallyDrop::new(1)); let p: &Packed<usize> = &p; let p: &Packed<dyn Send> = p; dbg!(core::mem::size_of_val(p), core::mem::align_of_val(p));

?

Hm, if such code is accepted then it would currently just compute the wrong size for that code. An ICE would still be an improvement. ;)

I'll play around with examples that involve ManuallyDrop to see if there's a bug here, thanks!

There is no justification for why packed types ignore the alignment of their fields. This is correct only because it is impossible to write a packed struct with dyn Trait tail, i.e., unsized packed structs always have statically known alignment. (With slices, the size is dynamic but the alignment is static; only with dyn Trait do we have both dynamic size and alignment.)

I don't think that's correct, given the example above? And either way, packed structs always ignore the alignment of their fields, don't they? I don't think Packed<dyn Send>'s size (or alignment) depends on the alignment of .0.

packed structs always ignore the alignment of their fields, don't they?

No they don't. Remember we have #[packed(N)]. The alignment of the fields in that struct is min(natural_field_align, N). For N > 1, this does depend on the alignment of the field type. And if that's a dyn type then it needs to be taken into account to compute the offset.

Right, the alignment should only be ignored if N = 1. The bug is trivial to trigger then, yikes: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=80f13045af7f1dd52cf6c79ee2897829 (and obviously it's easy to get unsound by boxing the value)

And this was a problem since at least stabilization of packed(N) in 1.33: https://godbolt.org/z/37s44fEKe. I assume when we implemented packed(N) we forgot to fixup size/align computation logic 💀

Yes indeed.

We also forgot to fixup the offset computation logic, I fixed that recently in #118540. I thought size computation wouldn't be affected since I was unable to construct a packed struct with a dyn-typed tail, but you managed to bring in that missing piece.

I was also made aware of #80925, did #118540 fix that one? (I can't reproduce miri failure anymore)

Yeah for offsets, #118540 removed all special cases for packed types, so they are now handled properly.

I updated the PR to fix this, and added tests.

rustbot · 2023-12-13T18:53:23Z

The Miri subtree was changed

cc @rust-lang/miri

…tails

WaffleLapkin

Thanks!

WaffleLapkin · 2023-12-13T22:40:12Z

@bors r

bors · 2023-12-13T22:40:14Z

📌 Commit 7e4c427 has been approved by WaffleLapkin

It is now in the queue for this repository.

WaffleLapkin · 2023-12-13T22:44:20Z

Marked as relnotes Marks issues that should be documented in the release notes of the next release. , since this fixes an unsound bug in the backend (and miri).

TL;DR: we were not computing the size and alignment of packed structs incorrectly, which lead to unsoundness for Packed<dyn Trait> where Packed is a struct with #[repr(packed(N)] and N > 1. This bug dates back to 1.33, where we stabilized packed(N), without updating layout computations in the backend.

…affleLapkin fix dynamic size/align computation logic for packed types with dyn trait tail This logic was never updated to support `packed(N)` where `N > 1`, and it turns out to be wrong for that case. Fixes rust-lang#80925 `@bjorn3` I have not looked at cranelift; I assume it basically copied the size-of-val logic and hence could use much the same patch.

…iaskrgr Rollup of 3 pull requests Successful merges: - rust-lang#118375 (Add -Zunpretty=stable-mir output test) - rust-lang#118538 (fix dynamic size/align computation logic for packed types with dyn trait tail) - rust-lang#118789 (fix --dry-run when the change-id warning is printed) r? `@ghost` `@rustbot` modify labels: rollup

bors · 2023-12-14T06:26:15Z

⌛ Testing commit 7e4c427 with merge b6f6800...

…fleLapkin fix dynamic size/align computation logic for packed types with dyn trait tail This logic was never updated to support `packed(N)` where `N > 1`, and it turns out to be wrong for that case. Fixes rust-lang#80925 `@bjorn3` I have not looked at cranelift; I assume it basically copied the size-of-val logic and hence could use much the same patch.

rust-log-analyzer · 2023-12-14T06:55:17Z

The job armhf-gnu failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

bors · 2023-12-14T06:55:49Z

💔 Test failed - checks-actions

RalfJung · 2023-12-14T07:07:28Z

@bors retry armhf-gnu: client.read_exact(&mut header) failed with Connection reset by peer (os error 104)

bors · 2023-12-14T07:19:10Z

⌛ Testing commit 7e4c427 with merge 1a8afa0...

bors · 2023-12-14T09:15:07Z

☀️ Test successful - checks-actions
Approved by: WaffleLapkin
Pushing 1a8afa0 to master...

rust-timer · 2023-12-14T10:37:29Z

Finished benchmarking commit (1a8afa0): 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

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.5%	[0.5%, 0.5%]	1
Regressions ❌ (secondary)	2.2%	[2.2%, 2.2%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-4.2%	[-4.2%, -4.2%]	1
All ❌✅ (primary)	0.5%	[0.5%, 0.5%]	1

Cycles

Results

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.4%	[0.4%, 0.4%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.4%	[-0.4%, -0.4%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.0%	[-0.4%, 0.4%]	2

Binary size

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

Bootstrap: 670.811s -> 670.867s (0.01%)
Artifact size: 312.42 MiB -> 312.41 MiB (-0.00%)

…ait tail cc rust-lang/rust#118538

Pkgsrc changes: * Adapt checksums and patches. Upstream chnages: Version 1.76.0 (2024-02-08) ========================== Language -------- - [Document Rust ABI compatibility between various types] (rust-lang/rust#115476) - [Also: guarantee that char and u32 are ABI-compatible] (rust-lang/rust#118032) - [Warn against ambiguous wide pointer comparisons] (rust-lang/rust#117758) Compiler -------- - [Lint pinned `#[must_use]` pointers (in particular, `Box<T>` where `T` is `#[must_use]`) in `unused_must_use`.] (rust-lang/rust#118054) - [Soundness fix: fix computing the offset of an unsized field in a packed struct] (rust-lang/rust#118540) - [Soundness fix: fix dynamic size/align computation logic for packed types with dyn Trait tail] (rust-lang/rust#118538) - [Add `$message_type` field to distinguish json diagnostic outputs] (rust-lang/rust#115691) - [Enable Rust to use the EHCont security feature of Windows] (rust-lang/rust#118013) - [Add tier 3 {x86_64,i686}-win7-windows-msvc targets] (rust-lang/rust#118150) - [Add tier 3 aarch64-apple-watchos target] (rust-lang/rust#119074) - [Add tier 3 arm64e-apple-ios & arm64e-apple-darwin targets] (rust-lang/rust#115526) Refer to Rust's [platform support page][platform-support-doc] for more information on Rust's tiered platform support. Libraries --------- - [Add a column number to `dbg!()`] (rust-lang/rust#114962) - [Add `std::hash::{DefaultHasher, RandomState}` exports] (rust-lang/rust#115694) - [Fix rounding issue with exponents in fmt] (rust-lang/rust#116301) - [Add T: ?Sized to `RwLockReadGuard` and `RwLockWriteGuard`'s Debug impls.] (rust-lang/rust#117138) - [Windows: Allow `File::create` to work on hidden files] (rust-lang/rust#116438) Stabilized APIs --------------- - [`Arc::unwrap_or_clone`] (https://doc.rust-lang.org/stable/std/sync/struct.Arc.html#method.unwrap_or_clone) - [`Rc::unwrap_or_clone`] (https://doc.rust-lang.org/stable/std/rc/struct.Rc.html#method.unwrap_or_clone) - [`Result::inspect`] (https://doc.rust-lang.org/stable/std/result/enum.Result.html#method.inspect) - [`Result::inspect_err`] (https://doc.rust-lang.org/stable/std/result/enum.Result.html#method.inspect_err) - [`Option::inspect`] (https://doc.rust-lang.org/stable/std/option/enum.Option.html#method.inspect) - [`type_name_of_val`] (https://doc.rust-lang.org/stable/std/any/fn.type_name_of_val.html) - [`std::hash::{DefaultHasher, RandomState}`] (https://doc.rust-lang.org/stable/std/hash/index.html#structs) These were previously available only through `std::collections::hash_map`. - [`ptr::{from_ref, from_mut}`] (https://doc.rust-lang.org/stable/std/ptr/fn.from_ref.html) - [`ptr::addr_eq`](https://doc.rust-lang.org/stable/std/ptr/fn.addr_eq.html) Cargo ----- See [Cargo release notes] (https://github.com/rust-lang/cargo/blob/master/CHANGELOG.md#cargo-176-2024-02-08). Rustdoc ------- - [Don't merge cfg and doc(cfg) attributes for re-exports] (rust-lang/rust#113091) - [rustdoc: allow resizing the sidebar / hiding the top bar] (rust-lang/rust#115660) - [rustdoc-search: add support for traits and associated types] (rust-lang/rust#116085) - [rustdoc: Add highlighting for comments in items declaration] (rust-lang/rust#117869) Compatibility Notes ------------------- - [Add allow-by-default lint for unit bindings] (rust-lang/rust#112380) This is expected to be upgraded to a warning by default in a future Rust release. Some macros emit bindings with type `()` with user-provided spans, which means that this lint will warn for user code. - [Remove x86_64-sun-solaris target.] (rust-lang/rust#118091) - [Remove asmjs-unknown-emscripten target] (rust-lang/rust#117338) - [Report errors in jobserver inherited through environment variables] (rust-lang/rust#113730) This [may warn](rust-lang/rust#120515) on benign problems too. - [Update the minimum external LLVM to 16.] (rust-lang/rust#117947) - [Improve `print_tts`](rust-lang/rust#114571) This change can break some naive manual parsing of token trees in proc macro code which expect a particular structure after `.to_string()`, rather than just arbitrary Rust code. - [Make `IMPLIED_BOUNDS_ENTAILMENT` into a hard error from a lint] (rust-lang/rust#117984) - [Vec's allocation behavior was changed when collecting some iterators] (rust-lang/rust#110353) Allocation behavior is currently not specified, nevertheless changes can be surprising. See [`impl FromIterator for Vec`] (https://doc.rust-lang.org/nightly/std/vec/struct.Vec.html#impl-FromIterator-for-Vec) for more details. - [Properly reject `default` on free const items] (rust-lang/rust#117818)

rustbot assigned petrochenkov Dec 2, 2023

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. labels Dec 2, 2023

rustbot assigned b-naber and unassigned petrochenkov Dec 4, 2023

rustbot assigned WaffleLapkin and unassigned b-naber Dec 4, 2023

RalfJung force-pushed the size-of-val-comments branch 2 times, most recently from c51dd07 to 0a2ac74 Compare December 4, 2023 18:16

WaffleLapkin reviewed Dec 13, 2023

View reviewed changes

RalfJung force-pushed the size-of-val-comments branch from 0a2ac74 to fe45ba6 Compare December 13, 2023 16:45

RalfJung changed the title ~~fix comments in dynamic size/align computation logic~~ fix dynamic size/align computation logic for packed types with dyn trait tail Dec 13, 2023

This comment has been minimized.

Sign in to view

RalfJung mentioned this pull request Dec 13, 2023

Accessing DST field of packed struct calculates the wrong field address #80925

Closed

RalfJung force-pushed the size-of-val-comments branch 2 times, most recently from 46d86aa to 7e4c427 Compare December 13, 2023 19:21

fix computing the dynamic alignment of packed structs with dyn trait …

7e4c427

…tails

WaffleLapkin approved these changes Dec 13, 2023

View reviewed changes

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 Dec 13, 2023

WaffleLapkin added the relnotes Marks issues that should be documented in the release notes of the next release. label Dec 13, 2023

matthiaskrgr mentioned this pull request Dec 14, 2023

Rollup of 3 pull requests #118932

Closed

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Dec 14, 2023

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 Dec 14, 2023

bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 14, 2023

bors merged commit 1a8afa0 into rust-lang:master Dec 14, 2023
12 checks passed

rustbot added this to the 1.76.0 milestone Dec 14, 2023

RalfJung mentioned this pull request Dec 14, 2023

codegen, miri: fix computing the offset of an unsized field in a packed struct #118540

Merged

RalfJung deleted the size-of-val-comments branch December 16, 2023 09:31

bjorn3 added a commit to rust-lang/rustc_codegen_cranelift that referenced this pull request Dec 18, 2023

fix dynamic size/align computation logic for packed types with dyn tr…

1ab05b6

…ait tail cc rust-lang/rust#118538

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix dynamic size/align computation logic for packed types with dyn trait tail #118538

fix dynamic size/align computation logic for packed types with dyn trait tail #118538

RalfJung commented Dec 2, 2023 •

edited

Loading

rustbot commented Dec 2, 2023

rustbot commented Dec 2, 2023

petrochenkov commented Dec 4, 2023

petrochenkov commented Dec 4, 2023

bors commented Dec 13, 2023

WaffleLapkin Dec 13, 2023

RalfJung Dec 13, 2023 •

edited

Loading

WaffleLapkin Dec 13, 2023

RalfJung Dec 13, 2023 •

edited

Loading

WaffleLapkin Dec 13, 2023 •

edited

Loading

WaffleLapkin Dec 13, 2023

RalfJung Dec 13, 2023

WaffleLapkin Dec 13, 2023

RalfJung Dec 13, 2023

RalfJung Dec 13, 2023

rustbot commented Dec 13, 2023

This comment has been minimized.

WaffleLapkin left a comment

WaffleLapkin commented Dec 13, 2023

bors commented Dec 13, 2023

WaffleLapkin commented Dec 13, 2023 •

edited

Loading

bors commented Dec 14, 2023

rust-log-analyzer commented Dec 14, 2023

bors commented Dec 14, 2023

RalfJung commented Dec 14, 2023

bors commented Dec 14, 2023

bors commented Dec 14, 2023

rust-timer commented Dec 14, 2023

fix dynamic size/align computation logic for packed types with dyn trait tail #118538

fix dynamic size/align computation logic for packed types with dyn trait tail #118538

Conversation

RalfJung commented Dec 2, 2023 • edited Loading

rustbot commented Dec 2, 2023

rustbot commented Dec 2, 2023

petrochenkov commented Dec 4, 2023

petrochenkov commented Dec 4, 2023

bors commented Dec 13, 2023

Choose a reason for hiding this comment

RalfJung Dec 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung Dec 13, 2023 • edited Loading

Choose a reason for hiding this comment

WaffleLapkin Dec 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rustbot commented Dec 13, 2023

This comment has been minimized.

WaffleLapkin left a comment

Choose a reason for hiding this comment

WaffleLapkin commented Dec 13, 2023

bors commented Dec 13, 2023

WaffleLapkin commented Dec 13, 2023 • edited Loading

bors commented Dec 14, 2023

rust-log-analyzer commented Dec 14, 2023

bors commented Dec 14, 2023

RalfJung commented Dec 14, 2023

bors commented Dec 14, 2023

bors commented Dec 14, 2023

rust-timer commented Dec 14, 2023

Overall result: no relevant changes - no action needed

RalfJung commented Dec 2, 2023 •

edited

Loading

RalfJung Dec 13, 2023 •

edited

Loading

RalfJung Dec 13, 2023 •

edited

Loading

WaffleLapkin Dec 13, 2023 •

edited

Loading

WaffleLapkin commented Dec 13, 2023 •

edited

Loading