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

_bindgen_union_align produces wrong alignment when compiling for 32-bit #1983

Closed
Gnurou opened this issue Feb 6, 2021 · 4 comments
Closed

Comments

@Gnurou
Copy link

Gnurou commented Feb 6, 2021

I am using bindgen to generate Rust equivalents of C structures that are part of the Linux kernel ABI. These structures are defined in public headers and (unfortunately) have different a size and layout on 32-bit and 64-bit targets.

This works fine with bindgen for the most part, except when unions are involved. Consider the following struct:

struct v4l2_buffer {
        /* snip */
	union {
		__u32           offset;
		unsigned long   userptr;
		struct v4l2_plane *planes;
		__s32		fd;
	} m;
        /* snip */
};

bindgen generates the following code for it:

#[repr(C)]
#[derive(Copy, Clone)]
pub struct v4l2_buffer {
    /* snip */
    pub m: v4l2_buffer__bindgen_ty_1,
    /* snip */
}

[repr(C)]
#[derive(Copy, Clone)]
pub union v4l2_buffer__bindgen_ty_1 {
    pub offset: __u32,
    pub userptr: ::std::os::raw::c_ulong,
    pub planes: *mut v4l2_plane,
    pub fd: __s32,
    _bindgen_union_align: u64,
}

Note the _bindgen_union_align member. While the layout is perfectly correct for a 64-bit target (because the pointer and c_ulong are the same size as u64), when building for 32-bit the size of the union is also 8 bytes, while it should be just 4. This results in the kernel rejecting the ioctls because the passed argument is not of the expected size.

If I just remove all the _bindgen_union_align from the generated bindings, the structures are now usable on both 64-bit and 32-bit, without any issue. The tests also keep passing on 64-bit (but not on 32-bit, due to issue #1213).

I see several ways to solve this:

  1. Stop generating the _bindgen_union_align. While the C representation should just do the right thing (and in my case it does), I suspect they are here for a reason though.
  2. Detect when the size of the union is dependent on architecture, and use a portable type (like c_ulong maybe?) for these cases.
  3. Maybe add a command-line option to skip generating these members for people who know they need it?

I am not sure what the correct way would be, but happy to gather some thoughts and maybe come with a patch for it?

@Gnurou Gnurou changed the title _bindgen_union_align produce wrong alignment when compiling for 32-bit _bindgen_union_align produces wrong alignment when compiling for 32-bit Feb 7, 2021
@emilio
Copy link
Contributor

emilio commented Feb 7, 2021

Are you passing the right target to bindgen?

For basically your test-case (with the typedefs removed):

struct v4l2_buffer {
	union {
		unsigned offset;
		unsigned long userptr;
		struct v4l2_plane *planes;
		int fd;
	} m;
};

I get the expected:

#[repr(C)]
#[derive(Copy, Clone)]
pub union v4l2_buffer__bindgen_ty_1 {
    pub offset: ::std::os::raw::c_uint,
    pub userptr: ::std::os::raw::c_ulong,
    pub planes: *mut v4l2_plane,
    pub fd: ::std::os::raw::c_int,
    _bindgen_union_align: u32,
}

If I run it with the right target:

$ bindgen t.h -- --target=i686-pc-linux-gnu

@emilio
Copy link
Contributor

emilio commented Feb 7, 2021

In this case the alignment is redundant, so we should be able to detect this kind of case and remove it like we do for structs, though.

@emilio
Copy link
Contributor

emilio commented Feb 7, 2021

#1984 should improve this significantly.

@Gnurou
Copy link
Author

Gnurou commented Feb 8, 2021

Wow, I was completely oblivious of the fact you could pass --target= to bindgen. By doing so to generate one binding per supported architecture and selecting the right one using #[cfg(target_pointer_width = "xx")] I get the expected behavior and the tests now pass in all cases.

#1984 is even better in that I could probably use the bindings for all cases with it. I guess we can close this issue, thanks a lot for the answer and quick fix!

@Gnurou Gnurou closed this as completed Feb 8, 2021
Gnurou added a commit to Gnurou/v4l2r that referenced this issue Feb 11, 2021
Due to rust-lang/rust-bindgen#1983 the
bindings generated on a 64-bit machine won't work well with 32-bit
targets. Work around this issue by generating 2 sets of bindings and
selecting the correct one with #[cfg(target_pointer_width)].
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