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

alignas alignment is not respected on Linux 32-bit #917

Closed
jryans opened this issue Aug 16, 2017 · 12 comments
Closed

alignas alignment is not respected on Linux 32-bit #917

jryans opened this issue Aug 16, 2017 · 12 comments

Comments

@jryans
Copy link

jryans commented Aug 16, 2017

Input C/C++ Header

template<class T>
class Maybe
{
  alignas(T) unsigned char mStorage[sizeof(T)];
  char mIsSome;
};

struct Keyframe
{
  Maybe<double> mOffset;
  int           mComputedOffset = 1.0;
};

Bindgen Invocation

$ bindgen maybe.h -o maybe_bindings_32.rs --opaque-type Maybe -- -x c++ -std=c++14 -m32

Actual Results

/* automatically generated by rust-bindgen */

#[repr(C)]
#[derive(Debug, Copy, Clone)]
pub struct Maybe {
}
#[repr(C)]
#[derive(Debug, Copy)]
pub struct Keyframe {
    pub mOffset: [u64; 2usize],
    pub mComputedOffset: ::std::os::raw::c_int,
}
#[test]
fn bindgen_test_layout_Keyframe() {
    assert_eq!(::std::mem::size_of::<Keyframe>() , 24usize , concat ! (
               "Size of: " , stringify ! ( Keyframe ) ));
    assert_eq! (::std::mem::align_of::<Keyframe>() , 8usize , concat ! (
                "Alignment of " , stringify ! ( Keyframe ) ));
    assert_eq! (unsafe {
                & ( * ( 0 as * const Keyframe ) ) . mOffset as * const _ as
                usize } , 0usize , concat ! (
                "Alignment of field: " , stringify ! ( Keyframe ) , "::" ,
                stringify ! ( mOffset ) ));
    assert_eq! (unsafe {
                & ( * ( 0 as * const Keyframe ) ) . mComputedOffset as * const
                _ as usize } , 16usize , concat ! (
                "Alignment of field: " , stringify ! ( Keyframe ) , "::" ,
                stringify ! ( mComputedOffset ) ));
}
impl Clone for Keyframe {
    fn clone(&self) -> Self { *self }
}

This layout test fails on Linux 32-bit:

$ rustc --test --target i686-unknown-linux-gnu maybe_bindings_32.rs
$ ./maybe_bindings_32

running 1 test
test bindgen_test_layout_Keyframe ... FAILED

failures:

---- bindgen_test_layout_Keyframe stdout ----
	thread 'bindgen_test_layout_Keyframe' panicked at 'assertion failed: `(left == right)` (left: `20`, right: `24`): Size of: Keyframe', maybe_bindings_32.rs:15
note: Run with `RUST_BACKTRACE=1` for a backtrace.


failures:
    bindgen_test_layout_Keyframe

test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out

because Rust believes the alignment should be 4 bytes. alignas is trying to mandate 8 bytes, but Rust doesn't seem to agree.

Expected Results

This is currently blocking Stylo from working on Linux 32-bit, see https://bugzilla.mozilla.org/show_bug.cgi?id=1366050#c49.

@upsuper
Copy link
Contributor

upsuper commented Aug 16, 2017

Oh, u64 has 4byte alignment on linux32?

@jryans
Copy link
Author

jryans commented Aug 16, 2017

Oh, u64 has 4byte alignment on linux32?

No, I don't think that's quite it... What I know so far is that without alignas, gcc believes Keyframe is 4 byte aligned like Rust:

$ gcc -fdump-class-hierarchy -x c++ -std=c++14 -m32 maybe.cpp

    Class Maybe<double>
       size=9 align=1
       base size=9 base align=1
    Maybe<double> (0x0x7fb342efd7e0) 0
     
    Class Keyframe
       size=16 align=4
       base size=16 base align=4
    Keyframe (0x0x7fb342efd780) 0 

With alignas, we get:

Class Maybe<double>
   size=16 align=8
   base size=9 base align=8
Maybe<double> (0x0x7fdae45257e0) 0

Class Keyframe
   size=24 align=8
   base size=20 base align=8
Keyframe (0x0x7fdae4525780) 0

@upsuper
Copy link
Contributor

upsuper commented Aug 16, 2017

So this is really kind of Rust bug. Rust thinks alignment of u64 and f64 is 4 bytes, while gcc and clang both think it should be 8 bytes.

@upsuper
Copy link
Contributor

upsuper commented Aug 16, 2017

Filed rust-lang/rust#43899 for the issue.

@emilio
Copy link
Contributor

emilio commented Aug 16, 2017

Filed rust-lang/rust#43899 for the issue.

That issue has more context, this is, I think, not a bindgen bug. We could work around it using #[align(..)], but that's unstable as of right now.

@upsuper
Copy link
Contributor

upsuper commented Aug 16, 2017

So I think another workaround is to move around fields in Maybe. If the mStorage is moved to after mIsSome, we may have the same layout then, according to my early bug report to gcc.

@froydnj
Copy link
Contributor

froydnj commented Aug 16, 2017

We moved around the fields in Maybe so it would pack better for small T. :( Maybe we could make Maybe's layout dependent on the storage type in some way, specializing it for small T?

@upsuper
Copy link
Contributor

upsuper commented Aug 16, 2017

How can it pack better for small T? IIRC the result should not change, because the bool needs to be put at the aligned place of T as well.

I believe for double and uint64_t on 32bit binary, putting bool before would actually pack better. Any type in your mind which can pack better with bool comes later?

@froydnj
Copy link
Contributor

froydnj commented Aug 16, 2017

Ah, re-looking at bug 1287006, I see that part of the issue was that we had an over-aligned storage for T initially, which meant that small T took up way more space than they were supposed to, and the bool member took up way more space as well. Now that we're using alignas, I think you're correct that it shouldn't matter how we order the members. Tests for that would be nice, though. :)

@upsuper
Copy link
Contributor

upsuper commented Aug 16, 2017

Oh wait, that wouldn't help actually. The issue isn't about packing, actually, it is about alignas, which treat uint64_t and double as 8byte-aligned, while they are actually 4byte-aligned in structs.

So one solution is to test the alignment in a struct rather than as its own.

I'll submit a patch to Gecko side and see if that helps.

@upsuper
Copy link
Contributor

upsuper commented Aug 16, 2017

I closed the Rust bug, because it seems that u64 and f64 actually should have 4byte alignment on 32bit Linux when in struct, so Rust's behavior is correct.

The issue here is actually that the code use alignas which aligning the field to 8byte, but we don't even have a 8byte aligned type in Rust on 32bit Linux, so what we actually need is the #[align] feature.

In the mean time, I think we can change Maybe to use something like alignas(struct { T t; }) instead of alignas(T) so that we get 4byte alignment for them, which should workaround this issue.

@upsuper
Copy link
Contributor

upsuper commented Jan 31, 2018

I found that Rust stablized #[repr(align(x))] in rust-lang/rust#47006. I guess it is possible now to implement support for alignas(T) in bindgen.

emilio added a commit to emilio/rust-bindgen that referenced this issue Mar 10, 2018
bors-servo pushed a commit that referenced this issue Mar 10, 2018
codegen: Support repr(align)

Fixes #917.
emilio added a commit to emilio/rust-bindgen that referenced this issue Mar 13, 2018
bors-servo pushed a commit that referenced this issue Mar 13, 2018
codegen: Support repr(align)

Fixes #917.
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

5 participants