non-c-like repr(u8) enum miscompilation?

[Gankra]

Kinda vague since we just ran into it and it's late for me, but we're seeing a behaviour change marking

enum Foo { A(f32), B }

as #[repr(u8)] and otherwise not changing the program:

servo/webrender#2673 (comment)

Making it #[repr(C, u8)] works correctly.

All 3 layouts of the enum should be identical, so this is especially surprising that literally anything is changing. We're only seeing the issue in release builds. I can help reduce/bisect/debug this further tomorrow.

[glennw]

In this specific case, the expression space != GlyphRasterSpace::Screen is being evaluated incorrectly.

[oli-obk]

Which nightly is servo on?

i can't reproduce on playpen: https://play.rust-lang.org/?gist=c18f53f98345751eb4afe3ac59ffc73e&version=stable

[glennw]

This occurred locally on my machine, which is using:

rustc 1.25.0 (84203cac6 2018-03-25)

I was also unable to reproduce it in a small test case. There appears to be other factors that cause it. My guess is that it's related to how that enum is laid out when it is a field within a structure, but that's just a guess.

[eddyb]

Does such an enum require being nested in another enum to exhibit the problem?
It could be something about the outer enum using a niche in the inner one, but getting it wrong.

[Gankra]

I'm actually unable to reproduce glenn's results; all tests appear to pass running script/headless.py reftest with rustc 1.25.0 (Ubuntu 16.04).

[Gankra]

Was able to reproduce a similar issue when doing C++ FFI with this feature. Now have a reproduction:

[Gankra]

https://play.rust-lang.org/?gist=5ac04c96bd654b01567d4eed362ddb6a&version=stable&mode=release

// Should Print:
// local!! 1
// local!! 2
//
// Does Print:
// local!! 1
//

#![allow(dead_code)]

#[repr(C, u8)] enum Space { Local(f32), Screen }

#[repr(C)]
struct SpaceRepr {
    tag: Tag,
    padding: [u8; 3],
    payload: Payloads,
}

#[repr(C)]
union Payloads {
    local: f32,
}

#[repr(u8)]
enum Tag {
    Local,
    Screen,
}

fn handle_space(space: &Space) {
    if let Space::Local(val) = *space {
        println!("local!! {}", val);
    }
}

fn main() {
    // We should be allowed to store any garbage in padding, right?
    let local1 = SpaceRepr { tag: Tag::Local, padding: [0, 0, 0], payload: Payloads { local: 1.0 }};
    let local2 = SpaceRepr { tag: Tag::Local, padding: [1, 2, 3], payload: Payloads { local: 2.0 }};
    
    let local1_ref: &Space = unsafe {::std::mem::transmute(&local1)};
    let local2_ref: &Space = unsafe {::std::mem::transmute(&local2)};
    
    handle_space(local1_ref);
    handle_space(local2_ref);
}

[Gankra]

The issue is that the code for accessing the tag thinks the tag is a full u32, but any raw repr code thinks that the tag is only a byte. As such it tosses garbage in the padding, and the code freaks out.

We were seeing this in pure rust because it was passing through my Magic Serde Code that uses raw reprs to deserialize enums in place.

We can patch around this in WR by making the tag the "right" size, but this should clearly be fixed in rustc.

[nikomatsakis]

cc @eddyb

[eddyb]

@gankro Ah, oops, #[repr(int)] (and #[repr(C, int)]?) enums still have the tag growth logic.
That is, we grow the tag to the minimum start of any variant, which may have a larger alignment.
This should be sound within Rust, but it is not what the RFC specifies as the exact layout.

[eddyb]

Solution: make this be min_ity (read ity as "tag" - #49938) if repr.c() || repr.int.is_some():

rust/src/librustc/ty/layout.rs

Lines 950 to 951 in 4745092

	// Use the initial field alignment
	let mut ity = Integer::for_abi_align(dl, start_align).unwrap_or(min_ity);