[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 ↗

(On Diff #432398)

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 ↗

(On Diff #432398)

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

2294

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

2294

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/CodeGen/CGStmt.cpp

if (ResultRegTypes.size() == 1) {
     for (unsigned i = 0, e = ResultRegTypes.size(); i != e; ++i) {
       llvm::Value *Tmp = CGF.Builder.CreateExtractValue(&Result, i, "asmresult");
       RegResults.push_back(Tmp);
     }
   }
 }
 
 void CodeGenFunction::EmitAsmStmt(const AsmStmt &S) {
+  // Pop all cleanup blocks at the end of the asm statement.
+  CodeGenFunction::RunCleanupsScope Cleanups(*this);
+
   // Assemble the final asm string.
   std::string AsmString = S.generateAsmString(getContext());
 
   // Get all the output and input constraints together.
   SmallVector<TargetInfo::ConstraintInfo, 4> OutputConstraintInfos;
   SmallVector<TargetInfo::ConstraintInfo, 4> InputConstraintInfos;
 
   for (unsigned i = 0, e = S.getNumOutputs(); i != e; i++) {

clang/lib/Parse/ParseStmt.cpp

Retry:
   case tok::kw_asm: {
     for (const ParsedAttr &AL : CXX11Attrs)
       Diag(AL.getRange().getBegin(), diag::warn_attribute_ignored) << AL;
     // Prevent these from being interpreted as statement attributes later on.
     CXX11Attrs.clear();
     ProhibitAttributes(GNUAttrs);
     bool msAsm = false;
     Res = ParseAsmStatement(msAsm);
-    Res = Actions.ActOnFinishFullStmt(Res.get());
     if (msAsm) return Res;
     SemiError = "asm";
     break;
   }
 
   case tok::kw___if_exists:
   case tok::kw___if_not_exists:
     ProhibitAttributes(CXX11Attrs);

clang/lib/Sema/SemaStmtAsm.cpp

Diag((Found + 1)->second->getBeginLoc(),
          diag::error_duplicate_asm_operand_name)
         << (Found + 1)->first;
     Diag(Found->second->getBeginLoc(), diag::note_duplicate_asm_operand_name)
         << Found->first;
     return StmtError();
   }
   if (NS->isAsmGoto())
     setFunctionHasBranchIntoScope();
+
+  CleanupVarDeclMarking();
+  DiscardCleanupsInEvaluationContext();
   return NS;
 }
 
 void Sema::FillInlineAsmIdentifierInfo(Expr *Res,
                                        llvm::InlineAsmIdentifierInfo &Info) {
   QualType T = Res->getType();
   Expr::EvalResult Eval;
   if (T->isFunctionType() || T->isDependentType())

clang/test/CodeGenCXX/asm.cpp

-// RUN: %clang_cc1 -triple i386-unknown-unknown -emit-llvm %s -o - | FileCheck %s
+// RUN: %clang_cc1 -triple i386-unknown-unknown -fblocks -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 {
+// Check that the temporary is destructed after the first asm statement.
+
+// CHECK: define {{.*}}void @_ZN12TestTemplate4foo0IvEEvR1A(
+// 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 "",
+
+template <class T>
+void foo0(A &a) {
+  asm("" : : "r"(foo(a)) );
+  asm("");
+}
+
+void test0(A &a) { foo0<void>(a); }
+
+// Check that the block capture is destructed at the end of the enclosing scope.
+
+// CHECK: define {{.*}}void @_ZN12TestTemplate4foo1IvEEv1A(
+// CHECK: %[[BLOCK:.*]] = alloca <{ ptr, i32, i32, ptr, ptr, %[[STRUCT_A]] }>, align 4
+// CHECK: %[[BLOCK_CAPTURED:.*]] = getelementptr inbounds <{ ptr, i32, i32, ptr, ptr, %[[STRUCT_A]] }>, ptr %[[BLOCK]], i32 0, i32 5
+// CHECK: call void asm sideeffect "", "r,~{dirflag},~{fpsr},~{flags}"(i32 %{{.*}})
+// CHECK: call void asm sideeffect "", "~{dirflag},~{fpsr},~{flags}"()
+// CHECK: call void @_ZN1AD1Ev({{.*}} %[[BLOCK_CAPTURED]])
+
+template <class T>
+void foo1(A a) {
+  asm("" : : "r"(^{ (void)a; return 0; }()));
+  asm("");
+}
+
+void test1(A &a) { foo1<void>(a); }
+} // namespace TestTemplate

clang/test/CodeGenObjC/asm.m

+// RUN: %clang_cc1 -triple x86_64-apple-darwin10 -fblocks -fobjc-arc -emit-llvm -o - %s | FileCheck %s
+
+// CHECK: %[[STRUCT_A:.*]] = type { ptr }
+
+typedef struct {
+  id f;
+} A;
+
+id a;
+
+// Check that the compound literal is destructed at the end of the enclosing scope.
+
+// CHECK-LABEL: define void @foo0()
+// CHECK: %[[_COMPOUNDLITERAL:.*]] = alloca %[[STRUCT_A]], align 8
+// CHECK: getelementptr inbounds %[[STRUCT_A]], ptr %[[_COMPOUNDLITERAL]], i32 0, i32 0
+// CHECK: %[[F1:.*]] = getelementptr inbounds %[[STRUCT_A]], ptr %[[_COMPOUNDLITERAL]], i32 0, i32 0
+// CHECK: %[[V2:.*]] = load ptr, ptr %[[F1]], align 8
+// CHECK: call void asm sideeffect "", "r,~{dirflag},~{fpsr},~{flags}"(ptr %[[V2]])
+// CHECK: call void asm sideeffect "",
+// CHECK: call void @__destructor_8_s0(ptr %[[_COMPOUNDLITERAL]])
+
+void foo0() {
+  asm("" : : "r"(((A){a}).f) );
+  asm("");
+}

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