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

Incorrect bindings generated for emscripten #947

Closed
pepyakin opened this issue Sep 4, 2017 · 10 comments
Closed

Incorrect bindings generated for emscripten #947

pepyakin opened this issue Sep 4, 2017 · 10 comments

Comments

@pepyakin
Copy link
Contributor

pepyakin commented Sep 4, 2017

Input C/C++ Header

adder.h:

unsigned char add(unsigned char a, unsigned char b);

Bindgen Invocation

bindgen::Builder::default()
        .header("adder.h")
        .generate()
        .expect("Unable to generate bindings");

Actual Results

Generated bindings:

/* automatically generated by rust-bindgen */

extern "C" {
    #[link_name = "_add"]
    pub fn add(a: ::std::os::raw::c_uchar, b: ::std::os::raw::c_uchar)
     -> ::std::os::raw::c_uchar;
}

rustc/emcc fails with a message:

error: unresolved symbol: _add

(full message)

Expected Results

Emscripten emits names without leading _:

$ nm libadder.a

adder_f1ebf194.bc:
-------- T add

so I expect following bindings to be generated:

/* automatically generated by rust-bindgen */

extern "C" {
    pub fn add(a: ::std::os::raw::c_uchar, b: ::std::os::raw::c_uchar)
     -> ::std::os::raw::c_uchar;
}

I created repository for this case: https://github.com/pepyakin/emcc-bindgen-repro

This can be workarounded with setting trust_clang_mangling to false.

@emilio
Copy link
Contributor

emilio commented Sep 4, 2017

Which clang version are you using?

@pepyakin
Copy link
Contributor Author

pepyakin commented Sep 4, 2017

$ clang --version                                                                                                                                                                                                          
clang version 4.0.0 (https://github.com/kripken/emscripten-fastcomp-clang/ c7c210fee24e0227f882337521b25b1ed9c36d5b) (https://github.com/kripken/emscripten-fastcomp/ 90b726ede4acf47c1bca089de6c79a0b8f2c5d9a) (emscripten 1.37.18 : 1.37.18)
Target: x86_64-apple-darwin16.6.0
Thread model: posix
InstalledDir: /Users/pepyakin/tmp/emsdk_portable/clang/fastcomp/build_incoming_64/bin

@emilio
Copy link
Contributor

emilio commented Sep 4, 2017

I need the actual libclang version bindgen is loading, sorry for the hassle.

You can get it with something like:

RUST_LOG=bindgen bindgen

Which should output something like:

INFO:bindgen: Clang Version: clang version 4.0.1 (tags/RELEASE_401/final)

@upsuper
Copy link
Contributor

upsuper commented Sep 4, 2017

Looks like another case that clang is trying to add _ prefix... We should probably just extend the check at https://github.com/rust-lang-nursery/rust-bindgen/blob/28de37efc4d265ef6a15468e95b2bc3aa8a76ec3/src/ir/context.rs#L359-L367

@pepyakin
Copy link
Contributor Author

pepyakin commented Sep 5, 2017

Interesting,

$ RUST_LOG=bindgen bindgen                                                                                                                                                                                                 
INFO:bindgen: Clang Version: clang version 4.0.0 (https://github.com/kripken/emscripten-fastcomp-clang/ c7c210fee24e0227f882337521b25b1ed9c36d5b) (https://github.com/kripken/emscripten-fastcomp/ 90b726ede4acf47c1bca089de6c79a0b8f2c5d9a) (emscripten 1.37.18 : 1.37.18)
WARN:bindgen: Using clang (4, 0), expected (3, 9)

@pepyakin
Copy link
Contributor Author

pepyakin commented Sep 5, 2017

Going further things getting more interesting...

For this header:

struct Foo {
    unsigned long derp;
};

unsigned char add(unsigned char a, unsigned char b);

bindgen (invoked with this same default config as in first post) generates these bindings:

/* automatically generated by rust-bindgen */

#[repr(C)]
#[derive(Debug, Copy)]
pub struct Foo {
    pub derp: ::std::os::raw::c_ulong,
}
#[test]
fn bindgen_test_layout_Foo() {
    assert_eq!(::std::mem::size_of::<Foo>() , 8usize , concat ! (
               "Size of: " , stringify ! ( Foo ) ));
    assert_eq! (::std::mem::align_of::<Foo>() , 8usize , concat ! (
                "Alignment of " , stringify ! ( Foo ) ));
    assert_eq! (unsafe {
                & ( * ( 0 as * const Foo ) ) . derp as * const _ as usize } ,
                0usize , concat ! (
                "Alignment of field: " , stringify ! ( Foo ) , "::" ,
                stringify ! ( derp ) ));
}
impl Clone for Foo {
    fn clone(&self) -> Self { *self }
}
extern "C" {
    #[link_name = "_add"]
    pub fn add(a: ::std::os::raw::c_uchar, b: ::std::os::raw::c_uchar)
     -> ::std::os::raw::c_uchar;
}
cargo test --target=asmjs-unknown-emscripten
node /Users/pepyakin/tmp/emcc-bindgen-repro/emcc-bindgen-repro/target/asmjs-unknown-emscripten/debug/emcc_bindgen_repro-0fb8f20a7ea6eea7.js

fails with the message

---- bindgen_test_layout_Foo stdout ----
	thread 'main' panicked at 'assertion failed: `(left == right)`
  left: `4`,
 right: `8`: Size of: Foo', /Users/pepyakin/tmp/emcc-bindgen-repro/emcc-bindgen-repro/target/asmjs-unknown-emscripten/debug/build/emcc-bindgen-repro-a4b6fa2b8f183cc1/out/bindings.rs:10:4
note: Run with `RUST_BACKTRACE=1` for a backtrace.

It seems to me different data model is used for generating these bindings. asmjs/wasm32 are both 32bit platforms, and I'm running on 64bit macOS machine.

@emilio
Copy link
Contributor

emilio commented Sep 5, 2017

Yes, I was doubting between that or #528, but given it's with clang 4.0, @upsuper is right.

@pepyakin, wanna submit a patch? :)

@pepyakin
Copy link
Contributor Author

pepyakin commented Sep 5, 2017

@emilio
Oh, nevermind my last comment, seems like just target is not passed, as in this case: #942

Yeah, sure!

@pepyakin
Copy link
Contributor Author

pepyakin commented Sep 5, 2017

@emilio

It seems that original issue is also fixed with .clang_arg(format!("--target={}", target))!

I'm wondering, if it would be better to pass --target arg to clang_args instead of adding one more exception for needs_mangling_hack? This also should fix #942

@emilio
Copy link
Contributor

emilio commented Sep 6, 2017

Yup, I don't love magic, but looks passing --target automatically if not found is a reasonable thing to do anyway...

emilio added a commit to emilio/rust-bindgen that referenced this issue Sep 6, 2017
bors-servo pushed a commit that referenced this issue Sep 6, 2017
ir: Pass the target to clang if it wasn't explicitly passed already.

Fixes #942
Fixes #947
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants