ille created this revision. ille added reviewers: jfb, rjmccall. Herald added a project: clang. ille requested review of this revision.
This applies to situations where a `__block` variable's initializer references a block that potentially captures the variable itself. Clang special-cases this because the initializer might call Block_copy and cause the variable to be moved to the heap while it's still being initialized. For example, in this snippet: int copyBlockAndReturnInt(void (^)(void)); __block int val = copyBlockAndReturnInt(^{ printf("%p\n", val); }); ...if `copyBlockAndReturnInt` calls `Block_copy`, `val` will be moved to the heap. When `copyBlockAndReturnInt` returns, the generated code needs to store the value to the new location. For scalar values like pointers, this is handled by deferring computation of the variable's address until after the initializer has been evaluated (and just before actually storing to that address). But what about structs? This case would be hard: struct Foo { int a, b; }; int copyBlockAndReturnInt(void (^)(void)); __block struct Foo foo = {42, copyBlockAndReturnInt(^{ printf("%p\n", &foo); })}; To support this, Clang would have to recalculate the location of `foo` before writing to each struct field, in case the initializer of the current field moved the struct. Block_copy would also end up copying a half-initialized struct, although that's no big deal with C structs. This C++ case, on the other hand, would be impossible: struct Foo { void (^block_)(void); Foo(void (^block)(void)) : { printf("I am at %p\n", this); block_ = Block_copy(block); printf("I am now at %p\n", this); } }; __block Foo foo(^{ printf("%p\n", &foo); }); To support *that*, Clang would have to magically move `foo` mid-construction. Normally, Clang uses the move or copy constructor when moving C++ classes to the heap, but C++ classes certainly don't expect a move constructor to be called in the middle of another constructor. memcpy might work in some cases, but it violates C++ classes' expectation that object addresses will remain stable. Thus there's no real way to make this work, and ideally it would result in a compile error (or, alternately, a runtime crash upon calling `Block_copy`). So how does Clang currently approach this situation? Well, there's just a comment: case TEK_Aggregate: [...] // TODO: how can we delay here if D is captured by its initializer? Clang never calls `drillIntoBlockVariable`, and ends up writing the variable to the beginning of the byref struct (not the corresponding field in it), which is never the correct location. This leaves both the variable and the byref struct corrupt - even if the block is never actually copied. This bug dates back to at least 2012, as I reproduced it on LLVM 3.1. The current patch is intended to be a very minimal fix, for the sake of landing sooner. It simply calls `CGM.ErrorUnsupported`, resulting in an error followed by an intentional crash, if (and only if) it would otherwise generate bad code. Note that I'd like to follow this patch with another one that (at least): - detects the issue earlier in the pipeline, and emits a proper compile error; and - makes the "potentially self-capturing" analysis more precise, since it currently has a lot of false positives. Repository: rG LLVM Github Monorepo https://reviews.llvm.org/D89903 Files: clang/lib/CodeGen/CGDecl.cpp clang/test/CodeGenCXX/block-capture-own-init.cpp Index: clang/test/CodeGenCXX/block-capture-own-init.cpp =================================================================== --- /dev/null +++ clang/test/CodeGenCXX/block-capture-own-init.cpp @@ -0,0 +1,9 @@ +// RUN: not %clang_cc1 -x c++ -std=c++11 -fblocks -emit-llvm %s 2>&1 | FileCheck %s + +void test_aggregate_captured_by_own_init() { + struct foo { int a[100]; }; + extern foo get_foo(foo *(^)()); + // CHECK: error: cannot compile this aggregate initialized with potentially self-capturing block yet + __block foo f = get_foo(^{ return &f; }); +} + Index: clang/lib/CodeGen/CGDecl.cpp =================================================================== --- clang/lib/CodeGen/CGDecl.cpp +++ clang/lib/CodeGen/CGDecl.cpp @@ -1921,7 +1921,12 @@ Overlap = AggValueSlot::DoesNotOverlap; else if (auto *FD = dyn_cast<FieldDecl>(D)) Overlap = getOverlapForFieldInit(FD); - // TODO: how can we delay here if D is captured by its initializer? + if (capturedByInit) { + // TODO: how can we delay here if D is captured by its initializer? + CGM.ErrorUnsupported( + init, + "aggregate initialized with potentially self-capturing block"); + } EmitAggExpr(init, AggValueSlot::forLValue( lvalue, *this, AggValueSlot::IsDestructed, AggValueSlot::DoesNotNeedGCBarriers,
Index: clang/test/CodeGenCXX/block-capture-own-init.cpp =================================================================== --- /dev/null +++ clang/test/CodeGenCXX/block-capture-own-init.cpp @@ -0,0 +1,9 @@ +// RUN: not %clang_cc1 -x c++ -std=c++11 -fblocks -emit-llvm %s 2>&1 | FileCheck %s + +void test_aggregate_captured_by_own_init() { + struct foo { int a[100]; }; + extern foo get_foo(foo *(^)()); + // CHECK: error: cannot compile this aggregate initialized with potentially self-capturing block yet + __block foo f = get_foo(^{ return &f; }); +} + Index: clang/lib/CodeGen/CGDecl.cpp =================================================================== --- clang/lib/CodeGen/CGDecl.cpp +++ clang/lib/CodeGen/CGDecl.cpp @@ -1921,7 +1921,12 @@ Overlap = AggValueSlot::DoesNotOverlap; else if (auto *FD = dyn_cast<FieldDecl>(D)) Overlap = getOverlapForFieldInit(FD); - // TODO: how can we delay here if D is captured by its initializer? + if (capturedByInit) { + // TODO: how can we delay here if D is captured by its initializer? + CGM.ErrorUnsupported( + init, + "aggregate initialized with potentially self-capturing block"); + } EmitAggExpr(init, AggValueSlot::forLValue( lvalue, *this, AggValueSlot::IsDestructed, AggValueSlot::DoesNotNeedGCBarriers,
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits