[PATCH] D64464: [CodeGen] Emit destructor calls to destruct compound literals

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak marked an inline comment as done.
ahatanak added inline comments.


================
Comment at: clang/lib/CodeGen/CGBlocks.cpp:869
+      if (auto *BD = C.dyn_cast<BlockDecl *>())
+        enterBlockScope(*this, BD);
   }
----------------
rjmccall wrote:
> I wonder if we could just switch blocks to the same thing.
I think we can, but I haven't tried. We can fix it in a separate patch.


================
Comment at: clang/lib/CodeGen/CGExprAgg.cpp:664
+  // Block-scope compound literals are destroyed at the end of the enclosing
+  // scope in C.
+  bool Destruct =
----------------
I fixed the way `IsExternallyDestructed` is used based on the comment in the other review.


================
Comment at: clang/lib/Serialization/ASTWriterStmt.cpp:1729
+      Record.push_back(serialization::COK_CompoundLiteral);
+      Record.AddStmt(CLE);
+    }
----------------
rjmccall wrote:
> Will this just serialize a second copy of the compound literal expression?
This should emit a `serialization::STMT_REF_PTR` record, which is a reference to the previously serialized compound literal (see `ASTWriter::WriteSubStmt`).


================
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:
> ahatanak wrote:
> > rjmccall wrote:
> > > ahatanak wrote:
> > > > rjmccall wrote:
> > > > > ahatanak wrote:
> > > > > > rjmccall wrote:
> > > > > > > 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. :)
> > > > > > There is a check `E->isFileScope()` above this. Is that sufficient to check for block-scoped compound literals?
> > > > > That plus the C/C++ difference; compound literals in C++ are just temporaries.
> > > > I haven't been able to come up with a piece of C++ code that executes `EmitCompoundLiteralLValue`. The following code gets rejected because you can't take the address of a temporary object in C++:
> > > > 
> > > > ```
> > > > StrongSmall *p = &(StrongSmall){ 1, 0 };
> > > > ```
> > > > 
> > > > If a bind a reference to it, `AggExprEmitter::VisitCompoundLiteralExpr` is called.
> > > That makes sense; they're not gl-values in C++.  It would be reasonable to assert that.  But the C++ point does apply elsewhere.
> > It turns out this function is called in C++ when the compound literal is a vector type, so I've just added a check for C++ instead of an assert.
> Really?  Is the expression actually an l-value in this case somehow?
I see this function being called when `ScalarExprEmitter::VisitCompoundLiteralExpr` calls `EmitLoadOfLValue`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64464/new/

https://reviews.llvm.org/D64464