Add reftest that checks the layout of repr(int) on non-c-like enums #45688
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
(rust_highfive has picked a reviewer for you, use r? to override) |
kennytm
added
the
S-waiting-on-review
|
@bors: r+ Nice! |
|
📌 Commit 95a7f29 has been approved by |
kennytm
added
S-waiting-on-bors
bors
added a commit
that referenced
this pull request
Nov 3, 2017
Add reftest that checks the layout of repr(int) on non-c-like enums This verifies the first layout specified in rust-lang/rfcs#2195 The second (`repr(C)`) layout isn't included here because it doesn't actually exist today. However if/when that's implemented a second test could be fairly easily derived from this one. CC @eddyb
|
💔 Test failed - status-travis |
|
The diff: --- 1.txt 2017-11-03 15:20:03.000000000 +0800
+++ 2.txt 2017-11-03 15:20:16.000000000 +0800
@@ -75,8 +75,10 @@
[01:22:58] let input: Vec<u8> =
[01:22:58] vec!(0 , 17 , 0 , 0 , 0 , 1 , 206 , 121 , 4 , 2 , 8 , 3 , 0 , 151 , 1
[01:22:58] , 0 , 0 , 3 , 1 , 4 , 100 , 0 , 0 , 0 , 0 , 0 , 0 , 0 , 0 , 0 , 0
-[01:22:58] , 0 , 0 ,); // Invalid tag value
-[01:22:58] // Incomplete value
+[01:22:58] , 0 , 0
+[01:22:58] ,); // Invalid tag value
+[01:22:58] // Incomplete value
+[01:22:58]
[01:22:58]
[01:22:58] let mut output = vec!();
[01:22:58] let mut buf = &input[..];(1.txt is "expected", 2.txt is "actual") You could run this test with ./x.py test src/test/run-pass/pretty --test-args enum-non-c-like-repr-int |
kennytm
added
S-waiting-on-author
RReverser
reviewed
Nov 8, 2017
| } | ||
|
|
||
| fn parse_my_enum<'a>(dest: &'a mut MyEnum, buf: &mut &[u8]) -> Result<(), ()> { | ||
| unsafe { |
There was a problem hiding this comment.
Would it be worth to limit this unsafe block only to transmute instruction? (doesn't look like anything else in the function needs it)
There was a problem hiding this comment.
All union accesses are unsafe, afaik
There was a problem hiding this comment.
@gankro Writing to a union doesn't require unsafe block, only reading does.
There was a problem hiding this comment.
I read the union at the start of the match, and if we're being honest, writing to the unions is Pretty Unsafe. It's just not unsafe for rules-lawyer reasons.
There was a problem hiding this comment.
@gankro Writing to unions just puts contents into Schrodinger's state - they might or might be not corrupt until you try to read them 😄
This verifies the layout specified in rfc rust-lang#2195
|
Tidy fixed. |
|
@bors r=alexcrichton |
|
📌 Commit e156f89 has been approved by |
kennytm
added
S-waiting-on-bors
bors
added a commit
that referenced
this pull request
Nov 9, 2017
Add reftest that checks the layout of repr(int) on non-c-like enums This verifies the first layout specified in rust-lang/rfcs#2195 The second (`repr(C)`) layout isn't included here because it doesn't actually exist today. However if/when that's implemented a second test could be fairly easily derived from this one. CC @eddyb
|
☀️ Test successful - status-appveyor, status-travis |
petertodd
added a commit
to petertodd/rust
that referenced
this pull request
Dec 8, 2018
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.
emilio
pushed a commit
to emilio/rust
that referenced
this pull request
Dec 16, 2018
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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This verifies the first layout specified in rust-lang/rfcs#2195
The second (
repr(C)) layout isn't included here because it doesn't actually exist today. However if/when that's implemented a second test could be fairly easily derived from this one.CC @eddyb