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

Struct containing a pointer to multi-dimensional array that has one zero sized dimension. #1153

Open
shnewto opened this issue Nov 16, 2017 · 4 comments

Comments

@shnewto
Copy link
Contributor

@shnewto shnewto commented Nov 16, 2017

Hello,
I'm working on generating some valid C headers as property tests for bindgen, issue #970.
While filling in the generation logic I'm starting to come across code that makes
the csmith-fuzzing/predicate.py script cough. I don't know how many of these there
might be (or how many might be false alarms like #1151) so I'm open to just keeping a
public running list somewhere if raising issues as I encounter them isn't the preference.
I'm also open to sharing some of the generated
bonkers preliminary code
that bindgen is handling happily.

This also seems related to issue #684 which the property tests have run into as well.
This one only fails the csmith-fuzzing/predicate.py script though,
rustc seems to compile without errors so I thought I'd bring it up.

Sorry if the code looks insane, it's generated! 😰

Here's the result of rustup show:

Default host: x86_64-unknown-linux-gnu

installed toolchains
--------------------

stable-x86_64-unknown-linux-gnu
nightly-x86_64-unknown-linux-gnu (default)

active toolchain
----------------

nightly-x86_64-unknown-linux-gnu (default)
rustc 1.23.0-nightly (ff0f5de3b 2017-11-14)

Input C/C++ Header

struct {
    char *decl_4_0[0][4]; // fails
} struct_2_0;

// struct {
//     char decl_4_0[0][4]; // succeeds
// } struct_2_0;

// struct {
//     char *decl_4_0[1][4]; // succeeds
// } struct_2_0;

Bindgen Invocation

I've been using the csmith-fuzzing/predicate.py script to test so the failing invocation would look like this assuming that definition is in a header named prop_test.h

$ ./csmith-fuzzing/predicate.py prop_test.h

Actual Results

Script output:

Running: ['cargo', 'run', '--manifest-path', '/home/username/src/rust-bindgen/Cargo.toml', '--', 'prop_test.h', '-o', '/tmp/bindings-7_gwph30.rs', '--with-derive-partialeq', '--with-derive-eq', '--with-derive-partialord', '--with-derive-ord', '--with-derive-hash', '--with-derive-default']
Running: ['rustc', '--crate-type', 'lib', '--test', '-o', '/tmp/layout-tests-br4ipbog', '/tmp/bindings-7_gwph30.rs']
Running: ['/tmp/layout-tests-br4ipbog']
Error: running the compiled bindings' layout tests failed
+
+running 1 test
+test bindgen_test_layout__bindgen_ty_1 ... FAILED
+
+failures:
+
+---- bindgen_test_layout__bindgen_ty_1 stdout ----
+	thread 'bindgen_test_layout__bindgen_ty_1' panicked at 'assertion failed: `(left == right)`
+  left: `1`,
+ right: `8`: Alignment of _bindgen_ty_1', /tmp/bindings-7_gwph30.rs:52:4
+stack backtrace:
+   0:     0x55f6f4d49f63 - std::sys::imp::backtrace::tracing::imp::unwind_backtrace::h58b4c7cfb400b18c
+                               at /checkout/src/libstd/sys/unix/backtrace/tracing/gcc_s.rs:49
+   1:     0x55f6f4d463ec - std::sys_common::backtrace::_print::h091e19687805d26e
+                               at /checkout/src/libstd/sys_common/backtrace.rs:68
+   2:     0x55f6f4d4c3b1 - std::panicking::default_hook::{{closure}}::h839e2a2e96c19d98
+                               at /checkout/src/libstd/sys_common/backtrace.rs:57
+                               at /checkout/src/libstd/panicking.rs:381
+   3:     0x55f6f4d4c0ac - std::panicking::default_hook::h892c050099854db5
+                               at /checkout/src/libstd/panicking.rs:391
+   4:     0x55f6f4d4c884 - std::panicking::rust_panic_with_hook::h09c5f7a0435ac463
+                               at /checkout/src/libstd/panicking.rs:577
+   5:     0x55f6f4d4c6bc - std::panicking::begin_panic::h321753ef82401d62
+                               at /checkout/src/libstd/panicking.rs:538
+   6:     0x55f6f4d4c639 - std::panicking::begin_panic_fmt::h24fe390b9054223d
+                               at /checkout/src/libstd/panicking.rs:522
+   7:     0x55f6f4d0839c - bindings_7_gwph30::bindgen_test_layout__bindgen_ty_1::hbdde4efabb33c38f
+   8:     0x55f6f4d17f61 - <F as test::FnBox<T>>::call_box::hc30672f430bdb600
+                               at /checkout/src/libtest/lib.rs:1491
+                               at /checkout/src/libcore/ops/function.rs:223
+                               at /checkout/src/libtest/lib.rs:142
+   9:     0x55f6f4d54ebe - __rust_maybe_catch_panic
+                               at /checkout/src/libpanic_unwind/lib.rs:99
+  10:     0x55f6f4d093b0 - std::sys_common::backtrace::__rust_begin_short_backtrace::h904f706bf2295342
+                               at /checkout/src/libstd/panicking.rs:459
+                               at /checkout/src/libstd/panic.rs:365
+                               at /checkout/src/libtest/lib.rs:1430
+                               at /checkout/src/libstd/sys_common/backtrace.rs:133
+  11:     0x55f6f4d0a052 - std::panicking::try::do_call::hba965e573c3ab44f
+                               at /checkout/src/libstd/thread/mod.rs:406
+                               at /checkout/src/libstd/panic.rs:300
+                               at /checkout/src/libstd/panicking.rs:480
+  12:     0x55f6f4d54ebe - __rust_maybe_catch_panic
+                               at /checkout/src/libpanic_unwind/lib.rs:99
+  13:     0x55f6f4d11942 - <F as alloc::boxed::FnBox<A>>::call_box::h3f8cad64dddfc3c1
+                               at /checkout/src/libstd/panicking.rs:459
+                               at /checkout/src/libstd/panic.rs:365
+                               at /checkout/src/libstd/thread/mod.rs:405
+                               at /checkout/src/liballoc/boxed.rs:762
+  14:     0x55f6f4d4b78b - std::sys::imp::thread::Thread::new::thread_start::he0774ede68aa21ab
+                               at /checkout/src/liballoc/boxed.rs:772
+                               at /checkout/src/libstd/sys_common/thread.rs:23
+                               at /checkout/src/libstd/sys/unix/thread.rs:90
+  15:     0x7f5de07de6b9 - start_thread
+  16:     0x7f5de02fe3dc - clone
+  17:                0x0 - <unknown>
+
+
+failures:
+    bindgen_test_layout__bindgen_ty_1
+
+test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out
+

rustc output when given the generated code:

warning: found zero-sized type composed only of phantom-data in a foreign-function.
  --> err.rs:60:32
   |
60 |     pub static mut struct_2_0: _bindgen_ty_1;
   |                                ^^^^^^^^^^^^^
   |
   = note: #[warn(improper_ctypes)] on by default

Generated code:

/* automatically generated by rust-bindgen */

#[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)]
pub struct _bindgen_ty_1 {
    pub decl_4_0: __IncompleteArrayField<[*mut ::std::os::raw::c_char; 4usize]>,
}
#[test]
fn bindgen_test_layout__bindgen_ty_1() {
    assert_eq!(
        ::std::mem::size_of::<_bindgen_ty_1>(),
        0usize,
        concat!("Size of: ", stringify!(_bindgen_ty_1))
    );
    assert_eq!(
        ::std::mem::align_of::<_bindgen_ty_1>(),
        8usize,
        concat!("Alignment of ", stringify!(_bindgen_ty_1))
    );
}
extern "C" {
    #[link_name = "\u{1}_Z10struct_2_0"]
    pub static mut struct_2_0: _bindgen_ty_1;
}

Expected Results

🙃 I'm not quite fluent enough in bindgen output to answer this one.

@fitzgen

This comment has been minimized.

Copy link
Member

@fitzgen fitzgen commented Nov 16, 2017

Expected Results

We generate types with the expected size and alignment ;)


Thanks for the bug report!

@fitzgen

This comment has been minimized.

Copy link
Member

@fitzgen fitzgen commented Nov 16, 2017

It seems like the input header in the issue isn't complete -- what is decl_4_0?

@fitzgen

This comment has been minimized.

Copy link
Member

@fitzgen fitzgen commented Nov 16, 2017

Doh I totally misread -- that's the name of a field.

@fitzgen

This comment has been minimized.

Copy link
Member

@fitzgen fitzgen commented Nov 16, 2017

I think we may need to differentiate between zero-sized and incomplete arrays to fix this. That's probably overdue either way.

shnewto added a commit to shnewto/rust-bindgen that referenced this issue Nov 23, 2017
… represents

a portion of the meta issue for fuzzing rust-lang#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

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

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

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.

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?

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

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.

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 added a commit that referenced this issue Nov 30, 2017
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
shnewto added a commit to shnewto/rust-bindgen that referenced this issue Dec 8, 2017
Logic to enable/disable special casing (due to known issues rust-lang#550, rust-lang#684, and rust-lang#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 rust-lang#1169

r? @fitzgen
bors-servo added 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants
You can’t perform that action at this time.