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

Generated tests are not compatible across architectures #1213

Open
kornelski opened this issue Jan 5, 2018 · 8 comments
Open

Generated tests are not compatible across architectures #1213

kornelski opened this issue Jan 5, 2018 · 8 comments

Comments

@kornelski
Copy link
Contributor

kornelski commented Jan 5, 2018

Input C/C++ Header

struct Foo {
   int bar;
};

Bindgen Invocation

Run on x86_64-apple-darwin:

$ bindgen input.h

Test with: cargo test --target=i686-apple-darwin

Actual Results

fn bindgen_test_layout_Foo ( ) …

Expected Results

The tests are valid only on the architecture bindgen was run on. It would be great if bindgen was able to make tests portable, e.g.:

#[cfg(target_pointer_width = "64")]
fn bindgen_test_layout_Foo ( )#[cfg(target_pointer_width = "32")]
fn bindgen_test_layout_Foo ( )

Currently this isn't easy to do even manually, because tests are interleaved with other code, so combining of 32- and 64-bit files requires advanced regex-fu.

@emilio
Copy link
Contributor

emilio commented Jan 6, 2018

It's pretty hard to do that, it not only depends on architecture but also on compiler flags and what not...

@emilio
Copy link
Contributor

emilio commented Jan 6, 2018

We support not generating tests of course, but that's not quite a solution...

@fitzgen
Copy link
Member

fitzgen commented Jan 8, 2018

Best thing to do is either generate bindings for the current target in build.rs or generate bindings for each target you plan to support and check them into your version control.

There is a lot more gotchas than just pointer width, and the biggest is #ifdefs that change definitions based on target arch or os etc.

@kornelski
Copy link
Contributor Author

kornelski commented Jan 8, 2018

IIRC all of the platform-specific ifdefs that I had to deal with were only used to get reliable/cross-platform fixed-size types (in codebases that think depending on stdint.h is too radical).

For libraries with stable ABI I prefer to generate bindings once and clean them up by hand, because it also allows me to translate feature-specific ifdefs into Cargo features.

@kornelski
Copy link
Contributor Author

kornelski commented Jan 8, 2018

I think the minimal fix here would be to add #[cfg(target_pointer_width = "…")] (or target_arch?) to the tests, so that at least they don't give false negatives on other architectures.

Sorting of output to put all tests at the end would be helpful, too.

@Dushistov
Copy link
Contributor

@kornelski

For libraries with stable ABI I prefer to generate bindings once
I think the minimal fix here would be to add target_arch
to the tests

But what about generated bindings by itself?
For example for:

#include <stdint.h>

union Foo {
        void *p;
        uint32_t a;
};

bindgen generate different code for different platforms:

#[repr(C)]
#[derive(Copy, Clone)]
pub union Foo {
    pub p: *mut ::std::os::raw::c_void,
    pub a: u32,
    _bindgen_union_align: u32,
}

#[repr(C)]
#[derive(Copy, Clone)]
pub union Foo {
    pub p: *mut ::std::os::raw::c_void,
    pub a: u32,
    _bindgen_union_align: u64,
}

and failed tests indicates that something wrong, and you should generate bindings in target directory, if tests don't fail with different --target=, then you may assume that all good, while really all bad.

May be leave current behaviour as is, because of it prevents errors and bugs?

@kornelski
Copy link
Contributor Author

In your example I actually don't know why bindgen does it this way, because ::std::mem::align_of::<::Foo>() is also correct without bindgen's extra field (4 on i686, 8 on x86_64).

I'm working under assumption that #[repr(C)] struct used with ::std::os::raw::c_* types (and raw pointers) is correct for C without bindgen's hacks if C doesn't use extra pragmas for packing structs, custom align or C++ features. Fortunately these assumptions happen to be true very often for small libraries with C ABI.

@emilio
Copy link
Contributor

emilio commented Feb 3, 2018

Yeah we could avoid generating the alignment for the union in some cases, I'm happy to accept patches for that as long as there are proper tests.

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

4 participants