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

Replace __align fields with #[repr(align(x))] #1042

Closed
faern opened this issue Jul 15, 2018 · 5 comments
Closed

Replace __align fields with #[repr(align(x))] #1042

faern opened this issue Jul 15, 2018 · 5 comments

Comments

@faern
Copy link
Contributor

faern commented Jul 15, 2018

This has been briefly touched in #963 and #58 but this issue intend to discuss it from a different angle.

When I grep in the libc repository I find 123 instances of __align:. The main problem is that instantiating a struct that uses this way to achieve alignment requires:

let mut foo: libc::some_struct = unsafe { mem::uninitialized() };
foo.field = my_value;

Which has many downsides, mainly

  • needing unsafe
  • has to be assigned to a mutable variable
  • and can't be done in a constant expression.

#repr(align(x))] was introduced in Rust 1.25, which at the time of writing this is only two releases behind current stable. Thus I understand we probably can't bump the required rustc version for libc that high yet. But I could not find any information about how backwards compatible libc intends to be, going forward. Since #972 bumped the required rustc version from 1.0 to 1.13 without much fanfare or breaking version bumping etc I guess there is some unofficial point where even libc don't care about your old compiler anymore.

Switching to #repr(align(x))] would, among removing the need for a lot of unsafe, help in making more things constant expressions. For example this issue where we need to create an in6_addr in an associated constant: rust-lang/rust#44582

Would it count as a breaking change to remove the __align fields? It would change the struct, but not the public part of it, so I guess not. Any code using the mem::uninitialized() dance should still work exactly the same I think.

I'm playing with the thought to somehow make this work in Edition 2018. But I guess compiling a crate with edition = "2018" that has edition 2015 dependencies only compiles libc once, even if crates from both editions depend on it? If it is compiled separately for each edition then something like the following could work (given that there will be such a cfg value to conditionally compile on to begin with):

#[cfg_attr(not(edition = "2015"), repr(align(4)))]
pub struct in6_addr {
    pub s6_addr: [u8; 16],
    #[cfg(edition = "2015")]
    __align: [u32; 0],
}

Another issue that discusses using newer rustc features in order to improve the libc implementation is #1020. Both implementing that issue and this one would raise the rustc version requirement. Although, the other issue would require rustc 1.19, while this is a lot more extreme as it requires 1.25.

@alexcrichton
Copy link
Member

Yes unfortunately the version policy for libc isn't strictly documented, but I agree that 1.25 is a bit too recent. Once we decide to upgrade to 1.25 it should be fine to remove all the private __align fields and use the attribute instead.

@faern
Copy link
Contributor Author

faern commented Jul 17, 2018

Idea: Since it does not break the API, and using the mem::uninitialized() way of creating the structs should work equally well for the new way of doing alignment. I think one way of making it work today is similar to my brainstorming about editions, but do it with a feature that is off by default:

#[cfg_attr(feature = "align", repr(align(4)))]
pub struct in6_addr {
    pub s6_addr: [u8; 16],
    #[cfg(not(feature = "align"))]
    __align: [u32; 0],
}

Since no crate depending on libc today does activate this, everything will continue to be exactly as now. But as soon as one crate in the dependency chain of some build does add the feature it becomes active for everyone. However, the new layout should not break any existing crates, it should just increase the compiler version required.

So any crate that activates the feature would be equivalent to that same crate doing anything else that makes it require Rust 1.25 or higher. Such as nested import groups or #[repr(align(x))] in that crate itself.

Downsides with this approach is that I have not proven the change to be 100% non breaking, even though I think it is. And it also "costs" two #[cfg(..)]s per struct that needs alignment.

@alexcrichton
Copy link
Member

Tha seems like a good solution to me!

@faern
Copy link
Contributor Author

faern commented Jul 17, 2018

Nice. I'll play around with that and get back with my findings.

bors added a commit that referenced this issue Jul 30, 2018
Add alignment feature and use #[repr(align(x))]

Trying to solve #1042.

Here I introduce the discussed feature that will allow going from struct alignment with a private `__align` field to using `#[repr(align(x))]`. However, I have not implemented it for all structs that require alignment yet, only `in6_addr`. This because I did not want to spend too much time before we have discussed and solved the remaining questions regarding this.

One thing to discuss is testing. I have so far not done anything to the CI scripts. So currently they will still test the crate only with the `align` feature disabled. Thus they will make sure the `__align` fields are still correct. But no automatic tests make sure everything is correct when the `align` feature is turned on. What do we want to do about that? Can we insert another `cargo test` with `--features align` to make all the Travis jobs run the test suite twice, or will that slow things down too much?

I have tried using this version of libc in rustc and the standard library. And successfully changed `Ipv6Addr::new` to not use any `unsafe` and to become a `const fn`. Whether or not we want that is out of scope for this PR, but my point was that the changes introduced with this PR allow much more flexible usage of the libc structs that have alignment.
@faern
Copy link
Contributor Author

faern commented Aug 7, 2018

Support for getting rid of __align and making libc use #[repr(align(x))] was merged in #1044. Just activate the "align" feature of the crate in order to use it.

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

No branches or pull requests

2 participants