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

Expected bool, found integral variable (0.30 -> 0.31 Regression) #1145

Closed
zacps opened this issue Nov 8, 2017 · 11 comments
Closed

Expected bool, found integral variable (0.30 -> 0.31 Regression) #1145

zacps opened this issue Nov 8, 2017 · 11 comments
Assignees

Comments

@zacps
Copy link

zacps commented Nov 8, 2017

Regression between the 0.30 and 0.31 releases that causes an invalid type (int instead of bool) in generated bindings.
Platform: MSVC x86_64

Input C/C++ Header

winpty.h
I unfortunately wasn't able to get creduce working on windows so unreduced is the best I can do for now.

Bindgen Invocation

  let bindings = bindgen::Builder::default()
        .header("src/include/winpty.h")
        .clang_arg("-x")
        .clang_arg("c++")
        // Enabling this causes a different error, probably rustfmt's fault
        .rustfmt_bindings(false)
        .generate()
        .expect("Unable to generate bindings");

Actual Results

error[E0308]: mismatched types
 --> C:\Users\Zac\Programming\c++\winpty\target\debug\build\winpty-sys-7016778a370aac5f\out/bindings.rs:5:217
  |
5 | } # [ repr ( C ) ] # [ derive ( Debug , Copy , Clone ) ] pub struct __vcrt_va_list_is_reference { pub _address : u8 , } pub const __vcrt_va_list_is_reference___the_value : __vcrt_va_list_is_reference__bindgen_ty_1 = 0 ; pub type __vcrt_va_list_is_reference__bindgen_ty_1 = bool ; pub type __vcrt_bool = bool ; extern "C" {
  |                                                                                                                                                                                                                         ^ expected bool, found integral variable
  |
  = note: expected type `bool`
             found type `{integer}`

Expected Results

Compilation succeeds

@fitzgen
Copy link
Member

fitzgen commented Nov 8, 2017

Thanks for the bug report!

@emilio
Copy link
Contributor

emilio commented Jan 20, 2018

Self-reminder to take a look at this one

@emilio
Copy link
Contributor

emilio commented Jan 21, 2018

Arrgh, this includes windows.h, and I'm on a Linux system.

@zacps, any chance to call .dump_preprocessed_input before generate() and attach it here? That way I can hopefully repro it.

@zacps
Copy link
Author

zacps commented Jan 21, 2018

Here.
Gist may not have been the best medium for this, given it's 200k lines but it appears to work for me. Tell me if you need a reupload.

@emilio
Copy link
Contributor

emilio commented Jan 22, 2018

Thanks!

Here's a reduced test-case out of that (probably can be reduced even further):

template <typename _Ty>
struct __vcrt_va_list_is_reference
{
    enum : bool { __the_value = false };
};

template <typename _Ty>
struct __vcrt_va_list_is_reference<_Ty&>
{
    enum : bool { __the_value = true };
};

template <typename _Ty>
struct __vcrt_va_list_is_reference<_Ty&&>
{
    enum : bool { __the_value = true };
};

template <typename _Ty>
void __vcrt_va_start_verify_argument_type() throw()
{
    static_assert(!__vcrt_va_list_is_reference<_Ty>::__the_value, "va_start argument must not have reference type and must not be parenthesized");
}

You should be able to work around this blacklisting the relevant bits, but still this is a bug of course.

@emilio
Copy link
Contributor

emilio commented Jan 22, 2018

This is probably due to us not handling enum : bool... It's rad that that's allowed, I didn't even know about that.

@emilio
Copy link
Contributor

emilio commented Jan 22, 2018

Aand indeed, more reduced test-case:

enum MyEnum : bool {
  Value = true,
};

@emilio
Copy link
Contributor

emilio commented Jan 22, 2018

And now the regression range makes sense, because it contains 89915f9.

@emilio
Copy link
Contributor

emilio commented Jan 22, 2018

Before that we'd generate instead:

#[repr(u8)]
#[derive(Debug, Copy, Clone, PartialEq, Eq, Hash)]
pub enum MyEnum {
    Value = 1,
}

@emilio emilio self-assigned this Jan 22, 2018
emilio added a commit to emilio/rust-bindgen that referenced this issue Jan 22, 2018
Just use the repr name we generate, since we generate constants for that.

It's not worth trying to guess the actual type to use IMO.

Bindings lose a bit of portability I guess, but that's really a lost bet
already, so instead of special-casing bool and map constants, let's use a
consistent representation everywhere.

Fixes rust-lang#1145
@emilio
Copy link
Contributor

emilio commented Jan 22, 2018

Submitted a fix in #1232.

@zacps
Copy link
Author

zacps commented Jan 22, 2018

Amazing, thanks!

bors-servo pushed a commit that referenced this issue Jan 22, 2018
codegen: Try to reasonably handle enum : bool.

Just use the repr name we generate, since we generate constants for that.

It's not worth trying to guess the actual type to use IMO.

Bindings lose a bit of portability I guess, but that's really a lost bet
already, so instead of special-casing bool and map constants, let's use a
consistent representation everywhere.

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

No branches or pull requests

3 participants