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

Bindgen incorrectly generates pointers to block pointers. #1580

Closed
gmnicke2 opened this issue Jun 20, 2019 · 6 comments
Closed

Bindgen incorrectly generates pointers to block pointers. #1580

gmnicke2 opened this issue Jun 20, 2019 · 6 comments

Comments

@gmnicke2
Copy link
Contributor

Input C/C++ Header

struct contains_ptr_to_block_ptr {
    void (^*val)(int);
};

Bindgen Invocation

$ bindgen input.h --generate-block -- -fblocks

Actual Results

use block;
#[repr(C)]
#[derive(Debug, Copy, Clone)]
pub struct contains_ptr_to_block_ptr {
    pub val: _bindgen_ty_id_4,
}
#[test]
fn bindgen_test_layout_contains_ptr_to_block_ptr() {
    assert_eq!(
        ::std::mem::size_of::<contains_ptr_to_block_ptr>(),
        8usize,
        concat!("Size of: ", stringify!(contains_ptr_to_block_ptr))
    );
    assert_eq!(
        ::std::mem::align_of::<contains_ptr_to_block_ptr>(),
        8usize,
        concat!("Alignment of ", stringify!(contains_ptr_to_block_ptr))
    );
    assert_eq!(
        unsafe {
            &(*(::std::ptr::null::<contains_ptr_to_block_ptr>())).val
                as *const _ as usize
        },
        0usize,
        concat!(
            "Offset of field: ",
            stringify!(contains_ptr_to_block_ptr),
            "::",
            stringify!(val)
        )
    );
}
pub type _bindgen_ty_id_4 = *const ::block::Block<(::std::os::raw::c_int,), ()>;

Expected Results

Bindgen should produce a struct whose field is a mutable pointer to the block pointer:

pub struct contains_ptr_to_block_ptr {
    pub val: *mut _bindgen_ty_id_4,
}
@emilio
Copy link
Contributor

emilio commented Jun 23, 2019

So seems like all block pointers are const at the moment:

*const ::block::Block<(#(#args,)*), #ret_ty>

We should be tracking constness of them already, so this should be trivial to fix (see the usages of Type::is_const over there).

Wanna give this a shot? :)

@highfive
Copy link

Hi! If you have any questions regarding this issue, feel free to make a comment here, or ask it in the #servo channel in IRC.

If you intend to work on this issue, then add @highfive: assign me to your comment, and I'll assign this to you. 😄

@emilio emilio added the bug label Jun 23, 2019
@gmnicke2
Copy link
Contributor Author

Thanks for the response @emilio .

If I'm understanding correctly, I think the permanent constness of the block pointer itself is fine, as it should exist as an immutable pointer to a block. What's as issue here, I believe, is that both a block pointer as a field, and a pointer to a block pointer as a field, both end up as *const ::block::Block, while the latter should be *mut *const ::block::Block after following aliases.

If I have these two C structs in the header

struct x {
    void (^val)(int);
};

struct y {
    void (^*ptr_val)(int);
};

Bindgen is producing the following (after following the _bindgen_ty_id_xxx aliases), noting the fields are the same:

pub struct x {
    pub val: *const ::block::Block<(::std::os::raw::c_int,), ()>;
}

pub struct y {
    pub ptr_val: *const ::block::Block<(::std::os::raw::c_int,), ()>;
}

I've looked into it a bit, and it seems like maybe we want to treat BlockPointer the way Pointer is treated at the IR level, and have its canonical type just be itself (Some(self)):

rust-bindgen/src/ir/ty.rs

Lines 348 to 358 in 8779c91

TypeKind::Pointer(..) |
TypeKind::ObjCId |
TypeKind::ObjCSel |
TypeKind::ObjCInterface(..) => Some(self),
TypeKind::ResolvedTypeRef(inner) |
TypeKind::Alias(inner) |
TypeKind::BlockPointer(inner) |
TypeKind::TemplateAlias(inner, _) => {
ctx.resolve_type(inner).safe_canonical_type(ctx)
}

Apologies if the initial issue statement was unclear or if I misunderstood your guidance. I have a local patch that fixes and tests this this way, if the above logic seems sane enough.

@highfive: assign me

@highfive
Copy link

Hey @gmnicke2! Thanks for your interest in working on this issue. It's now assigned to you!

@emilio
Copy link
Contributor

emilio commented Jun 25, 2019

Oh you're right, sorry. I misread the example.

But you're correct that that canonical type bit is wrong. It's treating blocks the same way as aliases, while it should do that the same way as pointers.

That change sounds good, so if you submit a PR with that and a test I'm happy to merge it.

Thank you!

@emilio
Copy link
Contributor

emilio commented Jul 2, 2019

Fixed by #1582, thanks!

@emilio emilio closed this as completed Jul 2, 2019
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