Skip to content

Document #[repr(u8)] enum to enable LLVM optimizations #8

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

Merged
merged 1 commit into from
May 26, 2017

Conversation

geofft
Copy link
Contributor

@geofft geofft commented Apr 4, 2017

See rust-lang/rust#27303 and rust-lang/libc#48 for discussion of the
optimization, or just try compiling unsafe {free(malloc(16))} with various
Rust definitions of void * in the FFI binding.

@Gankra
Copy link
Contributor

Gankra commented Apr 4, 2017

This is a bit unclear to me as written. I would maybe zoom out from LLVM a bit and focus on the ABI implications of C-like enums: it will be treated as an integer, and not a struct-containing-an-integer.

CC @mystor

@geofft
Copy link
Contributor Author

geofft commented Apr 4, 2017

Oh, hm, is a #[repr(u8)] enum Foo {Bar(())} represented as a struct, and thus passed on the stack in ABIs where structs are unconditionally on the stack? Then yeah, that's worth documenting too! Let me test that and then rework this paragraph, thanks.

@mystor
Copy link

mystor commented Apr 4, 2017

It might also be good to note that controlling the #[repr] of the enum doesn't change the set of legal values, and that unlike C/C++ enums, it is undefined behaviour to have a #[repr(C)] enum Foo { A = 0, B = 1 } with the value 10.

@geofft
Copy link
Contributor Author

geofft commented Apr 18, 2017

OK, I've rewritten it to take the above comments into account.

This has drifted a fair bit from my original subject; the specific optimization LLVM does with malloc and free (and particular, that LLVM considers void * to be a * u8) is more than I can fit in a passing comment. However, the comments on std::os::raw::c_void and libc::c_void, plus the paragraph here about the enum needing to be C-like, are probably clear enough.

@strega-nil
Copy link

strega-nil commented Apr 30, 2017

If a C-like enum is missing a repr(u*) or repr(C), it is treated like a one-member struct for ABI compatibility purposes.

This seems like a silly guarantee to make, since they should not be treated as a one-member struct for ABI purposes.

However, a non-C-like enum will always be treated like a struct (even if the only data is () or PhantomData). Adding repr simply forces the discriminant to be represented in memory by the specified integer size. This does have the effect of inhibiting certain optimizations like the null-pointer optimization.

Again, why are we guaranteeing this? The null pointer optimization inhibition, sure, but otherwise this seems like it should mostly be deleted.

@steveklabnik
Copy link
Member

I have no idea what's right here...

@mystor
Copy link

mystor commented May 15, 2017

I generally agree with @ubsan That any properties of repr(rust) enums, with the potential exception of Option<&T> and Option<Box<T>>, should not be documented. Any internal implementation details of them should remain internal.

@Gankra
Copy link
Contributor

Gankra commented May 16, 2017

Agreed, the only property of repr(rust) that's guaranteed is that OptionLike<&T> has the same abi as &T and *const T.

@steveklabnik
Copy link
Member

Great. @geofft care to make these changes?

@geofft
Copy link
Contributor Author

geofft commented May 19, 2017

Oh yeah, good points about not documenting the exact properties of repr(rust). Thanks!

I've updated the pull request. This doesn't seem like the right area to discuss OptionLike, so I just removed all discussion of repr(rust) representation, but I can add a bit about that if that's helpful.

@steveklabnik steveklabnik merged commit b4ada32 into rust-lang:master May 26, 2017
@steveklabnik
Copy link
Member

Let's :shipit: . Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants