rust-bindgen icon indicating copy to clipboard operation
rust-bindgen copied to clipboard

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

Open geofft opened this issue 4 years ago • 4 comments

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.

geofft avatar Jun 03 '21 16:06 geofft

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;
}

geofft avatar Jun 03 '21 16:06 geofft

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

thomcc avatar Aug 26 '21 18:08 thomcc

:umbrella: The latest upstream changes (presumably be6d07ac2c00ada005e5512b19ff6c6a34cfe985) made this pull request unmergeable. Please resolve the merge conflicts.

bors-servo avatar Jan 29 '22 10:01 bors-servo

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.

emilio avatar Feb 18 '22 18:02 emilio

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 avatar Sep 22 '22 20:09 pvdrz

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

pvdrz avatar Sep 24 '22 02:09 pvdrz