libcxx: std::function should be able to capture blocks

== Background:
std::function will wrap anything that is callable (and can be copied/moved). Blocks are copyable and callable, however you need to use the reference counting Block_copy/Block_release functions.

== Request for Comment on the Patch:
I am working on a patch that will allow std::function to capture blocks correctly (under the right feature guards).

Functionally, the patch is complete. However, it's not checkin-ready as I had a couple questions on the approach:
* Is the approach right? I specialized the __func storage class, as that seems to be the right thing to customize.
* I have included some unit test implementations. There are quite a few std::function unit tests, so I wanted to make sure I was on the right track before I do all of them.
* Should I add tests for ObjC++ code, under ARC and non-ARC scenarios? There are no ObjC++ files in libcxx, and the test-runner does not yet support them. Adding this is not hard, but it's a change in how things are today.

If this looks like it's on the right track, I will flush out the rest of the unit tests, and optionally add ObjC++ unit tests.

patch-libcxx-function-with-blocks (11.7 KB)

[+Marshall as libc++ maintainer]

What would it look like if we just provided a value-semantic-C++ wrapper around blocks, translating C++ move/copy operations down to the Block_copy/release/etc operations?

That way it’d be orthogonal to the type erasure of std::function and could be used just for a convenience when using blocks in C++ code anyway:

auto x = make_cpp_block(/* block stuff */);
auto y = x;
x();
etc…

(& then you’d compose them with “std::function<…> f = make_cpp_block(…);” (the block wrapper could have an implicit conversion to std::function))

It’s a little more text, but doesn’t require an extension to the standard library, and can be used independently.

Hi All,

I through a copy of your patch up on phabricator here with some inline comments, but for the most part it seems correct and the right way to do things.
http://reviews.llvm.org/D5596

I’m not convinced direction all together and I’ll leave that decision up to others.
I would prefer David’s suggestion and to leave libc++ unchanged, but I’m not opposed to this change all together.

/Eric

I spoke with Marshall briefly about this at CppCon. My suggestion is that std::function should either:

  • correctly capture blocks and just work (as this patch does)
  • compile with error if you try it (and also maybe add a convenience adaptor like you suggest to allow it to manually work)

The current problem is that if you write something like this:
template doSomething(F&& functor) { std::function<…> capturing = std::forward(functor); … }
This works great for lambdas. For blocks, it compiles just fine but has runtime issues.

Marshall suggested to me that we just make it work, so that’s the approach I’ve taken here. I agree it is a bit “un-pure” to add something like this into libcxx, but given that clang handles blocks natively (with -fblocks), maybe that’s ok.

But, after looking over this patch if it doesnt feel right, I would at least suggest we add =delete/enable_if’s to the right places so at least incorrect code wont compile.

Jared

I’m quite unfamiliar with Apple Blocks and their lifetime semantics. Would you be able to point to some documentation?

The current problem is that if you write something like this:
template doSomething(F&& functor) { std::function<…> capturing = std::forward(functor); … }
This works great for lambdas. For blocks, it compiles just fine but has runtime issues.

I think in understand this, but would you be able to flush out your example?

But, after looking over this patch if it doesnt feel right, I would at least suggest we add =delete/enable_if’s to the right places so at least incorrect code wont compile

This patch feels right. I would be happy to review it once it is a complete. Also, is it possible to test Apple Blocks on Linux?

/Eric

I spoke with Marshall briefly about this at CppCon. My suggestion is that
std::function should either:
* correctly capture blocks and just work (as this patch does)
* compile with error if you try it (and also maybe add a convenience
adaptor like you suggest to allow it to manually work)

The current problem is that if you write something like this:
   template <typename F> doSomething(F&& functor) { std::function<...>
capturing = std::forward<F>(functor); ... }
This works great for lambdas. For blocks, it compiles just fine but has
runtime issues.

What runtime issues are you referring to? Could you provide a simple
standalone complete example?

I assume the issue is that std::function will capture the block's
(presumably raw) pointer by value but do none of the increment/decrement.
This doesn't seem necessarily broken & I'm not sure (for myself) it merits
library support to protect.

This would be the same behavior as any other lifetime issues with pointer
captures.

  std::function<...> func() {
    int i;
    int *j = &i;
    return [=j] () { ... }; // captured j by value, but it points to
something that goes out of scope at the end of the function. Bad.
  }

But similar code isn't a problem at all:

  void func(std::function<...>); // func calls the std::function, maybe
copies it around, but doesn't copy it to global storage/anywhere that
outlives the
  ...
  int i;
  int *j = &i;
  func([=j]() { ... });

Sorry for the delay. I was traveling abroad this weekend.

I’m quite unfamiliar with Apple Blocks and their lifetime semantics. Would you be able to point to some documentation?

The official ref is a bit dense to read but here it is:

The short of it is that blocks, like lambdas, are structs. The block-pointer (“R (^)(Args)”) is a pointer to that struct. The block-pointer is callable, but there is no syntax to “dereference” the block-pointer, so you cant directly copy/move a block.

To extend the life of a block (eg, put it in a std::function), you must call Block_copy, save the (probably different) pointer that is returned, and eventually balance out with a call to Block_release on that returned pointer.

The hand-wavey description of what is happening is this:

  • If you call Block_copy on a stack-based block, it returns a (different) pointer to a heap-based block (with ref-count 1).
  • If you call Block_copy on a heap-based block, it returns the same pointer (and you have a new ref to it).

The ABI has the detailed nuances (eg, static-scoped blocks, blocks with trivial capture, blocks in ObjC), but the simple “Block_copy + keep + Block_release” sequence is how you extend a block’s life in C and C++.

The current problem is that if you write something like this:
template doSomething(F&& functor) { std::function<…> capturing = std::forward(functor); … }
This works great for lambdas. For blocks, it compiles just fine but has runtime issues.

I think in understand this, but would you be able to flush out your example?

Yes, let me do that in my next email since David also asked this.

But, after looking over this patch if it doesnt feel right, I would at least suggest we add =delete/enable_if’s to the right places so at least incorrect code wont compile

This patch feels right. I would be happy to review it once it is a complete. Also, is it possible to test Apple Blocks on Linux?

I dont know this personally, but as long as it has Block.h (part of compiler-rt) and clang is compiled with block support, it should work. But, I havent tried it personally.

Jared

Here’s an example.

int main(int argc, const char**) {
std::function<void()> f;

std::vector vec = {1, 2, 3};
if (argc == 1) {
f = ^{ std::cout << vec.size() << " (block)\n"; };
} else {
f = [=]{ std::cout << vec.size() << " (lambda)\n"; };
}

f();
}

On my machine, it doesnt crash, but the block part is definitely broken:
$ ./a.out

0 (block)
$ ./a.out 1
3 (lambda)

Other examples, especially multithreaded ones, will crash and behave in undefined ways.

Yes.

You’re right that it’s the same “bad pointer to stack” issue, but the problem is at a different level. In the example above, the closures themselves are perfectly fine. The problem is that std::function captures the closure incorrectly. That’s why I think it’s a library issue.

Jared

I spoke with Marshall briefly about this at CppCon. My suggestion is that
std::function should either:
* correctly capture blocks and just work (as this patch does)
* compile with error if you try it (and also maybe add a convenience
adaptor like you suggest to allow it to manually work)

The current problem is that if you write something like this:
   template <typename F> doSomething(F&& functor) { std::function<...>
capturing = std::forward<F>(functor); ... }
This works great for lambdas. For blocks, it compiles just fine but has
runtime issues.

What runtime issues are you referring to? Could you provide a simple
standalone complete example?

Here's an example.

int main(int argc, const char**) {
    std::function<void()> f;

    std::vector<int> vec = {1, 2, 3};
    if (argc == 1) {
        f = ^{ std::cout << vec.size() << " (block)\n"; };
    } else {
        f = [=]{ std::cout << vec.size() << " (lambda)\n"; };
    }

    f();
}

On my machine, it doesnt crash, but the block part is definitely broken:

Fair enough.

$ ./a.out
0 (block)
$ ./a.out 1
3 (lambda)

Other examples, especially multithreaded ones, will crash and behave in
undefined ways.

Hmm - which kind of multithreading do you have in mind? I'm not sure why
that'd be a particular problem.

I assume the issue is that std::function will capture the block's
(presumably raw) pointer by value but do none of the increment/decrement.

Yes.

Would it be possible/reasonable to warn/error on copying the raw block
pointer? (passing it to any function at all) std::function isn't the only
entity that could cause this kind of dangling block pointer bug & it might
be nicer to fix it in the language for all such cases, than fix one (albeit
common/easily triggered) case in the library?

I assume I could write the same bug with something like:

T *t; // not sure what 'T' is, but I imagine it's writeable?
if (x) {
  t = ^{ std::cout << vec.size() << " (block)\n"; };
}
t();

Here’s an example.

int main(int argc, const char**) {
std::function<void()> f;

std::vector vec = {1, 2, 3};
if (argc == 1) {
f = ^{ std::cout << vec.size() << " (block)\n"; };
} else {
f = [=]{ std::cout << vec.size() << " (lambda)\n"; };
}

f();
}

On my machine, it doesnt crash, but the block part is definitely broken:

Fair enough.

$ ./a.out

0 (block)
$ ./a.out 1
3 (lambda)

Other examples, especially multithreaded ones, will crash and behave in undefined ways.

Hmm - which kind of multithreading do you have in mind? I’m not sure why that’d be a particular problem.

Most of my uses of blocks are in multi-threading cases, where we throw blocks across threads (or dispatch_queues in OSX). Capturing blocks on one thread stack and running them on another stack exacerbates the core problem of “bad pointer to stack”.

Yes.

Would it be possible/reasonable to warn/error on copying the raw block pointer? (passing it to any function at all) std::function isn’t the only entity that could cause this kind of dangling block pointer bug & it might be nicer to fix it in the language for all such cases, than fix one (albeit common/easily triggered) case in the library?

I assume I could write the same bug with something like:

T *t; // not sure what ‘T’ is, but I imagine it’s writeable?
if (x) {
t = ^{ std::cout << vec.size() << " (block)\n"; };
}
t();

You make a great point here, and I think such a warning would be amazing. Block assignments that span different scopes have problems for all the reasons we’re describing.

But I think that warning is orthogonal to this patch. Another example that I’m hoping to solve with this patch is something like this, where ‘callback’ is a std::function:

template
void Foo::setCallback(F&& functor) {
this->callback = std::forward(functor);
}

Without this patch, you need to either overload for blocks, or use some sort of “make_function” helper that overloads on blocks.

My goal with this patch is to just make std::function accept block types correctly; it already captures them, it just does it incorrectly.

I spoke with Marshall briefly about this at CppCon. My suggestion is
that std::function should either:
* correctly capture blocks and just work (as this patch does)
* compile with error if you try it (and also maybe add a convenience
adaptor like you suggest to allow it to manually work)

The current problem is that if you write something like this:
   template <typename F> doSomething(F&& functor) { std::function<...>
capturing = std::forward<F>(functor); ... }
This works great for lambdas. For blocks, it compiles just fine but has
runtime issues.

What runtime issues are you referring to? Could you provide a simple
standalone complete example?

Here's an example.

int main(int argc, const char**) {
    std::function<void()> f;

    std::vector<int> vec = {1, 2, 3};
    if (argc == 1) {
        f = ^{ std::cout << vec.size() << " (block)\n"; };
    } else {
        f = [=]{ std::cout << vec.size() << " (lambda)\n"; };
    }

    f();
}

On my machine, it doesnt crash, but the block part is definitely broken:

Fair enough.

$ ./a.out
0 (block)
$ ./a.out 1
3 (lambda)

Other examples, especially multithreaded ones, will crash and behave in
undefined ways.

Hmm - which kind of multithreading do you have in mind? I'm not sure why
that'd be a particular problem.

Most of my uses of blocks are in multi-threading cases, where we throw
blocks across threads (or dispatch_queues in OSX). Capturing blocks on one
thread stack and running them on another stack exacerbates the core problem
of "bad pointer to stack".

I assume the issue is that std::function will capture the block's

(presumably raw) pointer by value but do none of the increment/decrement.

Yes.

Would it be possible/reasonable to warn/error on copying the raw block
pointer? (passing it to any function at all) std::function isn't the only
entity that could cause this kind of dangling block pointer bug & it might
be nicer to fix it in the language for all such cases, than fix one (albeit
common/easily triggered) case in the library?

I assume I could write the same bug with something like:

T *t; // not sure what 'T' is, but I imagine it's writeable?
if (x) {
  t = ^{ std::cout << vec.size() << " (block)\n"; };
}
t();

You make a great point here, and I think such a warning would be amazing.
Block assignments that span different scopes have problems for all the
reasons we're describing.

But I think that warning is orthogonal to this patch. Another example that
I'm hoping to solve with this patch is something like this, where
'callback' is a std::function:

template <typename F>
void Foo::setCallback(F&& functor) {
this->callback = std::forward<F>(functor);
}

Without this patch, you need to either overload for blocks, or use some
sort of "make_function" helper that overloads on blocks.

My goal with this patch is to just make std::function accept block types
correctly; it already captures them, it just does it incorrectly.

*nod* I'd just be hesitant to add such a special case to the standard
library for a variety of reasons - one would be that this could create
subtle portability concerns if you ever port the code to a standard library
without this feature? (granted the inverse would be true if you ever took
libc++ and ran it on a compiler without the hypothetical warnings I'm
suggesting)

Anyway - I'm not the decider in this instance & don't have much involvement
in libc++ at all - I just wanted to raise this possible alternative
path/concerns. I'll leave it up to Marshall to give it due consideration.

It's a great thing to bring up/figure out in any case, I'm sure.

- David