repr(i64) picks 32-bit discriminant on 32-bit platform

[bluss]

From the PR #25651 it looks like repr(i64) (and presumably repr(u64)) picks a 32-bit discriminant for the non C-like enum case.

Testcase was:

+#[repr(i64)]
+enum Ei64<T> {
+    _None,
+    _Some(T),
+}
+
+    assert_eq!(size_of::<Ei64<()>>(), 8);

Errored on platform auto-mac-32-opt with this:

thread '<main>' panicked at 'assertion failed:(left == right)(left:4, right:8)', /Users/rustbuild/src/rust-buildbot/slave/auto-mac-32-opt/build/src/test/run-pass/enum-discrim-manual-sizing-2.rs:89

[Aatch]

This is actually pretty simple to fix. Right now we select the type based on, mostly, the overall alignment of the type. We're also basically assuming that an alignment-sized integer is big enough to fit all discriminants in, what happens is that the alignment for a 64-bit integer on a 32-bit platform is 4 bytes, so we end up using a 4-byte integer for discriminant.

The easiest fix is to just skip the calculation for the discriminant type altogether when we have an repr attribute on the enum. A slightly better one might be to only allow the calculation to extend the int type to a larger size.

[Manishearth]

@Aatch could you post a link to the general area of code that needs tweaking?

[devonhollowood]

I'd be happy to work on this if no one else is doing so yet.

[Manishearth]

Go ahead! I'll help if necessary, though @Aatch knows more about this bug.

[Aatch]

The code you need to change is in trans/adt.rs. min_ity is the discriminant type chosen earlier, which will take account of the hint. To do the better fix, you want to add check to see if the alignment type is smaller than the min_ity type and use min_ity instead when setting the actual discriminant type, ity.

[devonhollowood]

I've been having some trouble getting the compiler to build with 32-bit emulation on my 64-bit machine. I thought ./configure --target=i686-apple-darwin or ./configure --host=i686-apple-darwin would work but neither has (it's also possible I'm running the tests wrong--I've been using make check -j4 and make check -j4 TESTNAME=src/test/run-pass/enum-discrim-manual-sizing.rs. As a result I haven't been able to reproduce the bug on my machine. So I submitted a pull request (#32010) containing tests, but no fix, which should hopefully not pass the CI. If the CI rejects it, then it means I have reproduced the bug successfully. Fingers crossed for failure!

[dotdash]

FWIW, I'm using --build=i686-unknown-linux-gnu to cross-compile. I'm doing that seldomly enough not to be sure what exactly is the difference between that, --host and --target.

[Manishearth]

Please ask for a try run when you need it 😄

IIRC Travis doesn't run all of the tests. Not sure.

[devonhollowood]

@Manishearth This might be a newb question, but what's a try run? Is that a bors-specific thing? If so, is there some documentation somewhere about the different bors options and when to use them?

[Manishearth]

It just runs all tests on the infrastructure without merging the PR. The commands are documented here, however you don't have permissions to run any of them. (but you can ask us to if you need something run)

[devonhollowood]

Gotcha. Thanks!

[devonhollowood]

@dotdash I successfully reproduced the issue on my machine by using --build instead of --target or --host. Thanks for the tip!

[devonhollowood]

@Aatch So I'm a little confused about this line on master. In the case covered by this issue, it seems to me like the discriminant should be bigger than the alignment, since the alignment will be 4 bytes but we want the discriminant to be 8 bytes. But this means that align_s % discr_size is 4, not 0, which fails the assert!. I'm a little nervous about changing that check though, since it's outside of the area of code that you linked, and I'm not totally sure why it's there in the first place. Should I change it? If so, what should I change it to? My thought was to maybe only do that check if align_s is greater than discr_size.

[Aatch]

@devonhollowood yeah, that'll need to be changed. I think the assertion is supposed to be ensuring that the alignment of the type is an integer multiple of the alignment of the discriminant, and is assuming that the alignment of an integer and the size of an integer are the same (which isn't true). Specifically, I think it's there to protect against a problem I encountered where I was accidentally increasing the size of the enum by using a too-large discriminant.

It should probably be let discr_align = machine::align_of_alloc(cx, discr_ty); and then assert_eq!(align_s % discr_align, 0);. I think align_of_alloc exists, if not take a look in the machine module and there should be an function that looks right.

[devonhollowood]

Okay, I'm making a lot of progress on this. I've got a question about the expected behavior, though. What should the output of this program be?

#[repr(u64)]
enum Eu64NonCLike<T> {
    _None,
    _Some(T),
}

fn main() {
    println!("{}", std::mem::size_of::<Eu64NonCLike<u8>>());
}

On 64-bit, I think the output should be 16, because the struct should have 8 bytes for the discriminant, 1 byte for the u8 in the _Some(T) variant, and then 7 bytes of padding to bring the total size up to the next multiple of the alignment (align = 8 bytes, so the next multiple of the alignment is 16).

On 32-bit, it seems like the same logic would imply that the output should be 12 (align = 4 bytes, so you only need an extra 3 bytes of padding to bring the total size up to 12 bytes, which is the next multiple of the alignment). This diverges the behavior for 32-bit and 64-bit builds, and I just wanted to double check that that was okay here.

[Aatch]

@devonhollowood the results of things like sizes is always going to be different on different platforms for this exact reason. It's no different to (u64, u8) having two different sizes on 32-bit vs 64-bit. So yes, it's okay.

[devonhollowood]

@Aatch Thanks for the clarification. I figured that was the case, but I wanted to double check.
@Manishearth I've pushed a fix for this issue. Assuming it passes the integration checks, do I need to do anything else, like ask for a review or anything? I'm still trying to get a hang of the rust contribution process.

[bluss]

Nice! Thanks for fixing this 🌟