Skip to content

Remove align cargo feature #1242

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.

Sign up for GitHub

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

Jump to bottom
Closed
gnzlbg opened this issue Feb 6, 2019 · 7 comments
Closed

Remove align cargo feature #1242

gnzlbg opened this issue Feb 6, 2019 · 7 comments

Comments

@gnzlbg
Copy link
Contributor

gnzlbg commented Feb 6, 2019

I'd like to remove the align cargo feature (or make it a dummy), and automatically use repr(align) if the Rust version is new enough (newer than Rust 1.25.0).

In particular, I'd like for libc to not expose the C APIs that use repr(align) if repr(align) is not available anymore. This would be a breaking change, but using these APIs without the proper alignment is undefined behavior anyways (unless one is super super super careful).

Users that want to use these APIs and are currently on Rust versions older than Rust 1.25.0 can either stick to an older libc version or upgrade to at least Rust 1.25.0.
@gnzlbg
Copy link
Contributor Author

gnzlbg commented Feb 6, 2019 •
edited
Loading

Well, this is worse than I thought. Using repr(align(N)) anywhere (even behind a #[cfg_attr]) requires at least Rust 1.25.0 . That is, any target that wants to optionally use repr(align) needs to bump the minimum required Rust version to 1.25.0 (~1 year old).

cc @alexcrichton - a couple of targets already do this, maybe we should bump the minimum supported libc version to Rust 1.25.0 and call it a day. It might well be that libc works with older Rust versions in some targets, but I don't think we should guarantee that. In any case, gating align behind a cargo feature doesn't do anything, since Rust 1.25 is required for the cargo feature to actually work.

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Feb 6, 2019 •
edited
Loading

Alternatively, we could move all code that uses repr(align(N)) into separate modules, and put the module declaration behind the feature gate. For example,

#[cfg(libc_align)]
mod align;

// align.rs
#[repr(align(N))] struct Foo;

@Mark-Simulacrum
Copy link
Member

The modules would/could be completely transparent to downstream users, right? Via re-exports?

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Feb 6, 2019

Yes, I am already working on that, but it is a pain due to how many targets are already depending on it.

@josephlr
Copy link
Contributor

josephlr commented Feb 6, 2019

@gnzlbg I think bumping libc's supported version to 1.25 is not unreasonable. Of the most popular crates, lazy_static has 1.24 as the min version, bitflags has 1.20, rand has 1.22. However, serde, syn, and log all have much lower required versions (~1.15)

gnzlbg added a commit to gnzlbg/libc that referenced this issue Feb 7, 2019
@gnzlbg
Fix build on all platforms
b5a98ca 
This PR fixes the build on all platforms and all Rust version down to the
minimum Rust version supported by libc: Rust 1.13.0.

The `build.rs` is extended with logic to detect the newer Rust features used by
`libc` since Rust 1.13.0:

* Rust 1.19.0: `untagged_unions`. APIs using untagged unions are gated on
  `cfg(libc_unions)` and not available on older Rust versions.

* Rust 1.25.0: `repr(align)`. Because `repr(align)` cannot be parsed by older
  Rust versions, all uses of `repr(align)` are split into `align.rs` and
  `no_align.rs` modules, which are gated on the `cfg(libc_align)` at the top
  level. These modules sometimes contain macros that are expanded at the top
  level to avoid privacy issues (`pub(crate)` is not available in older Rust
  versions). Closes rust-lang#1242 .

* Rust : `const` `mem::size_of`. These uses are worked around with hardcoded
  constants on older Rust versions.

Also, `repr(packed)` structs cannot automatically `derive()` some traits like
`Debug`. These have been moved into `s_no_extra_traits!` and the lint of missing
`Debug` implementations on public items is silenced for these. We can manually
implement the `extra_traits` for these in a follow up PR. This is tracked in

Since `f64::to_bits` is not available in older Rust versions, its usage
has been replaced with a `transmute` to an `u64` which is what that method
does under the hood.

Closes rust-lang#1232 .
gnzlbg added a commit to gnzlbg/libc that referenced this issue Feb 7, 2019
@gnzlbg
Fix build on all platforms
bdf4e73 
This PR fixes the build on all platforms and all Rust version down to the
minimum Rust version supported by libc: Rust 1.13.0.

The `build.rs` is extended with logic to detect the newer Rust features used by
`libc` since Rust 1.13.0:

* Rust 1.19.0: `untagged_unions`. APIs using untagged unions are gated on
  `cfg(libc_unions)` and not available on older Rust versions.

* Rust 1.25.0: `repr(align)`. Because `repr(align)` cannot be parsed by older
  Rust versions, all uses of `repr(align)` are split into `align.rs` and
  `no_align.rs` modules, which are gated on the `cfg(libc_align)` at the top
  level. These modules sometimes contain macros that are expanded at the top
  level to avoid privacy issues (`pub(crate)` is not available in older Rust
  versions). Closes rust-lang#1242 .

* Rust : `const` `mem::size_of`. These uses are worked around with hardcoded
  constants on older Rust versions.

Also, `repr(packed)` structs cannot automatically `derive()` some traits like
`Debug`. These have been moved into `s_no_extra_traits!` and the lint of missing
`Debug` implementations on public items is silenced for these. We can manually
implement the `extra_traits` for these in a follow up PR. This is tracked in

Since `f64::to_bits` is not available in older Rust versions, its usage
has been replaced with a `transmute` to an `u64` which is what that method
does under the hood.

Closes rust-lang#1232 .
@alexcrichton
Copy link
Member

1.25.0 is a bit too recent I think for libc, but we could detect this in the build script and auto-enable the feature I believe?

gnzlbg added a commit to gnzlbg/libc that referenced this issue Feb 7, 2019
@gnzlbg
Fix build on all platforms
a406fe6 
This PR fixes the build on all platforms and all Rust version down to the
minimum Rust version supported by libc: Rust 1.13.0.

The `build.rs` is extended with logic to detect the newer Rust features used by
`libc` since Rust 1.13.0:

* Rust 1.19.0: `untagged_unions`. APIs using untagged unions are gated on
  `cfg(libc_unions)` and not available on older Rust versions.

* Rust 1.25.0: `repr(align)`. Because `repr(align)` cannot be parsed by older
  Rust versions, all uses of `repr(align)` are split into `align.rs` and
  `no_align.rs` modules, which are gated on the `cfg(libc_align)` at the top
  level. These modules sometimes contain macros that are expanded at the top
  level to avoid privacy issues (`pub(crate)` is not available in older Rust
  versions). Closes rust-lang#1242 .

* Rust : `const` `mem::size_of`. These uses are worked around with hardcoded
  constants on older Rust versions.

Also, `repr(packed)` structs cannot automatically `derive()` some traits like
`Debug`. These have been moved into `s_no_extra_traits!` and the lint of missing
`Debug` implementations on public items is silenced for these. We can manually
implement the `extra_traits` for these in a follow up PR. This is tracked
in rust-lang#1243. Also, `extra_traits` does not enable `align` manually anymore.

Since `f64::to_bits` is not available in older Rust versions, its usage
has been replaced with a `transmute` to an `u64` which is what that method
does under the hood.

Closes rust-lang#1232 .
gnzlbg added a commit to gnzlbg/libc that referenced this issue Feb 7, 2019
@gnzlbg
Fix build on all platforms
072403d 
This PR fixes the build on all platforms and all Rust version down to the
minimum Rust version supported by libc: Rust 1.13.0.

The `build.rs` is extended with logic to detect the newer Rust features used by
`libc` since Rust 1.13.0:

* Rust 1.19.0: `untagged_unions`. APIs using untagged unions are gated on
  `cfg(libc_unions)` and not available on older Rust versions.

* Rust 1.25.0: `repr(align)`. Because `repr(align)` cannot be parsed by older
  Rust versions, all uses of `repr(align)` are split into `align.rs` and
  `no_align.rs` modules, which are gated on the `cfg(libc_align)` at the top
  level. These modules sometimes contain macros that are expanded at the top
  level to avoid privacy issues (`pub(crate)` is not available in older Rust
  versions). Closes rust-lang#1242 .

* Rust : `const` `mem::size_of`. These uses are worked around with hardcoded
  constants on older Rust versions.

Also, `repr(packed)` structs cannot automatically `derive()` some traits like
`Debug`. These have been moved into `s_no_extra_traits!` and the lint of missing
`Debug` implementations on public items is silenced for these. We can manually
implement the `extra_traits` for these in a follow up PR. This is tracked
in rust-lang#1243. Also, `extra_traits` does not enable `align` manually anymore.

Since `f64::to_bits` is not available in older Rust versions, its usage
has been replaced with a `transmute` to an `u64` which is what that method
does under the hood.

Closes rust-lang#1232 .
gnzlbg added a commit to gnzlbg/libc that referenced this issue Feb 7, 2019
@gnzlbg
Fix build on all platforms
c934678 
This PR fixes the build on all platforms and all Rust version down to the
minimum Rust version supported by libc: Rust 1.13.0.

The `build.rs` is extended with logic to detect the newer Rust features used by
`libc` since Rust 1.13.0:

* Rust 1.19.0: `untagged_unions`. APIs using untagged unions are gated on
  `cfg(libc_unions)` and not available on older Rust versions.

* Rust 1.25.0: `repr(align)`. Because `repr(align)` cannot be parsed by older
  Rust versions, all uses of `repr(align)` are split into `align.rs` and
  `no_align.rs` modules, which are gated on the `cfg(libc_align)` at the top
  level. These modules sometimes contain macros that are expanded at the top
  level to avoid privacy issues (`pub(crate)` is not available in older Rust
  versions). Closes rust-lang#1242 .

* Rust : `const` `mem::size_of`. These uses are worked around with hardcoded
  constants on older Rust versions.

Also, `repr(packed)` structs cannot automatically `derive()` some traits like
`Debug`. These have been moved into `s_no_extra_traits!` and the lint of missing
`Debug` implementations on public items is silenced for these. We can manually
implement the `extra_traits` for these in a follow up PR. This is tracked
in rust-lang#1243. Also, `extra_traits` does not enable `align` manually anymore.

Since `f64::to_bits` is not available in older Rust versions, its usage
has been replaced with a `transmute` to an `u64` which is what that method
does under the hood.

Closes rust-lang#1232 .
@bors bors closed this as completed in a17a91c Feb 8, 2019
@gnzlbg
Copy link
Contributor Author

gnzlbg commented Feb 8, 2019 •
edited
Loading

FWIW libc now auto-detect whether repr(align) is available. The align feature still works as it worked before.

I briefly thought about deprecating it, but I don't think we can ever remove it without breaking projects that want to be able to work properly on a very wide range of libc versions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@alexcrichton @gnzlbg @Mark-Simulacrum @josephlr