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

Layout tests fail on the 32 bit architecture i686 #1823

Closed
jbaublitz opened this issue Jul 1, 2020 · 5 comments
Closed

Layout tests fail on the 32 bit architecture i686 #1823

jbaublitz opened this issue Jul 1, 2020 · 5 comments

Comments

@jbaublitz
Copy link
Contributor

Currently I'm trying to package a crate for Fedora that uses bindgen. I see that I can disable memory layout checks but wanted see if this is a problem with the code generation or my invocation of bindgen. One noteworthy aspect of this is that all of the 64 bit architectures pass all tests so this seems to be specifically related to the alignment test on 32 bit architectures.

Input C/C++ Header

#include <libcryptsetup.h>

Bindgen Invocation

I've tried both:

n generate_bindings(safe_free_is_needed: bool) {
    let builder = bindgen::Builder::default().header("header.h")
        .size_t_is_usize(false);
    let builder_with_safe_free = if safe_free_is_needed {
        builder.header("safe_free.h")
    } else {
        builder
    };
    let bindings = builder_with_safe_free
        .generate()
        .expect("Unable to generate bindings");

    let out_path = PathBuf::from(env::var("OUT_DIR").unwrap());
    bindings
        .write_to_file(out_path.join("bindings.rs"))
        .expect("Couldn't write bindings");
}

and:

fn generate_bindings(safe_free_is_needed: bool) {
    let builder = bindgen::Builder::default().header("header.h")
        .size_t_is_usize(true);
    let builder_with_safe_free = if safe_free_is_needed {
        builder.header("safe_free.h")
    } else {
        builder
    };
    let bindings = builder_with_safe_free
        .generate()
        .expect("Unable to generate bindings");

    let out_path = PathBuf::from(env::var("OUT_DIR").unwrap());
    bindings
        .write_to_file(out_path.join("bindings.rs"))
        .expect("Couldn't write bindings");
}

The only difference between these two is whether .size_t_is_usize() is passed true in one and false in the other. Both fail with the same results. For this specific build, I can guarantee safe_free_is_needed is false due to the package version of libcryptsetup available on the build system.

Actual Results

The full build log is available here but the failure of note is when running the tests:

   Compiling libcryptsetup-rs-sys v0.1.3 (/builddir/build/BUILD/libcryptsetup-rs-sys-0.1.3)
     Running `/usr/bin/rustc --crate-name libcryptsetup_rs_sys --edition=2018 src/lib.rs --error-format=json --json=diagnostic-rendered-ansi --emit=dep-info,link -C opt-level=3 --test -C metadata=736abf458239cc88 -C extra-filename=-736abf458239cc88 --out-dir /builddir/build/BUILD/libcryptsetup-rs-sys-0.1.3/target/release/deps -L dependency=/builddir/build/BUILD/libcryptsetup-rs-sys-0.1.3/target/release/deps -Copt-level=3 -Cdebuginfo=2 -Clink-arg=-Wl,-z,relro,-z,now -Ccodegen-units=1 --cap-lints=warn -L native=/usr/lib -l cryptsetup`
    Finished release [optimized] target(s) in 1.38s
     Running `/builddir/build/BUILD/libcryptsetup-rs-sys-0.1.3/target/release/deps/libcryptsetup_rs_sys-736abf458239cc88`
running 14 tests
test bindgen_test_layout___fsid_t ... ok
test bindgen_test_layout_crypt_params_loopaes ... ok
test bindgen_test_layout_crypt_active_device ... ok
test bindgen_test_layout_crypt_params_luks1 ... ok
test bindgen_test_layout_crypt_params_integrity ... ok
test bindgen_test_layout_crypt_params_luks2 ... ok
test bindgen_test_layout_crypt_params_plain ... ok
test bindgen_test_layout_crypt_params_reencrypt ... ok
test bindgen_test_layout_crypt_params_tcrypt ... ok
test bindgen_test_layout_crypt_params_verity ... ok
test bindgen_test_layout_crypt_pbkdf_type ... ok
test bindgen_test_layout_crypt_token_handler ... ok
test bindgen_test_layout_crypt_token_params_luks2_keyring ... ok
test bindgen_test_layout_max_align_t ... FAILED
failures:
---- bindgen_test_layout_max_align_t stdout ----
thread 'bindgen_test_layout_max_align_t' panicked at 'assertion failed: `(left == right)`
  left: `16`,
 right: `24`: Size of: max_align_t', /builddir/build/BUILD/libcryptsetup-rs-sys-0.1.3/target/release/build/libcryptsetup-rs-sys-0dbdb6c27edad9c7/out/bindings.rs:3:7976
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
failures:
    bindgen_test_layout_max_align_t
test result: FAILED. 13 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out

Expected Results

I would expect the generated bindings on i686 to pass all tests. I'm currently in the process of getting access to a 32 bit machine to try to reproduce the test failure and inspect the autogenerated bindings and I will report back here. I was able to inspect 64 bit bindings and it appears the max_align_t test uses a u128 in the structure that it's using to test alignment so I'm interested to see if 32 bit does the same as I'm not sure what Rust support for u128s looks like on 32 bit architectures.

@jbaublitz
Copy link
Contributor Author

I tracked down the autogenerated test on 32 bit:

#[repr(C)]
#[repr(align(8))]
#[derive(Debug, Copy, Clone)]
pub struct max_align_t {
    pub __clang_max_align_nonce1: ::std::os::raw::c_longlong,
    pub __clang_max_align_nonce2: f64,
}
#[test]
fn bindgen_test_layout_max_align_t() {
    assert_eq!(
        ::std::mem::size_of::<max_align_t>(),
        24usize,
        concat!("Size of: ", stringify!(max_align_t))
    );
    assert_eq!(
        ::std::mem::align_of::<max_align_t>(),
        8usize,
        concat!("Alignment of ", stringify!(max_align_t))
    );
    assert_eq!(
        unsafe {
            &(*(::std::ptr::null::<max_align_t>())).__clang_max_align_nonce1 as *const _ as usize
        },
        0usize,
        concat!(
            "Offset of field: ",
            stringify!(max_align_t),
            "::",
            stringify!(__clang_max_align_nonce1)
        )
    );
    assert_eq!(
        unsafe {
            &(*(::std::ptr::null::<max_align_t>())).__clang_max_align_nonce2 as *const _ as usize
        },
        8usize,
        concat!(
            "Offset of field: ",
            stringify!(max_align_t),
            "::",
            stringify!(__clang_max_align_nonce2)
        )
    );
}

@emilio
Copy link
Contributor

emilio commented Jul 2, 2020

What is the definition of max_align_t in C in your machine?

@emilio
Copy link
Contributor

emilio commented Jul 2, 2020

It seems you can fix this by using blacklist_type("max_align_t"), assuming the crate doesn't use it. Though I want to check whether this is a known problem (we've hit issues in the past with max_align_t due to it using long double, and the ABI of that not being properly defined, see #1549).

@jbaublitz
Copy link
Contributor Author

Thanks for the response! Here is the definition of max_align_t. For now I'll disable the check as I'm not directly referencing max_align_t anywhere in my bindings.

typedef struct {
  long long __max_align_ll __attribute__((__aligned__(__alignof__(long long))));
  long double __max_align_ld __attribute__((__aligned__(__alignof__(long double))));
  /* _Float128 is defined as a basic type, so max_align_t must be
     sufficiently aligned for it.  This code must work in C++, so we
     use __float128 here; that is only available on some
     architectures, but only on i386 is extra alignment needed for
     __float128.  */
#ifdef __i386__
  __float128 __max_align_f128 __attribute__((__aligned__(__alignof(__float128))));
#endif
} max_align_t;

One note is that this is i386 as opposed to i686. I can't get my hands on Fedora 29 (the last 32 bit release) for i686 at the moment so this is the best I can do. However the same error message appears in tests on i386 and i686.

@emilio
Copy link
Contributor

emilio commented Jul 6, 2020

Yeah, I think this is expected at the moment, and blocked on float128 / u128 / etc being compatible with C's definition I think (rust-lang/rust#54341)... Otherwise making the type opaque is the right workaround. I don't think this is particularly fixable in bindgen otherwise.

@emilio emilio closed this as completed Jul 6, 2020
eager-signal added a commit to signalapp/boring that referenced this issue Oct 18, 2023
This adds the matrix `--target` to relevant `cargo` commands. Some
targets are now “check-only”, which confirms cross-compilation succeeds,
without doing a full test run, because running these tests may require
emulation.

musl and MinGW have been dropped, because musl has no standard C++
setup, and MinGW isn’t a target for us.

All this requires some adjustments to boring-sys:

- Blocklist max_align_t in bindgen
   - rust-lang/rust-bindgen#1823)
= Don't check for MSVC with target_env
   - x86_64-pc-windows-gnu is identified as `target_env = "msvc"` too,
but doesn't use the Visual Studio CMake generator.
eager-signal added a commit to signalapp/boring that referenced this issue Oct 18, 2023
This adds the matrix `--target` to relevant `cargo` commands. Some
targets are now “check-only”, which confirms cross-compilation succeeds,
without doing a full test run, because running these tests may require
emulation.

musl and MinGW have been dropped, because musl has no standard C++
setup, and MinGW isn’t a target for us.

All this requires some adjustments to boring-sys:

- Blocklist max_align_t in bindgen
   - rust-lang/rust-bindgen#1823)
- Don't check for MSVC with target_env
   - x86_64-pc-windows-gnu is identified as `target_env = "msvc"` too,
but doesn't use the Visual Studio CMake generator.
eager-signal added a commit to signalapp/boring that referenced this issue Oct 18, 2023
This adds the matrix `--target` to relevant `cargo` commands. Some
targets are now “check-only”, which confirms cross-compilation succeeds,
without doing a full test run, because running these tests may require
emulation.

musl and MinGW have been dropped, because musl has no standard C++
setup, and MinGW isn’t a target for us.

All this requires some adjustments to boring-sys:

- Blocklist max_align_t in bindgen
   - rust-lang/rust-bindgen#1823
- Don't check for MSVC with target_env
   - x86_64-pc-windows-gnu is identified as `target_env = "msvc"` too,
but doesn't use the Visual Studio CMake generator.
eager-signal added a commit to signalapp/boring that referenced this issue Oct 18, 2023
This adds the matrix `--target` to relevant `cargo` commands. Some
targets are now “check-only”, which confirms cross-compilation succeeds,
without doing a full test run, because running these tests may require
emulation.

musl and MinGW have been dropped, because musl has no standard C++
setup, and MinGW isn’t a target for us.

All this requires some adjustments to boring-sys:

- Blocklist max_align_t in bindgen
   - rust-lang/rust-bindgen#1823
- Don't check for MSVC with target_env
   - x86_64-pc-windows-gnu is identified as `target_env = "msvc"` too,
but doesn't use the Visual Studio CMake generator.
eager-signal added a commit to signalapp/boring that referenced this issue Oct 18, 2023
This adds the matrix `--target` to relevant `cargo` commands. Some
targets are now “check-only”, which confirms cross-compilation succeeds,
without doing a full test run, because running these tests may require
emulation.

musl and MinGW have been dropped, because musl has no standard C++
setup, and MinGW isn’t a target for us.

All this requires some adjustments to boring-sys:

- Blocklist max_align_t in bindgen
   - rust-lang/rust-bindgen#1823
- Don't check for MSVC with target_env
   - x86_64-pc-windows-gnu is identified as `target_env = "msvc"` too,
but doesn't use the Visual Studio CMake generator.
eager-signal added a commit to signalapp/boring that referenced this issue Oct 18, 2023
This adds the matrix `--target` to relevant `cargo` commands. Some
targets are now “check-only”, which confirms cross-compilation succeeds,
without doing a full test run, because running these tests may require
emulation.

musl and MinGW have been dropped, because musl has no standard C++
setup, and MinGW isn’t a target for us.

All this requires some adjustments to boring-sys:

- Blocklist max_align_t in bindgen
   - rust-lang/rust-bindgen#1823
- Don't check for MSVC with target_env
   - x86_64-pc-windows-gnu is identified as `target_env = "msvc"` too,
but doesn't use the Visual Studio CMake generator.
nox pushed a commit to cloudflare/boring that referenced this issue Dec 14, 2023
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