Skip to content

Missing memory alignment Rust generated code #1291

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

Closed
beltegeuse opened this issue Mar 31, 2018 · 4 comments
Closed

Missing memory alignment Rust generated code #1291

beltegeuse opened this issue Mar 31, 2018 · 4 comments

Comments

@beltegeuse
Copy link

beltegeuse commented Mar 31, 2018

Hi guys,
I developing a Rust wrapper for the Embree library. This library heavily relies on processor vector operation (SSE, AVX) to provide high performance. For now, I use explicit FFI declaration to interface with Embree. However, I was planning to use rust-bindgen to generate this interface automatically.

However, when I use bindgen, the code does not align correctly compared to what it is specified in the C++ header. Moreover, the test code generated by bindgen does not include a test to check if the current rust code have the proper alignment.

Input C/C++ Header

Extracted code from Embree (v2)

#ifdef _WIN32
#  define RTCORE_ALIGN(...) __declspec(align(__VA_ARGS__))
#else
#  define RTCORE_ALIGN(...) __attribute__((aligned(__VA_ARGS__)))
#endif

// ....

struct RTCORE_ALIGN(16)  RTCRay
{
  /* ray data */
public:
  float org[3];      //!< Ray origin
  float align0;
  
  float dir[3];      //!< Ray direction
  float align1;
  
  float tnear;       //!< Start of ray segment
  float tfar;        //!< End of ray segment (set to hit distance)

  float time;        //!< Time of this ray for motion blur
  unsigned mask;        //!< Used to mask out objects during traversal
  
  /* hit data */
public:
  float Ng[3];       //!< Unnormalized geometry normal
  float align2;
  
  float u;           //!< Barycentric u coordinate of hit
  float v;           //!< Barycentric v coordinate of hit

  unsigned geomID;        //!< geometry ID
  unsigned primID;        //!< primitive ID
  unsigned instID;        //!< instance ID
};

Bindgen Invocation

let bindings = bindgen::Builder::default()
        .clang_args(["-x", "c++", "-std=c++11"].iter())
        .enable_cxx_namespaces()
        .blacklist_type("max_align_t")
        .header("wrapper.h")
        .generate()
       .expect("Unable to generate bindings");

Actual Results

The code generated by rust-bindgen

 #[repr(C)]
    #[derive(Debug, Copy, Clone)]
    pub struct RTCRay { 
        pub org: [f32; 3usize], 
        pub align0: f32, 
        pub dir: [f32; 3usize], 
        pub align1: f32, 
        pub tnear: f32, 
        pub tfar: f32, 
        pub time: f32, 
        pub mask: ::std::os::raw::c_uint, 
        pub Ng: [f32; 3usize], 
        pub align2: f32, 
        pub u: f32, 
        pub v: f32, 
        pub geomID: ::std::os::raw::c_uint, 
        pub primID: ::std::os::raw::c_uint, 
        pub instID: ::std::os::raw::c_uint, 
        pub __bindgen_padding_0: [u32; 3usize] 
    }

    #[test]
    fn bindgen_test_layout_RTCRay() {
        assert_eq!(::std::mem::size_of::<RTCRay>(), 96usize, concat!( "Size of: " , stringify ! ( RTCRay ) ));
        assert_eq!(unsafe { &(*(::std::ptr::null::<RTCRay>())).org as *const _ as usize }, 0usize, concat!( "Offset of field: " , stringify ! ( RTCRay ) , "::" , stringify ! ( org ) ));
        assert_eq!(unsafe { &(*(::std::ptr::null::<RTCRay>())).align0 as *const _ as usize }, 12usize, concat!( "Offset of field: " , stringify ! ( RTCRay ) , "::" , stringify ! ( align0 ) ));
        assert_eq!(unsafe { &(*(::std::ptr::null::<RTCRay>())).dir as *const _ as usize }, 16usize, concat!( "Offset of field: " , stringify ! ( RTCRay ) , "::" , stringify ! ( dir ) ));
        assert_eq!(unsafe { &(*(::std::ptr::null::<RTCRay>())).align1 as *const _ as usize }, 28usize, concat!( "Offset of field: " , stringify ! ( RTCRay ) , "::" , stringify ! ( align1 ) ));
        assert_eq!(unsafe { &(*(::std::ptr::null::<RTCRay>())).tnear as *const _ as usize }, 32usize, concat!( "Offset of field: " , stringify ! ( RTCRay ) , "::" , stringify ! ( tnear ) ));
        assert_eq!(unsafe { &(*(::std::ptr::null::<RTCRay>())).tfar as *const _ as usize }, 36usize, concat!( "Offset of field: " , stringify ! ( RTCRay ) , "::" , stringify ! ( tfar ) ));
        assert_eq!(unsafe { &(*(::std::ptr::null::<RTCRay>())).time as *const _ as usize }, 40usize, concat!( "Offset of field: " , stringify ! ( RTCRay ) , "::" , stringify ! ( time ) ));
        assert_eq!(unsafe { &(*(::std::ptr::null::<RTCRay>())).mask as *const _ as usize }, 44usize, concat!( "Offset of field: " , stringify ! ( RTCRay ) , "::" , stringify ! ( mask ) ));
        assert_eq!(unsafe { &(*(::std::ptr::null::<RTCRay>())).Ng as *const _ as usize }, 48usize, concat!( "Offset of field: " , stringify ! ( RTCRay ) , "::" , stringify ! ( Ng ) ));
        assert_eq!(unsafe { &(*(::std::ptr::null::<RTCRay>())).align2 as *const _ as usize }, 60usize, concat!( "Offset of field: " , stringify ! ( RTCRay ) , "::" , stringify ! ( align2 ) ));
        assert_eq!(unsafe { &(*(::std::ptr::null::<RTCRay>())).u as *const _ as usize }, 64usize, concat!( "Offset of field: " , stringify ! ( RTCRay ) , "::" , stringify ! ( u ) ));
        assert_eq!(unsafe { &(*(::std::ptr::null::<RTCRay>())).v as *const _ as usize }, 68usize, concat!( "Offset of field: " , stringify ! ( RTCRay ) , "::" , stringify ! ( v ) ));
        assert_eq!(unsafe { &(*(::std::ptr::null::<RTCRay>())).geomID as *const _ as usize }, 72usize, concat!( "Offset of field: " , stringify ! ( RTCRay ) , "::" , stringify ! ( geomID ) ));
        assert_eq!(unsafe { &(*(::std::ptr::null::<RTCRay>())).primID as *const _ as usize }, 76usize, concat!( "Offset of field: " , stringify ! ( RTCRay ) , "::" , stringify ! ( primID ) ));
        assert_eq!(unsafe { &(*(::std::ptr::null::<RTCRay>())).instID as *const _ as usize }, 80usize, concat!( "Offset of field: " , stringify ! ( RTCRay ) , "::" , stringify ! ( instID ) ));
    }

The test that I have inside my lib.rs:

#[test]
fn test_ray_align() {
    assert_eq!(::std::mem::align_of::<root::RTCRay>(), 16usize);
}

this test failed and give me this output:

---- test_ray_align stdout ----
	thread 'test_ray_align' panicked at 'assertion failed: `(left == right)`
  left: `4`,
 right: `16`', src/lib.rs:22:5
note: Run with `RUST_BACKTRACE=1` for a backtrace.


failures:
    test_ray_align

Expected Results

From bindgen call, I was expecting two things:

  • #[repr(C,align(16))] (compared to #[repr(C)])
  • adding: assert_eq!(::std::mem::align_of::<RTCRay>(), 16usize); inside the test suite.

Does rust-bindgen have a way to generate this test and have explicit align automatically?
Thanks guys for your awsome tool. Hope that I can use it inside my project :)

Version

rustup 1.11.0, stable rust 1.25.0 (84203cac6 2018-03-25), version bindgen: "0.35.0"
My embree wrapper: https://github.com/beltegeuse/embree-rs (branch: bindgen)

@emilio
Copy link
Contributor

emilio commented Mar 31, 2018

Have you tried with rust_target(RustTarget::Stable_1_25)? That should add the necessary repr(align) bits.

Good point about the test though, that's definitely an oversight on my side when implementing #1271. We currently skip the test here:

https://github.com/rust-lang-nursery/rust-bindgen/blob/fb069e9391ebf40ec9ebef7c0ac2e50f5ead5644/src/codegen/mod.rs#L1780

Needs a feature check, should be trivial to fix. Want to submit a PR?

@highfive
Copy link

Hi! If you have any questions regarding this issue, feel free to make a comment here, or ask it in the #servo channel in IRC.

If you intend to work on this issue, then add @highfive: assign me to your comment, and I'll assign this to you. 😄

@beltegeuse
Copy link
Author

beltegeuse commented Mar 31, 2018

Thanks for the fast awnser.
I have added rust_target(RustTarget::Stable_1_25). But it does not change the code generated by rust-bindgen.

This is the updated call to the bindgen:

let bindings = bindgen::Builder::default()
        .clang_args(["-x", "c++", "-std=c++11"].iter())
        .enable_cxx_namespaces()
        .blacklist_type("max_align_t")
        .rust_target(bindgen::RustTarget::Stable_1_25)
        .header("wrapper.h")
        .generate()
       .expect("Unable to generate bindings");

I have tried to see why the "align" attribute is not added to the generated code. But I did not found the reason yet. If I have time, I will investigate.

@emilio
Copy link
Contributor

emilio commented Mar 31, 2018

That looks... Also like an oversight on my side. In particular, for alignments greater than a word we couldn't generate proper alignment until repr(align).

emilio added a commit to emilio/rust-bindgen that referenced this issue Mar 31, 2018
Plus fix the check that avoids us generating explicit alignment fields for
structs aligned to more than pointer-size.

Fixes rust-lang#1291
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants