[clang] [clang] Fix self-capturing `__block` variables (PR #89475)

[via cfe-commits]

================
@@ -2764,6 +2794,9 @@ void CodeGenFunction::emitByrefStructureInit(const AutoVarEmission &emission) {
     auto layoutInfo = CGM.getObjCRuntime().BuildByrefLayout(CGM, type);
     storeHeaderField(layoutInfo, getPointerSize(), "byref.layout");
   }
+
+  if (emission.NeedsInitOnHeap)
+    emitByrefInitOnHeap(pointer);
----------------
ille-apple wrote:

Argh, you're right.

In principle the `BLOCK_HAS_COPY_DISPOSE` flag could be used for that purpose, instead of creating a separate field in the payload.  It could start out unset (making the runtime skip the call to `byref_dispose`), and then be set after initialization.

But the initialization might send the block capturing the object to another thread.  If initialization then throws, I assume that nobody will subsequently _call_ the block, or if they do, the block won't actually try to use the object.  That's straightforward UB.  But it doesn't seem like it should be UB just to send the block.  So there might be concurrent calls to `_Block_object_dispose`.  Not only that, 'our' `_Block_object_dispose` call (the one that would hypothetically be added on the cleanup path to fix the memory leak) might not be the one to drop the last reference.

Therefore, `flags` would have to be modified atomically, particularly because the same word is also used for the reference count.  And even with an atomic operation, you would still have theoretical UB in the blocks runtime when it reads `flags` non-atomically.  Though, not any more UB than it's already incurring thanks to potential concurrent refcount modifications...  But what if an alternate block runtime tries to cache `flags`?  The only alternate runtime I know of is GNUstep's and it looks like [it doesn't cache it](https://github.com/gnustep/libobjc2/blob/master/blocks_runtime.m#L199), but this still feels very dubious.

So then there's the alternative you suggested of just acknowledging that this isn't exception-safe (specifically, that it leaks memory if initialization throws).  But that makes this whole patch feel like an awkward middle ground.  If self-capturing `__block` variables are not going to be supported properly (pending new APIs), then maybe they shouldn't be supported at all and should just be an error (pending new APIs).

But if new APIs are added, teaching Clang how to recognize when the target OS supports them would be quite a lot of work for what is ultimately an edge case.

I'm going to see how ugly it ends up being to add a "successfully initialized" flag.

https://github.com/llvm/llvm-project/pull/89475