[PATCH] D64464: [CodeGen] Emit destructor calls for non-trivial C structs

[John McCall via Phabricator via cfe-commits]

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<BlockDecl *, CompoundLiteralExpr *>;
----------------
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<ExprWithCleanups::CleanupObject, 8> 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