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

Add (failing) test to check order of repr(int) enum fields #56619

Conversation

petertodd
Copy link
Contributor

RFC #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 #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.

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.
@rust-highfive
Copy link
Collaborator

r? @nikomatsakis

(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 8, 2018
@petertodd
Copy link
Contributor Author

...as for how to actually fix this, that's above my pay-grade for now. :)

@rust-highfive
Copy link
Collaborator

The job x86_64-gnu-llvm-5.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
travis_time:end:14b1805d:start=1544232075486772405,finish=1544232076507649915,duration=1020877510
$ git checkout -qf FETCH_HEAD
travis_fold:end:git.checkout

Encrypted environment variables have been removed for security reasons.
See https://docs.travis-ci.com/user/pull-requests/#Pull-Requests-and-Security-Restrictions
$ export SCCACHE_BUCKET=rust-lang-ci-sccache2
$ export SCCACHE_REGION=us-west-1
Setting environment variables from .travis.yml
$ export IMAGE=x86_64-gnu-llvm-5.0
---

[00:03:08] travis_fold:start:tidy
travis_time:start:tidy
tidy check
[00:03:08] tidy error: /checkout/src/test/run-pass/structs-enums/enum-non-c-like-repr-int.rs:24: line longer than 100 chars
[00:03:09] tidy error: /checkout/src/test/run-pass/structs-enums/enum-non-c-like-repr-c-and-int.rs:24: line longer than 100 chars
[00:03:09] tidy error: /checkout/src/test/run-pass/structs-enums/enum-non-c-like-repr-c.rs:24: line longer than 100 chars
[00:03:10] some tidy checks failed
[00:03:10] 
[00:03:10] 
[00:03:10] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/src" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "--no-vendor" "--quiet"
[00:03:10] 
[00:03:10] 
[00:03:10] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
[00:03:10] Build completed unsuccessfully in 0:00:55
[00:03:10] Build completed unsuccessfully in 0:00:55
[00:03:10] make: *** [tidy] Error 1
[00:03:10] Makefile:79: recipe for target 'tidy' failed
The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:081eede2
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
Sat Dec  8 01:24:35 UTC 2018
---
travis_time:end:346fa3e8:start=1544232275942384796,finish=1544232275947218126,duration=4833330
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:09a98d56
$ ln -s . checkout && for CORE in obj/cores/core.*; do EXE=$(echo $CORE | sed 's|obj/cores/core\.[0-9]*\.!checkout!\(.*\)|\1|;y|!|/|'); if [ -f "$EXE" ]; then printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" "$CORE"; gdb --batch -q -c "$CORE" "$EXE" -iex 'set auto-load off' -iex 'dir src/' -iex 'set sysroot .' -ex bt -ex q; echo travis_fold":"end:crashlog; fi; done || true
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:0eb00717
travis_time:start:0eb00717
$ cat ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
cat: ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers: No such file or directory
travis_fold:end:after_failure.5
travis_fold:start:after_failure.6
travis_time:start:13c155a8
$ dmesg | grep -i kill

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@nikomatsakis
Copy link
Contributor

r? @eddyb

@rust-highfive rust-highfive assigned eddyb and unassigned nikomatsakis Dec 11, 2018
@petertodd
Copy link
Contributor Author

petertodd commented Dec 16, 2018

Note: looks like the tests added in #50354 also have this flaw.

edit: Also https://gist.github.com/Gankro/5223984900f2e2536fc4676fd8150653

@emilio
Copy link
Contributor

emilio commented Dec 16, 2018

I think a patch like this should fix it, though haven't tested it:

diff --git a/src/librustc/ty/mod.rs b/src/librustc/ty/mod.rs
index a1fc949137..227b75a772 100644
--- a/src/librustc/ty/mod.rs
+++ b/src/librustc/ty/mod.rs
@@ -2061,7 +2061,8 @@ impl ReprOptions {
     /// Returns `true` if this `#[repr()]` should inhibit struct field reordering
     /// optimizations, such as with repr(C) or repr(packed(1)).
     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()
     }
 
     /// Returns true if this `#[repr()]` should inhibit union abi optimisations

@emilio
Copy link
Contributor

emilio commented Dec 16, 2018

I just confirmed that fixes the test, will submit a PR with that and your test.

@emilio
Copy link
Contributor

emilio commented Dec 16, 2018

Opened #56887 (kept you as the author of the test of course :))

@emilio
Copy link
Contributor

emilio commented Dec 16, 2018

/cc @gankro

@Gankra
Copy link
Contributor

Gankra commented Dec 16, 2018

oops, yep, nice catch!

@petertodd
Copy link
Contributor Author

@emilio Thanks! I'll close this pull-req then in favor of #56887

@petertodd petertodd closed this Dec 18, 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.
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.
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.
@petertodd petertodd deleted the 2018-12-enum-non-c-like-repr-int-ordering branch October 2, 2019 22:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants