Skip to content

make the core::raw struct representation defined #18316

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
Oct 28, 2014
Merged

make the core::raw struct representation defined #18316

merged 1 commit into from
Oct 28, 2014

Conversation

thestinger
Copy link
Contributor

Closes #18313

@sfackler
Copy link
Member

I don't see why this is necessary. These structures are guaranteed to match whatever the compiler decides to represent their related built-in types as. It's the compiler's responsibility to make sure that it does the right thing with the resulting layout of these types. What does that have to do with a C-ABI equivalent layout?

@thestinger
Copy link
Contributor Author

@sfackler: The layout is entirely implementation defined. The compiler can use a different representation for struct Foo(u32, u64) and struct Bar(u32, u64). It was done with the intent of allowing features like struct randomization.

@thestinger
Copy link
Contributor Author

The standard library is incorrect whenever it relies on the compiler to order the fields in a certain way. It defines these built-in types as having a specific order / layout which is what #![repr(C)] is for.

@sfackler
Copy link
Member

I am aware of implementation defined layout. It's true in general that libraries can't depend on the result of undefined layout. However, these types exist to specify the otherwise unspecified layout of builtin types. They already directly rely on implementation details of the compiler, and will continue to do so even if they're tagged with #[repr(C)].

@cgaebel
Copy link
Contributor

cgaebel commented Oct 25, 2014

@sfackler It's also for people like me who want the representation of Vec and Slice to be the same, to allow for branch-free conversion from one to the other. See: #18303

@thestinger
Copy link
Contributor Author

@cgaebel: That's not why I'm marking them this way.

@cgaebel
Copy link
Contributor

cgaebel commented Oct 25, 2014

Then why are you? As long as the layouts match what the compiler generates, it shouldn't be a problem to re-order.

@thestinger
Copy link
Contributor Author

@sfackler: They need to be marked with #[repr(C)] because otherwise the compiler will reorder the fields when a feature like struct randomization is used. The implementation currently uses the C representation for all structs but code relying on that has UB and needs to be fixed. It is blocking the implementation of features taking advantage of the implementation-defined representation.

@thestinger
Copy link
Contributor Author

@sfackler: The fact that it's implementation defined does not mean that it will represent struct Foo(u32, u64) and struct Bar(u32, u64) in the same implementation-defined way. It means it is free to choose a different representation for both of those structures. The ability to randomize struct layout was part of the rationale laid out in the RFC.

@thestinger
Copy link
Contributor Author

@cgaebel: It will not match what the compiler generates when struct randomization is used. It has a chance of matching the representation. Any code using these has undefined behaviour and is blocking the implementation of the features laid out in the RFC.

@sfackler
Copy link
Member

If randomized struct layout is implemented, is there any reason we wouldn't want to randomize layout of [u8] etc? Should these be tagged #[repr(C)] or should they be made lang items?

@thestinger
Copy link
Contributor Author

@sfackler: It's think we would eventually want to randomize the layout of built-in types. However, I think the layout of those types is considered well-defined right now. There's a lot of low-level Rust code in the wild depending on the order and I think it would require an RFC amendment extending it to certain built-in types rather than just struct.

@thestinger
Copy link
Contributor Author

(this is just intended to be make it well-defined today and unblock implementing randomization)

@sfackler
Copy link
Member

Cool, SGTM.

alexcrichton added a commit to alexcrichton/rust that referenced this pull request Oct 27, 2014
@bors bors merged commit 3942ab9 into rust-lang:master Oct 28, 2014
@thestinger thestinger deleted the raw branch January 28, 2015 03:10
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.

core::raw type definitions are incorrect due to implementation-defined struct representations
4 participants