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

Incomplete/zero-sized arrays don't affect struct alignment. #684

Closed
cholcombe973 opened this issue May 2, 2017 · 3 comments
Closed

Incomplete/zero-sized arrays don't affect struct alignment. #684

cholcombe973 opened this issue May 2, 2017 · 3 comments

Comments

@cholcombe973
Copy link

I'm new to the build.rs bindings generation. Last time I generated bindings I used the old method of having it print to stdout and then manually fixing the errors. I'm seeing an alignment error that that I don't know how to solve just yet:


Input C/C++ Header

struct dm_deps {
        uint32_t count;
        uint32_t filler;
        uint64_t device[0];
};

Bindgen Invokation

    let bindings = bindgen::Builder::default()
        .no_unstable_rust()
        .header("wrapper.h")
        .generate()
        .expect("Unable to generate bindings");

Actual Results

---- bindgen_test_layout_dm_deps stdout ----
	thread 'bindgen_test_layout_dm_deps' panicked at 'assertion failed: `(left == right)` (left: `4`, right: `8`): Alignment of dm_deps', /mnt/sdb/repos/lvm-sys/target/debug/build/lvm-sys-7e5868edf14c1ccb/out/bindings.rs:3319
stack backtrace:
   0: std::sys::imp::backtrace::tracing::imp::unwind_backtrace
             at /checkout/src/libstd/sys/unix/backtrace/tracing/gcc_s.rs:49
   1: std::sys_common::backtrace::_print
             at /checkout/src/libstd/sys_common/backtrace.rs:71
   2: std::panicking::default_hook::{{closure}}
             at /checkout/src/libstd/sys_common/backtrace.rs:60
             at /checkout/src/libstd/panicking.rs:355
   3: std::panicking::default_hook
             at /checkout/src/libstd/panicking.rs:365
   4: std::panicking::rust_panic_with_hook
             at /checkout/src/libstd/panicking.rs:549
   5: std::panicking::begin_panic
             at /checkout/src/libstd/panicking.rs:511
   6: std::panicking::begin_panic_fmt
             at /checkout/src/libstd/panicking.rs:495
   7: lvm_sys::bindgen_test_layout_dm_deps
             at ./target/debug/build/lvm-sys-7e5868edf14c1ccb/out/bindings.rs:3319
   8: <F as test::FnBox<T>>::call_box
             at /checkout/src/libtest/lib.rs:1368
             at /checkout/src/libtest/lib.rs:140
   9: std::panicking::try::do_call
             at /checkout/src/libtest/lib.rs:1314
             at /checkout/src/libstd/panic.rs:296
             at /checkout/src/libstd/panicking.rs:454
  10: __rust_maybe_catch_panic
             at /checkout/src/libpanic_unwind/lib.rs:98
  11: std::panicking::try::do_call
             at /checkout/src/libstd/panicking.rs:433
             at /checkout/src/libstd/panic.rs:361
             at /checkout/src/libtest/lib.rs:1313
             at /checkout/src/libstd/panic.rs:296
             at /checkout/src/libstd/panicking.rs:454
  12: __rust_maybe_catch_panic
             at /checkout/src/libpanic_unwind/lib.rs:98
  13: <F as alloc::boxed::FnBox<A>>::call_box
             at /checkout/src/libstd/panicking.rs:433
             at /checkout/src/libstd/panic.rs:361
             at /checkout/src/libstd/thread/mod.rs:360
             at /checkout/src/liballoc/boxed.rs:640
  14: std::sys::imp::thread::Thread::new::thread_start
             at /checkout/src/liballoc/boxed.rs:650
             at /checkout/src/libstd/sys_common/thread.rs:21
             at /checkout/src/libstd/sys/unix/thread.rs:84
  15: start_thread
  16: clone

Expected Results

RUST_LOG=bindgen Output

I don't have any additional details from RUST_LOG=bindgen Here's the generated rust binding code:
#[repr(C)]
#[derive(Default)]
pub struct __IncompleteArrayField<T>(::std::marker::PhantomData<T>);
impl <T> __IncompleteArrayField<T> {
    #[inline]
    pub fn new() -> Self {
        __IncompleteArrayField(::std::marker::PhantomData)
    }
    #[inline]
    pub unsafe fn as_ptr(&self) -> *const T { ::std::mem::transmute(self) }
    #[inline]
    pub unsafe fn as_mut_ptr(&mut self) -> *mut T {
        ::std::mem::transmute(self)
    }
    #[inline]
    pub unsafe fn as_slice(&self, len: usize) -> &[T] {
        ::std::slice::from_raw_parts(self.as_ptr(), len)
    }
    #[inline]
    pub unsafe fn as_mut_slice(&mut self, len: usize) -> &mut [T] {
        ::std::slice::from_raw_parts_mut(self.as_mut_ptr(), len)
    }
}
impl <T> ::std::fmt::Debug for __IncompleteArrayField<T> {
    fn fmt(&self, fmt: &mut ::std::fmt::Formatter) -> ::std::fmt::Result {
        fmt.write_str("__IncompleteArrayField")
    }
}
impl <T> ::std::clone::Clone for __IncompleteArrayField<T> {
    #[inline]
    fn clone(&self) -> Self { Self::new() }
}
impl <T> ::std::marker::Copy for __IncompleteArrayField<T> { }
#[repr(C)]
#[derive(Debug, Copy)]
pub struct dm_deps {
    pub count: u32,
    pub filler: u32,
    pub device: __IncompleteArrayField<u64>,
}
#[test]
fn bindgen_test_layout_dm_deps() {
    assert_eq!(::std::mem::size_of::<dm_deps>() , 8usize , concat ! (
               "Size of: " , stringify ! ( dm_deps ) ));
    assert_eq! (::std::mem::align_of::<dm_deps>() , 8usize , concat ! (
                "Alignment of " , stringify ! ( dm_deps ) ));
}
@emilio emilio added the bug label May 2, 2017
@emilio emilio changed the title liblvm2app alignment failure on generation Incomplete/zero-sized arrays don't affect struct alignment. May 2, 2017
@emilio
Copy link
Contributor

emilio commented May 2, 2017

Hmm, interesting!

So this is basically that __IncompleteArrayField<T>() doesn't affect alignment.

I tried to fix it converting _IncompleteArrayField<T> from being PhantomData<T> to [T; 0], and it works, but that prevent's unconditionally implementing Copy, which may be a problem...

@cholcombe973
Copy link
Author

If i had done the whitelisting correctly this probably wouldn't have even come up. It pulled in a bunch of devmapper dependencies that I'm not sure I want yet. If you want to try this out for yourself to recreate the problem I can push my changes up to github.

@emilio
Copy link
Contributor

emilio commented May 7, 2017

No, that's fine, the test-case is perfect, but thanks for the offer!

bors-servo pushed a commit that referenced this issue Nov 30, 2017

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Property testing with quickcheck

This PR represents an attempt to address issue #970. It also represents a portion of the meta issue for fuzzing #972.

The code base reflected here uses quickcheck to generate C headers that
include a variety of types including basic types, structs, unions,
function prototypes and function pointers. The headers generated by quickcheck
are passed to the `csmith-fuzzing/predicate.py` script. Examples of headers
generated by this iteration of the tooling can be viewed
[here](https://gist.github.com/snewt/03ce934f35c5b085807d2d5cf11d1d5c).

At the top of each header are two simple struct definitions,
`whitelistable` and `blacklistable`. Those types are present in the vector that
represents otherwise primitive types used to generate. They represent a naive
approach to exposing custom types without having to intuit generated type names like
`struct_21_8` though _any actual whitelisting logic isn't implemented
here_.

Test success is measured by the success of the
`csmith-fuzzing/predicate.py`
script. This means that for a test to pass the following must be true:
- bindgen doesn't panic
- the resulting bindings compile
- the resulting bindings layout tests pass

#### Usage
```bash
cd tests/property_test
cargo test
```

Some things I'm unsure of:
#### Where should this feature live?
At the moment it lives in `tests/property_test` but isn't run when
`cargo test` is invoked from bindgen's cargo manifest directory.

#### What's an acceptable ammount of time for these tests to take?
At this point, the source is genereated in ~1 second but the files are
large enough that it takes the `predicate.py` script ~30 seconds to run
through each one. In order for the tests to run in under a minute only 2 are
generated by quickcheck by default. This can be changed in the `test_bindgen`
function of the `tests/property_test/tests/fuzzed-c-headers.rs` file.

#### How do we expose the generated code for easy inspection?
For now the `run_predicate_script` function in the
`tests/property_test/tests/fuzzed-c-headers.rs` file contains a
commented block that will copy generated source in the `tests/property_test/tests`
directory. Should it be easier?

#### Special casing
There is some logic in the fuzzer that disallows 0 sized arrays because
tests will regulary fail due to issues documented in #684 and #1153. Should
this be special casing?

#### Does the fuzzer warrant its own crate?
After any iterations the reviewers are interested in required to make
this a functional testing tool, should/could the fuzzing library be made into
its own crate? I didn't move in that direction yet because having it all in one
place seemed like the best way to figure out what works an doesn't but I'm
interested in whether it might be useful as a standalone library.

#### What does it look like to expose more useful functionality?
I'm looking forward to feedback on how to make this a more useful tool
and one that provides the right configurability.

Thanks!

r? @fitzgen
bors-servo pushed a commit that referenced this issue Dec 9, 2017
Enable Cargo features for quickchecking crate

Logic to enable/disable special casing (due to known issues #550, #684, and #1153) has been exposed as features in the `quickchecking` crate's Cargo.toml file and corresponding `cfg` attributes in the source.

In addition to adding Cargo features, this PR represents the following:
- Documentation in `bindgen`'s CONTRIBUTING.md that points to a new README.md located in the `quickchecking` crate's directory.
- The Debug trait was implemented for the `HeaderC` type. This enables failing property tests to be reported as C source code rather than a Rust data structure.
- The ArrayDimensionC type is now used in header generation for union, struct, and basic declarations.

Thanks for taking a look and for any feedback!

Closes #1169

r? @fitzgen
emilio added a commit to emilio/rust-bindgen that referenced this issue Dec 28, 2018
dgreid pushed a commit to dgreid/crosvm that referenced this issue Nov 17, 2020

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Regenerate bindings.rs for kvm and add comments about how to generate it.
As a result, manually-added hack related to zero-sized arrays' alignment
was removed, as the bug had been fixed:
rust-lang/rust-bindgen#684

BUG=none
TEST=build_test

Change-Id: I257975ce3cd4667b39381ddafd8b08d9e91de655
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/2532546
Tested-by: Keiichi Watanabe <keiichiw@chromium.org>
Tested-by: kokoro <noreply+kokoro@google.com>
Reviewed-by: Daniel Verkamp <dverkamp@chromium.org>
Commit-Queue: Keiichi Watanabe <keiichiw@chromium.org>
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