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

rustc: do not raise the alignment of optimized enums to the niche's alignment. #46809

Merged
merged 1 commit into from Dec 22, 2017

Conversation

Projects
None yet
6 participants
@eddyb
Member

eddyb commented Dec 18, 2017

This is the improved fix for #46769 that does not increase the size of any types (see also #46808).

@rust-highfive

This comment has been minimized.

Show comment
Hide comment
@rust-highfive

rust-highfive Dec 18, 2017

Collaborator

r? @estebank

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

Collaborator

rust-highfive commented Dec 18, 2017

r? @estebank

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

@eddyb

This comment has been minimized.

Show comment
Hide comment
@eddyb
Member

eddyb commented Dec 18, 2017

@arielb1

This comment has been minimized.

Show comment
Hide comment
@arielb1

arielb1 Dec 18, 2017

Contributor

@bors r+

Contributor

arielb1 commented Dec 18, 2017

@bors r+

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Dec 18, 2017

Contributor

📌 Commit 090a192 has been approved by arielb1

Contributor

bors commented Dec 18, 2017

📌 Commit 090a192 has been approved by arielb1

@arielb1

This comment has been minimized.

Show comment
Hide comment
@arielb1

arielb1 Dec 18, 2017

Contributor

@bors r_

Contributor

arielb1 commented Dec 18, 2017

@bors r_

@arielb1

This comment has been minimized.

Show comment
Hide comment
@arielb1

arielb1 Dec 18, 2017

Contributor

@bors r-

Contributor

arielb1 commented Dec 18, 2017

@bors r-

@eddyb

This comment has been minimized.

Show comment
Hide comment
@eddyb

eddyb Dec 18, 2017

Member

(don't mind me, just Travis-cycling the PR)

Member

eddyb commented Dec 18, 2017

(don't mind me, just Travis-cycling the PR)

@eddyb eddyb closed this Dec 18, 2017

@eddyb eddyb reopened this Dec 18, 2017

bors added a commit that referenced this pull request Dec 18, 2017

Auto merge of #46808 - eddyb:issue-46769-quick, r=arielb1
rustc: ensure optimized enums have a properly aligned size.

Fixes #46769 by padding the optimized enums wrapping packed data as necessary.

Note that this is not the only way to solve this - on nightly, #46436 makes it easier to fix without adding new padding because of the replacement of `packed` flags with a non-redundant scheme.
But because it can't be backported, the optimal fix will be in a separate nightly-only PR (#46809).
@kennytm

This comment has been minimized.

Show comment
Hide comment
@kennytm

kennytm Dec 19, 2017

Member

#46808 has been merged.

Member

kennytm commented Dec 19, 2017

#46808 has been merged.

@eddyb

This comment has been minimized.

Show comment
Hide comment
@eddyb

eddyb Dec 19, 2017

Member

@bors r=arielb1

Member

eddyb commented Dec 19, 2017

@bors r=arielb1

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Dec 19, 2017

Contributor

📌 Commit 8fc4afe has been approved by arielb1

Contributor

bors commented Dec 19, 2017

📌 Commit 8fc4afe has been approved by arielb1

@arielb1

This comment has been minimized.

Show comment
Hide comment
@arielb1

arielb1 Dec 19, 2017

Contributor

Could you please add a test that the size of a packed enum is what it's supposed to be?

@bors r-

Contributor

arielb1 commented Dec 19, 2017

Could you please add a test that the size of a packed enum is what it's supposed to be?

@bors r-

@arielb1

This comment has been minimized.

Show comment
Hide comment
@arielb1

arielb1 Dec 19, 2017

Contributor

Something like this:

// This struct has a single niche - the dealigned pointer within. Check that
// it is null-pointer-optimized correctly
struct Interesting {
    packed: Packed<&'static ()>,
    other: u8
}

assert_eq!(mem::size_of::<Interesting>(), mem::size_of::<Option<Interesting>>());
Contributor

arielb1 commented Dec 19, 2017

Something like this:

// This struct has a single niche - the dealigned pointer within. Check that
// it is null-pointer-optimized correctly
struct Interesting {
    packed: Packed<&'static ()>,
    other: u8
}

assert_eq!(mem::size_of::<Interesting>(), mem::size_of::<Option<Interesting>>());
@eddyb

This comment has been minimized.

Show comment
Hide comment
@eddyb

eddyb Dec 20, 2017

Member

@bors r=arielb1

Member

eddyb commented Dec 20, 2017

@bors r=arielb1

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Dec 20, 2017

Contributor

📌 Commit 5c3dcfa has been approved by arielb1

Contributor

bors commented Dec 20, 2017

📌 Commit 5c3dcfa has been approved by arielb1

kennytm added a commit to kennytm/rust that referenced this pull request Dec 21, 2017

Rollup merge of rust-lang#46809 - eddyb:issue-46769-optimal, r=arielb1
 rustc: do not raise the alignment of optimized enums to the niche's alignment.

This is the improved fix for rust-lang#46769 that does not increase the size of any types (see also rust-lang#46808).

bors added a commit that referenced this pull request Dec 21, 2017

Auto merge of #46922 - kennytm:rollup, r=kennytm
Rollup of 15 pull requests

- Successful merges: #46636, #46780, #46784, #46809, #46814, #46820, #46839, #46847, #46858, #46878, #46884, #46890, #46898, #46916, #46918
- Failed merges:

bors added a commit that referenced this pull request Dec 21, 2017

Auto merge of #46922 - kennytm:rollup, r=kennytm
Rollup of 14 pull requests

- Successful merges: #46636, #46780, #46784, #46809, #46814, #46820, #46839, #46847, #46858, #46878, #46884, #46890, #46898, #46918
- Failed merges:

@bors bors merged commit 5c3dcfa into rust-lang:master Dec 22, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@eddyb eddyb deleted the eddyb:issue-46769-optimal branch Dec 22, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment