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

android JNINativeMethod wrong size #942

Closed
Dushistov opened this issue Sep 2, 2017 · 9 comments
Closed

android JNINativeMethod wrong size #942

Dushistov opened this issue Sep 2, 2017 · 9 comments

Comments

@Dushistov
Copy link
Contributor

Input C/C++ Header

typedef struct {
    const char* name;
    const char* signature;
    void*       fnPtr;
} JNINativeMethod;

Bindgen Invocation

fn gen_binding<P1, P2>(
    target: &str,
    include_dirs: &[P1],
    c_headers: &[P2],
    output_rust: &Path,
) -> Result<(), String>
where
    P1: AsRef<Path> + fmt::Debug,
    P2: AsRef<Path> + fmt::Debug,
{
    assert!(!c_headers.is_empty());
    let c_file_path = &c_headers[0];

    let mut bindings: bindgen::Builder =
        bindgen::builder().header(c_file_path.as_ref().to_str().unwrap());
    bindings = include_dirs.iter().fold(bindings, |acc, x| {
        acc.clang_arg("-I".to_string() + x.as_ref().to_str().unwrap())
    });
    println!("Generate binding for {:?}", c_headers);
    bindings = bindings
        .rust_target(RustTarget::Stable_1_19)
        //long double not supported yet, see https://github.com/servo/rust-bindgen/issues/550
        .hide_type("max_align_t");
    bindings = if target.contains("windows") {
        //see https://github.com/servo/rust-bindgen/issues/578
        bindings.trust_clang_mangling(false)
    } else {
        bindings
    };
    bindings = c_headers[1..].iter().fold(
        Ok(bindings),
        |acc: Result<bindgen::Builder, String>, header| {
            let c_file_path = header;
            let c_file_str = c_file_path
                .as_ref()
                .to_str()
                .ok_or_else(|| {
                    format!("Invalid unicode in path to {:?}", c_file_path.as_ref())
                })?;
            Ok(acc.unwrap().clang_arg("-include").clang_arg(c_file_str))
        },
    )?;

    let generated_bindings = bindings
        .generate()
        .map_err(|_| "Failed to generate bindings".to_string())?;
    generated_bindings
        .write_to_file(output_rust)
        .map_err(|err| err.to_string())?;

    Ok(())
}

Actual Results

Expected Results

I compile on linux/amd64 for arm-linux-androideabi.
If I run generated by bindgen unit tests I got:

---- android_c_headers::bindgen_test_layout_JNINativeMethod stdout ----
        thread 'android_c_headers::bindgen_test_layout_JNINativeMethod' panicked at 'assertion failed: `(left == right)`
  left: `12`,
 right: `24`: Size of: JNINativeMethod', /home/evgeniy/projects/rust-infra/swig/target/arm-linux-androideabi/debug/build/android-528bccd24539c265/out/android_c_headers.rs:275:4

Test that failed:

#[repr(C)]
#[derive(Debug, Copy)]
pub struct JNINativeMethod {
    pub name: *const ::std::os::raw::c_char,
    pub signature: *const ::std::os::raw::c_char,
    pub fnPtr: *mut ::std::os::raw::c_void,
}
#[test]
fn bindgen_test_layout_JNINativeMethod() {
    assert_eq!(::std::mem::size_of::<JNINativeMethod>() , 24usize , concat ! (
               "Size of: " , stringify ! ( JNINativeMethod ) ));
    assert_eq! (::std::mem::align_of::<JNINativeMethod>() , 8usize , concat !
                ( "Alignment of " , stringify ! ( JNINativeMethod ) ));
    assert_eq! (unsafe {
                & ( * ( 0 as * const JNINativeMethod ) ) . name as * const _
                as usize } , 0usize , concat ! (
                "Alignment of field: " , stringify ! ( JNINativeMethod ) ,
                "::" , stringify ! ( name ) ));
    assert_eq! (unsafe {
                & ( * ( 0 as * const JNINativeMethod ) ) . signature as *
                const _ as usize } , 8usize , concat ! (
                "Alignment of field: " , stringify ! ( JNINativeMethod ) ,
                "::" , stringify ! ( signature ) ));
    assert_eq! (unsafe {
                & ( * ( 0 as * const JNINativeMethod ) ) . fnPtr as * const _
                as usize } , 16usize , concat ! (
                "Alignment of field: " , stringify ! ( JNINativeMethod ) ,
                "::" , stringify ! ( fnPtr ) ));
}
@emilio
Copy link
Contributor

emilio commented Sep 2, 2017

Looks a lot like you're generating methods for a 64-bit target and compiling with a 32-bit target.

@emilio
Copy link
Contributor

emilio commented Sep 2, 2017

Have you tried passing the target down to libclang using clang_arg? I don't recall if I did it so it was autodetected or not.

@Dushistov
Copy link
Contributor Author

@emilio

Have you tried passing the target down to libclang using clang_arg?

I tried this:

    //build.rs
    let target = env::var("TARGET").unwrap();
    let generated_bindings = bindings
        .clang_arg(format!("-target {}", target))
        .generate()
        .map_err(|_| "Failed to generate bindings".to_string())?;

got this:

Generate binding for ["/home/evgeniy/toolchains/android-16-arm-linux-androideabi-4.9/bin/../sysroot/usr/include/android/bitmap.h", "/home/evgeniy/toolchains/android-16-arm-linux-androideabi-4.9/bin/../sysroot/usr/include/jni.h"]
error: unknown argument: '-target arm-linux-androideabi', err: true

--- stderr
thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: "Failed to generate bindings"', /checkout/src/libcore/result.rs:860:4

-target option I took from here: https://clang.llvm.org/docs/CrossCompilation.html

@Dushistov
Copy link
Contributor Author

@emilio

I was able to fix issue with target like this:

        .clang_arg(format!("--target={}", target))

but for some reason with --target=arm-linux-androideabi bindgen emit two __va_list:

pub type __va_list = __builtin_va_list;

pub struct __va_list {
    pub __ap: *mut ::std::os::raw::c_void,
}

and resulted code not compiled,

as you see I do not use emit_builtins or similar, so
looks like --target changes something.

Android jni.h has only

#include <sys/cdefs.h>
#include <stdarg.h>

at start of jni.h

So here is two issues:

  1. bindgen not use TARGET supplied by cargo to build.rs,
    may be any option to bindgen::Builder like .use_cargo_env_cfg(mut self)?

  2. if use --target then some defines or includes changed, and bindgen take to __va_list from somewhere, instead of one.

@Dushistov
Copy link
Contributor Author

Dushistov commented Sep 3, 2017

The difference between --target=arm-linux-androideabi except right sizeof values in unit tests is this:

//no --target=
pub type __builtin_va_list = [__va_list_tag; 1usize];
#[repr(C)]
#[derive(Debug, Copy)]
pub struct __va_list_tag {
    pub gp_offset: ::std::os::raw::c_uint,
    pub fp_offset: ::std::os::raw::c_uint,
    pub overflow_arg_area: *mut ::std::os::raw::c_void,
    pub reg_save_area: *mut ::std::os::raw::c_void,
}

//with --target=
pub type __builtin_va_list = __va_list;
#[repr(C)]
#[derive(Debug, Copy)]
pub struct __va_list {
    pub __ap: *mut ::std::os::raw::c_void,
}

@Dushistov
Copy link
Contributor Author

Here difference difference of bindgen --dump-preprocessed-input with and without --target=:

--- __bindgen.i	2017-09-03 14:14:27.780836405 +0300
+++ __bindgen-bad.i	2017-09-03 14:14:08.600835491 +0300
@@ -1,7 +1,7 @@
 # 1 "__bindgen.c"
 # 1 "<built-in>" 1
 # 1 "<built-in>" 3
-# 322 "<built-in>" 3
+# 320 "<built-in>" 3
 # 1 "<command line>" 1
 # 1 "<built-in>" 2
 # 1 "./bitmap.h" 1
@@ -484,7 +484,7 @@
 /* Define this type if we are doing the whole job,
    or if we want this type in particular.  */
 # 147 "/home/evgeniy/toolchains/android-16-arm-linux-androideabi-4.9/bin/../lib/gcc/arm-linux-androideabi/4.9.x/include/stddef.h"
-typedef long int ptrdiff_t;
+typedef int ptrdiff_t;
 # 157 "/home/evgeniy/toolchains/android-16-arm-linux-androideabi-4.9/bin/../lib/gcc/arm-linux-androideabi/4.9.x/include/stddef.h"
 /* If this symbol has done its job, get rid of it.  */
 
@@ -496,7 +496,7 @@
 /* Define this type if we are doing the whole job,
    or if we want this type in particular.  */
 # 212 "/home/evgeniy/toolchains/android-16-arm-linux-androideabi-4.9/bin/../lib/gcc/arm-linux-androideabi/4.9.x/include/stddef.h"
-typedef long unsigned int size_t;
+typedef unsigned int size_t;
 # 238 "/home/evgeniy/toolchains/android-16-arm-linux-androideabi-4.9/bin/../lib/gcc/arm-linux-androideabi/4.9.x/include/stddef.h"
 /* Wide character type.
    Locale-writers should change this as necessary to
@@ -521,7 +521,7 @@
    since it no longer defines _BSD_RUNE_T_ yet still desires to export
    rune_t in some cases... */
 # 324 "/home/evgeniy/toolchains/android-16-arm-linux-androideabi-4.9/bin/../lib/gcc/arm-linux-androideabi/4.9.x/include/stddef.h"
-typedef int wchar_t;
+typedef unsigned int wchar_t;
 # 358 "/home/evgeniy/toolchains/android-16-arm-linux-androideabi-4.9/bin/../lib/gcc/arm-linux-androideabi/4.9.x/include/stddef.h"
 /*  In 4.3bsd-net2, leave these undefined to indicate that size_t, etc.
     are already defined.  */
@@ -784,8 +784,11 @@
  */
 
 
-typedef long intptr_t;
-typedef unsigned long uintptr_t;
+
+
+
+typedef int intptr_t;
+typedef unsigned int uintptr_t;
 # 216 "/home/evgeniy/toolchains/android-16-arm-linux-androideabi-4.9/bin/../sysroot/usr/include/stdint.h"
 /*
  *  intmax_t & uintmax_t

@Dushistov
Copy link
Contributor Author

Dushistov commented Sep 3, 2017

Reduced tes case:

$ cat bitmap.h

//from include/machine/_types.h from sys/_types.h from stdint.h
typedef __builtin_va_list       __va_list;
$ bindgen --version
bindgen 0.30.0
$ bindgen --no-layout-tests bitmap.h 
/* automatically generated by rust-bindgen */

pub type __va_list = __builtin_va_list;
pub type __builtin_va_list = [__va_list_tag; 1usize];
#[repr(C)]
#[derive(Debug, Copy)]
pub struct __va_list_tag {
    pub gp_offset: ::std::os::raw::c_uint,
    pub fp_offset: ::std::os::raw::c_uint,
    pub overflow_arg_area: *mut ::std::os::raw::c_void,
    pub reg_save_area: *mut ::std::os::raw::c_void,
}
impl Clone for __va_list_tag {
    fn clone(&self) -> Self { *self }
}

$ bindgen --no-layout-tests bitmap.h -- --target=arm-linux-androideabi
/* automatically generated by rust-bindgen */

pub type __va_list = __builtin_va_list;
pub type __builtin_va_list = __va_list;
#[repr(C)]
#[derive(Debug, Copy)]
pub struct __va_list {
    pub __ap: *mut ::std::os::raw::c_void,
}
impl Clone for __va_list {
    fn clone(&self) -> Self { *self }
}

@Dushistov
Copy link
Contributor Author

I suppose __va_list issue is separate issues, not related that bindgen in build.rs not analyze TARGET environment variable, so I move it to #946

@pepyakin
Copy link
Contributor

pepyakin commented Sep 5, 2017

Just wanted to note, that I experienced same problem with the Emscripten (see #947 (comment))

This worked for me as well:

// build.rs
let target = env::var("TARGET").unwrap();
let generated_bindings = bindings
        .clang_arg(format!("--target={}", target))
        .generate()

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