generate type alias for the `block` type #1378

flier · 2018-08-31T17:25:08Z

Consider the c/c++ code with the clang Block type.

// bindgen-flags: --block-extern-crate -- -fblocks

typedef unsigned long long size_t;

void atexit_b(void (^)(void));

typedef void *dispatch_data_t;
typedef bool (^dispatch_data_applier_t)(dispatch_data_t region,
                                        size_t offset,
                                        const void *buffer,
                                        size_t size);
bool dispatch_data_apply(dispatch_data_t data,
                         dispatch_data_applier_t applier);

The generated code will use the block crate to wrap function/closure as a block pointer, instead of a *const c_void pointer without arguments/return type.

/* automatically generated by rust-bindgen */

#![allow(dead_code, non_snake_case, non_camel_case_types, non_upper_case_globals)]

#[macro_use]
extern crate block;
extern "C" {
    #[link_name = "\u{1}_Z8atexit_bU13block_pointerFvvE"]
    pub fn atexit_b(arg1: _bindgen_ty_id_19);
}
pub type dispatch_data_t = *mut ::std::os::raw::c_void;
pub type dispatch_data_applier_t = _bindgen_ty_id_26;
extern "C" {
    #[link_name = "\u{1}_Z19dispatch_data_applyPvU13block_pointerFbS_yPKvyE"]
    pub fn dispatch_data_apply(data: dispatch_data_t, applier: dispatch_data_applier_t) -> bool;
}
pub type _bindgen_ty_id_19 = *const ::block::Block<(), ()>;
pub type _bindgen_ty_id_26 =
    *const ::block::Block<(dispatch_data_t, usize, *const ::std::os::raw::c_void, usize), bool>;

Then, we can use a simple wrapper to call those functions.

pub fn block<A, R, F>(closure: F) -> *const Block<A, R>
where
    A: BlockArguments,
    F: 'static + IntoConcreteBlock<A, Ret = R>,
{
    let block = ConcreteBlock::new(closure);
    let block = block.copy();
    let ptr = (&*block) as *const _;
    let ptr = unsafe { _Block_copy(ptr as *const c_void) };

    ptr as *const Block<A, R>
}

#[derive(Debug)]
pub struct Data {
    ptr: dispatch_data_t,
}

impl Data
    /// Traverses the memory of a dispatch data object and executes custom code on each region.
    pub fn apply<F>(&self, applier: F) -> bool
    where
        F: 'static + Fn(&Self, usize, &[u8]) -> bool,
    {
        let data = self.clone();
        let applier = block(
            move |ptr: dispatch_data_t, offset: usize, buffer: *const c_void, size: usize| {
                assert_eq!(data.ptr, ptr);

                let buf = unsafe { slice::from_raw_parts(buffer as *const u8, size) };

                applier(&data, offset, buf)
            },
        );

        unsafe { dispatch_data_apply(self.ptr, applier) }
    }
}

    #[test]
    fn test_data() {
         //...
        assert!(data.apply(move |_data: &Data, offset: usize, buf: &[u8]| {
            assert_eq!(offset, 0);
            assert_eq!(buf, b"lowor");

            c.set(buf.len());

            true
        }));
    }

emilio

Hi @flier, thanks for the patch!

I think code using blocks should still build by default without dependencies, even if that means keeping them as *mut c_void, wdyt? I think it shouldn't be hard to extend the code for that (even if it means that we don't derive some stuff that we could derive, I think that's not a huge issue and all the derive code should remain as your patch).

This looks like a nice improvement other than that. I need to properly review it over the next day or too, sorry for the lag.

Thanks again!

flier · 2018-09-03T10:31:01Z

I agree the *mut c_void have better compatibility and less dependency, but I believe the block pointer with c_void is not a good practice, it is same to a callback function pointer without signature.

If you prefer to keep the default generated code simple, I suggest to add a new option like --block-signature instead of clang's -fblocks, without the option will generate *mut c_void for the block pointer. But the behavior will be different to the objc options (--objc-extern-crate -- -x objective-c)

emilio · 2018-09-04T12:53:01Z

src/codegen/mod.rs

+
+        let void = helpers::ast_ty::raw_type(ctx, "c_void");
+        let def_block = quote! {
+            pub fn block<A, R, F>(closure: F) -> *const ::block::Block<A, R>


I think this should not really belong in bindgen, fwiw. This ties bindgen to whatever blocks API is. And if something in that crate changes, then bindgen needs a new release.

I think this should live in the block crate, if anything.

agree, its not necessary, I will remove it

emilio · 2018-09-04T12:54:05Z

src/codegen/mod.rs

+                TypeKind::Pointer(inner) => {
+                    let inner = ctx.resolve_item(inner);
+                    let inner_ty = inner.expect_type();
+                    if let TypeKind::ObjCInterface(_) = *inner_ty.canonical_type(ctx).kind() {


Should this be in Type::to_rust_ty instead, on a pointer branch? Looks like it should to me, then this code would become much simpler. I don't think we really want to restrict this to blocks, right?

agree, change to use

arg_ty.try_to_rust_ty(ctx, &arg_item) .unwrap_or_else(|_| arg_item.to_rust_ty_or_opaque(ctx, &()))

emilio · 2018-09-04T12:55:05Z

src/codegen/mod.rs

-                let spelling = self.name().expect("Unnamed alias?");
-                if utils::type_from_named(ctx, spelling).is_some() {
-                    return;
+                if let Some(spelling) = self.name() {


Hmm, we're not changing which Alias we generate, why is this change needed? It doesn't really make sense to have an unnamed alias.

sorry, forget to restore it before commit, my fault.

emilio · 2018-09-04T12:57:39Z

src/codegen/mod.rs

                } else {
-                    utils::build_path(item, ctx)
+                    if let Some(spelling) = self.name() {
+                        if let Some(ty) = utils::type_from_named(ctx, spelling) {


I understand this can be needed, if we share this code between aliases and blocks. Do we have tests for anonymous blocks?

Also, this could use and_then to avoid duplicating the else branches. Something like:

} else if let Some(ty) = self.name().and_then(|name| utils::type_from_named(ctx, name)) { Ok(ty) } else { utils::build_path(..) }

Though I don't feel terribly strongly about it.

I'm not sure what's your meanings about anonymous blocks, but I test two cases, I guess most of function signature will use those.

void atexit_b(void (^)(void)); typedef void *dispatch_data_t; typedef bool (^dispatch_data_applier_t)(dispatch_data_t region, size_t offset, const void *buffer, size_t size); bool dispatch_data_apply(dispatch_data_t data, dispatch_data_applier_t applier);

emilio

I think this is fine, with the nits below. It'd be really nice to specify what is supposed to exist in the block module, maybe in a file like book/src/objc.rs or something.

But the rest of the objective c stuff is undocumented, so it doesn't feel right to block on that :).

Please do file an issue if you don't have the time to document it though.

Thanks!

emilio · 2018-09-05T00:26:58Z

src/codegen/mod.rs

+                let inner_rust_type = {
+                    // Its possible that we have better layout information than
+                    // the inner type does, so fall back to an opaque blob based
+                    // on our layout if converting the inner item fails.


This comment doesn't seem to make sense.

It makes me a bit uncomfortable to not have a switch to preserve the old behavior by default... But I won't block on that if you really think it's not worth it.

That being said, I think we should do it if it breaks any non-objc build :)

I'm fine, we can add a new option like --block-signature instead of clang's -fblocks if you want, and use *const c_void without the option.

emilio · 2018-09-05T00:28:07Z

src/codegen/mod.rs

+        ctx: &BindgenContext,
+        sig: &FunctionSig,
+    ) -> quote::Tokens {
+        let args = sig.argument_types().iter().map(|&(_, ty)| {


I think ty.to_rust_ty_or_opaque(ctx, &()) would do just fine here, no need to try_to_rust_ty, unless you have a test-case in which it makes a difference.

emilio · 2018-09-05T00:28:40Z

src/codegen/mod.rs

+        let use_block = if ctx.options().block_extern_crate {
+            quote! {
+                #![allow(improper_ctypes)]
+


nit: stray newline.

emilio · 2018-09-05T00:28:45Z

src/codegen/mod.rs

+        } else {
+            quote! {
+                #![allow(improper_ctypes)]
+


ditto regarding the two previous points.

emilio · 2018-09-05T00:31:54Z

src/codegen/mod.rs

+                    if let TypeKind::Function(fnsig) = inner_item.kind().expect_type().kind() {
+                        utils::fnsig_block(ctx, fnsig)
+                    } else {
+                        panic!("invalid block typedef")


It'd be nice to include in the message more diagnostics, like inner_item or something.

emilio · 2018-09-05T00:37:35Z

src/codegen/mod.rs

+    ) {
+        let use_block = if ctx.options().block_extern_crate {
+            quote! {
+                #![allow(improper_ctypes)]


Please don't generate this attribute. There's no guarantee that bindings will be in a module on their own. We don't do it anywhere else.

I understand your concern, but the generated code may introduce a compile warning because *const Block<..> has a PhantomData field.

/// An Objective-C block that takes arguments of `A` when called and /// returns a value of `R`. #[repr(C)] pub struct Block<A, R> { _base: PhantomData<BlockBase<A, R>>, }

Can I add the attribute to the module level when we saw the block?

Perhaps, but that won't get executed unless you enable C++ namespaces. I'd leave the responsibility of ignoring the warning to the bindgen consumer.

flier · 2018-09-08T13:49:50Z

@emilio do you mean write a new section like Generating Bindings to Objective C?

emilio

Yes, though as I said that can be a followup if you want.

This looks good, could you squash the commits? Thanks!

flier · 2018-09-10T02:46:25Z

Sure, I have submit a new issue #1386 to follow up it.

emilio · 2018-09-10T08:34:08Z

@bors-servo r+

Thanks!

bors-servo · 2018-09-10T08:34:09Z

📌 Commit 15c97cb has been approved by emilio

bors-servo · 2018-09-10T08:34:12Z

⌛ Testing commit 15c97cb with merge 770052c...

generate type alias for the `block` type Consider the c/c++ code with [the clang `Block` type](http://clang.llvm.org/docs/BlockLanguageSpec.html). ```c++ // bindgen-flags: --block-extern-crate -- -fblocks typedef unsigned long long size_t; void atexit_b(void (^)(void)); typedef void *dispatch_data_t; typedef bool (^dispatch_data_applier_t)(dispatch_data_t region, size_t offset, const void *buffer, size_t size); bool dispatch_data_apply(dispatch_data_t data, dispatch_data_applier_t applier); ``` The generated code will use the [block](https://crates.io/crates/block) crate to wrap function/closure as a `block` pointer, instead of a `*const c_void` pointer without arguments/return type. ```rust /* automatically generated by rust-bindgen */ #![allow(dead_code, non_snake_case, non_camel_case_types, non_upper_case_globals)] #[macro_use] extern crate block; extern "C" { #[link_name = "\u{1}_Z8atexit_bU13block_pointerFvvE"] pub fn atexit_b(arg1: _bindgen_ty_id_19); } pub type dispatch_data_t = *mut ::std::os::raw::c_void; pub type dispatch_data_applier_t = _bindgen_ty_id_26; extern "C" { #[link_name = "\u{1}_Z19dispatch_data_applyPvU13block_pointerFbS_yPKvyE"] pub fn dispatch_data_apply(data: dispatch_data_t, applier: dispatch_data_applier_t) -> bool; } pub type _bindgen_ty_id_19 = *const ::block::Block<(), ()>; pub type _bindgen_ty_id_26 = *const ::block::Block<(dispatch_data_t, usize, *const ::std::os::raw::c_void, usize), bool>; ``` Then, we can use [a simple wrapper](https://github.com/SSheldon/rust-dispatch/blob/e979ba2e7b8d52f2487516931e73f5d9736f16c4/src/data.rs#L144) to call those functions. ```rust pub fn block<A, R, F>(closure: F) -> *const Block<A, R> where A: BlockArguments, F: 'static + IntoConcreteBlock<A, Ret = R>, { let block = ConcreteBlock::new(closure); let block = block.copy(); let ptr = (&*block) as *const _; let ptr = unsafe { _Block_copy(ptr as *const c_void) }; ptr as *const Block<A, R> } #[derive(Debug)] pub struct Data { ptr: dispatch_data_t, } impl Data /// Traverses the memory of a dispatch data object and executes custom code on each region. pub fn apply<F>(&self, applier: F) -> bool where F: 'static + Fn(&Self, usize, &[u8]) -> bool, { let data = self.clone(); let applier = block( move |ptr: dispatch_data_t, offset: usize, buffer: *const c_void, size: usize| { assert_eq!(data.ptr, ptr); let buf = unsafe { slice::from_raw_parts(buffer as *const u8, size) }; applier(&data, offset, buf) }, ); unsafe { dispatch_data_apply(self.ptr, applier) } } } #[test] fn test_data() { //... assert!(data.apply(move |_data: &Data, offset: usize, buf: &[u8]| { assert_eq!(offset, 0); assert_eq!(buf, b"lowor"); c.set(buf.len()); true })); } ```

bors-servo · 2018-09-10T09:26:40Z

💔 Test failed - status-travis

flier · 2018-09-10T09:30:41Z

@emilio could you restart the test again?

No output has been received in the last 10m0s, this potentially indicates a stalled build or something wrong with the build itself.
Check the details on how to adjust your build configuration on: https://docs.travis-ci.com/user/common-build-problems/#Build-times-out-because-no-output-was-received
The build has been terminated

emilio · 2018-09-10T11:04:34Z

Yeah, looks like a fluke.

Since rust-lang#1378 broke a bunch of OSX builds. Most people don't care about them and they're in some OSX system headers which means that this could break normal C and C++ stuff. This introduces --generate-block / generate_block to generate these signatures, and adds tests so that this is getting tested.

Puts blocks behind a switch. Since #1378 broke a bunch of OSX builds. Most people don't care about them and they're in some OSX system headers which means that this could break normal C and C++ stuff. This introduces --generate-block / generate_block to generate these signatures, and adds tests so that this is getting tested.

benw · 2019-02-11T00:37:09Z

src/codegen/mod.rs

+        };
+
+        quote! {
+            *const ::block::Block<(#(#args),*), #ret_ty>


Late to the party, but if there is a single arg then this expands to Block<(arg), ret> which parses the same as Block<arg, ret>. What's needed here is Block<(arg,), ret> so that args is a single-element tuple.

agree, this should be a bug, I have submitted a PR #1519 for this, thanks very much!

fix one argument block pointer issue, #1378

highfive added the S-awaiting-review label Aug 31, 2018

flier force-pushed the blocks branch 2 times, most recently from f899fa2 to 9264f19 Compare September 2, 2018 02:32

emilio reviewed Sep 3, 2018

View reviewed changes

emilio reviewed Sep 4, 2018

View reviewed changes

emilio approved these changes Sep 5, 2018

View reviewed changes

flier force-pushed the blocks branch from f6420d7 to fb3dc63 Compare September 8, 2018 13:46

emilio approved these changes Sep 9, 2018

View reviewed changes

generate type alias for the block type

15c97cb

flier force-pushed the blocks branch from fb3dc63 to 15c97cb Compare September 10, 2018 02:38

flier mentioned this pull request Sep 10, 2018

Document Objective C and clang Block Usage in the users guide #1386

Closed

highfive assigned emilio Sep 10, 2018

highfive added S-awaiting-merge and removed S-awaiting-review labels Sep 10, 2018

highfive added S-tests-failed and removed S-awaiting-merge labels Sep 10, 2018

emilio merged commit 83751c8 into rust-lang:master Sep 10, 2018

flier deleted the blocks branch September 10, 2018 14:14

emilio mentioned this pull request Oct 4, 2018

Put blocks behind a switch. #1410

Merged

benw reviewed Feb 11, 2019

View reviewed changes

flier added a commit to flier/rust-bindgen that referenced this pull request Feb 11, 2019

fix one argument block pointer issue, rust-lang#1378

5063e94

emilio added a commit that referenced this pull request Feb 11, 2019

Merge pull request #1519 from flier/fix-one-argument-block-pointer

9fb016e

fix one argument block pointer issue, #1378

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

generate type alias for the `block` type #1378

generate type alias for the `block` type #1378

flier commented Aug 31, 2018

emilio left a comment

flier commented Sep 3, 2018

emilio Sep 4, 2018

flier Sep 4, 2018

emilio Sep 4, 2018

flier Sep 4, 2018

emilio Sep 4, 2018

flier Sep 4, 2018

emilio Sep 4, 2018

flier Sep 4, 2018

emilio left a comment

emilio Sep 5, 2018

emilio Sep 5, 2018

flier Sep 5, 2018

emilio Sep 5, 2018

flier Sep 5, 2018

emilio Sep 5, 2018

emilio Sep 5, 2018

emilio Sep 5, 2018

emilio Sep 5, 2018

flier Sep 5, 2018

emilio Sep 7, 2018

flier Sep 8, 2018

flier commented Sep 8, 2018

emilio left a comment

flier commented Sep 10, 2018

emilio commented Sep 10, 2018

bors-servo commented Sep 10, 2018

bors-servo commented Sep 10, 2018

bors-servo commented Sep 10, 2018

flier commented Sep 10, 2018

emilio commented Sep 10, 2018

benw Feb 11, 2019

flier Feb 11, 2019 •

edited

Loading

generate type alias for the block type #1378

generate type alias for the block type #1378

Conversation

flier commented Aug 31, 2018

emilio left a comment

Choose a reason for hiding this comment

flier commented Sep 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emilio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flier commented Sep 8, 2018

emilio left a comment

Choose a reason for hiding this comment

flier commented Sep 10, 2018

emilio commented Sep 10, 2018

bors-servo commented Sep 10, 2018

bors-servo commented Sep 10, 2018

bors-servo commented Sep 10, 2018

flier commented Sep 10, 2018

emilio commented Sep 10, 2018

Choose a reason for hiding this comment

flier Feb 11, 2019 • edited Loading

Choose a reason for hiding this comment

generate type alias for the `block` type #1378

generate type alias for the `block` type #1378

flier Feb 11, 2019 •

edited

Loading