[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

15746

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

15746

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/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) {
   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() ||
+          (!Compound->body_empty() && isa<AsmStmt>(Compound->body_front()))) &&
          "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

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)) );
+}
+
+// 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]])
+
+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