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

Disable field reordering for repr(int). #56887

Merged
merged 2 commits into from
Dec 22, 2018

Conversation

emilio
Copy link
Contributor

@emilio emilio commented Dec 16, 2018

This fixes the problem that the test in #56619 uncovers.

Closes #56619.

@rust-highfive
Copy link
Collaborator

r? @petrochenkov

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 16, 2018
@emilio
Copy link
Contributor Author

emilio commented Dec 16, 2018

r? @eddyb

@rust-highfive rust-highfive assigned eddyb and unassigned petrochenkov Dec 16, 2018
@emilio
Copy link
Contributor Author

emilio commented Dec 16, 2018

The order of commits is the opposite github shows because github doesn't show the topological order... :(

Anyway. cc @BorisChiou, I think this is the problem you hit with cbindgen that time.

@rust-highfive

This comment has been minimized.

RFC rust-lang#2195 specifies that a repr(int) enum such as:

    #[repr(u8)]
    enum MyEnum {
        B { x: u8, y: i16, z: u8 },
    }

has a layout that is equivalent to:

    #[repr(C)]
    enum MyEnumVariantB { tag: u8, x: u8, y: i16, z: u8 },

However this isn't actually implemented, with the actual layout being
roughly equivalent to:

    union MyEnumPayload {
        B { x: u8, y: i16, z: u8 },
    }

    #[repr(packed)]
    struct MyEnum {
        tag: u8,
        payload: MyEnumPayload,
    }

Thus the variant payload is *not* subject to repr(C) ordering rules, and
gets re-ordered as `{ x: u8, z: u8, z: i16 }`

The existing tests added in pull-req rust-lang#45688 fail to catch this as the
repr(C) ordering just happens to match the current Rust ordering in this
case; adding a third field reveals the problem.
pub fn inhibit_struct_field_reordering_opt(&self) -> bool {
!(self.flags & ReprFlags::IS_UNOPTIMISABLE).is_empty() || (self.pack == 1)
self.flags.intersects(ReprFlags::IS_UNOPTIMISABLE) || self.pack == 1 ||
self.int.is_some()
Copy link
Member

@eddyb eddyb Dec 19, 2018

Choose a reason for hiding this comment

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

cc @rkruppe @nagisa This seems somewhat fragile (I mean the pre-existing code).
The check for pack == 1 means things will still be reordered with #[repr(packed(N))], is that intentional?
Also, it seems to me that inhibit_enum_layout_opt is a subset of this function and maybe we should have just one function that checks all conditions?

Copy link
Member

@nagisa nagisa Dec 19, 2018

Choose a reason for hiding this comment

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

The repr RFC does not specify any behaviour /wrt repr(packed(N)) for enums and I’m not sure I recall us guaranteeing that repr(packed) specifies any representation and the reference seems to agree.

Copy link
Member

Choose a reason for hiding this comment

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

In fact as per strict reading of the reference, self.pack == 1 inhibiting reordering would be somewhat wrong, as it should cause "packing" for the original ordering, but since the "default" ordering is not specified, it seems to be fine either way...

@eddyb
Copy link
Member

eddyb commented Dec 19, 2018

@bors r+

@bors
Copy link
Contributor

bors commented Dec 19, 2018

📌 Commit d84bdba has been approved by eddyb

@bors 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 19, 2018
pietroalbini added a commit to pietroalbini/rust that referenced this pull request Dec 20, 2018
Disable field reordering for repr(int).

This fixes the problem that the test in rust-lang#56619 uncovers.

Closes rust-lang#56619.
Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request Dec 21, 2018
Disable field reordering for repr(int).

This fixes the problem that the test in rust-lang#56619 uncovers.

Closes rust-lang#56619.
pietroalbini added a commit to pietroalbini/rust that referenced this pull request Dec 21, 2018
Disable field reordering for repr(int).

This fixes the problem that the test in rust-lang#56619 uncovers.

Closes rust-lang#56619.
bors added a commit that referenced this pull request Dec 21, 2018
Rollup of 20 pull requests

Successful merges:

 - #56802 (Add DoubleEndedIterator::nth_back)
 - #56842 (Add unstable VecDeque::rotate_{left|right})
 - #56869 (Reduce search-index.js size)
 - #56887 (Disable field reordering for repr(int).)
 - #56892 (rustc: Update Clang used to build LLVM on Linux)
 - #56909 (static eval: Do not ICE on layout size overflow)
 - #56914 (Ignore ui/target-feature-gate on sparc, sparc64, powerpc, powerpc64 and powerpc64le)
 - #56917 (Simplify MIR generation for logical operations)
 - #56919 (Remove a wrong multiplier on relocation offset computation)
 - #56933 (Add --progress to git submodule commands in x.py)
 - #56941 (deny intra-doc link resolution failures in libstd)
 - #56964 (Remove `TokenStream::JointTree`.)
 - #56970 (Mem uninit doc ptr drop)
 - #56973 (make basic CTFE tracing available on release builds)
 - #56979 (Adding unwinding support for x86_64_fortanix_unknown_sgx target.)
 - #56981 (miri: allocation is infallible)
 - #56984 (A few tweaks to dropck_outlives)
 - #56989 (Fix compiletest `trim` deprecation warnings)
 - #56992 (suggest similar lint names for unknown lints)
 - #57002 (Stabilize Vec(Deque)::resize_with)

Failed merges:

r? @ghost
bors added a commit that referenced this pull request Dec 21, 2018
Rollup of 20 pull requests

Successful merges:

 - #56802 (Add DoubleEndedIterator::nth_back)
 - #56842 (Add unstable VecDeque::rotate_{left|right})
 - #56869 (Reduce search-index.js size)
 - #56887 (Disable field reordering for repr(int).)
 - #56892 (rustc: Update Clang used to build LLVM on Linux)
 - #56909 (static eval: Do not ICE on layout size overflow)
 - #56914 (Ignore ui/target-feature-gate on sparc, sparc64, powerpc, powerpc64 and powerpc64le)
 - #56917 (Simplify MIR generation for logical operations)
 - #56919 (Remove a wrong multiplier on relocation offset computation)
 - #56933 (Add --progress to git submodule commands in x.py)
 - #56941 (deny intra-doc link resolution failures in libstd)
 - #56964 (Remove `TokenStream::JointTree`.)
 - #56970 (Mem uninit doc ptr drop)
 - #56973 (make basic CTFE tracing available on release builds)
 - #56979 (Adding unwinding support for x86_64_fortanix_unknown_sgx target.)
 - #56981 (miri: allocation is infallible)
 - #56984 (A few tweaks to dropck_outlives)
 - #56989 (Fix compiletest `trim` deprecation warnings)
 - #56992 (suggest similar lint names for unknown lints)
 - #57002 (Stabilize Vec(Deque)::resize_with)

Failed merges:

r? @ghost
Centril added a commit to Centril/rust that referenced this pull request Dec 22, 2018
Disable field reordering for repr(int).

This fixes the problem that the test in rust-lang#56619 uncovers.

Closes rust-lang#56619.
@kennytm
Copy link
Member

kennytm commented Dec 22, 2018

@bors p=30

(rollup fairness)

@bors
Copy link
Contributor

bors commented Dec 22, 2018

⌛ Testing commit d84bdba with merge 2d3e909...

bors added a commit that referenced this pull request Dec 22, 2018
Disable field reordering for repr(int).

This fixes the problem that the test in #56619 uncovers.

Closes #56619.
@bors
Copy link
Contributor

bors commented Dec 22, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: eddyb
Pushing 2d3e909 to master...

@bors bors merged commit d84bdba into rust-lang:master Dec 22, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants