Skip to content
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

Various cleanups. #36

Merged
merged 1 commit into from
Dec 6, 2017
Merged

Various cleanups. #36

merged 1 commit into from
Dec 6, 2017

Conversation

ahmedcharles
Copy link
Contributor

Add debug implementations.
Mark everything as repr(C) and repr(packed), just to be sure.

Add debug implementations.
Mark everything as repr(C) and repr(packed), just to be sure.
@robert-w-gries
Copy link
Contributor

👍 LGTM


On an semi-related note, I'd appreciate help in understanding what's going on with repr(packed).

This note from the repr documentation is intimidating:

In particular, most architectures strongly prefer values to be aligned. This may mean the unaligned loads are penalized (x86), or even fault (some ARM chips). For simple cases like directly loading or storing a packed field, the compiler might be able to paper over alignment issues with shifts and masks. However if you take a reference to a packed field, it's unlikely that the compiler will be able to emit code to avoid an unaligned load.

As of Rust 1.0 this can cause undefined behavior.
repr(packed) is not to be used lightly. Unless you have extreme requirements, this should not be used.

Since our case requires liberal use of repr(C, packed), is there a way to minimize penalties and undefined behavior? Perhaps through more asserts or wrappers?

Let me know if there's something to this, and I'll open a separate issue.

@phil-opp
Copy link
Member

phil-opp commented Dec 6, 2017

As I understand it, repr(C) should suffice as long as the fields are aligned. With repr(packed) everything should still work without undefined behavior as long as we avoid references to unaligned fields.

rust-lang/rust#44884 was merged about a week ago, so this should make things safer (but I don't know details either).

@robert-w-gries robert-w-gries merged commit aeeb282 into rust-osdev:master Dec 6, 2017
@ahmedcharles ahmedcharles deleted the cleanup branch December 7, 2017 07:50
@ahmedcharles
Copy link
Contributor Author

I wish Rust had static assert, which would allow asserting the size of a struct to ensure there is no padding. I'll probably look into that and switch to using that as a way to verify that what you see is what you get, since using unsafe to access fields is somewhat silly in many cases.

@phil-opp
Copy link
Member

phil-opp commented Dec 7, 2017

We could add a unit test that checks the struct sizes.

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.

3 participants