[PATCH] D64464: [CodeGen] Emit destructor calls for non-trivial C structs
ahatanak created this revision.
ahatanak added a reviewer: rjmccall.
ahatanak added a project: clang.
Herald added subscribers: dexonsmith, jkorous.
This patch fixes a bug in IRGen where it wasn't calling the destructors for
non-trivial C structs in the following cases:
- member access of structs that are returned from a function.
- compound literal.
- load from volatile types.
rdar://problem/51867864
Repository:
rC Clang
https://reviews.llvm.org/D64464
Files:
lib/CodeGen/CGExpr.cpp
lib/CodeGen/CGExprAgg.cpp
test/CodeGenObjC/strong-in-c-struct.m
Index: test/CodeGenObjC/strong-in-c-struct.m
===
--- test/CodeGenObjC/strong-in-c-struct.m
+++ test/CodeGenObjC/strong-in-c-struct.m
@@ -521,7 +521,9 @@
// CHECK: define void @test_copy_constructor_StrongVolatile0(
// CHECK: call void @__copy_constructor_8_8_t0w4_sv8(
+// CHECK-NOT: call
// CHECK: call void @__destructor_8_sv8(
+// CHECK-NOT: call
// CHECK: define linkonce_odr hidden void @__copy_constructor_8_8_t0w4_sv8(
// CHECK: %[[V8:.*]] = load volatile i8*, i8** %{{.*}}, align 8
@@ -718,4 +720,42 @@
VolatileArray t = *a;
}
+// CHECK: define void @test_member_access(
+// CHECK: %[[TMP:.*]] = alloca %[[STRUCT_STRONGSMALL]],
+// CHECK: %[[V3:.*]] = bitcast %[[STRUCT_STRONGSMALL]]* %[[TMP]] to i8**
+// CHECK: call void @__destructor_8_s8(i8** %[[V3]])
+
+void test_member_access(void) {
+ id t = getStrongSmall().f1;
+}
+
+// CHECK: define void @test_compound_literal(
+// CHECK: %[[_COMPOUNDLITERAL:.*]] = alloca %[[STRUCT_STRONGSMALL]],
+// CHECK: %[[V0:.*]] = bitcast %[[STRUCT_STRONGSMALL]]* %[[_COMPOUNDLITERAL]] to i8**
+// CHECK: call void @__destructor_8_s8(i8** %[[V0]])
+
+void test_compound_literal(void) {
+ StrongSmall *p = &(StrongSmall){ 1, 0 };
+}
+
+// CHECK: define void @test_compound_literal2(
+// CHECK: %[[AGG_TMP_ENSURED:.*]] = alloca %[[STRUCT_STRONGSMALL]],
+// CHECK: %[[V0:.*]] = bitcast %[[STRUCT_STRONGSMALL]]* %[[AGG_TMP_ENSURED]] to i8**
+// CHECK: call void @__destructor_8_s8(i8** %[[V0]])
+
+void test_compound_literal2(void) {
+ (void)(StrongSmall){ 1, 0 };
+}
+
+// CHECK: define void @test_volatile_variable_reference(
+// CHECK: %[[AGG_TMP_ENSURED:.*]] = alloca %[[STRUCT_STRONGSMALL]], align 8
+// CHECK: %[[V1:.*]] = bitcast %[[STRUCT_STRONGSMALL]]* %[[AGG_TMP_ENSURED]] to i8**
+// CHECK: call void @__copy_constructor_8_8_tv0w32_sv8(i8** %[[V1]], i8** %{{.*}})
+// CHECK: %[[V3:.*]] = bitcast %[[STRUCT_STRONGSMALL]]* %[[AGG_TMP_ENSURED]] to i8**
+// CHECK: call void @__destructor_8_s8(i8** %[[V3]])
+
+void test_volatile_variable_reference(volatile StrongSmall *a) {
+ (void)*a;
+}
+
#endif /* USESTRUCT */
Index: lib/CodeGen/CGExprAgg.cpp
===
--- lib/CodeGen/CGExprAgg.cpp
+++ lib/CodeGen/CGExprAgg.cpp
@@ -245,7 +245,7 @@
const Expr *E, llvm::function_ref EmitCall) {
QualType RetTy = E->getType();
bool RequiresDestruction =
- Dest.isIgnored() &&
+ !Dest.isExternallyDestructed() &&
RetTy.isDestructedType() == QualType::DK_nontrivial_c_struct;
// If it makes no observable difference, save a memcpy + temporary.
@@ -656,6 +656,16 @@
}
AggValueSlot Slot = EnsureSlot(E->getType());
+
+ // Push a destructor cleanup if this is a non-trivial C struct. A compound
+ // literal object that doesn't have a file scope has automatic
+ // storage duration.
+ if (!Dest.isExternallyDestructed() &&
+ E->getType().isDestructedType() == QualType::DK_nontrivial_c_struct) {
+CGF.pushDestroy(QualType::DK_nontrivial_c_struct, Slot.getAddress(),
+E->getType());
+Dest.setExternallyDestructed();
+ }
CGF.EmitAggExpr(E->getInitializer(), Slot);
}
@@ -792,6 +802,12 @@
// into existence.
if (E->getSubExpr()->getType().isVolatileQualified()) {
EnsureDest(E->getType());
+ if (!Dest.isExternallyDestructed() &&
+ E->getType().isDestructedType() == QualType::DK_nontrivial_c_struct) {
+CGF.pushDestroy(QualType::DK_nontrivial_c_struct, Dest.getAddress(),
+E->getType());
+Dest.setExternallyDestructed();
+ }
return Visit(E->getSubExpr());
}
Index: lib/CodeGen/CGExpr.cpp
===
--- lib/CodeGen/CGExpr.cpp
+++ lib/CodeGen/CGExpr.cpp
@@ -4093,6 +4093,12 @@
EmitAnyExprToMem(InitExpr, DeclPtr, E->getType().getQualifiers(),
/*Init*/ true);
+ // Push a destructor cleanup if this is a non-trivial C struct. A compound
+ // literal object that doesn't have a file scope has automatic
+ // storage duration.
+ if (E->getType().isDestructedType() == QualType::DK_nontrivial_c_struct)
+pushDestroy(QualType::DK_nontrivial_c_struct, DeclPtr, E->getType());
+
return Result;
}
@@ -4574,6 +4580,10 @@
LValue CodeGenFunction::EmitCallExprLValue(const CallExpr *E) {
RValue RV
[PATCH] D64464: [CodeGen] Emit destructor calls for non-trivial C structs
ahatanak marked an inline comment as done. ahatanak added inline comments. Comment at: lib/CodeGen/CGExprAgg.cpp:248 bool RequiresDestruction = - Dest.isIgnored() && + !Dest.isExternallyDestructed() && RetTy.isDestructedType() == QualType::DK_nontrivial_c_struct; This change wasn't needed to fix the bugs, but I think `isExternallyDestructed` instead of `isIgnored` should be called to determine whether the returned value requires destruction. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64464/new/ https://reviews.llvm.org/D64464 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D64464: [CodeGen] Emit destructor calls for non-trivial C structs
rjmccall added inline comments. Comment at: lib/CodeGen/CGExpr.cpp:4100 + if (E->getType().isDestructedType() == QualType::DK_nontrivial_c_struct) +pushDestroy(QualType::DK_nontrivial_c_struct, DeclPtr, E->getType()); + Unfortunately, the lifetime of compound literals in C is not this simple; they're like blocks in that they're destroyed at the end of the enclosing scope rather than at the end of the current statement. (The cleanup here will be popped at the end of the full-expression if we've entered an `ExprWithCleanups`.) And the l-value case is exactly the case where this matters. I think you need to do something like what we do with blocks, where we record all the blocks in the full-expression on the `ExprWithCleanups` so that we can push an inactive cleanup for them and then activate it when we emit the block. Comment at: lib/CodeGen/CGExprAgg.cpp:248 bool RequiresDestruction = - Dest.isIgnored() && + !Dest.isExternallyDestructed() && RetTy.isDestructedType() == QualType::DK_nontrivial_c_struct; ahatanak wrote: > This change wasn't needed to fix the bugs, but I think > `isExternallyDestructed` instead of `isIgnored` should be called to determine > whether the returned value requires destruction. Agreed. Comment at: lib/CodeGen/CGExprAgg.cpp:668 +Dest.setExternallyDestructed(); + } CGF.EmitAggExpr(E->getInitializer(), Slot); The cleanup shouldn't be pushed until after you've finished evaluating the initializer. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64464/new/ https://reviews.llvm.org/D64464 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D64464: [CodeGen] Emit destructor calls for non-trivial C structs
ahatanak marked an inline comment as done. ahatanak added inline comments. Comment at: lib/CodeGen/CGExpr.cpp:4647 +pushDestroy(QualType::DK_nontrivial_c_struct, RV.getAggregateAddress(), +E->getType()); + rjmccall wrote: > Does `EmitCallExpr` not enter a cleanup when it returns an aggregate that's > not into an externally-destructed slot? That seems wrong and dangerous. I'm going to split this patch into two parts, one for compound literals and the other for everything else. The patch is getting too large and I also found another bug: a cleanup isn't pushed for ObjC message send. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64464/new/ https://reviews.llvm.org/D64464 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D64464: [CodeGen] Emit destructor calls for non-trivial C structs
martong added inline comments.
Comment at: test/Import/objc-arc/Inputs/cleanup-objects.m:6
+id getObj(int c, id a) {
+ // Commenting out the following line because AST importer crashes when trying
+ // to import a BlockExpr.
Perhaps then this patch depends on another patch which implements the import of
a BlockExpr?
Or maybe the branch `if (auto *BD = From.dyn_cast())` should be
left out from the ASTImporter code, and this way BlockExpr and BlockDecl would
be implemented later in another patch.
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D64464/new/
https://reviews.llvm.org/D64464
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D64464: [CodeGen] Emit destructor calls for non-trivial C structs
ahatanak updated this revision to Diff 212942.
ahatanak marked 2 inline comments as done.
ahatanak added a comment.
Herald added a reviewer: martong.
Herald added a reviewer: shafik.
- Emit member access, compound literal, and call expressions as subexpressions
of `ExprWithCleanups` if the expressions are of C struct types that require
non-trivial destruction. This fixes the bug in IRGen where it was destructing
the function return at the end of the enclosing scope rather than at the end of
the full expression (see the changes made in test/CodeGenObjC/arc.m).
- Add compound literal expressions with automatic storage duration to the list
of cleanup objects of `ExprWithCleanups` if the expressions have C struct types
requiring non-trivial destruction. This enables IRGen to destruct the compound
literals at the end of their enclosing scopes.
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D64464/new/
https://reviews.llvm.org/D64464
Files:
include/clang/AST/ASTImporter.h
include/clang/AST/ExprCXX.h
include/clang/AST/TextNodeDumper.h
include/clang/Basic/DiagnosticSemaKinds.td
include/clang/Sema/Sema.h
include/clang/Serialization/ASTBitCodes.h
lib/AST/ASTImporter.cpp
lib/AST/JSONNodeDumper.cpp
lib/AST/TextNodeDumper.cpp
lib/CodeGen/CGBlocks.cpp
lib/CodeGen/CGCleanup.cpp
lib/CodeGen/CGDecl.cpp
lib/CodeGen/CGExpr.cpp
lib/CodeGen/CGExprAgg.cpp
lib/CodeGen/CodeGenFunction.cpp
lib/CodeGen/CodeGenFunction.h
lib/Sema/JumpDiagnostics.cpp
lib/Sema/SemaExpr.cpp
lib/Serialization/ASTReaderStmt.cpp
lib/Serialization/ASTWriterStmt.cpp
test/AST/ast-dump-objc-arc-json.m
test/AST/ast-dump-stmt.m
test/CodeGenObjC/arc.m
test/CodeGenObjC/strong-in-c-struct.m
test/Import/objc-arc/Inputs/cleanup-objects.m
test/Import/objc-arc/test-cleanup-object.m
test/PCH/non-trivial-c-compound-literal.m
test/SemaObjC/strong-in-c-struct.m
tools/clang-import-test/clang-import-test.cpp
Index: tools/clang-import-test/clang-import-test.cpp
===
--- tools/clang-import-test/clang-import-test.cpp
+++ tools/clang-import-test/clang-import-test.cpp
@@ -64,6 +64,10 @@
llvm::cl::desc("The language to parse (default: c++)"),
llvm::cl::init("c++"));
+static llvm::cl::opt
+ObjCARC("objc-arc", llvm::cl::init(false),
+llvm::cl::desc("Emable ObjC ARC"));
+
static llvm::cl::opt DumpAST("dump-ast", llvm::cl::init(false),
llvm::cl::desc("Dump combined AST"));
@@ -185,6 +189,8 @@
Inv->getLangOpts()->ObjC = 1;
}
}
+ Inv->getLangOpts()->ObjCAutoRefCount = ObjCARC;
+
Inv->getLangOpts()->Bool = true;
Inv->getLangOpts()->WChar = true;
Inv->getLangOpts()->Blocks = true;
Index: test/SemaObjC/strong-in-c-struct.m
===
--- test/SemaObjC/strong-in-c-struct.m
+++ test/SemaObjC/strong-in-c-struct.m
@@ -54,3 +54,21 @@
func(^{ func2(x); });
goto *ips; // expected-error {{cannot jump}}
}
+
+void test_compound_literal0(int cond, id x) {
+ switch (cond) {
+ case 0:
+(void)(Strong){ .a = x }; // expected-note {{jump enters lifetime of a compound literal that is non-trivial to destruct}}
+break;
+ default: // expected-error {{cannot jump from switch statement to this case label}}
+break;
+ }
+}
+
+void test_compound_literal1(id x) {
+ static void *ips[] = { &&L0 };
+L0: // expected-note {{possible target of indirect goto}}
+ ;
+ (void)(Strong){ .a = x }; // expected-note {{jump exits lifetime of a compound literal that is non-trivial to destruct}}
+ goto *ips; // expected-error {{cannot jump}}
+}
Index: test/PCH/non-trivial-c-compound-literal.m
===
--- /dev/null
+++ test/PCH/non-trivial-c-compound-literal.m
@@ -0,0 +1,29 @@
+// RUN: %clang_cc1 -triple x86_64-apple-darwin10 -x objective-c -fobjc-arc -emit-pch -o %t %s
+// RUN: %clang_cc1 -triple x86_64-apple-darwin10 -x objective-c -fobjc-arc -include-pch %t -emit-llvm -o - %s | FileCheck %s
+
+#ifndef HEADER
+#define HEADER
+
+typedef struct {
+ id f;
+} S;
+
+static inline id getObj(id a) {
+ S *p = &(S){ .f = a };
+ return p->f;
+}
+
+#else
+
+// CHECK: %[[STRUCT_S:.*]] = type { i8* }
+
+// CHECK: define internal i8* @getObj(
+// CHECK: %[[_COMPOUNDLITERAL:.*]] = alloca %[[STRUCT_S]],
+// CHECK: %[[V5:.*]] = bitcast %[[STRUCT_S]]* %[[_COMPOUNDLITERAL]] to i8**
+// CHECK: call void @__destructor_8_s0(i8** %[[V5]])
+
+id test(id a) {
+ return getObj(a);
+}
+
+#endif
Index: test/Import/objc-arc/test-cleanup-object.m
===
--- /dev/null
+++ test/Import/objc-arc/test-cleanup-object.m
@@ -0,0 +1,10 @@
+// RUN: clang-import-test -x objective-c -objc-arc -import %S/Inputs/cleanup-objects.m -dump-ast -expression %s | FileCheck %s
+
+// CHECK: FunctionDecl {{.*}} getO
[PATCH] D64464: [CodeGen] Emit destructor calls for non-trivial C structs
rjmccall added inline comments.
Herald added a subscriber: rnkovacs.
Comment at: include/clang/AST/ExprCXX.h:3220
+ /// It's useful to remember the set of blocks and compound literals; we could
+ /// also remember the set of temporaries, but there's currently no need.
+ using CleanupObject = llvm::PointerUnion;
Might be worth spelling out `and block-scoped compound literals` here.
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:5171
+def note_enters_compound_literal_scope : Note<
+ "jump enters lifetime of a compound literal that is non-trivial to
destruct">;
Ah, good catch.
Comment at: include/clang/Sema/Sema.h:587
/// cleanup that are created by the current full expression. The
/// element type here is ExprWithCleanups::Object.
+ SmallVector ExprCleanupObjects;
I think the second sentence in this comment is no longer useful; you can just
strike it.
Comment at: lib/CodeGen/CGBlocks.cpp:863
/// clean up. This is in this file because, at the moment, the only
/// kind of cleanup object is a BlockDecl*.
void CodeGenFunction::enterNonTrivialFullExpression(const FullExpr *E) {
This comment is no longer accurate. If you want to move the function, please
do so in a separate commit, though.
Comment at: lib/CodeGen/CGCleanup.cpp:1283
+ // the end of its enclosing block. Push an inactive cleanup here so that its
+ // destructor is called when the lifetime ends.
+ QualType Ty = CLE->getType();
Please include in the comment here that this doesn't apply in C++, where the
compound literal has temporary lifetime.
Can we clarify that we're talking about block-scope literals in all the new
method names?
Comment at: lib/CodeGen/CGDecl.cpp:555
+ bool useEHCleanupForArray =
+ flags.isForNormalCleanup() && this->useEHCleanupForArray;
+ CGF.emitDestroy(CGF.CompoundLiteralCleanupInfoMap[CLE].getAddr(),
It'd be nice to not shadow the enclosing variable.
Comment at: lib/CodeGen/CGExpr.cpp:4100
+ if (E->getType().isDestructedType() == QualType::DK_nontrivial_c_struct)
+pushDestroy(QualType::DK_nontrivial_c_struct, DeclPtr, E->getType());
+
rjmccall wrote:
> Unfortunately, the lifetime of compound literals in C is not this simple;
> they're like blocks in that they're destroyed at the end of the enclosing
> scope rather than at the end of the current statement. (The cleanup here will
> be popped at the end of the full-expression if we've entered an
> `ExprWithCleanups`.) And the l-value case is exactly the case where this
> matters.
>
> I think you need to do something like what we do with blocks, where we record
> all the blocks in the full-expression on the `ExprWithCleanups` so that we
> can push an inactive cleanup for them and then activate it when we emit the
> block.
Can we make the check here something like (1) this is a block-scope compound
literal and (2) it has a non-trivially-destructed type (of any kind)? That way
we're not conflating two potentially unrelated elements, the lifetime of the
object and the kinds of types that can be constructed by the literal.
Oh, actually, there's a concrete reason to do this: C99 compound literals are
not required to have struct type; they can have any object type, including
arrays but also scalars. So we could, even without non-trivial C structs, have
a block-scope compound of type `__strong id[]`; I guess we've always just
gotten this wrong. Please add tests for this case. :)
Comment at: lib/CodeGen/CGExpr.cpp:4647
+pushDestroy(QualType::DK_nontrivial_c_struct, RV.getAggregateAddress(),
+E->getType());
+
Does `EmitCallExpr` not enter a cleanup when it returns an aggregate that's not
into an externally-destructed slot? That seems wrong and dangerous.
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D64464/new/
https://reviews.llvm.org/D64464
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
