[PATCH] D58514: Avoid needlessly copying blocks that initialize or are assigned to local auto variables to the heap

[Hao Wu via Phabricator via cfe-commits]

wuhao5 added a comment.

Hey guys, this is Hao, I am working with Chrome team to sort this issue out.

In D58514#1428606 , @rjmccall wrote:

> I remember this coming up 7-8 years ago.  I intentionally decided against 
> doing a copy/release when assigning to `__weak` because if the block wasn't 
> already guaranteed to be copied then it was probably better to crash than to 
> silently assign a value that's about to be deallocated.  Note that copying 
> the block we assign into `wb` doesn't change anything about the block stored 
> in `b`.
>
> I don't know why Chromium is building a weak reference to a block in the 
> first place, but assuming they have a good reason to be doing it, they should 
> fix their code to force a copy before forming a weak reference.


The culprit code is here 


it's true that it can be copied before assigning to the weak var, and I 
understand the reasoning behind. however, my question is: just from the code 
itself, each variable has the proper scope and assignment, if the block copy 
happen automatically, just like what we should expect ARC would do, should it 
not mutate itself to something else. to be more precise, should the block 
assigned to the weak var be the same after the block is copied? (and in the 
code, the block should be moved to the heap after calling -addObject: a few 
line below.)

so in the end of day, as a user, should we expect the compiler would move the 
block from stack to heap in time and the variable we hold is consistent?


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58514/new/

https://reviews.llvm.org/D58514



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58514: Avoid needlessly copying blocks that initialize or are assigned to local auto variables to the heap

[Hao Wu via Phabricator via cfe-commits]

wuhao5 added a comment.

> The specified user model of blocks is that they stay on the stack until they 
> get copied, and there can be multiple such copies.  ARC just automates that 
> process.  So the address of a block is not consistent before you've forced a 
> copy.
> 
> I tend to agree that a better user model would have been for blocks to be 
> allocated in one place, without any of this copying business, and for the 
> compiler to make an intelligent decision about stack vs. heap based on how 
> the block is used.  That's the model we've used for closures in Swift.  But 
> that  would've required the compiler to have a better ability to propagate 
> information about how the block was used, which Clang isn't really set up 
> for, and it would've required `noescape` annotations to be introduced and 
> used reliably throughout the SDK, which seemed like a big request at the 
> time.  So it's not how it works.
> 
> There is no way in the existing ABI for copying a block to cause other 
> references to the block to become references to the heap block.  We do do 
> that for `__block` variables, but not for block objects themselves.

I see - this makes sense. Right that I'd expect the compiler would know more 
about where the block is being used and make the variable consistent. my other 
worry is, although not realistically, that there can be other projects to use 
this weak/strong pointer trick to do a recursive block invocation. it becomes 
to me a bit counter-intuitive that I will need to know more about how block and 
where it should be copied, which currently we don't have to worry about it at 
all.

Right now we force an explicit copy before using it, but still like to request 
that this would be handled by Clang at some later point :)


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58514/new/

https://reviews.llvm.org/D58514



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58514: Avoid needlessly copying blocks that initialize or are assigned to local auto variables to the heap

[Hao Wu via Phabricator via cfe-commits]

wuhao5 added a comment.

> Can I ask why you want a weak reference to a block in the first place?  It 
> seems basically useless — blocks can certainly appear in reference cycles, 
> but I don't know why you'd ever try to break that cycle with the block 
> instead of somewhere else.

The simplified version:

auto b = ^{

  if (check) {
dispatch_after(queue, 1, b);
  } else {
   // done.
  }

};
dispatch_after(queue, 1, b);


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58514/new/

https://reviews.llvm.org/D58514



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58514: Avoid needlessly copying blocks that initialize or are assigned to local auto variables to the heap

[Hao Wu via Phabricator via cfe-commits]

wuhao5 added a comment.



> Okay, so really just a block self-reference.  We could really just add a 
> feature for that that would avoid both the complexity and the expense of the 
> self-capture dance.

Is there a plan to cover this case? or is it a legitimate use case that Clang 
should handle?


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58514/new/

https://reviews.llvm.org/D58514



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58514: Avoid needlessly copying blocks that initialize or are assigned to local auto variables to the heap

[Hao Wu via Phabricator via cfe-commits]

wuhao5 added a comment.

In D58514#1435296 , @rjmccall wrote:

> In D58514#1435228 , @wuhao5 wrote:
>
> > > Okay, so really just a block self-reference.  We could really just add a 
> > > feature for that that would avoid both the complexity and the expense of 
> > > the self-capture dance.
> >
> > Is there a plan to cover this case? or is it a legitimate use case that 
> > Clang should handle?
>
>
> You are currently relying on something that ARC doesn't guarantee, so the 
> client code should be fixed to explicitly copy the block.  I think we would 
> be happy to consider a proposal in the long run to allow blocks to 
> self-reference more easily, which will effectively bypass the problem.


I am not sure if I follow here - is it not that the weak pointer holds a block 
that's in the stack but is supposed to be in the heap?


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58514/new/

https://reviews.llvm.org/D58514



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits