Point to the --whitelist flags or reporting a bug when we error on binding generation.

[SimonSapin]

Running bindgen on the headers of any nontrivial API tends to generate bindings for most of the C/C++ standard library, which is at best undesirable, often doesn’t compile, and sometimes makes even the binding generation fail. The work-around is to use --match to only generate bindings for declarations in relevant header files. (Fortunately, matching can be on a directory name so that there is no need to list each single file individually.)

So it seems to me that --match is necessary in practice. Running without it should probably print a message explaining all this, rather than leave the user wondering why they have dozens of compiler errors.

[emilio]

--match does no longer exist. Whitelisting and the new bindgen should work better, though I'm skeptical we can generate bindings for the whole C++ standard library, so the new "necessary" thing would be --whitelist-{type,function,var}.

Agreed, though.

[highfive]

Please make a comment here if you intend to work on this issue. Thank you!

[emilio]

Updated the title to be more descriptive. Bonus point for catching panic, I guess.

[thuibr]

I'd like to work on this issue. To be clear, I should (1) change all instances of --whitelist-* to --whitelist-{type,function,var}, and (2) print a message if --whitelist is not used?

[emilio]

@thuibr: Hi, thanks for wanting to work on this! I just marked this as assigned :)

So the idea would be to print a message (or similar) if the all the whitelist flags are empty (you can check those in BindgenOptions) and the code generation or parsing failed, yes. Ideally we'll use something like catch_unwind.

I don't know what you mean with changing all instances of --whitelist-* to --whitelist-{type, function, var}, can you specify a bit more?

Thanks once again! :)

[thuibr]

Hi, I'm excited to help!

Here is what comes up when I grep for whitelist:

grep -R "\-\-whitelist" ./
./components/style/binding_tools/regen.py: flags.append("--whitelist-type")
./components/style/binding_tools/regen.py: flags.append("--whitelist-function")
./components/style/binding_tools/regen.py: flags.append("--whitelist-var")

I thought I might need to change those flags to {type,function,var} because of your previous statement.

so the new "necessary" thing would be --whitelist-{type,function,var}

[emilio]

On Sun, Oct 02, 2016 at 02:01:39PM -0700, Thomas Huibregtse wrote:

grep -R "\-\-whitelist" ./
./components/style/binding_tools/regen.py: flags.append("--whitelist-type")
./components/style/binding_tools/regen.py: flags.append("--whitelist-function")
./components/style/binding_tools/regen.py: flags.append("--whitelist-var")

No, that's not necessary, --whitelist-{type, function, var} is just a
way to say "all those flags".

By the way, you seem to be grepping in the servo/servo repo. To work on
this issue you'll need to clone this repository (servo/rust-bindgen).

Thanks!

[thuibr]

Okay. Thanks for that last part about cloning rust-bindgen. I'll get to work!

[thuibr]

@emilio: Would this solution work? If none of ty_pat, function_pat, and var_pat are in options, returned by parse_args_or_exit() in bindgen.rs main(), could I println!() a message about it?

Or do you still think that a catch_unwind is still the way to go? If so, I'm not really sure where I would put that into code. Could you help me out?

Thanks!

[emilio]

Yeah, that'd work. I think a catch_unwind would be better still, and you probably would want to catch the call to generate in the main function in src/bin/bindgen.rs, but that can also be left as a followup :)

[KiChjang]

@thuibr Are you still working on this?

[thuibr]

I am not, sorry.

[choudhary001]

I would like to give this a try if this is not being worked upon. Thanks!

[emilio]

Sure, thanks!

[choudhary001]

@emilio Hi, I am having a little trouble in getting started with this. I didn't find src/bin/bindgen.rs in the repository.

[emilio]

Hi! Sorry, things have moved around a bit lately :).

That file is now src/main.rs.

[dengjeffrey]

Hello! Is this still being worked on @choudhary001? If not I would like to work on this :)

[choudhary001]

@dengjeffrey Yes, please go ahead. I am not working on this currently.

[dengjeffrey]

Great! @emilio are you mentoring this issue? If not that's okay, it would be helpful if this could be marked assigned though

[emilio]

Sure, thanks for working on it!

[emilio]

Let me know if you need anything from me :)

[dengjeffrey]

I spent a good amount of time looking at this issue and reading up on Rust. Being that this is the first time writing in Rust, I need some assistance to how to proceed. It would be really helpful if you can give me some pointers if I am heading in the right direction @emilio 😄

From the description of this issue, I take it that we only point to --whitelist flags when the call to generate() panics.

match builder_from_flags(bind_args.into_iter()) {
    Ok((builder, output)) => {
    
        let builder_result = panic::catch_unwind(|| { 
            builder.generate().unwrap();
        });

        if builder_result.is_err() {
            // Should we be more specific and check that a --whitelist flag has not been specified?
            println!("Unable to generate binding");
            println!("Error is likely caused by not providing a --whitelist-{{type,function,var}} option.");
            std::process::exit(1);
        } 
        else {
            // Here I try to make a call to bindings.write()
            // however the type of bindings is out of scope, so method write() is not found
            let mut bindings = builder_result.unwrap();
            bindings.write(output)
                .expect("Unable to write output");
            bindings.write_dummy_uses()
                .expect("Unable to write dummy uses to file.");
        }
    }
    Err(error) => {
        println!("{}", error);
        std::process::exit(1);
    }
};

This could be fixed by declaring the return type of the closure in panic::catch_unwind. However I am unsure if this is best practices, or how to declare a return type with a structure that has a lifetime declared in another file, Bindings<'ctx>.

[emilio]

Are you sure that you need the type of the closure, and not just a use std::io::Write import? If that use statement doesn't fix it, could you please point me to a branch with your changes so I can try locally and see the error message?

Thanks! :)

[dengjeffrey]

Sure! Here it is, https://github.com/dengjeffrey/rust-bindgen/tree/whitelist-binding-panic

[emilio]

So this is the minimum changeset that makes your changes compile:

diff --git a/src/chooser.rs b/src/chooser.rs
index 51392d70..d7a20771 100644
--- a/src/chooser.rs
+++ b/src/chooser.rs
@@ -3,10 +3,11 @@
 pub use ir::int::IntKind;
 pub use ir::enum_ty::{EnumVariantValue, EnumVariantCustomBehavior};
 use std::fmt;
+use std::panic::UnwindSafe;
 
 /// A trait to allow configuring different kinds of types in different
 /// situations.
-pub trait TypeChooser: fmt::Debug {
+pub trait TypeChooser: fmt::Debug + UnwindSafe {
     /// The integer kind an integer macro should have, given a name and the
     /// value of that macro, or `None` if you want the default to be chosen.
     fn int_macro(&self, _name: &str, _value: i64) -> Option<IntKind> {
diff --git a/src/lib.rs b/src/lib.rs
index dabab152..1b77e2a1 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -544,6 +544,11 @@ pub struct BindgenOptions {
     pub whitelist_recursively: bool,
 }
 
+/// TODO(emilio): This is sort of a lie (see the error message that results from
+/// removing this), but since we don't share references across panic boundaries
+/// it's ok.
+impl ::std::panic::UnwindSafe for BindgenOptions {}
+
 impl BindgenOptions {
     fn build(&mut self) {
         self.whitelisted_vars.build();
diff --git a/src/main.rs b/src/main.rs
index 1ddd1aab..7af360bb 100644
--- a/src/main.rs
+++ b/src/main.rs
@@ -45,9 +45,9 @@ pub fn main() {
 
     match builder_from_flags(bind_args.into_iter()) {
         Ok((builder, output)) => {
-        
+
             let builder_result =
-                panic::catch_unwind(|| { builder.generate().unwrap(); });
+                panic::catch_unwind(|| builder.generate().unwrap());
 
             if builder_result.is_err() {
                 println!("Unable to generate binding");

[emilio]

Nontheless, I think the flow ideally would be something like:

let builder_result =
    panic::catch_unwind(|| {
        let bindings = builder.generate()
            .expect("Unable to generate bindings");

        if !bindings.are_valid_rust() {
            println!("WARNING: The generated bindings are not valid rust code.");
            println!("If this is one of the known-unsupported things (<link>), \
                     please modify the bindgen flags to work around it as described in <link>.");
            println!("Otherwise, please file an issue at https://github.com/servo/rust-bindgen/issues/new");
        }

        bindings.write(output).expect("unable to write output");
        bindings.write_dummy_uses()
            .expect("Unable to write dummy uses to file.");
    });

if let Err(which) = builder_result {
    println!("Bindgen unexpectedly panicked");
    println!("The error message was: {:?}", /* try to downcast which to a suitable error */);
    println!("We'd appreciate a report at https://github.com/servo/rust-bindgen/issues/new");
    std::process::exit(1);
}

Note that we'd need to polish error handling after this so a few common errors that cause panic don't, but something like it could be a good start.

Somewhat tricky is the is_valid_rust check (which doesn't exist right now). If you don't want to write that bit it's fine to land your changes without it, but it wouldn't be difficult to implement because we already depend on aster and syntex for code generation, and those can try to parse arbitrary strings of code as rust and error if the bindings are invalid.

[dengjeffrey]

Thanks! I've taken a look into implementing are_valid_rust, assuming I walk the AST created by aster I'm not sure if there is some crate I could use to verify that it is rust. I think I'm going to leave that out of the scope of this issue for the time being.