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

Add alignment feature and use #[repr(align(x))] #1044

Merged
merged 5 commits into from
Jul 30, 2018

Conversation

faern
Copy link
Contributor

@faern faern commented Jul 20, 2018

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.

@rust-highfive
Copy link

r? @alexcrichton

(rust_highfive has picked a reviewer for you, use r? to override)

@faern faern force-pushed the modern-alignment branch from 26eb8e5 to 555bf67 Compare July 20, 2018 09:40
@faern
Copy link
Contributor Author

faern commented Jul 20, 2018

Experimented with running an extra iteration of cargo test with the feature activated if the build job is using Rust >=1.25.

@faern
Copy link
Contributor Author

faern commented Jul 20, 2018

If you like this approach and find it OK to merge, ping me and I'll add the same things to the remaining __align structs.

@alexcrichton
Copy link
Member

Looks good to me! I'd be ok using this for all remaining structs as well

@faern faern force-pushed the modern-alignment branch from aa28472 to 6e6c0a5 Compare July 20, 2018 22:52
@alexcrichton
Copy link
Member

I think some tests may still be failing?

@faern
Copy link
Contributor Author

faern commented Jul 24, 2018

Yes. Still some things to solve. I have been busy the last few days, so not entirely sure when I will be done with this. But will ping when I feel I have a finished solution :)

@faern faern force-pushed the modern-alignment branch from 6e6c0a5 to 3665c88 Compare July 27, 2018 09:39
@faern
Copy link
Contributor Author

faern commented Jul 27, 2018

Now I got a bit further. But are there maybe nicer ways to write what I have already done?

Also, I struggle with figuring out the test failure on i686-unknown-linux-gnu where the test says bad pthread_cond_t align: rust: 8 (0x8) != c 4 (0x4). The old alignment there is __align: [::c_longlong; 0],, so alignment should be c_longlong which should be 64 bit on all platforms(?), and align(8) works on all other 32 bit platforms.

@alexcrichton
Copy link
Member

Ah I think u64 has a 4 byte alignment on 32-bit platforms (or at least on Linux), so that may need a conditional alignment attribute

@faern faern force-pushed the modern-alignment branch from 3665c88 to b594bf2 Compare July 29, 2018 18:12
@faern
Copy link
Contributor Author

faern commented Jul 29, 2018

Finally had time to finish the rest of the structs and their corresponding constants. Some open questions:

  • Do you think I should document this feature in some way? Either by comment just in Cargo.toml, or a section in the README?
  • Is the extra test, no_private_align.rs, an acceptable/reasonable approach to make sure future changes don't accidentally make in6_addr not possible to create instances of without unsafe?
  • Is the use pthread_mutex_t; in order to change ::pthread_mutex_t into pthread_mutex_t acceptable? I did not find a way to make the macro accept anything other than an ident for the $t2 argument.

@alexcrichton
Copy link
Member

Thanks! All the changes look good to me yeah, although I'd be fine without the no_private_align.rs tests as well, we have no more reason to add fields that don't exist!

For the feature, yeah, mind adding a blurb to the README about it?

@faern faern force-pushed the modern-alignment branch from b594bf2 to e167a73 Compare July 30, 2018 14:58
@faern
Copy link
Contributor Author

faern commented Jul 30, 2018

Done. I removed the commit that added the custom test and added a commit with an extra paragraph in the README.

@alexcrichton
Copy link
Member

@bors: r+

Thanks!

@bors
Copy link
Contributor

bors commented Jul 30, 2018

📌 Commit e167a73 has been approved by alexcrichton

@bors
Copy link
Contributor

bors commented Jul 30, 2018

⌛ Testing commit e167a73 with merge 8565755...

bors added a commit that referenced this pull request 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.
@bors
Copy link
Contributor

bors commented Jul 30, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing 8565755 to master...

cramertj added a commit to cramertj/rust that referenced this pull request Aug 3, 2018
…TimNN

Make IpvXAddr::new const fns and the well known addresses associated constants

Implements/fixes rust-lang#44582

I just got a PR towards libc (rust-lang/libc#1044) merged. With the new feature added in that PR it is now possible to create `in6_addr` instances as consts. This enables us to finally make the constructors of the IP structs const fns and to make the localhost/unspecified addresses associated constants, as agreed in the above mentioned tracking issue.

I also added a BROADCAST constant. Personally this is the well known address I tend to need the most often.
bors added a commit to rust-lang/rust that referenced this pull request Aug 4, 2018
Make IpvXAddr::new const fns and the well known addresses associated constants

Implements/fixes #44582

I just got a PR towards libc (rust-lang/libc#1044) merged. With the new feature added in that PR it is now possible to create `in6_addr` instances as consts. This enables us to finally make the constructors of the IP structs const fns and to make the localhost/unspecified addresses associated constants, as agreed in the above mentioned tracking issue.

I also added a BROADCAST constant. Personally this is the well known address I tend to need the most often.
@faern faern deleted the modern-alignment branch August 4, 2018 20:47
@faern faern mentioned this pull request Aug 5, 2018
bors added a commit that referenced this pull request Aug 6, 2018
Bump version to 0.2.43

Would be nice to have the new align feature from #1044 available for general use. But mostly I want this released since I have problems using the align feature for a PR on libstd, and I suspect it's somehow because I try to use an unpublished libc (rust-lang/rust#52872).
bors added a commit to rust-lang/rust that referenced this pull request Aug 8, 2018
Make IpvXAddr::new const fns and the well known addresses associated constants

Implements/fixes #44582

I just got a PR towards libc (rust-lang/libc#1044) merged. With the new feature added in that PR it is now possible to create `in6_addr` instances as consts. This enables us to finally make the constructors of the IP structs const fns and to make the localhost/unspecified addresses associated constants, as agreed in the above mentioned tracking issue.

I also added a BROADCAST constant. Personally this is the well known address I tend to need the most often.
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.

4 participants