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

Uninhabited types are ZSTs despite partial initialization being allowed in safe code. #49298

Closed
eddyb opened this issue Mar 23, 2018 · 14 comments
Assignees
Labels
A-codegen Area: Code generation C-bug Category: This is a bug. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness P-medium Medium priority regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@eddyb
Copy link
Member

eddyb commented Mar 23, 2018

When we optimized product types with uninhabited fields to be ZST, partial initialization was overlooked, i.e. writes to the inhabited sibling fields are allowed despite missing in the layout.
Thankfully, it's impossible to read or borrow such fields, because the whole value must be initialized (although this may be relaxed in the future). However , the initialized fields are still dropped.

One way we could work around this is track uninhabitedness but only take advantage of it in enums.
(I am not aware of a solution that works with variant types, which I think we should keep in mind.)
EDIT: we can disallow creating the enum from an incomplete variant. This is also required by niche optimizations because the niche slot being uninitialized would result in UB.
EDIT2: incomplete variants might be possible today already (#49298 (comment)).

As an example, in debug mode, this snippet prints 0, because x.1 and y overlap:

enum Void {}

fn main() {
    let mut x: (Void, usize);
    let y = 1;
    x.1 = 0;
    println!("{}", y)
}

cc @nikomatsakis @nagisa

@eddyb eddyb added A-codegen Area: Code generation I-nominated T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Mar 23, 2018
@KamilaBorowska
Copy link
Contributor

KamilaBorowska commented Mar 23, 2018

Seems I-unsound.

enum Void {}

fn main() {
    let mut x: (Void, [u8; 20]);
    x.1 = [0x12; 20];
}

@eddyb eddyb added the I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness label Mar 23, 2018
@KamilaBorowska
Copy link
Contributor

KamilaBorowska commented Mar 23, 2018

Also, this regression was introduced in Rust 1.24.0 from what I see, this correctly works in 1.23.0.

@cuviper cuviper added the C-bug Category: This is a bug. label Mar 24, 2018
@ExpHP
Copy link
Contributor

ExpHP commented Mar 24, 2018

To pile on the scary labels, that also makes this a regression from stable to stable!

@scottmcm scottmcm added the regression-from-stable-to-stable Performance or correctness regression from one stable version to another. label Mar 25, 2018
@eddyb
Copy link
Member Author

eddyb commented Mar 28, 2018

#48493 looks like an instance of this - cc @alexcrichton

@hanna-kruppe
Copy link
Contributor

hanna-kruppe commented Mar 28, 2018

@eddyb Weak creates a value of the uninhabited type via mem::uninitialized, it does not perform partial initalization. It's UB anyway. This issue (edit: resolving this issue) would at best make the code working-but-still-UB.

@bstrie
Copy link
Contributor

bstrie commented Mar 28, 2018

@rkruppe is there a separate bug for that?

@hanna-kruppe
Copy link
Contributor

@eddyb Uh, I guess #48493 is that bug? Although if we land a workaround before the proper fix (MaybeUninitialized with unsizing support) becomes available, we should make sure we open a new bug for tracking removal of the workaround and introduction of the proper fix.

@eddyb
Copy link
Member Author

eddyb commented Apr 11, 2018

There's potentially an even more problematic implication here: initializing an uninhabited (because of its fields) variant and panicking while doing so could also result in writing over data following the whole enum, assuming the data is larger than the enum itself, e.g.:

let x: Option<(String, !)>;
let y = vec!["foo".to_string()];
x = Some((String::new("bar"), panic!("{:?}", y)));

This might be hard to demonstrate today, because of some extra temporaries, but optimizations would likely prefer to write to x directly, which would spell disaster.
I'm not sure what a proper solution would be here, we certainly need to discuss it.

@nikomatsakis
Copy link
Contributor

We discussed in the meeting. It seems like the rules want to be:

  • Ignore uninhabitedness when laying out product types (! is treated like any ZST)
  • Ignore variants for enums if they are both uninhabited and ZST

This ensures there is space for non-ZST data... we need to do this if we want to be able to optimize temporaries away pre-monomorphization.

@eddyb
Copy link
Member Author

eddyb commented Apr 12, 2018

Ignore variants for enums if they are both uninhabited and ZST

Specifically, the data inside the variant (not including any tag or anything else enum-specific).

@nikomatsakis nikomatsakis added P-medium Medium priority and removed I-nominated labels Apr 26, 2018
@eddyb eddyb self-assigned this Apr 26, 2018
bors added a commit that referenced this issue May 13, 2018
rustc: leave space for fields of uninhabited types to allow partial initialization.

Fixes #49298 by only collapsing uninhabited enum variants, and only if they only have ZST fields.
Fixes #50442 incidentally (@nox's optimization didn't take into account uninhabited variants).
@vi
Copy link
Contributor

vi commented Mar 30, 2019

Shall there be some #[attribute] to opt out partial initialisation, but gain "multiply by zero" layout optimisation?

Related idea: #[repr(blackhole)] forcing any enum, struct or union to be ZST, making all writes drops and statically forbidding reads or referencings.

@eddyb
Copy link
Member Author

eddyb commented Mar 30, 2019

@vi Note that, IIRC, Option<(T, !)> should be zero-sized even if (T, !) isn't (unless I'm misremembering).
Anyway, the tricky cases are tricky because they make "in-place optimizations" much harder, and we'd prefer to cater to usecases without making (T, !) a ZST.

Note that #49298 (comment) is more general than partial initialization, and it concerns writes to return destination, followed by a panic later, in the context of wanting to optimize away copies.

@vi
Copy link
Contributor

vi commented Mar 30, 2019

IIRC, Option<(T, !)> should be zero-sized even if (T, !) isn't (unless I'm misremembering).

println!("{}", std::mem::size_of::<Option<(!,u32)>>());
8.

@eddyb
Copy link
Member Author

eddyb commented Mar 30, 2019

Ah, right, I was thinking of a subtler distinction, nevermind.

bors added a commit to rust-lang-ci/rust that referenced this issue Aug 29, 2023
fix some issues around ZST handling

This fixes two bugs:
- We used to entirely skip enum variants like `B([u16; 0], !)`, even failing to properly align the enum!  Honoring the alignment of uninhabited variants is important for the same reason that we must reserve space for their fields -- see [here](rust-lang#49298 (comment)) for why.
- ~~We uses to reject `repr(transparent)` on `struct MyType([u16; 0])`, which is weird because a one-field struct should always be allowed to be transparent around that field.~~ (moved to separate PR)

I also found two places in the layout code that did something special for ZST without explaining why, and removing those special cases doesn't seem to have any effect except for reordering some zero-sized fields which shouldn't be an issue... maybe PR CI will explain why those cases were needed, or maybe they became obsolete at some point.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-codegen Area: Code generation C-bug Category: This is a bug. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness P-medium Medium priority regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

9 participants