Skip to content

Even more failing layout tests with SpiderMonkey bindings #573

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
fitzgen opened this issue Mar 10, 2017 · 1 comment · Fixed by #581
Closed

Even more failing layout tests with SpiderMonkey bindings #573

fitzgen opened this issue Mar 10, 2017 · 1 comment · Fixed by #581

Comments

@fitzgen
Copy link
Member

fitzgen commented Mar 10, 2017

Input C/C++ Header

template <int b> struct c { char d[b]; };
template <int e> struct f { static const long g = e; };
template <int e> struct h { static const long g = e; };
class i {
  int j;
};
template <typename k, class l> class m : l {
  struct n {
    static const long g = sizeof(k);
  };
  static const long a = f<n::g>::g;
  static const long b = h<a * n::g>::g;
  k o;
  long p;
  long q;
  long r;
  c<b> s;
  bool t;
};
template <typename k> class G {
  using af = void *;
  af ag;
  k ah;
};
template <typename k> using aj = G<k>;
template <typename k> class u {
  u *a;
  u *am;
  aj<k> an;
};
struct I {
  long ap;
};
namespace JS {
class v {
  m<I, i> ar;
};
class AutoIdVector : u<v> {};
}

Bindgen Invokation

$ bindgen input.hpp -- -std=c++14

Actual Results

Emits these bindings:

/* automatically generated by rust-bindgen */

#[repr(C)]
#[derive(Debug, Copy)]
pub struct i {
    pub j: ::std::os::raw::c_int,
}
#[test]
fn bindgen_test_layout_i() {
    assert_eq!(::std::mem::size_of::<i>() , 4usize , concat ! (
               "Size of: " , stringify ! ( i ) ));
    assert_eq! (::std::mem::align_of::<i>() , 4usize , concat ! (
                "Alignment of " , stringify ! ( i ) ));
    assert_eq! (unsafe { & ( * ( 0 as * const i ) ) . j as * const _ as usize
                } , 0usize , concat ! (
                "Alignment of field: " , stringify ! ( i ) , "::" , stringify
                ! ( j ) ));
}
impl Clone for i {
    fn clone(&self) -> Self { *self }
}
#[repr(C)]
pub struct m<k, l> {
    pub _base: l,
    pub o: k,
    pub p: ::std::os::raw::c_long,
    pub q: ::std::os::raw::c_long,
    pub r: ::std::os::raw::c_long,
    pub s: (),
    pub t: bool,
}
#[repr(C)]
#[derive(Debug, Copy, Clone)]
pub struct m_n {
    pub _address: u8,
}
#[repr(C)]
#[derive(Debug, Copy, Clone)]
pub struct G<k> {
    pub ag: G_af,
    pub ah: k,
}
pub type G_af = *mut ::std::os::raw::c_void;
pub type aj<k> = G<k>;
#[repr(C)]
#[derive(Debug, Copy, Clone)]
pub struct u<k> {
    pub a: *mut u<k>,
    pub am: *mut u<k>,
    pub an: aj<k>,
}
#[repr(C)]
#[derive(Debug, Copy)]
pub struct I {
    pub ap: ::std::os::raw::c_long,
}
#[test]
fn bindgen_test_layout_I() {
    assert_eq!(::std::mem::size_of::<I>() , 8usize , concat ! (
               "Size of: " , stringify ! ( I ) ));
    assert_eq! (::std::mem::align_of::<I>() , 8usize , concat ! (
                "Alignment of " , stringify ! ( I ) ));
    assert_eq! (unsafe { & ( * ( 0 as * const I ) ) . ap as * const _ as usize
                } , 0usize , concat ! (
                "Alignment of field: " , stringify ! ( I ) , "::" , stringify
                ! ( ap ) ));
}
impl Clone for I {
    fn clone(&self) -> Self { *self }
}
#[repr(C)]
pub struct JS_v {
    pub ar: m<I, i>,
}
#[test]
fn bindgen_test_layout_JS_v() {
    assert_eq!(::std::mem::size_of::<JS_v>() , 112usize , concat ! (
               "Size of: " , stringify ! ( JS_v ) ));
    assert_eq! (::std::mem::align_of::<JS_v>() , 8usize , concat ! (
                "Alignment of " , stringify ! ( JS_v ) ));
    assert_eq! (unsafe {
                & ( * ( 0 as * const JS_v ) ) . ar as * const _ as usize } ,
                0usize , concat ! (
                "Alignment of field: " , stringify ! ( JS_v ) , "::" ,
                stringify ! ( ar ) ));
}
#[repr(C)]
pub struct JS_AutoIdVector {
    pub _base: u<JS_v>,
}
#[test]
fn bindgen_test_layout_JS_AutoIdVector() {
    assert_eq!(::std::mem::size_of::<JS_AutoIdVector>() , 136usize , concat !
               ( "Size of: " , stringify ! ( JS_AutoIdVector ) ));
    assert_eq! (::std::mem::align_of::<JS_AutoIdVector>() , 8usize , concat !
                ( "Alignment of " , stringify ! ( JS_AutoIdVector ) ));
}
#[test]
fn __bindgen_test_layout_u_instantiation_81() {
    assert_eq!(::std::mem::size_of::<u<JS_v>>() , 136usize , concat ! (
               "Size of template specialization: " , stringify ! ( u<JS_v> )
               ));
    assert_eq!(::std::mem::align_of::<u<JS_v>>() , 8usize , concat ! (
               "Alignment of template specialization: " , stringify ! (
               u<JS_v> ) ));
}

Compiling with --test and running the layout tests results in this failure:

running 5 tests
test __bindgen_test_layout_u_instantiation_81 ... FAILED
test bindgen_test_layout_I ... ok
test bindgen_test_layout_JS_AutoIdVector ... FAILED
test bindgen_test_layout_JS_v ... FAILED
test bindgen_test_layout_i ... ok

failures:

---- __bindgen_test_layout_u_instantiation_81 stdout ----
	thread '__bindgen_test_layout_u_instantiation_81' panicked at 'assertion failed: `(left == right)` (left: `72`, right: `136`): Size of template specialization: u < JS_v >', js.rs:100
note: Run with `RUST_BACKTRACE=1` for a backtrace.

---- bindgen_test_layout_JS_AutoIdVector stdout ----
	thread 'bindgen_test_layout_JS_AutoIdVector' panicked at 'assertion failed: `(left == right)` (left: `72`, right: `136`): Size of: JS_AutoIdVector', js.rs:93

---- bindgen_test_layout_JS_v stdout ----
	thread 'bindgen_test_layout_JS_v' panicked at 'assertion failed: `(left == right)` (left: `48`, right: `112`): Size of: JS_v', js.rs:77


failures:
    __bindgen_test_layout_u_instantiation_81
    bindgen_test_layout_JS_AutoIdVector
    bindgen_test_layout_JS_v

test result: FAILED. 2 passed; 3 failed; 0 ignored; 0 measured

Expected Results

We shouldn't fail layout tests. Even in the face of constructs for which we cannot meaningfully make Rust equivalents, we should generate opaque blobs of the correct size.

@fitzgen
Copy link
Member Author

fitzgen commented Mar 14, 2017

More reduced test case:

template <int>
struct UnusedIntTemplateParam {};

template <class>
class Outer {
    static const long SIZE = 1;
    UnusedIntTemplateParam<SIZE> i;
};

class AutoIdVector {
  Outer<int> ar;
};

Interesting that removing the unused template parameter from Outer makes the test pass...

fitzgen added a commit to fitzgen/rust-bindgen that referenced this issue Mar 14, 2017
The old ToRustTy and ItemToRustTy conversion traits were problematic
in that they assumed infallibity. That assumption is problematic
because we are often dealing with C++ constructs for which Rust has no
equivalent *and* we don't have a usable layout from which to generate
an opaque blob in its stead. But, a usable layout is often "up the
stack" if only we had a way to get up there.

For example, Rust does not currently have an equivalent of const value
template parameters, and libclang will not give us a layout for
template definitions with const value template parameters. However,
libclang will give us the layout for an instantiation of such a
template definition.

First, this commit separates the concepts of generating an equivalent
Rust type from generating an opaque blob of the same size and
alignment as an IR thing. This consolidates and DRYs up a *ton* of
code involved in falling back to an opaque blob (and doing our best to
get a Layout for the blob) when we can't generate an equivalent Rust
type for some IR thing.

Second, it separates fallible and infallible conversions, and provides
a nice little framework for when to choose which. This gives us one
single place where we do this whole dance:

    if could not generate equivalent Rust type:
        if we have a layout:
            return opaque blob based on layout
        else:
            return opaque blob based on a wish and a prayer

The ToRustTy trait is replaced by the TryToOpaque, ToOpaque,
TryToRustTyOrOpaque, and ToRustTyOrOpaque traits. The ItemToRustTy
helper was just a way to avoid ToRustTy's Self::Extra parameter when
it was not needed, and is simply removed without a replacement. We
suck it up and pass `&()` at call sites now. We *could* introduce
ItemTryToOpaque, ItemToOpaque, etc... traits, but the cost of the
added boiler plate would outweigh the benefits of not passing `&()` at
call sites, IMHO.

In addition to being a nice code cleanup, this also fixes rust-lang#573.
fitzgen added a commit to fitzgen/rust-bindgen that referenced this issue Mar 14, 2017
The old ToRustTy and ItemToRustTy conversion traits were problematic
in that they assumed infallibity. That assumption is problematic
because we are often dealing with C++ constructs for which Rust has no
equivalent *and* we don't have a usable layout from which to generate
an opaque blob in its stead. But, a usable layout is often "up the
stack" if only we had a way to get up there.

For example, Rust does not currently have an equivalent of const value
template parameters, and libclang will not give us a layout for
template definitions with const value template parameters. However,
libclang will give us the layout for an instantiation of such a
template definition.

First, this commit separates the concepts of generating an equivalent
Rust type from generating an opaque blob of the same size and
alignment as an IR thing. This consolidates and DRYs up a *ton* of
code involved in falling back to an opaque blob (and doing our best to
get a Layout for the blob) when we can't generate an equivalent Rust
type for some IR thing.

Second, it separates fallible and infallible conversions, and provides
a nice little framework for when to choose which. This gives us one
single place where we do this whole dance:

    if could not generate equivalent Rust type:
        if we have a layout:
            return opaque blob based on layout
        else:
            return opaque blob based on a wish and a prayer

The ToRustTy trait is replaced by the TryToOpaque, ToOpaque,
TryToRustTyOrOpaque, and ToRustTyOrOpaque traits. The ItemToRustTy
helper was just a way to avoid ToRustTy's Self::Extra parameter when
it was not needed, and is simply removed without a replacement. We
suck it up and pass `&()` at call sites now. We *could* introduce
ItemTryToOpaque, ItemToOpaque, etc... traits, but the cost of the
added boiler plate would outweigh the benefits of not passing `&()` at
call sites, IMHO.

In addition to being a nice code cleanup, this also fixes rust-lang#573.
fitzgen added a commit to fitzgen/rust-bindgen that referenced this issue Mar 15, 2017
The old ToRustTy and ItemToRustTy conversion traits were problematic
in that they assumed infallibity. That assumption is problematic
because we are often dealing with C++ constructs for which Rust has no
equivalent *and* we don't have a usable layout from which to generate
an opaque blob in its stead. But, a usable layout is often "up the
stack" if only we had a way to get up there.

For example, Rust does not currently have an equivalent of const value
template parameters, and libclang will not give us a layout for
template definitions with const value template parameters. However,
libclang will give us the layout for an instantiation of such a
template definition.

First, this commit separates the concepts of generating an equivalent
Rust type from generating an opaque blob of the same size and
alignment as an IR thing. This consolidates and DRYs up a *ton* of
code involved in falling back to an opaque blob (and doing our best to
get a Layout for the blob) when we can't generate an equivalent Rust
type for some IR thing.

Second, it separates fallible and infallible conversions, and provides
a nice little framework for when to choose which. This gives us one
single place where we do this whole dance:

    if could not generate equivalent Rust type:
        if we have a layout:
            return opaque blob based on layout
        else:
            return opaque blob based on a wish and a prayer

The ToRustTy trait is replaced by the TryToOpaque, ToOpaque,
TryToRustTyOrOpaque, and ToRustTyOrOpaque traits. The ItemToRustTy
helper was just a way to avoid ToRustTy's Self::Extra parameter when
it was not needed, and is simply removed without a replacement. We
suck it up and pass `&()` at call sites now. We *could* introduce
ItemTryToOpaque, ItemToOpaque, etc... traits, but the cost of the
added boiler plate would outweigh the benefits of not passing `&()` at
call sites, IMHO.

In addition to being a nice code cleanup, this also fixes rust-lang#573.
fitzgen added a commit to fitzgen/rust-bindgen that referenced this issue Mar 15, 2017
The old ToRustTy and ItemToRustTy conversion traits were problematic
in that they assumed infallibity. That assumption is problematic
because we are often dealing with C++ constructs for which Rust has no
equivalent *and* we don't have a usable layout from which to generate
an opaque blob in its stead. But, a usable layout is often "up the
stack" if only we had a way to get up there.

For example, Rust does not currently have an equivalent of const value
template parameters, and libclang will not give us a layout for
template definitions with const value template parameters. However,
libclang will give us the layout for an instantiation of such a
template definition.

First, this commit separates the concepts of generating an equivalent
Rust type from generating an opaque blob of the same size and
alignment as an IR thing. This consolidates and DRYs up a *ton* of
code involved in falling back to an opaque blob (and doing our best to
get a Layout for the blob) when we can't generate an equivalent Rust
type for some IR thing.

Second, it separates fallible and infallible conversions, and provides
a nice little framework for when to choose which. This gives us one
single place where we do this whole dance:

    if could not generate equivalent Rust type:
        if we have a layout:
            return opaque blob based on layout
        else:
            return opaque blob based on a wish and a prayer

The ToRustTy trait is replaced by the TryToOpaque, ToOpaque,
TryToRustTyOrOpaque, and ToRustTyOrOpaque traits. The ItemToRustTy
helper was just a way to avoid ToRustTy's Self::Extra parameter when
it was not needed, and is simply removed without a replacement. We
suck it up and pass `&()` at call sites now. We *could* introduce
ItemTryToOpaque, ItemToOpaque, etc... traits, but the cost of the
added boiler plate would outweigh the benefits of not passing `&()` at
call sites, IMHO.

In addition to being a nice code cleanup, this also fixes rust-lang#573.
bors-servo pushed a commit that referenced this issue Mar 15, 2017
…test-failures, r=emilio

Refactor fallibility of conversions from IR to Rust types

The old `ToRustTy` and `ItemToRustTy` conversion traits were problematic in that they assumed infallibity. That assumption is problematic because we are often dealing with C++ constructs for which Rust has no equivalent *and* we don't have a usable layout from which to generate an opaque blob in its stead. But, a usable layout is often "up the stack" if only we had a way to get up there.

For example, Rust does not currently have an equivalent of const value template parameters, and libclang will not give us a layout for template definitions with const value template parameters. However, libclang will give us the layout for an instantiation of such a template definition.

First, this commit separates the concepts of generating an equivalent Rust type from generating an opaque blob of the same size and alignment as an IR thing. This consolidates and DRYs up a *ton* of code involved in falling back to an opaque blob (and doing our best to get a `Layout` for the blob) when we can't generate an equivalent Rust type for some IR thing.

Second, it separates fallible and infallible conversions, and provides a nice little framework for when to choose which. This gives us one single place where we do this whole dance:

    if could not generate equivalent Rust type:
        if we have a layout:
            return opaque blob based on layout
        else:
            return opaque blob based on a wish and a prayer

The `ToRustTy` trait is replaced by the `TryToOpaque`, `ToOpaque`, `TryToRustTyOrOpaque`, and `ToRustTyOrOpaque` traits. The `ItemToRustTy` helper was just a way to avoid `ToRustTy`'s `Self::Extra` parameter when it was not needed, and is simply removed without a replacement. We suck it up and pass `&()` at call sites now. We *could* introduce `ItemTryToOpaque`, `ItemToOpaque`, etc... traits, but the cost of the added boiler plate would outweigh the benefits of not passing `&()` at call sites, IMHO.

In addition to being a nice code cleanup, this also fixes #573.

r? @emilio
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant