[clang] [clang] Fix self-capturing `__block` variables (PR #89475)
@@ -2764,6 +2794,9 @@ void CodeGenFunction::emitByrefStructureInit(const
AutoVarEmission ) {
auto layoutInfo = CGM.getObjCRuntime().BuildByrefLayout(CGM, type);
storeHeaderField(layoutInfo, getPointerSize(), "byref.layout");
}
+
+ if (emission.NeedsInitOnHeap)
+emitByrefInitOnHeap(pointer);
ille-apple wrote:
Updated to add the flag. It's not all that bad.
In fact, if, as I suggested in my last comment, we want to support the byref
lingering after unwinding, then there *must* be some kind of runtime state – to
track whether whoever drops the last reference needs to run the destructor. It
would be more natural for the Blocks runtime to track that state rather than
relying on compiler-generated code, but the state has to live somewhere.
If, on the other hand, we don't want to support the byref lingering after
unwinding, then I could go back to the idea of mutating the existing `flags`
field. In that scenario, the existence of runtime state would be unnecessary,
but the implementation would be relatively unobtrusive.
https://github.com/llvm/llvm-project/pull/89475
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Fix self-capturing `__block` variables (PR #89475)
https://github.com/ille-apple updated
https://github.com/llvm/llvm-project/pull/89475
>From 26be4b6332bf6b58b3d99bb0065b854dcce2a944 Mon Sep 17 00:00:00 2001
From: ille-apple
Date: Fri, 19 Apr 2024 15:36:44 -0700
Subject: [PATCH 1/3] [clang] Fix self-capturing `__block` variables
Clang special-cases `__block` variables whose initializer contains a
block literal that captures the variable itself. But this special case
doesn't work for variables of record types, causing Clang to emit broken
code.
Fix this by initializing such variables directly on the heap, and add a
warning for when this needs to be done.
rdar://70498802
---
clang/include/clang/AST/Decl.h| 22 ++-
.../clang/Basic/DiagnosticSemaKinds.td| 6 +
clang/lib/AST/Decl.cpp| 13 ++
clang/lib/CodeGen/CGBlocks.cpp| 89
clang/lib/CodeGen/CGBlocks.h | 1 +
clang/lib/CodeGen/CGDecl.cpp | 116 +---
clang/lib/CodeGen/CodeGenFunction.h | 12 +-
clang/lib/CodeGen/CodeGenModule.h | 8 +-
clang/lib/Sema/Sema.cpp | 66 +
clang/lib/Serialization/ASTReaderDecl.cpp | 1 +
clang/lib/Serialization/ASTWriterDecl.cpp | 6 +-
clang/test/CodeGenCXX/block-capture.cpp | 127 +-
12 files changed, 347 insertions(+), 120 deletions(-)
diff --git a/clang/include/clang/AST/Decl.h b/clang/include/clang/AST/Decl.h
index 5e485ccb85a13..83c54144366b5 100644
--- a/clang/include/clang/AST/Decl.h
+++ b/clang/include/clang/AST/Decl.h
@@ -1103,6 +1103,11 @@ class VarDecl : public DeclaratorDecl, public
Redeclarable {
LLVM_PREFERRED_TYPE(bool)
unsigned IsCXXCondDecl : 1;
+
+/// Whether this is a __block variable that is captured by a block
+/// referenced in its own initializer.
+LLVM_PREFERRED_TYPE(bool)
+unsigned CapturedByOwnInit : 1;
};
union {
@@ -1588,10 +1593,23 @@ class VarDecl : public DeclaratorDecl, public
Redeclarable {
/// escaping block.
bool isNonEscapingByref() const;
- void setEscapingByref() {
-NonParmVarDeclBits.EscapingByref = true;
+ void setEscapingByref();
+
+ /// Indicates this is a __block variable that is captured by a block
+ /// referenced in its own initializer.
+ bool isCapturedByOwnInit() const {
+return !isa(this) && NonParmVarDeclBits.CapturedByOwnInit;
}
+ void setCapturedByOwnInit() {
+assert(!isa(this));
+NonParmVarDeclBits.CapturedByOwnInit = true;
+ }
+
+ /// Check if this is a __block variable which needs to be initialized
+ /// directly on the heap.
+ bool needsInitOnHeap() const;
+
bool isCXXCondDecl() const {
return isa(this) ? false : NonParmVarDeclBits.IsCXXCondDecl;
}
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td
b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 5a32463763aa6..c1b214fa28f07 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -2358,6 +2358,12 @@ def note_var_fixit_add_initialization : Note<
def note_uninit_fixit_remove_cond : Note<
"remove the %select{'%1' if its condition|condition if it}0 "
"is always %select{false|true}2">;
+def warn_implicit_block_var_alloc : Warning<
+ "variable %q0 will be initialized in a heap allocation">,
+ InGroup>, DefaultIgnore;
+def note_because_captured_by_block : Note<
+ "because it is captured by a block used in its own initializer">;
+
def err_init_incomplete_type : Error<"initialization of incomplete type %0">;
def err_list_init_in_parens : Error<
"cannot initialize %select{non-class|reference}0 type %1 with a "
diff --git a/clang/lib/AST/Decl.cpp b/clang/lib/AST/Decl.cpp
index 41fbfe281ef65..3a584849160a8 100644
--- a/clang/lib/AST/Decl.cpp
+++ b/clang/lib/AST/Decl.cpp
@@ -2680,6 +2680,19 @@ bool VarDecl::isNonEscapingByref() const {
return hasAttr() && !NonParmVarDeclBits.EscapingByref;
}
+void VarDecl::setEscapingByref() {
+ assert(!isa(this) && hasAttr());
+ NonParmVarDeclBits.EscapingByref = true;
+}
+
+bool VarDecl::needsInitOnHeap() const {
+ // We need direct initialization on the heap for self-capturing __block
+ // variables of types that cause CodeGenFunction::getEvaluationKind to return
+ // TEK_Aggregate. The only such types that can be captured are records.
+ return isCapturedByOwnInit() &&
+ getType().getAtomicUnqualifiedType()->isRecordType();
+}
+
bool VarDecl::hasDependentAlignment() const {
QualType T = getType();
return T->isDependentType() || T->isUndeducedType() ||
diff --git a/clang/lib/CodeGen/CGBlocks.cpp b/clang/lib/CodeGen/CGBlocks.cpp
index bf50f2025de57..2b381a5301180 100644
--- a/clang/lib/CodeGen/CGBlocks.cpp
+++ b/clang/lib/CodeGen/CGBlocks.cpp
@@ -45,6 +45,13 @@ CGBlockInfo::CGBlockInfo(const BlockDecl *block, StringRef
name)
name = name.substr(1);
}
+BlockByrefHelpers::BlockByrefHelpers(const BlockByrefInfo
[clang] [clang] Fix self-capturing `__block` variables (PR #89475)
https://github.com/ille-apple updated
https://github.com/llvm/llvm-project/pull/89475
>From 26be4b6332bf6b58b3d99bb0065b854dcce2a944 Mon Sep 17 00:00:00 2001
From: ille-apple
Date: Fri, 19 Apr 2024 15:36:44 -0700
Subject: [PATCH 1/2] [clang] Fix self-capturing `__block` variables
Clang special-cases `__block` variables whose initializer contains a
block literal that captures the variable itself. But this special case
doesn't work for variables of record types, causing Clang to emit broken
code.
Fix this by initializing such variables directly on the heap, and add a
warning for when this needs to be done.
rdar://70498802
---
clang/include/clang/AST/Decl.h| 22 ++-
.../clang/Basic/DiagnosticSemaKinds.td| 6 +
clang/lib/AST/Decl.cpp| 13 ++
clang/lib/CodeGen/CGBlocks.cpp| 89
clang/lib/CodeGen/CGBlocks.h | 1 +
clang/lib/CodeGen/CGDecl.cpp | 116 +---
clang/lib/CodeGen/CodeGenFunction.h | 12 +-
clang/lib/CodeGen/CodeGenModule.h | 8 +-
clang/lib/Sema/Sema.cpp | 66 +
clang/lib/Serialization/ASTReaderDecl.cpp | 1 +
clang/lib/Serialization/ASTWriterDecl.cpp | 6 +-
clang/test/CodeGenCXX/block-capture.cpp | 127 +-
12 files changed, 347 insertions(+), 120 deletions(-)
diff --git a/clang/include/clang/AST/Decl.h b/clang/include/clang/AST/Decl.h
index 5e485ccb85a13..83c54144366b5 100644
--- a/clang/include/clang/AST/Decl.h
+++ b/clang/include/clang/AST/Decl.h
@@ -1103,6 +1103,11 @@ class VarDecl : public DeclaratorDecl, public
Redeclarable {
LLVM_PREFERRED_TYPE(bool)
unsigned IsCXXCondDecl : 1;
+
+/// Whether this is a __block variable that is captured by a block
+/// referenced in its own initializer.
+LLVM_PREFERRED_TYPE(bool)
+unsigned CapturedByOwnInit : 1;
};
union {
@@ -1588,10 +1593,23 @@ class VarDecl : public DeclaratorDecl, public
Redeclarable {
/// escaping block.
bool isNonEscapingByref() const;
- void setEscapingByref() {
-NonParmVarDeclBits.EscapingByref = true;
+ void setEscapingByref();
+
+ /// Indicates this is a __block variable that is captured by a block
+ /// referenced in its own initializer.
+ bool isCapturedByOwnInit() const {
+return !isa(this) && NonParmVarDeclBits.CapturedByOwnInit;
}
+ void setCapturedByOwnInit() {
+assert(!isa(this));
+NonParmVarDeclBits.CapturedByOwnInit = true;
+ }
+
+ /// Check if this is a __block variable which needs to be initialized
+ /// directly on the heap.
+ bool needsInitOnHeap() const;
+
bool isCXXCondDecl() const {
return isa(this) ? false : NonParmVarDeclBits.IsCXXCondDecl;
}
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td
b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 5a32463763aa6..c1b214fa28f07 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -2358,6 +2358,12 @@ def note_var_fixit_add_initialization : Note<
def note_uninit_fixit_remove_cond : Note<
"remove the %select{'%1' if its condition|condition if it}0 "
"is always %select{false|true}2">;
+def warn_implicit_block_var_alloc : Warning<
+ "variable %q0 will be initialized in a heap allocation">,
+ InGroup>, DefaultIgnore;
+def note_because_captured_by_block : Note<
+ "because it is captured by a block used in its own initializer">;
+
def err_init_incomplete_type : Error<"initialization of incomplete type %0">;
def err_list_init_in_parens : Error<
"cannot initialize %select{non-class|reference}0 type %1 with a "
diff --git a/clang/lib/AST/Decl.cpp b/clang/lib/AST/Decl.cpp
index 41fbfe281ef65..3a584849160a8 100644
--- a/clang/lib/AST/Decl.cpp
+++ b/clang/lib/AST/Decl.cpp
@@ -2680,6 +2680,19 @@ bool VarDecl::isNonEscapingByref() const {
return hasAttr() && !NonParmVarDeclBits.EscapingByref;
}
+void VarDecl::setEscapingByref() {
+ assert(!isa(this) && hasAttr());
+ NonParmVarDeclBits.EscapingByref = true;
+}
+
+bool VarDecl::needsInitOnHeap() const {
+ // We need direct initialization on the heap for self-capturing __block
+ // variables of types that cause CodeGenFunction::getEvaluationKind to return
+ // TEK_Aggregate. The only such types that can be captured are records.
+ return isCapturedByOwnInit() &&
+ getType().getAtomicUnqualifiedType()->isRecordType();
+}
+
bool VarDecl::hasDependentAlignment() const {
QualType T = getType();
return T->isDependentType() || T->isUndeducedType() ||
diff --git a/clang/lib/CodeGen/CGBlocks.cpp b/clang/lib/CodeGen/CGBlocks.cpp
index bf50f2025de57..2b381a5301180 100644
--- a/clang/lib/CodeGen/CGBlocks.cpp
+++ b/clang/lib/CodeGen/CGBlocks.cpp
@@ -45,6 +45,13 @@ CGBlockInfo::CGBlockInfo(const BlockDecl *block, StringRef
name)
name = name.substr(1);
}
+BlockByrefHelpers::BlockByrefHelpers(const BlockByrefInfo
[clang] [clang] Fix self-capturing `__block` variables (PR #89475)
@@ -2764,6 +2794,9 @@ void CodeGenFunction::emitByrefStructureInit(const
AutoVarEmission ) {
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
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Fix self-capturing `__block` variables (PR #89475)
@@ -2120,27 +2066,33 @@ void CodeGenFunction::EmitAutoVarCleanups(const
AutoVarEmission ) {
// us from jumping into any of these scopes anyway.
if (!HaveInsertPoint()) return;
- const VarDecl = *emission.Variable;
+ // If we're initializing directly on the heap, _Block_object_destroy will
+ // handle destruction, so we don't need to perform cleanups directly.
ille-apple wrote:
Indeed... and looking at this again, I put too much code inside the if
statement.
Not only that, there's an existing bug here when combining
`__attribute__((cleanup))` with `__block`: if the variable was copied to the
heap, the cleanup function will be called after the byref allocation has
already been freed.
Will fix.
https://github.com/llvm/llvm-project/pull/89475
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Fix self-capturing `__block` variables (PR #89475)
@@ -2653,11 +2655,39 @@ const BlockByrefInfo
::getBlockByrefInfo(const VarDecl *D) {
info.FieldOffset = varOffset;
info.ByrefAlignment = std::max(varAlign, getPointerAlign());
+ // If we're initializing directly on the heap, then we should emit a
+ // no-op copy helper, both because we don't need a real one (the
+ // object will never move), and because a real one would break the
+ // pre-init _Block_object_assign.
+ info.ForceNoopCopy = D->needsInitOnHeap();
+
auto pair = BlockByrefInfos.insert({D, info});
assert(pair.second && "info was inserted recursively?");
return pair.first->second;
}
+void CodeGenFunction::emitByrefInitOnHeap(llvm::Value *P) {
+ // The object itself is initialized directly on the heap. But for ABI
+ // backwards compatibility reasons, we have to set up a fake byref struct on
+ // the stack (with the structural components initialized but not the object
+ // itself), then call _Block_object_assign to move it to the heap (which is
+ // safe because we forced a no-op copy helper), then call
+ // _Block_object_dispose to release the extra ref from _Block_object_assign.
+ //
+ // 'P' points to the fake byref struct.
+
+ BlockFieldFlags flags = BLOCK_FIELD_IS_BYREF;
+ // Ignored out-parameter. We'll use the forwarding pointer instead.
+ RawAddress out = CreateDefaultAlignTempAlloca(P->getType(),
"initOnHeap.out");
+
+ llvm::Value *args[] = {Builder.CreateBitCast(out.getPointer(), VoidPtrTy),
+ Builder.CreateBitCast(P, VoidPtrTy),
ille-apple wrote:
Ah, this is an artifact of the change having originally been written in 2020.
Looks like I can just drop the bitcast. I don't think address-space
differences are possible.
https://github.com/llvm/llvm-project/pull/89475
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Fix self-capturing `__block` variables (PR #89475)
@@ -2653,11 +2655,39 @@ const BlockByrefInfo
::getBlockByrefInfo(const VarDecl *D) {
info.FieldOffset = varOffset;
info.ByrefAlignment = std::max(varAlign, getPointerAlign());
+ // If we're initializing directly on the heap, then we should emit a
+ // no-op copy helper, both because we don't need a real one (the
+ // object will never move), and because a real one would break the
+ // pre-init _Block_object_assign.
+ info.ForceNoopCopy = D->needsInitOnHeap();
+
auto pair = BlockByrefInfos.insert({D, info});
assert(pair.second && "info was inserted recursively?");
return pair.first->second;
}
+void CodeGenFunction::emitByrefInitOnHeap(llvm::Value *P) {
ille-apple wrote:
Good point. I'll change the name to `emitByrefHeapAlloc`.
https://github.com/llvm/llvm-project/pull/89475
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Fix self-capturing `__block` variables (PR #89475)
@@ -2499,21 +2501,21 @@ CodeGenFunction::buildByrefHelpers(llvm::StructType
,
// byref routines.
case Qualifiers::OCL_Weak:
return ::buildByrefHelpers(CGM, byrefInfo,
- ARCWeakByrefHelpers(valueAlignment));
+ ARCWeakByrefHelpers(byrefInfo));
// ARC __strong __block variables need to be retained.
case Qualifiers::OCL_Strong:
// Block pointers need to be copied, and there's no direct
// transfer possible.
if (type->isBlockPointerType()) {
return ::buildByrefHelpers(CGM, byrefInfo,
- ARCStrongBlockByrefHelpers(valueAlignment));
+ ARCStrongBlockByrefHelpers(byrefInfo));
- // Otherwise, we transfer ownership of the retain from the stack
- // to the heap.
+// Otherwise, we transfer ownership of the retain from the stack
+// to the heap.
ille-apple wrote:
Makes sense, but the re-indentation came from clang-format (because I modified
the code next to it), and not doing it will upset the CI checks.
How about I move the first comment after the `if` and indent both of them?
```cpp
if (type->isBlockPointerType()) {
// Block pointers need to be copied, and there's no direct
// transfer possible.
return ::buildByrefHelpers(CGM, byrefInfo,
ARCStrongBlockByrefHelpers(byrefInfo));
} else {
// Otherwise, we transfer ownership of the retain from the stack
// to the heap.
return ::buildByrefHelpers(CGM, byrefInfo,
ARCStrongByrefHelpers(byrefInfo));
}
```
https://github.com/llvm/llvm-project/pull/89475
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Fix self-capturing `__block` variables (PR #89475)
ille-apple wrote: Weekly ping. https://github.com/llvm/llvm-project/pull/89475 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Fix self-capturing `__block` variables (PR #89475)
https://github.com/ChuanqiXu9 commented: Serialization related change looks trivial and good. But I feel better to leave the formal approval to CG part reviewers. https://github.com/llvm/llvm-project/pull/89475 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Fix self-capturing `__block` variables (PR #89475)
ille-apple wrote: Rebased due to merge conflict. https://github.com/llvm/llvm-project/pull/89475 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Fix self-capturing `__block` variables (PR #89475)
https://github.com/ille-apple updated
https://github.com/llvm/llvm-project/pull/89475
>From 26be4b6332bf6b58b3d99bb0065b854dcce2a944 Mon Sep 17 00:00:00 2001
From: ille-apple
Date: Fri, 19 Apr 2024 15:36:44 -0700
Subject: [PATCH] [clang] Fix self-capturing `__block` variables
Clang special-cases `__block` variables whose initializer contains a
block literal that captures the variable itself. But this special case
doesn't work for variables of record types, causing Clang to emit broken
code.
Fix this by initializing such variables directly on the heap, and add a
warning for when this needs to be done.
rdar://70498802
---
clang/include/clang/AST/Decl.h| 22 ++-
.../clang/Basic/DiagnosticSemaKinds.td| 6 +
clang/lib/AST/Decl.cpp| 13 ++
clang/lib/CodeGen/CGBlocks.cpp| 89
clang/lib/CodeGen/CGBlocks.h | 1 +
clang/lib/CodeGen/CGDecl.cpp | 116 +---
clang/lib/CodeGen/CodeGenFunction.h | 12 +-
clang/lib/CodeGen/CodeGenModule.h | 8 +-
clang/lib/Sema/Sema.cpp | 66 +
clang/lib/Serialization/ASTReaderDecl.cpp | 1 +
clang/lib/Serialization/ASTWriterDecl.cpp | 6 +-
clang/test/CodeGenCXX/block-capture.cpp | 127 +-
12 files changed, 347 insertions(+), 120 deletions(-)
diff --git a/clang/include/clang/AST/Decl.h b/clang/include/clang/AST/Decl.h
index 5e485ccb85a13..83c54144366b5 100644
--- a/clang/include/clang/AST/Decl.h
+++ b/clang/include/clang/AST/Decl.h
@@ -1103,6 +1103,11 @@ class VarDecl : public DeclaratorDecl, public
Redeclarable {
LLVM_PREFERRED_TYPE(bool)
unsigned IsCXXCondDecl : 1;
+
+/// Whether this is a __block variable that is captured by a block
+/// referenced in its own initializer.
+LLVM_PREFERRED_TYPE(bool)
+unsigned CapturedByOwnInit : 1;
};
union {
@@ -1588,10 +1593,23 @@ class VarDecl : public DeclaratorDecl, public
Redeclarable {
/// escaping block.
bool isNonEscapingByref() const;
- void setEscapingByref() {
-NonParmVarDeclBits.EscapingByref = true;
+ void setEscapingByref();
+
+ /// Indicates this is a __block variable that is captured by a block
+ /// referenced in its own initializer.
+ bool isCapturedByOwnInit() const {
+return !isa(this) && NonParmVarDeclBits.CapturedByOwnInit;
}
+ void setCapturedByOwnInit() {
+assert(!isa(this));
+NonParmVarDeclBits.CapturedByOwnInit = true;
+ }
+
+ /// Check if this is a __block variable which needs to be initialized
+ /// directly on the heap.
+ bool needsInitOnHeap() const;
+
bool isCXXCondDecl() const {
return isa(this) ? false : NonParmVarDeclBits.IsCXXCondDecl;
}
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td
b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 5a32463763aa6..c1b214fa28f07 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -2358,6 +2358,12 @@ def note_var_fixit_add_initialization : Note<
def note_uninit_fixit_remove_cond : Note<
"remove the %select{'%1' if its condition|condition if it}0 "
"is always %select{false|true}2">;
+def warn_implicit_block_var_alloc : Warning<
+ "variable %q0 will be initialized in a heap allocation">,
+ InGroup>, DefaultIgnore;
+def note_because_captured_by_block : Note<
+ "because it is captured by a block used in its own initializer">;
+
def err_init_incomplete_type : Error<"initialization of incomplete type %0">;
def err_list_init_in_parens : Error<
"cannot initialize %select{non-class|reference}0 type %1 with a "
diff --git a/clang/lib/AST/Decl.cpp b/clang/lib/AST/Decl.cpp
index 41fbfe281ef65..3a584849160a8 100644
--- a/clang/lib/AST/Decl.cpp
+++ b/clang/lib/AST/Decl.cpp
@@ -2680,6 +2680,19 @@ bool VarDecl::isNonEscapingByref() const {
return hasAttr() && !NonParmVarDeclBits.EscapingByref;
}
+void VarDecl::setEscapingByref() {
+ assert(!isa(this) && hasAttr());
+ NonParmVarDeclBits.EscapingByref = true;
+}
+
+bool VarDecl::needsInitOnHeap() const {
+ // We need direct initialization on the heap for self-capturing __block
+ // variables of types that cause CodeGenFunction::getEvaluationKind to return
+ // TEK_Aggregate. The only such types that can be captured are records.
+ return isCapturedByOwnInit() &&
+ getType().getAtomicUnqualifiedType()->isRecordType();
+}
+
bool VarDecl::hasDependentAlignment() const {
QualType T = getType();
return T->isDependentType() || T->isUndeducedType() ||
diff --git a/clang/lib/CodeGen/CGBlocks.cpp b/clang/lib/CodeGen/CGBlocks.cpp
index bf50f2025de57..2b381a5301180 100644
--- a/clang/lib/CodeGen/CGBlocks.cpp
+++ b/clang/lib/CodeGen/CGBlocks.cpp
@@ -45,6 +45,13 @@ CGBlockInfo::CGBlockInfo(const BlockDecl *block, StringRef
name)
name = name.substr(1);
}
+BlockByrefHelpers::BlockByrefHelpers(const BlockByrefInfo )
+
[clang] [clang] Fix self-capturing `__block` variables (PR #89475)
ille-apple wrote: Ping :) https://github.com/llvm/llvm-project/pull/89475 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Fix self-capturing `__block` variables (PR #89475)
https://github.com/ille-apple updated
https://github.com/llvm/llvm-project/pull/89475
>From 9c61a1d684a15153ea73e2036cdf978ff187c44d Mon Sep 17 00:00:00 2001
From: ille-apple
Date: Fri, 19 Apr 2024 15:36:44 -0700
Subject: [PATCH 1/2] [clang] Fix self-capturing `__block` variables
Clang special-cases `__block` variables whose initializer contains a
block literal that captures the variable itself. But this special case
doesn't work for variables of record types, causing Clang to emit broken
code.
Fix this by initializing such variables directly on the heap, and add a
warning for when this needs to be done.
rdar://70498802
---
clang/include/clang/AST/Decl.h| 22 ++-
.../clang/Basic/DiagnosticSemaKinds.td| 6 +
clang/lib/AST/Decl.cpp| 13 ++
clang/lib/CodeGen/CGBlocks.cpp| 81 +++
clang/lib/CodeGen/CGBlocks.h | 1 +
clang/lib/CodeGen/CGDecl.cpp | 114 +---
clang/lib/CodeGen/CodeGenFunction.h | 12 +-
clang/lib/CodeGen/CodeGenModule.h | 8 +-
clang/lib/Sema/Sema.cpp | 65 +
clang/lib/Serialization/ASTReaderDecl.cpp | 1 +
clang/lib/Serialization/ASTWriterDecl.cpp | 6 +-
clang/test/CodeGenCXX/block-capture.cpp | 127 +-
12 files changed, 340 insertions(+), 116 deletions(-)
diff --git a/clang/include/clang/AST/Decl.h b/clang/include/clang/AST/Decl.h
index 8b121896d66d15..dc816786d7f11d 100644
--- a/clang/include/clang/AST/Decl.h
+++ b/clang/include/clang/AST/Decl.h
@@ -1103,6 +1103,11 @@ class VarDecl : public DeclaratorDecl, public
Redeclarable {
LLVM_PREFERRED_TYPE(bool)
unsigned IsCXXCondDecl : 1;
+
+/// Whether this is a __block variable that is captured by a block
+/// referenced in its own initializer.
+LLVM_PREFERRED_TYPE(bool)
+unsigned CapturedByOwnInit : 1;
};
union {
@@ -1588,10 +1593,23 @@ class VarDecl : public DeclaratorDecl, public
Redeclarable {
/// escaping block.
bool isNonEscapingByref() const;
- void setEscapingByref() {
-NonParmVarDeclBits.EscapingByref = true;
+ void setEscapingByref();
+
+ /// Indicates this is a __block variable that is captured by a block
+ /// referenced in its own initializer.
+ bool isCapturedByOwnInit() const {
+return !isa(this) && NonParmVarDeclBits.CapturedByOwnInit;
}
+ void setCapturedByOwnInit() {
+assert(!isa(this));
+NonParmVarDeclBits.CapturedByOwnInit = true;
+ }
+
+ /// Check if this is a __block variable which needs to be initialized
+ /// directly on the heap.
+ bool needsInitOnHeap() const;
+
bool isCXXCondDecl() const {
return isa(this) ? false : NonParmVarDeclBits.IsCXXCondDecl;
}
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td
b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 63e951daec7477..859f218c2779de 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -2356,6 +2356,12 @@ def note_var_fixit_add_initialization : Note<
def note_uninit_fixit_remove_cond : Note<
"remove the %select{'%1' if its condition|condition if it}0 "
"is always %select{false|true}2">;
+def warn_implicit_block_var_alloc : Warning<
+ "variable %q0 will be initialized in a heap allocation">,
+ InGroup>, DefaultIgnore;
+def note_because_captured_by_block : Note<
+ "because it is captured by a block used in its own initializer">;
+
def err_init_incomplete_type : Error<"initialization of incomplete type %0">;
def err_list_init_in_parens : Error<
"cannot initialize %select{non-class|reference}0 type %1 with a "
diff --git a/clang/lib/AST/Decl.cpp b/clang/lib/AST/Decl.cpp
index 474e0ccde5bbf7..fda9010ba126d7 100644
--- a/clang/lib/AST/Decl.cpp
+++ b/clang/lib/AST/Decl.cpp
@@ -2680,6 +2680,19 @@ bool VarDecl::isNonEscapingByref() const {
return hasAttr() && !NonParmVarDeclBits.EscapingByref;
}
+void VarDecl::setEscapingByref() {
+ assert(!isa(this) && hasAttr());
+ NonParmVarDeclBits.EscapingByref = true;
+}
+
+bool VarDecl::needsInitOnHeap() const {
+ // We need direct initialization on the heap for self-capturing __block
+ // variables of types that cause CodeGenFunction::getEvaluationKind to return
+ // TEK_Aggregate. The only such types that can be captured are records.
+ return isCapturedByOwnInit() &&
+ getType().getAtomicUnqualifiedType()->isRecordType();
+}
+
bool VarDecl::hasDependentAlignment() const {
QualType T = getType();
return T->isDependentType() || T->isUndeducedType() ||
diff --git a/clang/lib/CodeGen/CGBlocks.cpp b/clang/lib/CodeGen/CGBlocks.cpp
index 2742c39965b2c8..4bea057a15a52c 100644
--- a/clang/lib/CodeGen/CGBlocks.cpp
+++ b/clang/lib/CodeGen/CGBlocks.cpp
@@ -45,6 +45,13 @@ CGBlockInfo::CGBlockInfo(const BlockDecl *block, StringRef
name)
name = name.substr(1);
}
+BlockByrefHelpers::BlockByrefHelpers(const
[clang] [clang] Fix self-capturing `__block` variables (PR #89475)
github-actions[bot] wrote:
:warning: C/C++ code formatter, clang-format found issues in your code.
:warning:
You can test this locally with the following command:
``bash
git-clang-format --diff b6628c24ef017138b8d6eb288e94c141e7c846b0
9c61a1d684a15153ea73e2036cdf978ff187c44d -- clang/include/clang/AST/Decl.h
clang/lib/AST/Decl.cpp clang/lib/CodeGen/CGBlocks.cpp
clang/lib/CodeGen/CGBlocks.h clang/lib/CodeGen/CGDecl.cpp
clang/lib/CodeGen/CodeGenFunction.h clang/lib/CodeGen/CodeGenModule.h
clang/lib/Sema/Sema.cpp clang/lib/Serialization/ASTReaderDecl.cpp
clang/lib/Serialization/ASTWriterDecl.cpp
clang/test/CodeGenCXX/block-capture.cpp
``
View the diff from clang-format here.
``diff
diff --git a/clang/lib/CodeGen/CGBlocks.cpp b/clang/lib/CodeGen/CGBlocks.cpp
index 4bea057a15..2fdfee8e4f 100644
--- a/clang/lib/CodeGen/CGBlocks.cpp
+++ b/clang/lib/CodeGen/CGBlocks.cpp
@@ -2257,7 +2257,7 @@ class CXXByrefHelpers final : public BlockByrefHelpers {
public:
CXXByrefHelpers(const BlockByrefInfo , QualType type,
const Expr *copyExpr)
-: BlockByrefHelpers(info), VarType(type), CopyExpr(copyExpr) {}
+ : BlockByrefHelpers(info), VarType(type), CopyExpr(copyExpr) {}
bool needsCopy() const override { return CopyExpr != nullptr; }
void emitCopy(CodeGenFunction , Address destField,
@@ -2470,16 +2470,16 @@ CodeGenFunction::buildByrefHelpers(llvm::StructType
,
CGM.getContext().getBlockVarCopyInit().getCopyExpr();
if (!copyExpr && record->hasTrivialDestructor()) return nullptr;
-return ::buildByrefHelpers(
-CGM, byrefInfo, CXXByrefHelpers(byrefInfo, type, copyExpr));
+return ::buildByrefHelpers(CGM, byrefInfo,
+ CXXByrefHelpers(byrefInfo, type, copyExpr));
}
// If type is a non-trivial C struct type that is non-trivial to
// destructly move or destroy, build the copy and dispose helpers.
if (type.isNonTrivialToPrimitiveDestructiveMove() == QualType::PCK_Struct ||
type.isDestructedType() == QualType::DK_nontrivial_c_struct)
-return ::buildByrefHelpers(
-CGM, byrefInfo, NonTrivialCStructByrefHelpers(byrefInfo, type));
+return ::buildByrefHelpers(CGM, byrefInfo,
+ NonTrivialCStructByrefHelpers(byrefInfo, type));
// Otherwise, if we don't have a retainable type, there's nothing to do.
// that the runtime does extra copies.
@@ -2511,8 +2511,8 @@ CodeGenFunction::buildByrefHelpers(llvm::StructType
,
return ::buildByrefHelpers(CGM, byrefInfo,
ARCStrongBlockByrefHelpers(byrefInfo));
- // Otherwise, we transfer ownership of the retain from the stack
- // to the heap.
+// Otherwise, we transfer ownership of the retain from the stack
+// to the heap.
} else {
return ::buildByrefHelpers(CGM, byrefInfo,
ARCStrongByrefHelpers(byrefInfo));
diff --git a/clang/lib/CodeGen/CGDecl.cpp b/clang/lib/CodeGen/CGDecl.cpp
index 954dd10792..65366c65fc 100644
--- a/clang/lib/CodeGen/CGDecl.cpp
+++ b/clang/lib/CodeGen/CGDecl.cpp
@@ -2088,8 +2088,10 @@ void CodeGenFunction::EmitAutoVarCleanups(const
AutoVarEmission ) {
llvm::Constant *F = CGM.GetAddrOfFunction(FD);
assert(F && "Could not find function!");
- const CGFunctionInfo =
CGM.getTypes().arrangeFunctionDeclaration(FD);
- EHStack.pushCleanup(NormalAndEHCleanup, F, ,
);
+ const CGFunctionInfo =
+ CGM.getTypes().arrangeFunctionDeclaration(FD);
+ EHStack.pushCleanup(NormalAndEHCleanup, F, ,
+ );
}
}
diff --git a/clang/lib/Sema/Sema.cpp b/clang/lib/Sema/Sema.cpp
index 0ac81bfc82..e7b3020556 100644
--- a/clang/lib/Sema/Sema.cpp
+++ b/clang/lib/Sema/Sema.cpp
@@ -2210,6 +2210,7 @@ class CapturedByOwnInitVisitor
: public EvaluatedExprVisitor {
typedef EvaluatedExprVisitor Inherited;
VarDecl *const VD;
+
public:
const BlockExpr *FoundBE;
CapturedByOwnInitVisitor(Sema , VarDecl *VD)
@@ -2226,7 +2227,7 @@ public:
}
}
};
-}
+} // namespace
static void checkCapturedByOwnInit(VarDecl *VD, Sema ) {
Expr *I = VD->getInit();
``
https://github.com/llvm/llvm-project/pull/89475
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Fix self-capturing `__block` variables (PR #89475)
ille-apple wrote: Rebased, because the original version failed CI due to a regression on `main` (#89476). https://github.com/llvm/llvm-project/pull/89475 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Fix self-capturing `__block` variables (PR #89475)
https://github.com/ille-apple updated
https://github.com/llvm/llvm-project/pull/89475
>From 9c61a1d684a15153ea73e2036cdf978ff187c44d Mon Sep 17 00:00:00 2001
From: ille-apple
Date: Fri, 19 Apr 2024 15:36:44 -0700
Subject: [PATCH] [clang] Fix self-capturing `__block` variables
Clang special-cases `__block` variables whose initializer contains a
block literal that captures the variable itself. But this special case
doesn't work for variables of record types, causing Clang to emit broken
code.
Fix this by initializing such variables directly on the heap, and add a
warning for when this needs to be done.
rdar://70498802
---
clang/include/clang/AST/Decl.h| 22 ++-
.../clang/Basic/DiagnosticSemaKinds.td| 6 +
clang/lib/AST/Decl.cpp| 13 ++
clang/lib/CodeGen/CGBlocks.cpp| 81 +++
clang/lib/CodeGen/CGBlocks.h | 1 +
clang/lib/CodeGen/CGDecl.cpp | 114 +---
clang/lib/CodeGen/CodeGenFunction.h | 12 +-
clang/lib/CodeGen/CodeGenModule.h | 8 +-
clang/lib/Sema/Sema.cpp | 65 +
clang/lib/Serialization/ASTReaderDecl.cpp | 1 +
clang/lib/Serialization/ASTWriterDecl.cpp | 6 +-
clang/test/CodeGenCXX/block-capture.cpp | 127 +-
12 files changed, 340 insertions(+), 116 deletions(-)
diff --git a/clang/include/clang/AST/Decl.h b/clang/include/clang/AST/Decl.h
index 8b121896d66d15..dc816786d7f11d 100644
--- a/clang/include/clang/AST/Decl.h
+++ b/clang/include/clang/AST/Decl.h
@@ -1103,6 +1103,11 @@ class VarDecl : public DeclaratorDecl, public
Redeclarable {
LLVM_PREFERRED_TYPE(bool)
unsigned IsCXXCondDecl : 1;
+
+/// Whether this is a __block variable that is captured by a block
+/// referenced in its own initializer.
+LLVM_PREFERRED_TYPE(bool)
+unsigned CapturedByOwnInit : 1;
};
union {
@@ -1588,10 +1593,23 @@ class VarDecl : public DeclaratorDecl, public
Redeclarable {
/// escaping block.
bool isNonEscapingByref() const;
- void setEscapingByref() {
-NonParmVarDeclBits.EscapingByref = true;
+ void setEscapingByref();
+
+ /// Indicates this is a __block variable that is captured by a block
+ /// referenced in its own initializer.
+ bool isCapturedByOwnInit() const {
+return !isa(this) && NonParmVarDeclBits.CapturedByOwnInit;
}
+ void setCapturedByOwnInit() {
+assert(!isa(this));
+NonParmVarDeclBits.CapturedByOwnInit = true;
+ }
+
+ /// Check if this is a __block variable which needs to be initialized
+ /// directly on the heap.
+ bool needsInitOnHeap() const;
+
bool isCXXCondDecl() const {
return isa(this) ? false : NonParmVarDeclBits.IsCXXCondDecl;
}
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td
b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 63e951daec7477..859f218c2779de 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -2356,6 +2356,12 @@ def note_var_fixit_add_initialization : Note<
def note_uninit_fixit_remove_cond : Note<
"remove the %select{'%1' if its condition|condition if it}0 "
"is always %select{false|true}2">;
+def warn_implicit_block_var_alloc : Warning<
+ "variable %q0 will be initialized in a heap allocation">,
+ InGroup>, DefaultIgnore;
+def note_because_captured_by_block : Note<
+ "because it is captured by a block used in its own initializer">;
+
def err_init_incomplete_type : Error<"initialization of incomplete type %0">;
def err_list_init_in_parens : Error<
"cannot initialize %select{non-class|reference}0 type %1 with a "
diff --git a/clang/lib/AST/Decl.cpp b/clang/lib/AST/Decl.cpp
index 474e0ccde5bbf7..fda9010ba126d7 100644
--- a/clang/lib/AST/Decl.cpp
+++ b/clang/lib/AST/Decl.cpp
@@ -2680,6 +2680,19 @@ bool VarDecl::isNonEscapingByref() const {
return hasAttr() && !NonParmVarDeclBits.EscapingByref;
}
+void VarDecl::setEscapingByref() {
+ assert(!isa(this) && hasAttr());
+ NonParmVarDeclBits.EscapingByref = true;
+}
+
+bool VarDecl::needsInitOnHeap() const {
+ // We need direct initialization on the heap for self-capturing __block
+ // variables of types that cause CodeGenFunction::getEvaluationKind to return
+ // TEK_Aggregate. The only such types that can be captured are records.
+ return isCapturedByOwnInit() &&
+ getType().getAtomicUnqualifiedType()->isRecordType();
+}
+
bool VarDecl::hasDependentAlignment() const {
QualType T = getType();
return T->isDependentType() || T->isUndeducedType() ||
diff --git a/clang/lib/CodeGen/CGBlocks.cpp b/clang/lib/CodeGen/CGBlocks.cpp
index 2742c39965b2c8..4bea057a15a52c 100644
--- a/clang/lib/CodeGen/CGBlocks.cpp
+++ b/clang/lib/CodeGen/CGBlocks.cpp
@@ -45,6 +45,13 @@ CGBlockInfo::CGBlockInfo(const BlockDecl *block, StringRef
name)
name = name.substr(1);
}
+BlockByrefHelpers::BlockByrefHelpers(const
[clang] [clang] Fix self-capturing `__block` variables (PR #89475)
llvmbot wrote:
@llvm/pr-subscribers-clang-modules
Author: None (ille-apple)
Changes
This is an updated version of https://reviews.llvm.org/D90434 from 2020. Due
to time constraints I stopped working on the patch, and unfortunately never got
back around to it until now.
Clang special-cases `__block` variables whose initializer contains a block
literal that captures the variable itself. But this special case doesn't work
for variables of record types, causing Clang to emit broken code.
Fix this by initializing such variables directly on the heap, and add a warning
for when this needs to be done.
### Background on the special case
`__block` variables are unique in that their address can change during the
lifetime of the variable. Each `__block` variable starts on the stack. But
when a block is retained using `Block_copy`, both the block itself and any
`__block` variables it captures are moved to the heap.
What's tricky is when this move happens while the variable is still being
initialized. For example:
```c
int copyBlockAndReturnInt(void (^block)(void)) {
Block_copy(block);
return 42;
}
int main() {
__block int val = copyBlockAndReturnInt(^{
printf("%d\n", val);
});
}
```
Here, when `Block_copy` is called, `val` is moved to the heap while still in an
uninitialized state. Then when `copyBlockAndReturnInt` returns, `main` needs
to store the return value to the variable's new location, not to the original
stack slot.
This can only happen if a block literal that captures the variable is located
syntactically within the initializer itself. If it's before the initializer
then it couldn't capture the variable, and if it's after the initializer then
it couldn't execute during initialization.
So Clang detects when such a literal exists inside the initializer;
`CGDecl.cpp` refers to this condition as `capturedByInit`. If it does, then
the initialization code operates in a special mode. In several functions in
that file, there's an `LValue` object and an accompanying `capturedByInit`
flag. If `capturedByInit` is false (the typical case), then the lvalue points
to the object being initialized. But if it's true, the lvalue points to the
byref header. Then, depending on the type of the variable, you end up at one
of many checks of the form:
```cpp
if (capturedByInit)
drillIntoBlockVariable(*this, lvalue, castVarDecl(D));
```
`drillIntoBlockVariable` emits a load of the forwarding pointer from the byref
header (to find the object's current location), and then modifies `lvalue` to
point to the object, unifying the two cases. Importantly, this happens at the
last minute: after emitting code to evaluate the initializer, but before
emitting the `store` instruction to write the evaluated value into the variable.
Okay, but then why are there two different code paths? If the address
calculation can be safely deferred, why not just do that all the time rather
than having a separate mode just for self-capturing `__block` variables?
Because it *can't* always be safely deferred.
### Harder cases
First, consider the case where variable being initialized is a C struct. This
is already problematic. `drillIntoBlockVariable` is supposed to be called
after evaluating the initializer but before doing a `store`. But when
initializing a struct, Clang doesn't evaluate the whole initializer before
storing. Instead it evaluates the first field, stores the first field,
evaluates the second field, stores the second field, and so on. This is
actually necessary for correctness[^1] given code like:
```c
struct Foo { int a, b; };
struct Foo foo = {1, foo.a};
```
But if any one of the field evaluations can move the object, then Clang would
need to reload the forwarding pointer before storing each field. And that
would need to apply recursively to any subfields. Such a scheme is definitely
possible, but Clang doesn't implement it.
And if instead of a C struct you have a C++ class, supporting relocation to the
heap becomes flat-out impossible. Consider:
```cpp
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); // shouldn't change!
}
};
__block Foo foo(^{
printf("%p\n", foo);
});
```
C++ constructors can observe the address of the object being constructed, and
expect that address not to change. Once the object has finished being
constructed, it can be relocated by calling the move or copy constructor, and
that's what Clang uses for block captures in the non-self-capturing case. But
in this case, `Block_copy` is called while the stack object is still running
its constructor. It would be nonsensical to try to move out of it in this
state.
### Clang's current behavior
So what does Clang currently do? Well, for both C structs and C++ classes (as
well as unions[^2]), it runs into this TODO dating
[clang] [clang] Fix self-capturing `__block` variables (PR #89475)
llvmbot wrote:
@llvm/pr-subscribers-clang
Author: None (ille-apple)
Changes
This is an updated version of https://reviews.llvm.org/D90434 from 2020. Due
to time constraints I stopped working on the patch, and unfortunately never got
back around to it until now.
Clang special-cases `__block` variables whose initializer contains a block
literal that captures the variable itself. But this special case doesn't work
for variables of record types, causing Clang to emit broken code.
Fix this by initializing such variables directly on the heap, and add a warning
for when this needs to be done.
### Background on the special case
`__block` variables are unique in that their address can change during the
lifetime of the variable. Each `__block` variable starts on the stack. But
when a block is retained using `Block_copy`, both the block itself and any
`__block` variables it captures are moved to the heap.
What's tricky is when this move happens while the variable is still being
initialized. For example:
```c
int copyBlockAndReturnInt(void (^block)(void)) {
Block_copy(block);
return 42;
}
int main() {
__block int val = copyBlockAndReturnInt(^{
printf("%d\n", val);
});
}
```
Here, when `Block_copy` is called, `val` is moved to the heap while still in an
uninitialized state. Then when `copyBlockAndReturnInt` returns, `main` needs
to store the return value to the variable's new location, not to the original
stack slot.
This can only happen if a block literal that captures the variable is located
syntactically within the initializer itself. If it's before the initializer
then it couldn't capture the variable, and if it's after the initializer then
it couldn't execute during initialization.
So Clang detects when such a literal exists inside the initializer;
`CGDecl.cpp` refers to this condition as `capturedByInit`. If it does, then
the initialization code operates in a special mode. In several functions in
that file, there's an `LValue` object and an accompanying `capturedByInit`
flag. If `capturedByInit` is false (the typical case), then the lvalue points
to the object being initialized. But if it's true, the lvalue points to the
byref header. Then, depending on the type of the variable, you end up at one
of many checks of the form:
```cpp
if (capturedByInit)
drillIntoBlockVariable(*this, lvalue, castVarDecl(D));
```
`drillIntoBlockVariable` emits a load of the forwarding pointer from the byref
header (to find the object's current location), and then modifies `lvalue` to
point to the object, unifying the two cases. Importantly, this happens at the
last minute: after emitting code to evaluate the initializer, but before
emitting the `store` instruction to write the evaluated value into the variable.
Okay, but then why are there two different code paths? If the address
calculation can be safely deferred, why not just do that all the time rather
than having a separate mode just for self-capturing `__block` variables?
Because it *can't* always be safely deferred.
### Harder cases
First, consider the case where variable being initialized is a C struct. This
is already problematic. `drillIntoBlockVariable` is supposed to be called
after evaluating the initializer but before doing a `store`. But when
initializing a struct, Clang doesn't evaluate the whole initializer before
storing. Instead it evaluates the first field, stores the first field,
evaluates the second field, stores the second field, and so on. This is
actually necessary for correctness[^1] given code like:
```c
struct Foo { int a, b; };
struct Foo foo = {1, foo.a};
```
But if any one of the field evaluations can move the object, then Clang would
need to reload the forwarding pointer before storing each field. And that
would need to apply recursively to any subfields. Such a scheme is definitely
possible, but Clang doesn't implement it.
And if instead of a C struct you have a C++ class, supporting relocation to the
heap becomes flat-out impossible. Consider:
```cpp
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); // shouldn't change!
}
};
__block Foo foo(^{
printf("%p\n", foo);
});
```
C++ constructors can observe the address of the object being constructed, and
expect that address not to change. Once the object has finished being
constructed, it can be relocated by calling the move or copy constructor, and
that's what Clang uses for block captures in the non-self-capturing case. But
in this case, `Block_copy` is called while the stack object is still running
its constructor. It would be nonsensical to try to move out of it in this
state.
### Clang's current behavior
So what does Clang currently do? Well, for both C structs and C++ classes (as
well as unions[^2]), it runs into this TODO dating to 2011:
[clang] [clang] Fix self-capturing `__block` variables (PR #89475)
github-actions[bot] wrote: Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using `@` followed by their GitHub username. If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the [LLVM GitHub User Guide](https://llvm.org/docs/GitHub.html). You can also ask questions in a comment on this PR, on the [LLVM Discord](https://discord.com/invite/xS7Z362) or on the [forums](https://discourse.llvm.org/). https://github.com/llvm/llvm-project/pull/89475 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Fix self-capturing `__block` variables (PR #89475)
https://github.com/ille-apple created
https://github.com/llvm/llvm-project/pull/89475
This is an updated version of https://reviews.llvm.org/D90434 from 2020. Due
to time constraints I stopped working on the patch, and unfortunately never got
back around to it until now.
Clang special-cases `__block` variables whose initializer contains a block
literal that captures the variable itself. But this special case doesn't work
for variables of record types, causing Clang to emit broken code.
Fix this by initializing such variables directly on the heap, and add a warning
for when this needs to be done.
### Background on the special case
`__block` variables are unique in that their address can change during the
lifetime of the variable. Each `__block` variable starts on the stack. But
when a block is retained using `Block_copy`, both the block itself and any
`__block` variables it captures are moved to the heap.
What's tricky is when this move happens while the variable is still being
initialized. For example:
```c
int copyBlockAndReturnInt(void (^block)(void)) {
Block_copy(block);
return 42;
}
int main() {
__block int val = copyBlockAndReturnInt(^{
printf("%d\n", val);
});
}
```
Here, when `Block_copy` is called, `val` is moved to the heap while still in an
uninitialized state. Then when `copyBlockAndReturnInt` returns, `main` needs
to store the return value to the variable's new location, not to the original
stack slot.
This can only happen if a block literal that captures the variable is located
syntactically within the initializer itself. If it's before the initializer
then it couldn't capture the variable, and if it's after the initializer then
it couldn't execute during initialization.
So Clang detects when such a literal exists inside the initializer;
`CGDecl.cpp` refers to this condition as `capturedByInit`. If it does, then
the initialization code operates in a special mode. In several functions in
that file, there's an `LValue` object and an accompanying `capturedByInit`
flag. If `capturedByInit` is false (the typical case), then the lvalue points
to the object being initialized. But if it's true, the lvalue points to the
byref header. Then, depending on the type of the variable, you end up at one
of many checks of the form:
```cpp
if (capturedByInit)
drillIntoBlockVariable(*this, lvalue, cast(D));
```
`drillIntoBlockVariable` emits a load of the forwarding pointer from the byref
header (to find the object's current location), and then modifies `lvalue` to
point to the object, unifying the two cases. Importantly, this happens at the
last minute: after emitting code to evaluate the initializer, but before
emitting the `store` instruction to write the evaluated value into the variable.
Okay, but then why are there two different code paths? If the address
calculation can be safely deferred, why not just do that all the time rather
than having a separate mode just for self-capturing `__block` variables?
Because it *can't* always be safely deferred.
### Harder cases
First, consider the case where variable being initialized is a C struct. This
is already problematic. `drillIntoBlockVariable` is supposed to be called
after evaluating the initializer but before doing a `store`. But when
initializing a struct, Clang doesn't evaluate the whole initializer before
storing. Instead it evaluates the first field, stores the first field,
evaluates the second field, stores the second field, and so on. This is
actually necessary for correctness[^1] given code like:
```c
struct Foo { int a, b; };
struct Foo foo = {1, foo.a};
```
But if any one of the field evaluations can move the object, then Clang would
need to reload the forwarding pointer before storing each field. And that
would need to apply recursively to any subfields. Such a scheme is definitely
possible, but Clang doesn't implement it.
And if instead of a C struct you have a C++ class, supporting relocation to the
heap becomes flat-out impossible. Consider:
```cpp
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); // shouldn't change!
}
};
__block Foo foo(^{
printf("%p\n", );
});
```
C++ constructors can observe the address of the object being constructed, and
expect that address not to change. Once the object has finished being
constructed, it can be relocated by calling the move or copy constructor, and
that's what Clang uses for block captures in the non-self-capturing case. But
in this case, `Block_copy` is called while the stack object is still running
its constructor. It would be nonsensical to try to move out of it in this
state.
### Clang's current behavior
So what does Clang currently do? Well, for both C structs and C++ classes (as
well as unions[^2]), it runs into this TODO dating to 2011:
