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

Map size_t to usize by default and check compatibility (fixes #1901, #1903) #2062

Closed
wants to merge 1 commit into from

Conversation

geofft
Copy link
Contributor

@geofft geofft commented Jun 3, 2021

This addresses the underlying issue identified in #1671, that size_t
(integer that can hold any object size) isn't guaranteed to match usize,
which is defined more like uintptr_t (integer that can hold any
pointer). However, on almost all platforms, this is true, and in fact
Rust already uses usize extensively in contexts where size_t would be
more appropriate, such as slice indexing. So, it's better for ergonomics
when interfacing with C code to map the C size_t type to usize. (See
also discussion in rust-lang/rust#65473 about how usize really should be
defined as size_t, not uintptr_t.)

The previous fix for #1671 removed the special case for size_t and
defaulted to binding it as a normal typedef. This change effectively
reverts that and goes back to mapping size_t to usize (and ssize_t to
isize), but also ensures that if size_t is emitted, the typedef'd type
of size_t in fact is compatible with usize (defined by checking that the
size and alignment match the target pointer width). For (hypothetical)
platforms where this is not true, or for compatibility with the default
behavior of bindgen between 0.53 and this commit, onwards, you can
disable this mapping with --no-size_t-is-usize.

…ng#1901, rust-lang#1903)

This addresses the underlying issue identified in rust-lang#1671, that size_t
(integer that can hold any object size) isn't guaranteed to match usize,
which is defined more like uintptr_t (integer that can hold any
pointer). However, on almost all platforms, this is true, and in fact
Rust already uses usize extensively in contexts where size_t would be
more appropriate, such as slice indexing. So, it's better for ergonomics
when interfacing with C code to map the C size_t type to usize. (See
also discussion in rust-lang/rust#65473 about how usize really should be
defined as size_t, not uintptr_t.)

The previous fix for rust-lang#1671 removed the special case for size_t and
defaulted to binding it as a normal typedef.  This change effectively
reverts that and goes back to mapping size_t to usize (and ssize_t to
isize), but also ensures that if size_t is emitted, the typedef'd type
of size_t in fact is compatible with usize (defined by checking that the
size and alignment match the target pointer width). For (hypothetical)
platforms where this is not true, or for compatibility with the default
behavior of bindgen between 0.53 and this commit, onwards, you can
disable this mapping with --no-size_t-is-usize.
@geofft
Copy link
Contributor Author

geofft commented Jun 3, 2021

Note that this is a breaking change and needs to be released in a semver-incompatible version (so like 0.59). It's backwards-compatible with existing users passing --size_t-is-usize, and backwards-compatible with users of bindgen < 0.53, but incompatible with users of >= 0.53 not passing that option.

Tests pass locally (except for #2061).

Is there a way to do a test that asserts that bindgen fails? The following header is expected to fail unless you pass --no-size_t-is-usize, so maybe we should add it as a test:

$ cat myheader.h 
typedef char size_t;
extern size_t a;
$ target/debug/bindgen myheader.h
thread 'main' panicked at 'Target platform requires --no-size_t-is-usize', src/codegen/mod.rs:830:25
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
$ target/debug/bindgen --no-size_t-is-usize myheader.h
/* automatically generated by rust-bindgen 0.58.1 */

pub type size_t = ::std::os::raw::c_char;
extern "C" {
    pub static mut a: size_t;
}

@thomcc
Copy link
Member

thomcc commented Aug 26, 2021

This a great PR, seems like a way better fix than the old one, which leads to a lot of nonportable pregenerated headers :(

@bors-servo
Copy link

☔ The latest upstream changes (presumably be6d07a) made this pull request unmergeable. Please resolve the merge conflicts.

@emilio
Copy link
Contributor

emilio commented Feb 18, 2022

This seems ok if tests pass, sorry for missing it. I can try to rebase and merge if you don't have the cycles to do it.

@pvdrz
Copy link
Contributor

pvdrz commented Sep 22, 2022

I took the liberty of rebasing this PR and opening #2278. The tests are passing so it should be ready for review I think

@pvdrz
Copy link
Contributor

pvdrz commented Sep 24, 2022

#2278 was just merged so I'm closing this. Thanks a lot @geofft!

@pvdrz pvdrz closed this Sep 24, 2022
nick-mobilecoin added a commit to mobilecoinfoundation/sgx that referenced this pull request Oct 18, 2022
Bindgen now supports mapping size_t to usize by default and doing a
build time check to ensure they are the same size.  See
rust-lang/rust-bindgen#2062.
nick-mobilecoin added a commit to mobilecoinfoundation/sgx that referenced this pull request Oct 24, 2022
Bindgen now supports mapping size_t to usize by default and doing a
build time check to ensure they are the same size.  See
rust-lang/rust-bindgen#2062.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants