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

#[repr(X)] should inhibit enum layout optimization #40029

Closed
bluss opened this issue Feb 22, 2017 · 7 comments
Closed

#[repr(X)] should inhibit enum layout optimization #40029

bluss opened this issue Feb 22, 2017 · 7 comments
Assignees
Labels
P-high High priority regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@bluss
Copy link
Member

bluss commented Feb 22, 2017

This is a regression stable to nightly (breaking nodrop and arrayvec at the moment; not their compilation but their tests, which indicate their implementation is not sound for element types like &T or Vec<T>).

Found by cargobomb report

Note that the regression applies to enums with #[repr(u8)] (and similar) but not #[repr(C)].

PR #25651 was never merged, but it would document this aspect of #[repr(u8)]

Evaluating #39595 for its impact. Thanks @eddyb for the pointers!

@bluss bluss added the regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. label Feb 22, 2017
@bluss
Copy link
Member Author

bluss commented Feb 22, 2017

As usual filing an issue for every regression; it doesn't mean that the change in behaviour must be fixed. In this case it probably should, though.

@bluss
Copy link
Member Author

bluss commented Feb 22, 2017

First nightly with change is rustc 1.17.0-nightly (24a70eb59 2017-02-09) (rustup's nightly-2017-02-10)

@bluss
Copy link
Member Author

bluss commented Feb 23, 2017

All crates affected by the nodrop breakage are fixed by just released nodrop 0.1.9.

It changed from this:

    #[repr(u8)]
    enum Flag<T> {
        Alive(T),
        Dropped,
    }

to this:

    #[repr(u8)]
    enum Flag<T> {
        Alive(T),
        Dropped(u8),
    }

I didn't find a reason to not use this workaround.

@nikomatsakis
Copy link
Contributor

I believe we should restore the old behavior. I feel like if you request a representation with a u8 discriminant, we should give you a u8 discriminant, not no discriminant at all. =) More specifically, I've always thought of the u8 repr as being struct { u8, X } where X is the union of all arguments. (Same for u16 etc.)

I could see other choices, but given that this is what we've done till now (right?) I'd say we ought to stick with it. If you want the option for us to apply further optimizations (use "u8" if you need a discriminant, but otherwise who cares...) we should have people opt in to that.

@brson brson added the P-high High priority label Mar 1, 2017
@brson
Copy link
Contributor

brson commented Mar 1, 2017

I agree this is a regression that should be fixed.

@brson brson added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. I-nominated labels Mar 1, 2017
@nikomatsakis
Copy link
Contributor

on it.

@nikomatsakis
Copy link
Contributor

See #40188

nikomatsakis added a commit to nikomatsakis/rust that referenced this issue Mar 1, 2017
bors added a commit that referenced this issue Mar 2, 2017
inhibit enum layout optimizations under `#[repr(C)]` or `#[repr(u8)]`

Fixes #40029
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P-high High priority regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. 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

3 participants