[Sema] Relax an assertion in BuildStmtExpr

[ahatanak]

Summary

The assertion doesn't hold if there are temporaries created in the AsmStmt passed to the method.

[ahatanak]

ping

I think the assertion is assuming StmtExpr is created only for GNU statement expressions, but it's created for asm statements too (see https://github.com/llvm/llvm-project/commit/3050d9bdb84e322e122219d54aedfe2d8ef7c51c).

[akyrtzi]

The assertion was assuming "the expression doesn't need cleanups", have you considered adding a test that checks that the destructor of the temporary inside the asm statement is called, to ensure these temporaries are properly handled?

[ahatanak]

Add a codegen test that checks destructors for temporaries inside asm statements are called.

[rsmith]

clang/test/CodeGenCXX/asm.cpp

22

Please add another statement here after the asm statement to check that the destructor is called at the end of the asm statement, not at the }.

[rsmith]

clang/lib/Sema/SemaExpr.cpp

15772–15773

Is it possible to pass a flag into here to indicate if we're really building an asm statement? I don't think we want to remove the assert for a user-written statement expression that happens to begin with an asm statement.

It's ironic that we're building a statement expression to wrap an asm statement in order to make sure that cleanups are handled properly, and the StmtExpr is asserting because it doesn't expect to need to handle cleanups...

[ahatanak]

clang/lib/Sema/SemaExpr.cpp

15772–15773

Maybe we can add a flag to StmtExpr and set it in Sema::MaybeCreateStmtWithCleanups to distinguish StmtExprs that are created for gnu statement expressions from those that aren't?

If we don't want to use StmtExpr for asm statements, maybe we have to create a new Stmt type just for that purpose as suggested in Sema::MaybeCreateStmtWithCleanups.

[ahatanak]

Add a flag to StmtExpr that indicates whether it needs cleanup.

[ahatanak]

Check that the destructor is called at the end of the asm.

[ahatanak]

ping

[rjmccall]

The cleanups arise from expressions in the constraints, right? There are a number of places where ExprWithCleanups does not mean the cleanups are independently nested within that expression; it's notably not how it works ever for something as basic as a variable initializer. If we want the lifetime of temporaries in asm operands to include the full duration of the asm statement, I think we can just declare by fiat that that's how it works for AsmStmt instead of unnaturally wrapping things this way.

Or do we have a problem where the semantics are different for different kinds of asm statements?

[ahatanak]

No, I don't think there are any problems. We can probably just pop the cleanups at the end of the asm statement.

The original commit message said the asm calls had to be wrapped in StmtExprs because temporaries would get destroyed before the asm calls, but that doesn't seem to happen anymore. It looks like they'd be destroyed at the end of the enclosing scope instead.

[rjmccall]

Yeah, I think the appropriate thing is to just treat the entire asm statement like it's a single full-expression. As always, that implies an annoying lifetime for blocks and C compound literals.

[ahatanak]

Stop wrapping a GCCAsmStmt with an ExprWithCleanups. Instead, just pop the cleanups after the asm statement to destruct the temporaries.

[ahatanak]

I've removed the call to ActOnFinishFullStmt in CoroutineStmtBuilder::makeOnFallthrough too. BuildCoreturnStmt calls ActOnFinishFullExpr, so I don't think it's necessary to call ActOnFinishFullStmt after that.

[ahatanak]

Adding more people to get more eyes on the changes I made to SemaCoroutine.cpp.

[ChuanqiXu]

In D125936#3587542, @ahatanak wrote:

Adding more people to get more eyes on the changes I made to SemaCoroutine.cpp.

The change to SemaCoroutine.cpp might be good. But I feel like it is irreverent to the revision. If you want, could you please do the change in another revision?

[ahatanak]

Revert the changes made to SemaCoroutine.cpp.

[rjmccall]

clang/lib/CodeGen/CGStmt.cpp

2291 ↗

(On Diff #437566)

Do we need to do anything special when entering the full expressions to make sure that enclosing-block features like blocks and C compound literals have the right lifetime?

[ahatanak]

Fix misleading comment and add a test case for C compound literal.

clang/lib/CodeGen/CGStmt.cpp

2291 ↗

(On Diff #437566)

I don't think there is anything special we have to do here. The cleanups for both blocks and C compound literals are pushed by calling pushLifetimeExtendedDestroy so that their lifetime is extended till the end of the enclosing scope.

[rjmccall]

Okay, LGTM then

clang/include/clang/AST/Expr.h

 ///
 /// A StmtExpr is always an r-value; values "returned" out of a
 /// StmtExpr will be copied.
 class StmtExpr : public Expr {
   Stmt *SubStmt;
   SourceLocation LParenLoc, RParenLoc;
 public:
   StmtExpr(CompoundStmt *SubStmt, QualType T, SourceLocation LParenLoc,
-           SourceLocation RParenLoc, unsigned TemplateDepth)
+           SourceLocation RParenLoc, unsigned TemplateDepth,
+           bool NeedsCleanup = false)
       : Expr(StmtExprClass, T, VK_PRValue, OK_Ordinary), SubStmt(SubStmt),
         LParenLoc(LParenLoc), RParenLoc(RParenLoc) {
     setDependence(computeDependence(this, TemplateDepth));
     // FIXME: A templated statement expression should have an associated
     // DeclContext so that nested declarations always have a dependent context.
     StmtExprBits.TemplateDepth = TemplateDepth;
+    StmtExprBits.NeedsCleanup = NeedsCleanup;
   }
 
   /// Build an empty statement expression.
   explicit StmtExpr(EmptyShell Empty) : Expr(StmtExprClass, Empty) { }
 
   CompoundStmt *getSubStmt() { return cast<CompoundStmt>(SubStmt); }
   const CompoundStmt *getSubStmt() const { return cast<CompoundStmt>(SubStmt); }
   void setSubStmt(CompoundStmt *S) { SubStmt = S; }
 
   SourceLocation getBeginLoc() const LLVM_READONLY { return LParenLoc; }
   SourceLocation getEndLoc() const LLVM_READONLY { return RParenLoc; }
 
   SourceLocation getLParenLoc() const { return LParenLoc; }
   void setLParenLoc(SourceLocation L) { LParenLoc = L; }
   SourceLocation getRParenLoc() const { return RParenLoc; }
   void setRParenLoc(SourceLocation L) { RParenLoc = L; }
 
   unsigned getTemplateDepth() const { return StmtExprBits.TemplateDepth; }
 
+  bool needsCleanup() const { return StmtExprBits.NeedsCleanup; }
+
   static bool classof(const Stmt *T) {
     return T->getStmtClass() == StmtExprClass;
   }
 
   // Iterators
   child_range children() { return child_range(&SubStmt, &SubStmt+1); }
   const_child_range children() const {
     return const_child_range(&SubStmt, &SubStmt + 1);

clang/include/clang/AST/Stmt.h

class StmtExprBitfields {
     friend class ASTStmtReader;
     friend class StmtExpr;
 
     unsigned : NumExprBits;
 
     /// The number of levels of template parameters enclosing this statement
     /// expression. Used to determine if a statement expression remains
     /// dependent after instantiation.
-    unsigned TemplateDepth;
+    unsigned TemplateDepth : 7;
+
+    /// Whether this StmtExpr needs cleanup. This is always false when it is
+    /// created for a GNU statement expression.
+    unsigned NeedsCleanup : 1;
   };
 
   //===--- C++ Expression bitfields classes ---===//
 
   class CXXOperatorCallExprBitfields {
     friend class ASTStmtReader;
     friend class CXXOperatorCallExpr;
 

clang/include/clang/Sema/Sema.h

public:
   /// ActOnAddrLabel - Parse the GNU address of label extension: "&&foo".
   ExprResult ActOnAddrLabel(SourceLocation OpLoc, SourceLocation LabLoc,
                             LabelDecl *TheDecl);
 
   void ActOnStartStmtExpr();
   ExprResult ActOnStmtExpr(Scope *S, SourceLocation LPLoc, Stmt *SubStmt,
                            SourceLocation RPLoc);
   ExprResult BuildStmtExpr(SourceLocation LPLoc, Stmt *SubStmt,
-                           SourceLocation RPLoc, unsigned TemplateDepth);
+                           SourceLocation RPLoc, unsigned TemplateDepth,
+                           bool NeedsCleanup = false);
   // Handle the final expression in a statement expression.
   ExprResult ActOnStmtExprResult(ExprResult E);
   void ActOnStmtExprError();
 
   // __builtin_offsetof(type, identifier(.identifier|[expr])*)
   struct OffsetOfComponent {
     SourceLocation LocStart, LocEnd;
     bool isBrackets;  // true if [expr], false if .ident

clang/lib/Sema/SemaExpr.cpp

 }
 
 ExprResult Sema::ActOnStmtExpr(Scope *S, SourceLocation LPLoc, Stmt *SubStmt,
                                SourceLocation RPLoc) {
   return BuildStmtExpr(LPLoc, SubStmt, RPLoc, getTemplateDepth(S));
 }
 
 ExprResult Sema::BuildStmtExpr(SourceLocation LPLoc, Stmt *SubStmt,
-                               SourceLocation RPLoc, unsigned TemplateDepth) {
+                               SourceLocation RPLoc, unsigned TemplateDepth,
+                               bool NeedsCleanup) {
   assert(SubStmt && isa<CompoundStmt>(SubStmt) && "Invalid action invocation!");
   CompoundStmt *Compound = cast<CompoundStmt>(SubStmt);
 
   if (hasAnyUnrecoverableErrorsInThisFunction())
     DiscardCleanupsInEvaluationContext();
-  assert(!Cleanup.exprNeedsCleanups() &&
+  // Cleanups may be needed if temporaries are created in an AsmStmt.
+  assert((!Cleanup.exprNeedsCleanups() || NeedsCleanup) &&
          "cleanups within StmtExpr not correctly bound!");
   PopExpressionEvaluationContext();
 
   // FIXME: there are a variety of strange constraints to enforce here, for
   // example, it is not possible to goto into a stmt expression apparently.
   // More semantic analysis is needed.
 
   // If there are sub-stmts in the compound stmt, take the type of the last one
   // as the type of the stmtexpr.
   QualType Ty = Context.VoidTy;
   bool StmtExprMayBindToTemp = false;
   if (!Compound->body_empty()) {
     // For GCC compatibility we get the last Stmt excluding trailing NullStmts.
     if (const auto *LastStmt =
             dyn_cast<ValueStmt>(Compound->getStmtExprResult())) {
       if (const Expr *Value = LastStmt->getExprStmt()) {
         StmtExprMayBindToTemp = true;
         Ty = Value->getType();
       }
     }
   }
 
   // FIXME: Check that expression type is complete/non-abstract; statement
   // expressions are not lvalues.
-  Expr *ResStmtExpr =
-      new (Context) StmtExpr(Compound, Ty, LPLoc, RPLoc, TemplateDepth);
+  Expr *ResStmtExpr = new (Context)
+      StmtExpr(Compound, Ty, LPLoc, RPLoc, TemplateDepth, NeedsCleanup);
   if (StmtExprMayBindToTemp)
     return MaybeBindToTemporary(ResStmtExpr);
   return ResStmtExpr;
 }
 
 ExprResult Sema::ActOnStmtExprResult(ExprResult ER) {
   if (ER.isInvalid())
     return ExprError();

clang/lib/Sema/SemaExprCXX.cpp

Stmt *Sema::MaybeCreateStmtWithCleanups(Stmt *SubStmt) {
   // FIXME: In order to attach the temporaries, wrap the statement into
   // a StmtExpr; currently this is only used for asm statements.
   // This is hacky, either create a new CXXStmtWithTemporaries statement or
   // a new AsmStmtWithTemporaries.
   CompoundStmt *CompStmt = CompoundStmt::Create(
       Context, SubStmt, SourceLocation(), SourceLocation());
   Expr *E = new (Context)
       StmtExpr(CompStmt, Context.VoidTy, SourceLocation(), SourceLocation(),
-               /*FIXME TemplateDepth=*/0);
+               /*FIXME TemplateDepth=*/0, /*NeedsCleanup=*/true);
   return MaybeCreateExprWithCleanups(E);
 }
 
 /// Process the expression contained within a decltype. For such expressions,
 /// certain semantic checks on temporaries are delayed until this point, and
 /// are omitted for the 'topmost' call in the decltype expression. If the
 /// topmost call bound a temporary, strip that temporary off the expression.
 ExprResult Sema::ActOnDecltypeExpression(Expr *E) {

clang/lib/Sema/TreeTransform.h

ExprResult RebuildAddrLabelExpr(SourceLocation AmpAmpLoc,
     return getSema().ActOnAddrLabel(AmpAmpLoc, LabelLoc, Label);
   }
 
   /// Build a new GNU statement expression.
   ///
   /// By default, performs semantic analysis to build the new expression.
   /// Subclasses may override this routine to provide different behavior.
   ExprResult RebuildStmtExpr(SourceLocation LParenLoc, Stmt *SubStmt,
-                             SourceLocation RParenLoc, unsigned TemplateDepth) {
-    return getSema().BuildStmtExpr(LParenLoc, SubStmt, RParenLoc,
-                                   TemplateDepth);
+                             SourceLocation RParenLoc, unsigned TemplateDepth,
+                             bool NeedsCleanup) {
+    return getSema().BuildStmtExpr(LParenLoc, SubStmt, RParenLoc, TemplateDepth,
+                                   NeedsCleanup);
   }
 
   /// Build a new __builtin_choose_expr expression.
   ///
   /// By default, performs semantic analysis to build the new expression.
   /// Subclasses may override this routine to provide different behavior.
   ExprResult RebuildChooseExpr(SourceLocation BuiltinLoc,
                                      Expr *Cond, Expr *LHS, Expr *RHS,
TreeTransform<Derived>::TransformStmtExpr(StmtExpr *E) {
   if (!getDerived().AlwaysRebuild() && OldDepth == NewDepth &&
       SubStmt.get() == E->getSubStmt()) {
     // Calling this an 'error' is unintuitive, but it does the right thing.
     SemaRef.ActOnStmtExprError();
     return SemaRef.MaybeBindToTemporary(E);
   }
 
   return getDerived().RebuildStmtExpr(E->getLParenLoc(), SubStmt.get(),
-                                      E->getRParenLoc(), NewDepth);
+                                      E->getRParenLoc(), NewDepth,
+                                      E->needsCleanup());
 }
 
 template<typename Derived>
 ExprResult
 TreeTransform<Derived>::TransformChooseExpr(ChooseExpr *E) {
   ExprResult Cond = getDerived().TransformExpr(E->getCond());
   if (Cond.isInvalid())
     return ExprError();

clang/test/CodeGenCXX/asm.cpp

 // RUN: %clang_cc1 -triple i386-unknown-unknown -emit-llvm %s -o - | FileCheck %s
 
+// CHECK: %[[STRUCT_A:.*]] = type { i8 }
+
 struct A
 {
     ~A();
 };
 int foo(A);
 
 void bar(A &a)
 {
     // CHECK: call void asm
     asm("" : : "r"(foo(a)) ); // rdar://8540491
     // CHECK: call void @_ZN1AD1Ev
 }
+
+namespace TestTemplate {
+template <class T>
+void bar(A &a) {
+  asm("" : : "r"(foo(a)) );
+  asm("");
+}
+
+// CHECK: define {{.*}}void @_ZN12TestTemplate3barIvEEvR1A(
+// CHECK: %[[AGG_TMP:.*]] = alloca %[[STRUCT_A]],
+// CHECK: %[[CALL:.*]] = call noundef i32 @_Z3foo1A({{.*}}%[[AGG_TMP]])
+// CHECK: call void asm sideeffect "", "r,~{dirflag},~{fpsr},~{flags}"(i32 %[[CALL]])
+// CHECK: call void @_ZN1AD1Ev({{.*}}%[[AGG_TMP]])
+// CHECK: call void asm sideeffect "",
+
+void test(A &a) { bar<void>(a); }
+} // namespace TestTemplate

clang/test/SemaTemplate/instantiate-expr-1.cpp

namespace PR10864 {
   template<typename T> void test_asm_tied(T o) {
     __asm("addl $1, %0" : "=r" (o) : "0"(Vals<T>::i)); // expected-error {{input with type 'double' matching output with type 'float'}}
   }
   void test_asm_tied() {
     test_asm_tied(1);
     test_asm_tied(1.f); // expected-note {{instantiation of}}
   }
 }
+
+namespace TestAsmCleanup {
+struct S {
+  operator int() const { return 1; }
+  ~S();
+};
+
+template <class T>
+void foo() {
+  __asm__ __volatile__("%[i]"
+                       :
+                       : [i] "r"(-S()));
+}
+
+void test() { foo<void>(); }
+} // namespace TestAsmCleanup