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

Global enum in the wrong namespace #888

Closed
Xaldew opened this issue Aug 2, 2017 · 5 comments · Fixed by #926
Closed

Global enum in the wrong namespace #888

Xaldew opened this issue Aug 2, 2017 · 5 comments · Fixed by #926

Comments

@Xaldew
Copy link

Xaldew commented Aug 2, 2017

Hello,

While attempting to generate rust bindings for Halide, I think that I encountered a bug. It seems like when the declaration and definition of a type occurs in two separate namespace 'wrappers' and the definition uses an enum from the global scope some bogus rust code is generated.

Input C/C++ Header

namespace Halide {
struct Type;
}
typedef enum {} a;
namespace Halide {
struct Type {
  static a b;
};
}

Bindgen Invocation

extern crate bindgen;

use std::env;
use std::path::PathBuf;

fn main()
{
    let bindings = bindgen::Builder::default()
        .header("test.hpp")
        .clang_args(["-x", "c++", "-std=c++11"].iter())
        .enable_cxx_namespaces()
        .whitelisted_type("Halide::Type")
        .generate()
        .expect("Unable to generate rust bindings");

    let out_path = PathBuf::from(env::var("OUT_DIR").unwrap());
    bindings
        .write_to_file(out_path.join("bindings.rs"))
        .expect("Couldn't write bindings!");
}

Actual Results

error[E0412]: cannot find type `a` in module `root::Halide`
  --> /work/git/rust-halide/hello-cpp-rs/target/debug/build/hello-cpp-a0439f5fa3eaba26/out/bindings.rs:17:50
   |
17 |             pub static mut Type_b: root::Halide::a;
   |                                                  ^ not found in `root::Halide`
   |
help: possible candidate is found in another module, you can import it into scope
   |
7  |     use root::a;
   |

The generated bindings end up like this:

#[allow(non_snake_case, non_camel_case_types, non_upper_case_globals)]
pub mod root {
    #[allow(unused_imports)]
    use self::super::root;
    pub mod Halide {
        #[allow(unused_imports)]
        use self::super::super::root;
        #[repr(C)]
        #[derive(Debug, Copy)]
        pub struct Type {
            pub _address: u8,
        }
        extern "C" {
            #[link_name = "_ZN6Halide4Type1bE"]
            pub static mut Type_b: root::Halide::a;
        }
        #[test]
        fn bindgen_test_layout_Type() {
            assert_eq!(::std::mem::size_of::<Type>() , 1usize , concat ! (
                       "Size of: " , stringify ! ( Type ) ));
            assert_eq! (::std::mem::align_of::<Type>() , 1usize , concat ! (
                        "Alignment of " , stringify ! ( Type ) ));
        }
        impl Clone for Type {
            fn clone(&self) -> Self { *self }
        }
    }
    #[derive(Debug, Copy, Clone, PartialEq, Eq, Hash)]
    pub enum a { }
}

and/or

Expected Results

I expected the member variable b to be of the type root::a rather than root::Halide::a since it is technically defined in the global namespace rather than inside the Halide namespace.

@fitzgen
Copy link
Member

fitzgen commented Aug 3, 2017

Thanks for the bug report!

@fitzgen fitzgen added the A-C++ label Aug 3, 2017
@emilio
Copy link
Contributor

emilio commented Aug 23, 2017

First of all, sorry for taking so long to look at this.

So this is because the code in Var::parse assumes that the type is already resolved, which is in general a reasonable assumption, given the way we parse.

However, this is not true for this case, since we jump to the struct definition when we see the first declaration.

Making Var not assuming that is sort of difficult, but could be done. I think the right fix is not to jump to the definition of a struct directly... I don't know how easy this is going to be though, testing a patch now.

Thanks a lot for the report!

@emilio
Copy link
Contributor

emilio commented Aug 23, 2017

Huh, that actually worked quite easily... Shrug. Will submit a pull request.

emilio added a commit to emilio/rust-bindgen that referenced this issue Aug 23, 2017
emilio added a commit to emilio/rust-bindgen that referenced this issue Aug 23, 2017
…s until we parse it.

This ensures that we see all the relevant types that are defined when parsing
the definition, avoiding problems like rust-lang#888.

Fixes rust-lang#888
@emilio
Copy link
Contributor

emilio commented Aug 23, 2017

I opened #926 with the fix for this.

@Xaldew
Copy link
Author

Xaldew commented Aug 23, 2017

@emilio No worries! I've been busy with other stuff anyways. I'm just happy to help, even if its just with a bug report :)

Keep up the good work!

emilio added a commit to emilio/rust-bindgen that referenced this issue Sep 4, 2017
…s until we parse it.

This ensures that we see all the relevant types that are defined when parsing
the definition, avoiding problems like rust-lang#888.

Fixes rust-lang#888
bors-servo pushed a commit that referenced this issue Sep 5, 2017
ir: When something has a definition, return unresolved type references until we parse it.

This fixes #888
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants