Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Fix -Z print-type-sizes for generators with discriminant field ordered first #105623

Merged
merged 2 commits into from
Dec 15, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 23 additions & 7 deletions compiler/rustc_ty_utils/src/layout.rs
Original file line number Diff line number Diff line change
Expand Up @@ -919,7 +919,7 @@ fn variant_info_for_generator<'tcx>(
def_id: DefId,
substs: ty::SubstsRef<'tcx>,
) -> (Vec<VariantInfo>, Option<Size>) {
let Variants::Multiple { tag, ref tag_encoding, .. } = layout.variants else {
let Variants::Multiple { tag, ref tag_encoding, tag_field, .. } = layout.variants else {
return (vec![], None);
};

Expand Down Expand Up @@ -975,12 +975,28 @@ fn variant_info_for_generator<'tcx>(
if variant_size == Size::ZERO {
variant_size = upvars_size;
}
// We need to add the discriminant size back into min_size, since it is subtracted
// later during printing.
variant_size += match tag_encoding {
TagEncoding::Direct => tag.size(cx),
_ => Size::ZERO,
};

// This `if` deserves some explanation.
//
// The layout code has a choice of where to place the discriminant of this generator.
// If the discriminant of the generator is placed early in the layout (before the
// variant's own fields), then it'll implicitly be counted towards the size of the
// variant, since we use the maximum offset to calculate size.
// (side-note: I know this is a bit problematic given upvars placement, etc).
//
// This is important, since the layout printing code always subtracts this discriminant
// size from the variant size if the struct is "enum"-like, so failing to account for it
// will either lead to numerical underflow, or an underreported variant size...
//
// However, if the discriminant is placed past the end of the variant, then we need
// to factor in the size of the discriminant manually. This really should be refactored
// better, but this "works" for now.
if layout.fields.offset(tag_field) >= variant_size {
Copy link
Member

@Noratrieb Noratrieb Dec 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It took me a while to understand why this change was correct, probably mostly because the variant_size variable is a little confusingly named. Maybe something like last_field_end would better?

Thinking about this a little more, what this code really wants to do is variant_size.max(tag_end), maybe that would also make it simpler. Actually no, that would be incorrect if the discriminant was at the end.

Copy link
Member Author

@compiler-errors compiler-errors Dec 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure this is any clearer with the name last_field_end, because it really is the variant size (plus the upvars), modulo a hack to deal with needing to conditionally subtract one from the variant size depending on where the layout code chooses to put the tag.

Anyways, what I can do is leave a more detailed comment explaining why this is needed. This is probably still broken, but I don't know if it's worth taking much more time to fix.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is true, all of this code is trying to fit generator layout into a normal layout shaped hole and it's not going great. The current name is fine too and yeah, spending too much time on this is probably not worth it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But a quick comment would be nice.

variant_size += match tag_encoding {
TagEncoding::Direct => tag.size(cx),
_ => Size::ZERO,
};
}

VariantInfo {
name: Some(Symbol::intern(&ty::GeneratorSubsts::variant_name(variant_idx))),
Expand Down
12 changes: 2 additions & 10 deletions src/test/ui/print_type_sizes/async.rs
Original file line number Diff line number Diff line change
@@ -1,19 +1,11 @@
// compile-flags: -Z print-type-sizes
// compile-flags: -Z print-type-sizes --crate-type lib
// edition:2021
// build-pass
// ignore-pass

#![feature(start)]

async fn wait() {}

async fn test(arg: [u8; 8192]) {
pub async fn test(arg: [u8; 8192]) {
wait().await;
drop(arg);
}

#[start]
fn start(_: isize, _: *const *const u8) -> isize {
let _ = test([0; 8192]);
0
}
8 changes: 4 additions & 4 deletions src/test/ui/print_type_sizes/async.stdout
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
print-type-size type: `[async fn body@$DIR/async.rs:10:32: 13:2]`: 16386 bytes, alignment: 1 bytes
print-type-size type: `[async fn body@$DIR/async.rs:8:36: 11:2]`: 16386 bytes, alignment: 1 bytes
print-type-size discriminant: 1 bytes
print-type-size variant `Suspend0`: 16385 bytes
print-type-size field `.arg`: 8192 bytes, offset: 0 bytes, alignment: 1 bytes
Expand All @@ -16,14 +16,14 @@ print-type-size type: `std::mem::MaybeUninit<[u8; 8192]>`: 8192 bytes, alignment
print-type-size variant `MaybeUninit`: 8192 bytes
print-type-size field `.uninit`: 0 bytes
print-type-size field `.value`: 8192 bytes
print-type-size type: `[async fn body@$DIR/async.rs:8:17: 8:19]`: 1 bytes, alignment: 1 bytes
print-type-size type: `[async fn body@$DIR/async.rs:6:17: 6:19]`: 1 bytes, alignment: 1 bytes
print-type-size discriminant: 1 bytes
print-type-size variant `Unresumed`: 0 bytes
print-type-size variant `Returned`: 0 bytes
print-type-size variant `Panicked`: 0 bytes
print-type-size type: `std::mem::ManuallyDrop<[async fn body@$DIR/async.rs:8:17: 8:19]>`: 1 bytes, alignment: 1 bytes
print-type-size type: `std::mem::ManuallyDrop<[async fn body@$DIR/async.rs:6:17: 6:19]>`: 1 bytes, alignment: 1 bytes
print-type-size field `.value`: 1 bytes
print-type-size type: `std::mem::MaybeUninit<[async fn body@$DIR/async.rs:8:17: 8:19]>`: 1 bytes, alignment: 1 bytes
print-type-size type: `std::mem::MaybeUninit<[async fn body@$DIR/async.rs:6:17: 6:19]>`: 1 bytes, alignment: 1 bytes
print-type-size variant `MaybeUninit`: 1 bytes
print-type-size field `.uninit`: 0 bytes
print-type-size field `.value`: 1 bytes
Expand Down
8 changes: 3 additions & 5 deletions src/test/ui/print_type_sizes/generator.rs
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
// compile-flags: -Z print-type-sizes
// compile-flags: -Z print-type-sizes --crate-type=lib
// build-pass
// ignore-pass

#![feature(start, generators, generator_trait)]
#![feature(generators, generator_trait)]

use std::ops::Generator;

Expand All @@ -13,8 +13,6 @@ fn generator<const C: usize>(array: [u8; C]) -> impl Generator<Yield = (), Retur
}
}

#[start]
fn start(_: isize, _: *const *const u8) -> isize {
pub fn foo() {
let _ = generator([0; 8192]);
0
}
23 changes: 23 additions & 0 deletions src/test/ui/print_type_sizes/generator_discr_placement.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
// compile-flags: -Z print-type-sizes --crate-type lib
// build-pass
// ignore-pass

// Tests a generator that has its discriminant as the *final* field.

// Avoid emitting panic handlers, like the rest of these tests...
compiler-errors marked this conversation as resolved.
Show resolved Hide resolved
#![feature(generators)]

pub fn foo() {
let a = || {
{
let w: i32 = 4;
yield;
drop(w);
}
{
let z: i32 = 7;
yield;
drop(z);
}
};
}
11 changes: 11 additions & 0 deletions src/test/ui/print_type_sizes/generator_discr_placement.stdout
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
print-type-size type: `[generator@$DIR/generator_discr_placement.rs:11:13: 11:15]`: 8 bytes, alignment: 4 bytes
print-type-size discriminant: 1 bytes
print-type-size variant `Suspend0`: 7 bytes
print-type-size padding: 3 bytes
print-type-size field `.w`: 4 bytes, alignment: 4 bytes
print-type-size variant `Suspend1`: 7 bytes
print-type-size padding: 3 bytes
print-type-size field `.z`: 4 bytes, alignment: 4 bytes
print-type-size variant `Unresumed`: 0 bytes
print-type-size variant `Returned`: 0 bytes
print-type-size variant `Panicked`: 0 bytes
24 changes: 2 additions & 22 deletions src/test/ui/print_type_sizes/generics.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// compile-flags: -Z print-type-sizes
// compile-flags: -Z print-type-sizes --crate-type=lib
// build-pass
// ignore-pass
// ^-- needed because `--pass check` does not emit the output needed.
Expand All @@ -8,24 +8,6 @@
// monomorphized, in the MIR of the original function in which they
// occur, to have their size reported.

#![feature(start)]

// In an ad-hoc attempt to avoid the injection of unwinding code
// (which clutters the output of `-Z print-type-sizes` with types from
// `unwind::libunwind`):
//
// * I am not using Default to build values because that seems to
// cause the injection of unwinding code. (Instead I just make `fn new`
// methods.)
//
// * Pair derive Copy to ensure that we don't inject
// unwinding code into generic uses of Pair when T itself is also
// Copy.
//
// (I suspect this reflect some naivety within the rust compiler
// itself; it should be checking for drop glue, i.e., a destructor
// somewhere in the monomorphized types. It should not matter whether
// the type is Copy.)
#[derive(Copy, Clone)]
pub struct Pair<T> {
_car: T,
Expand Down Expand Up @@ -61,11 +43,9 @@ pub fn f1<T:Copy>(x: T) {
Pair::new(FiftyBytes::new(), FiftyBytes::new());
}

#[start]
fn start(_: isize, _: *const *const u8) -> isize {
pub fn start() {
let _b: Pair<u8> = Pair::new(0, 0);
let _s: Pair<SevenBytes> = Pair::new(SevenBytes::new(), SevenBytes::new());
let ref _z: ZeroSized = ZeroSized;
f1::<SevenBytes>(SevenBytes::new());
0
}
12 changes: 1 addition & 11 deletions src/test/ui/print_type_sizes/multiple_types.rs
Original file line number Diff line number Diff line change
@@ -1,23 +1,13 @@
// compile-flags: -Z print-type-sizes
// compile-flags: -Z print-type-sizes --crate-type=lib
// build-pass

// This file illustrates that when multiple structural types occur in
// a function, every one of them is included in the output.

#![feature(start)]

pub struct SevenBytes([u8; 7]);
pub struct FiftyBytes([u8; 50]);

pub enum Enum {
Small(SevenBytes),
Large(FiftyBytes),
}

#[start]
fn start(_: isize, _: *const *const u8) -> isize {
let _e: Enum;
let _f: FiftyBytes;
let _s: SevenBytes;
0
}
10 changes: 3 additions & 7 deletions src/test/ui/print_type_sizes/niche-filling.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// compile-flags: -Z print-type-sizes
// compile-flags: -Z print-type-sizes --crate-type=lib
// build-pass
// ignore-pass
// ^-- needed because `--pass check` does not emit the output needed.
Expand All @@ -14,7 +14,6 @@
// aligned (while on most it is 8-byte aligned) and so the resulting
// padding and overall computed sizes can be quite different.

#![feature(start)]
#![feature(rustc_attrs)]
#![allow(dead_code)]

Expand Down Expand Up @@ -56,7 +55,7 @@ pub struct NestedNonZero {

impl Default for NestedNonZero {
fn default() -> Self {
NestedNonZero { pre: 0, val: NonZeroU32::new(1).unwrap(), post: 0 }
NestedNonZero { pre: 0, val: unsafe { NonZeroU32::new_unchecked(1) }, post: 0 }
}
}

Expand All @@ -76,8 +75,7 @@ pub union Union2<A: Copy, B: Copy> {
b: B,
}

#[start]
fn start(_: isize, _: *const *const u8) -> isize {
pub fn test() {
let _x: MyOption<NonZeroU32> = Default::default();
let _y: EmbeddedDiscr = Default::default();
let _z: MyOption<IndirectNonZero> = Default::default();
Expand All @@ -96,6 +94,4 @@ fn start(_: isize, _: *const *const u8) -> isize {
// ...even when theoretically possible.
let _j: MyOption<Union1<NonZeroU32>> = Default::default();
let _k: MyOption<Union2<NonZeroU32, NonZeroU32>> = Default::default();

0
}
8 changes: 2 additions & 6 deletions src/test/ui/print_type_sizes/no_duplicates.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// compile-flags: -Z print-type-sizes
// compile-flags: -Z print-type-sizes --crate-type=lib
// build-pass
// ignore-pass
// ^-- needed because `--pass check` does not emit the output needed.
Expand All @@ -8,16 +8,12 @@
// (even if multiple functions), it is only printed once in the
// print-type-sizes output.

#![feature(start)]

pub struct SevenBytes([u8; 7]);

pub fn f1() {
let _s: SevenBytes = SevenBytes([0; 7]);
}

#[start]
fn start(_: isize, _: *const *const u8) -> isize {
pub fn test() {
let _s: SevenBytes = SevenBytes([0; 7]);
0
}
20 changes: 5 additions & 15 deletions src/test/ui/print_type_sizes/packed.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// compile-flags: -Z print-type-sizes
// compile-flags: -Z print-type-sizes --crate-type=lib
// build-pass
// ignore-pass
// ^-- needed because `--pass check` does not emit the output needed.
Expand All @@ -13,11 +13,10 @@
// padding and overall computed sizes can be quite different.

#![allow(dead_code)]
#![feature(start)]

#[derive(Default)]
#[repr(packed)]
struct Packed1 {
pub struct Packed1 {
a: u8,
b: u8,
g: i32,
Expand All @@ -28,7 +27,7 @@ struct Packed1 {

#[derive(Default)]
#[repr(packed(2))]
struct Packed2 {
pub struct Packed2 {
a: u8,
b: u8,
g: i32,
Expand All @@ -40,7 +39,7 @@ struct Packed2 {
#[derive(Default)]
#[repr(packed(2))]
#[repr(C)]
struct Packed2C {
pub struct Packed2C {
a: u8,
b: u8,
g: i32,
Expand All @@ -50,20 +49,11 @@ struct Packed2C {
}

#[derive(Default)]
struct Padded {
pub struct Padded {
a: u8,
b: u8,
g: i32,
c: u8,
h: i16,
d: u8,
}

#[start]
fn start(_: isize, _: *const *const u8) -> isize {
let _c: Packed1 = Default::default();
let _d: Packed2 = Default::default();
let _e: Packed2C = Default::default();
let _f: Padded = Default::default();
0
}
8 changes: 1 addition & 7 deletions src/test/ui/print_type_sizes/padding.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// compile-flags: -Z print-type-sizes
// compile-flags: -Z print-type-sizes --crate-type=lib
// build-pass

// This file illustrates how padding is handled: alignment
Expand All @@ -9,7 +9,6 @@
// aligned (while on most it is 8-byte aligned) and so the resulting
// padding and overall computed sizes can be quite different.

#![feature(start)]
#![allow(dead_code)]

struct S {
Expand All @@ -27,8 +26,3 @@ enum E2 {
A(i8, i32),
B(S),
}

#[start]
fn start(_: isize, _: *const *const u8) -> isize {
0
}
12 changes: 3 additions & 9 deletions src/test/ui/print_type_sizes/repr-align.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// compile-flags: -Z print-type-sizes
// compile-flags: -Z print-type-sizes --crate-type=lib
// build-pass
// ignore-pass
// ^-- needed because `--pass check` does not emit the output needed.
Expand All @@ -11,7 +11,7 @@
// It avoids using u64/i64 because on some targets that is only 4-byte
// aligned (while on most it is 8-byte aligned) and so the resulting
// padding and overall computed sizes can be quite different.
#![feature(start)]

#![allow(dead_code)]

#[repr(align(16))]
Expand All @@ -24,15 +24,9 @@ enum E {
}

#[derive(Default)]
struct S {
pub struct S {
a: i32,
b: i32,
c: A,
d: i8,
}

#[start]
fn start(_: isize, _: *const *const u8) -> isize {
let _s: S = Default::default();
0
}
Loading