[PATCH] D125936: [Sema] Relax an assertion in BuildStmtExpr

2022-06-17 Thread Akira Hatanaka via Phabricator via cfe-commits 
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG8fc3d719eee7: Stop wrapping GCCAsmStmts inside StmtExprs to 
destruct temporaries (authored by ahatanak).

Changed prior to commit:
  https://reviews.llvm.org/D125936?vs=437796=438076#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125936

Files:
  clang/lib/CodeGen/CGStmt.cpp
  clang/lib/Parse/ParseStmt.cpp
  clang/lib/Sema/SemaStmtAsm.cpp
  clang/test/CodeGenCXX/asm.cpp
  clang/test/CodeGenObjC/asm.m
  clang/test/SemaTemplate/instantiate-expr-1.cpp

Index: clang/test/SemaTemplate/instantiate-expr-1.cpp
===
--- clang/test/SemaTemplate/instantiate-expr-1.cpp
+++ clang/test/SemaTemplate/instantiate-expr-1.cpp
@@ -190,3 +190,19 @@
 test_asm_tied(1.f); // expected-note {{instantiation of}}
   }
 }
+
+namespace TestAsmCleanup {
+struct S {
+  operator int() const { return 1; }
+  ~S();
+};
+
+template 
+void foo() {
+  __asm__ __volatile__("%[i]"
+   :
+   : [i] "r"(-S()));
+}
+
+void test() { foo(); }
+} // namespace TestAsmCleanup
Index: clang/test/CodeGenObjC/asm.m
===
--- /dev/null
+++ clang/test/CodeGenObjC/asm.m
@@ -0,0 +1,25 @@
+// 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("");
+}
Index: clang/test/CodeGenCXX/asm.cpp
===
--- clang/test/CodeGenCXX/asm.cpp
+++ clang/test/CodeGenCXX/asm.cpp
@@ -1,4 +1,6 @@
-// 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
 {
@@ -12,3 +14,39 @@
 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 
+void foo0(A ) {
+  asm("" : : "r"(foo(a)) );
+  asm("");
+}
+
+void test0(A ) { foo0(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 
+void foo1(A a) {
+  asm("" : : "r"(^{ (void)a; return 0; }()));
+  asm("");
+}
+
+void test1(A ) { foo1(a); }
+} // namespace TestTemplate
Index: clang/lib/Sema/SemaStmtAsm.cpp
===
--- clang/lib/Sema/SemaStmtAsm.cpp
+++ clang/lib/Sema/SemaStmtAsm.cpp
@@ -733,6 +733,9 @@
   }
   if (NS->isAsmGoto())
 setFunctionHasBranchIntoScope();
+
+  CleanupVarDeclMarking();
+  DiscardCleanupsInEvaluationContext();
   return NS;
 }
 
Index: clang/lib/Parse/ParseStmt.cpp
===
--- clang/lib/Parse/ParseStmt.cpp
+++ clang/lib/Parse/ParseStmt.cpp
@@ -325,7 +325,6 @@
 ProhibitAttributes(GNUAttrs);
 bool msAsm = false;
 Res = ParseAsmStatement(msAsm);
-Res = Actions.ActOnFinishFullStmt(Res.get());
 if (msAsm) return Res;
 SemiError = "asm";
 break;
Index: clang/lib/CodeGen/CGStmt.cpp

[PATCH] D125936: [Sema] Relax an assertion in BuildStmtExpr

2022-06-17 Thread John McCall via Phabricator via cfe-commits 
rjmccall accepted this revision.
rjmccall added a comment.

Okay, LGTM then


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125936

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D125936: [Sema] Relax an assertion in BuildStmtExpr

2022-06-16 Thread Akira Hatanaka via Phabricator via cfe-commits 
ahatanak added inline comments.



Comment at: clang/lib/CodeGen/CGStmt.cpp:2291
+  // statement.
+  CodeGenFunction::RunCleanupsScope Cleanups(*this);
+

rjmccall wrote:
> 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?
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125936

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D125936: [Sema] Relax an assertion in BuildStmtExpr

2022-06-16 Thread Akira Hatanaka via Phabricator via cfe-commits 
ahatanak updated this revision to Diff 437796.
ahatanak added a comment.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125936

Files:
  clang/lib/CodeGen/CGStmt.cpp
  clang/lib/Parse/ParseStmt.cpp
  clang/lib/Sema/SemaStmtAsm.cpp
  clang/test/CodeGenCXX/asm.cpp
  clang/test/CodeGenObjC/asm.m
  clang/test/SemaTemplate/instantiate-expr-1.cpp

Index: clang/test/SemaTemplate/instantiate-expr-1.cpp
===
--- clang/test/SemaTemplate/instantiate-expr-1.cpp
+++ clang/test/SemaTemplate/instantiate-expr-1.cpp
@@ -190,3 +190,19 @@
 test_asm_tied(1.f); // expected-note {{instantiation of}}
   }
 }
+
+namespace TestAsmCleanup {
+struct S {
+  operator int() const { return 1; }
+  ~S();
+};
+
+template 
+void foo() {
+  __asm__ __volatile__("%[i]"
+   :
+   : [i] "r"(-S()));
+}
+
+void test() { foo(); }
+} // namespace TestAsmCleanup
Index: clang/test/CodeGenObjC/asm.m
===
--- /dev/null
+++ clang/test/CodeGenObjC/asm.m
@@ -0,0 +1,25 @@
+// 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("");
+}
Index: clang/test/CodeGenCXX/asm.cpp
===
--- clang/test/CodeGenCXX/asm.cpp
+++ clang/test/CodeGenCXX/asm.cpp
@@ -1,4 +1,6 @@
-// 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
 {
@@ -12,3 +14,39 @@
 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 
+void foo0(A ) {
+  asm("" : : "r"(foo(a)) );
+  asm("");
+}
+
+void test0(A ) { foo0(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 
+void foo1(A a) {
+  asm("" : : "r"(^{ (void)a; return 0; }()));
+  asm("");
+}
+
+void test1(A ) { foo1(a); }
+} // namespace TestTemplate
Index: clang/lib/Sema/SemaStmtAsm.cpp
===
--- clang/lib/Sema/SemaStmtAsm.cpp
+++ clang/lib/Sema/SemaStmtAsm.cpp
@@ -733,6 +733,9 @@
   }
   if (NS->isAsmGoto())
 setFunctionHasBranchIntoScope();
+
+  CleanupVarDeclMarking();
+  DiscardCleanupsInEvaluationContext();
   return NS;
 }
 
Index: clang/lib/Parse/ParseStmt.cpp
===
--- clang/lib/Parse/ParseStmt.cpp
+++ clang/lib/Parse/ParseStmt.cpp
@@ -300,7 +300,6 @@
 ProhibitAttributes(Attrs);
 bool msAsm = false;
 Res = ParseAsmStatement(msAsm);
-Res = Actions.ActOnFinishFullStmt(Res.get());
 if (msAsm) return Res;
 SemiError = "asm";
 break;
Index: clang/lib/CodeGen/CGStmt.cpp
===
--- clang/lib/CodeGen/CGStmt.cpp
+++ clang/lib/CodeGen/CGStmt.cpp
@@ -2286,6 +2286,9 @@
 }
 
 void CodeGenFunction::EmitAsmStmt(const AsmStmt )

[PATCH] D125936: [Sema] Relax an assertion in BuildStmtExpr

2022-06-16 Thread John McCall via Phabricator via cfe-commits 
rjmccall added inline comments.



Comment at: clang/lib/CodeGen/CGStmt.cpp:2291
+  // statement.
+  CodeGenFunction::RunCleanupsScope Cleanups(*this);
+

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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125936

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D125936: [Sema] Relax an assertion in BuildStmtExpr

2022-06-16 Thread Akira Hatanaka via Phabricator via cfe-commits 
ahatanak updated this revision to Diff 437566.
ahatanak added a comment.

Revert the changes made to SemaCoroutine.cpp.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125936

Files:
  clang/lib/CodeGen/CGStmt.cpp
  clang/lib/Parse/ParseStmt.cpp
  clang/lib/Sema/SemaStmtAsm.cpp
  clang/test/CodeGenCXX/asm.cpp
  clang/test/SemaTemplate/instantiate-expr-1.cpp

Index: clang/test/SemaTemplate/instantiate-expr-1.cpp
===
--- clang/test/SemaTemplate/instantiate-expr-1.cpp
+++ clang/test/SemaTemplate/instantiate-expr-1.cpp
@@ -190,3 +190,19 @@
 test_asm_tied(1.f); // expected-note {{instantiation of}}
   }
 }
+
+namespace TestAsmCleanup {
+struct S {
+  operator int() const { return 1; }
+  ~S();
+};
+
+template 
+void foo() {
+  __asm__ __volatile__("%[i]"
+   :
+   : [i] "r"(-S()));
+}
+
+void test() { foo(); }
+} // namespace TestAsmCleanup
Index: clang/test/CodeGenCXX/asm.cpp
===
--- clang/test/CodeGenCXX/asm.cpp
+++ clang/test/CodeGenCXX/asm.cpp
@@ -1,4 +1,6 @@
-// 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
 {
@@ -12,3 +14,39 @@
 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 
+void foo0(A ) {
+  asm("" : : "r"(foo(a)) );
+  asm("");
+}
+
+void test0(A ) { foo0(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 
+void foo1(A a) {
+  asm("" : : "r"(^{ (void)a; return 0; }()));
+  asm("");
+}
+
+void test1(A ) { foo1(a); }
+} // namespace TestTemplate
Index: clang/lib/Sema/SemaStmtAsm.cpp
===
--- clang/lib/Sema/SemaStmtAsm.cpp
+++ clang/lib/Sema/SemaStmtAsm.cpp
@@ -733,6 +733,9 @@
   }
   if (NS->isAsmGoto())
 setFunctionHasBranchIntoScope();
+
+  CleanupVarDeclMarking();
+  DiscardCleanupsInEvaluationContext();
   return NS;
 }
 
Index: clang/lib/Parse/ParseStmt.cpp
===
--- clang/lib/Parse/ParseStmt.cpp
+++ clang/lib/Parse/ParseStmt.cpp
@@ -300,7 +300,6 @@
 ProhibitAttributes(Attrs);
 bool msAsm = false;
 Res = ParseAsmStatement(msAsm);
-Res = Actions.ActOnFinishFullStmt(Res.get());
 if (msAsm) return Res;
 SemiError = "asm";
 break;
Index: clang/lib/CodeGen/CGStmt.cpp
===
--- clang/lib/CodeGen/CGStmt.cpp
+++ clang/lib/CodeGen/CGStmt.cpp
@@ -2286,6 +2286,10 @@
 }
 
 void CodeGenFunction::EmitAsmStmt(const AsmStmt ) {
+  // Temporaries created in GCC AsmStmts have to be destructed at the end of the
+  // statement.
+  CodeGenFunction::RunCleanupsScope Cleanups(*this);
+
   // Assemble the final asm string.
   std::string AsmString = S.generateAsmString(getContext());
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D125936: [Sema] Relax an assertion in BuildStmtExpr

2022-06-15 Thread Chuanqi Xu via Phabricator via cfe-commits 
ChuanqiXu added a comment.

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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125936

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D125936: [Sema] Relax an assertion in BuildStmtExpr

2022-06-15 Thread Akira Hatanaka via Phabricator via cfe-commits 
ahatanak added subscribers: ChuanqiXu, EricWF.
ahatanak added a comment.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125936

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D125936: [Sema] Relax an assertion in BuildStmtExpr

2022-06-15 Thread Akira Hatanaka via Phabricator via cfe-commits 
ahatanak added a comment.

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125936

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D125936: [Sema] Relax an assertion in BuildStmtExpr

2022-06-15 Thread Akira Hatanaka via Phabricator via cfe-commits 
ahatanak updated this revision to Diff 437248.
ahatanak added a comment.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125936

Files:
  clang/include/clang/Sema/Sema.h
  clang/lib/CodeGen/CGStmt.cpp
  clang/lib/Parse/ParseStmt.cpp
  clang/lib/Sema/SemaCoroutine.cpp
  clang/lib/Sema/SemaExprCXX.cpp
  clang/lib/Sema/SemaStmtAsm.cpp
  clang/test/CodeGenCXX/asm.cpp
  clang/test/SemaTemplate/instantiate-expr-1.cpp

Index: clang/test/SemaTemplate/instantiate-expr-1.cpp
===
--- clang/test/SemaTemplate/instantiate-expr-1.cpp
+++ clang/test/SemaTemplate/instantiate-expr-1.cpp
@@ -190,3 +190,19 @@
 test_asm_tied(1.f); // expected-note {{instantiation of}}
   }
 }
+
+namespace TestAsmCleanup {
+struct S {
+  operator int() const { return 1; }
+  ~S();
+};
+
+template 
+void foo() {
+  __asm__ __volatile__("%[i]"
+   :
+   : [i] "r"(-S()));
+}
+
+void test() { foo(); }
+} // namespace TestAsmCleanup
Index: clang/test/CodeGenCXX/asm.cpp
===
--- clang/test/CodeGenCXX/asm.cpp
+++ clang/test/CodeGenCXX/asm.cpp
@@ -1,4 +1,6 @@
-// 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
 {
@@ -12,3 +14,39 @@
 asm("" : : "r"(foo(a)) ); // rdar://8540491
 // CHECK: call void @_ZN1AD1Ev
 }
+
+namespace TestTemplate {
+template 
+void foo0(A ) {
+  asm("" : : "r"(foo(a)) );
+  asm("");
+}
+
+// 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 "",
+
+void test0(A ) { foo0(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 
+void foo1(A a) {
+  asm("" : : "r"(^{ (void)a; return 0; }()));
+  asm("");
+}
+
+void test1(A ) { foo1(a); }
+} // namespace TestTemplate
Index: clang/lib/Sema/SemaStmtAsm.cpp
===
--- clang/lib/Sema/SemaStmtAsm.cpp
+++ clang/lib/Sema/SemaStmtAsm.cpp
@@ -733,6 +733,9 @@
   }
   if (NS->isAsmGoto())
 setFunctionHasBranchIntoScope();
+
+  CleanupVarDeclMarking();
+  DiscardCleanupsInEvaluationContext();
   return NS;
 }
 
Index: clang/lib/Sema/SemaExprCXX.cpp
===
--- clang/lib/Sema/SemaExprCXX.cpp
+++ clang/lib/Sema/SemaExprCXX.cpp
@@ -7283,26 +7283,6 @@
   return E;
 }
 
-Stmt *Sema::MaybeCreateStmtWithCleanups(Stmt *SubStmt) {
-  assert(SubStmt && "sub-statement can't be null!");
-
-  CleanupVarDeclMarking();
-
-  if (!Cleanup.exprNeedsCleanups())
-return 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);
-  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
@@ -8803,12 +8783,6 @@
   return MaybeCreateExprWithCleanups(FullExpr);
 }
 
-StmtResult Sema::ActOnFinishFullStmt(Stmt *FullStmt) {
-  if (!FullStmt) return StmtError();
-
-  return MaybeCreateStmtWithCleanups(FullStmt);
-}
-
 Sema::IfExistsResult
 Sema::CheckMicrosoftIfExistsSymbol(Scope *S,
CXXScopeSpec ,

[PATCH] D125936: [Sema] Relax an assertion in BuildStmtExpr

2022-06-14 Thread John McCall via Phabricator via cfe-commits 
rjmccall added a comment.

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125936

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D125936: [Sema] Relax an assertion in BuildStmtExpr

2022-06-13 Thread Akira Hatanaka via Phabricator via cfe-commits 
ahatanak added a comment.

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 `StmtExpr`s 
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125936

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D125936: [Sema] Relax an assertion in BuildStmtExpr

2022-06-11 Thread John McCall via Phabricator via cfe-commits 
rjmccall added a comment.

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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125936

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D125936: [Sema] Relax an assertion in BuildStmtExpr

2022-06-10 Thread Akira Hatanaka via Phabricator via cfe-commits 
ahatanak added a comment.

ping


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125936

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D125936: [Sema] Relax an assertion in BuildStmtExpr

2022-06-07 Thread Akira Hatanaka via Phabricator via cfe-commits 
ahatanak updated this revision to Diff 434960.
ahatanak marked an inline comment as done.
ahatanak added a comment.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125936

Files:
  clang/include/clang/AST/Expr.h
  clang/include/clang/AST/Stmt.h
  clang/include/clang/Sema/Sema.h
  clang/lib/Sema/SemaExpr.cpp
  clang/lib/Sema/SemaExprCXX.cpp
  clang/lib/Sema/TreeTransform.h
  clang/test/CodeGenCXX/asm.cpp
  clang/test/SemaTemplate/instantiate-expr-1.cpp

Index: clang/test/SemaTemplate/instantiate-expr-1.cpp
===
--- clang/test/SemaTemplate/instantiate-expr-1.cpp
+++ clang/test/SemaTemplate/instantiate-expr-1.cpp
@@ -190,3 +190,19 @@
 test_asm_tied(1.f); // expected-note {{instantiation of}}
   }
 }
+
+namespace TestAsmCleanup {
+struct S {
+  operator int() const { return 1; }
+  ~S();
+};
+
+template 
+void foo() {
+  __asm__ __volatile__("%[i]"
+   :
+   : [i] "r"(-S()));
+}
+
+void test() { foo(); }
+} // namespace TestAsmCleanup
Index: clang/test/CodeGenCXX/asm.cpp
===
--- clang/test/CodeGenCXX/asm.cpp
+++ clang/test/CodeGenCXX/asm.cpp
@@ -1,5 +1,7 @@
 // RUN: %clang_cc1 -triple i386-unknown-unknown -emit-llvm %s -o - | FileCheck %s
 
+// CHECK: %[[STRUCT_A:.*]] = type { i8 }
+
 struct A
 {
 ~A();
@@ -12,3 +14,20 @@
 asm("" : : "r"(foo(a)) ); // rdar://8540491
 // CHECK: call void @_ZN1AD1Ev
 }
+
+namespace TestTemplate {
+template 
+void bar(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 ) { bar(a); }
+} // namespace TestTemplate
Index: clang/lib/Sema/TreeTransform.h
===
--- clang/lib/Sema/TreeTransform.h
+++ clang/lib/Sema/TreeTransform.h
@@ -2873,9 +2873,10 @@
   /// 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.
@@ -11565,7 +11566,8 @@
   }
 
   return getDerived().RebuildStmtExpr(E->getLParenLoc(), SubStmt.get(),
-  E->getRParenLoc(), NewDepth);
+  E->getRParenLoc(), NewDepth,
+  E->needsCleanup());
 }
 
 template
Index: clang/lib/Sema/SemaExprCXX.cpp
===
--- clang/lib/Sema/SemaExprCXX.cpp
+++ clang/lib/Sema/SemaExprCXX.cpp
@@ -7299,7 +7299,7 @@
   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);
 }
 
Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -15769,13 +15769,15 @@
 }
 
 ExprResult Sema::BuildStmtExpr(SourceLocation LPLoc, Stmt *SubStmt,
-   SourceLocation RPLoc, unsigned TemplateDepth) {
+   SourceLocation RPLoc, unsigned TemplateDepth,
+   bool NeedsCleanup) {
   assert(SubStmt && isa(SubStmt) && "Invalid action invocation!");
   CompoundStmt *Compound = cast(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();
 
@@ -15800,8 +15802,8 @@
 
   // FIXME: Check that expression type is

[PATCH] D125936: [Sema] Relax an assertion in BuildStmtExpr

2022-06-03 Thread Akira Hatanaka via Phabricator via cfe-commits 
ahatanak updated this revision to Diff 434144.
ahatanak added a comment.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125936

Files:
  clang/include/clang/AST/Expr.h
  clang/include/clang/AST/Stmt.h
  clang/include/clang/Sema/Sema.h
  clang/lib/Sema/SemaExpr.cpp
  clang/lib/Sema/SemaExprCXX.cpp
  clang/lib/Sema/TreeTransform.h
  clang/test/CodeGenCXX/asm.cpp
  clang/test/SemaTemplate/instantiate-expr-1.cpp

Index: clang/test/SemaTemplate/instantiate-expr-1.cpp
===
--- clang/test/SemaTemplate/instantiate-expr-1.cpp
+++ clang/test/SemaTemplate/instantiate-expr-1.cpp
@@ -190,3 +190,19 @@
 test_asm_tied(1.f); // expected-note {{instantiation of}}
   }
 }
+
+namespace TestAsmCleanup {
+struct S {
+  operator int() const { return 1; }
+  ~S();
+};
+
+template 
+void foo() {
+  __asm__ __volatile__("%[i]"
+   :
+   : [i] "r"(-S()));
+}
+
+void test() { foo(); }
+} // namespace TestAsmCleanup
Index: clang/test/CodeGenCXX/asm.cpp
===
--- clang/test/CodeGenCXX/asm.cpp
+++ clang/test/CodeGenCXX/asm.cpp
@@ -1,5 +1,7 @@
 // RUN: %clang_cc1 -triple i386-unknown-unknown -emit-llvm %s -o - | FileCheck %s
 
+// CHECK: %[[STRUCT_A:.*]] = type { i8 }
+
 struct A
 {
 ~A();
@@ -12,3 +14,18 @@
 asm("" : : "r"(foo(a)) ); // rdar://8540491
 // CHECK: call void @_ZN1AD1Ev
 }
+
+namespace TestTemplate {
+template 
+void bar(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 ) { bar(a); }
+} // namespace TestTemplate
Index: clang/lib/Sema/TreeTransform.h
===
--- clang/lib/Sema/TreeTransform.h
+++ clang/lib/Sema/TreeTransform.h
@@ -2873,9 +2873,10 @@
   /// 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.
@@ -11565,7 +11566,8 @@
   }
 
   return getDerived().RebuildStmtExpr(E->getLParenLoc(), SubStmt.get(),
-  E->getRParenLoc(), NewDepth);
+  E->getRParenLoc(), NewDepth,
+  E->needsCleanup());
 }
 
 template
Index: clang/lib/Sema/SemaExprCXX.cpp
===
--- clang/lib/Sema/SemaExprCXX.cpp
+++ clang/lib/Sema/SemaExprCXX.cpp
@@ -7299,7 +7299,7 @@
   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);
 }
 
Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -15743,13 +15743,15 @@
 }
 
 ExprResult Sema::BuildStmtExpr(SourceLocation LPLoc, Stmt *SubStmt,
-   SourceLocation RPLoc, unsigned TemplateDepth) {
+   SourceLocation RPLoc, unsigned TemplateDepth,
+   bool NeedsCleanup) {
   assert(SubStmt && isa(SubStmt) && "Invalid action invocation!");
   CompoundStmt *Compound = cast(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();
 
@@ -15774,8 +15776,8 @@
 
   // FIXME: Check that expression type is complete/non-abstract; statement
   // expressions are not lvalues.
-  Expr *ResStmtExpr =
-  new

[PATCH] D125936: [Sema] Relax an assertion in BuildStmtExpr

2022-05-31 Thread Akira Hatanaka via Phabricator via cfe-commits 
ahatanak added inline comments.



Comment at: clang/lib/Sema/SemaExpr.cpp:15746
 ExprResult Sema::BuildStmtExpr(SourceLocation LPLoc, Stmt *SubStmt,
SourceLocation RPLoc, unsigned TemplateDepth) {
   assert(SubStmt && isa(SubStmt) && "Invalid action 
invocation!");

rsmith wrote:
> 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...
Maybe we can add a flag to `StmtExpr` and set it in 
`Sema::MaybeCreateStmtWithCleanups` to distinguish `StmtExpr`s 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`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125936

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D125936: [Sema] Relax an assertion in BuildStmtExpr

2022-05-26 Thread Richard Smith - zygoloid via Phabricator via cfe-commits 
rsmith added inline comments.



Comment at: clang/lib/Sema/SemaExpr.cpp:15746
 ExprResult Sema::BuildStmtExpr(SourceLocation LPLoc, Stmt *SubStmt,
SourceLocation RPLoc, unsigned TemplateDepth) {
   assert(SubStmt && isa(SubStmt) && "Invalid action 
invocation!");

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...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125936

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D125936: [Sema] Relax an assertion in BuildStmtExpr

2022-05-26 Thread Richard Smith - zygoloid via Phabricator via cfe-commits 
rsmith added inline comments.



Comment at: clang/test/CodeGenCXX/asm.cpp:22
+  asm("" : : "r"(foo(a)) );
+}
+

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 `}`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125936

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D125936: [Sema] Relax an assertion in BuildStmtExpr

2022-05-26 Thread Akira Hatanaka via Phabricator via cfe-commits 
ahatanak updated this revision to Diff 432398.
ahatanak edited the summary of this revision.
ahatanak added a comment.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125936

Files:
  clang/lib/Sema/SemaExpr.cpp
  clang/test/CodeGenCXX/asm.cpp
  clang/test/SemaTemplate/instantiate-expr-1.cpp


Index: clang/test/SemaTemplate/instantiate-expr-1.cpp
===
--- clang/test/SemaTemplate/instantiate-expr-1.cpp
+++ clang/test/SemaTemplate/instantiate-expr-1.cpp
@@ -190,3 +190,19 @@
 test_asm_tied(1.f); // expected-note {{instantiation of}}
   }
 }
+
+namespace TestAsmCleanup {
+struct S {
+  operator int() const { return 1; }
+  ~S();
+};
+
+template 
+void foo() {
+  __asm__ __volatile__("%[i]"
+   :
+   : [i] "r"(-S()));
+}
+
+void test() { foo(); }
+} // namespace TestAsmCleanup
Index: clang/test/CodeGenCXX/asm.cpp
===
--- clang/test/CodeGenCXX/asm.cpp
+++ clang/test/CodeGenCXX/asm.cpp
@@ -1,5 +1,7 @@
 // RUN: %clang_cc1 -triple i386-unknown-unknown -emit-llvm %s -o - | FileCheck 
%s
 
+// CHECK: %[[STRUCT_A:.*]] = type { i8 }
+
 struct A
 {
 ~A();
@@ -12,3 +14,18 @@
 asm("" : : "r"(foo(a)) ); // rdar://8540491
 // CHECK: call void @_ZN1AD1Ev
 }
+
+namespace TestTemplate {
+template 
+void bar(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 ) { bar(a); }
+} // namespace TestTemplate
Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -15749,7 +15749,9 @@
 
   if (hasAnyUnrecoverableErrorsInThisFunction())
 DiscardCleanupsInEvaluationContext();
-  assert(!Cleanup.exprNeedsCleanups() &&
+  // Cleanups may be needed if temporaries are created in an AsmStmt.
+  assert((!Cleanup.exprNeedsCleanups() ||
+  (!Compound->body_empty() && isa(Compound->body_front( &&
  "cleanups within StmtExpr not correctly bound!");
   PopExpressionEvaluationContext();
 


Index: clang/test/SemaTemplate/instantiate-expr-1.cpp
===
--- clang/test/SemaTemplate/instantiate-expr-1.cpp
+++ clang/test/SemaTemplate/instantiate-expr-1.cpp
@@ -190,3 +190,19 @@
 test_asm_tied(1.f); // expected-note {{instantiation of}}
   }
 }
+
+namespace TestAsmCleanup {
+struct S {
+  operator int() const { return 1; }
+  ~S();
+};
+
+template 
+void foo() {
+  __asm__ __volatile__("%[i]"
+   :
+   : [i] "r"(-S()));
+}
+
+void test() { foo(); }
+} // namespace TestAsmCleanup
Index: clang/test/CodeGenCXX/asm.cpp
===
--- clang/test/CodeGenCXX/asm.cpp
+++ clang/test/CodeGenCXX/asm.cpp
@@ -1,5 +1,7 @@
 // RUN: %clang_cc1 -triple i386-unknown-unknown -emit-llvm %s -o - | FileCheck %s
 
+// CHECK: %[[STRUCT_A:.*]] = type { i8 }
+
 struct A
 {
 ~A();
@@ -12,3 +14,18 @@
 asm("" : : "r"(foo(a)) ); // rdar://8540491
 // CHECK: call void @_ZN1AD1Ev
 }
+
+namespace TestTemplate {
+template 
+void bar(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 ) { bar(a); }
+} // namespace TestTemplate
Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -15749,7 +15749,9 @@
 
   if (hasAnyUnrecoverableErrorsInThisFunction())
 DiscardCleanupsInEvaluationContext();
-  assert(!Cleanup.exprNeedsCleanups() &&
+  // Cleanups may be needed if temporaries are created in an AsmStmt.
+  assert((!Cleanup.exprNeedsCleanups() ||
+  (!Compound->body_empty() && isa(Compound->body_front( &&
  "cleanups within StmtExpr not correctly bound!");
   PopExpressionEvaluationContext();
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D125936: [Sema] Relax an assertion in BuildStmtExpr

2022-05-26 Thread Argyrios Kyrtzidis via Phabricator via cfe-commits 
akyrtzi added a comment.

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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125936

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D125936: [Sema] Relax an assertion in BuildStmtExpr

2022-05-25 Thread Akira Hatanaka via Phabricator via cfe-commits 
ahatanak added a comment.

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).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125936

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D125936: [Sema] Relax an assertion in BuildStmtExpr

2022-05-18 Thread Akira Hatanaka via Phabricator via cfe-commits 
ahatanak created this revision.
ahatanak added reviewers: rjmccall, akyrtzi.
ahatanak added a project: clang.
Herald added a project: All.
ahatanak requested review of this revision.

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

rdar://92845835


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D125936

Files:
  clang/lib/Sema/SemaExpr.cpp
  clang/test/SemaTemplate/instantiate-expr-1.cpp


Index: clang/test/SemaTemplate/instantiate-expr-1.cpp
===
--- clang/test/SemaTemplate/instantiate-expr-1.cpp
+++ clang/test/SemaTemplate/instantiate-expr-1.cpp
@@ -190,3 +190,19 @@
 test_asm_tied(1.f); // expected-note {{instantiation of}}
   }
 }
+
+namespace TestAsmCleanup {
+struct S {
+  operator int() const { return 1; }
+  ~S();
+};
+
+template 
+void foo() {
+  __asm__ __volatile__("%[i]"
+   :
+   : [i] "r"(-S()));
+}
+
+void test() { foo(); }
+} // namespace TestAsmCleanup
Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -15745,7 +15745,9 @@
 
   if (hasAnyUnrecoverableErrorsInThisFunction())
 DiscardCleanupsInEvaluationContext();
-  assert(!Cleanup.exprNeedsCleanups() &&
+  // Cleanups may be needed if temporaries are created in an AsmStmt.
+  assert((!Cleanup.exprNeedsCleanups() ||
+  (!Compound->body_empty() && isa(Compound->body_front( &&
  "cleanups within StmtExpr not correctly bound!");
   PopExpressionEvaluationContext();
 


Index: clang/test/SemaTemplate/instantiate-expr-1.cpp
===
--- clang/test/SemaTemplate/instantiate-expr-1.cpp
+++ clang/test/SemaTemplate/instantiate-expr-1.cpp
@@ -190,3 +190,19 @@
 test_asm_tied(1.f); // expected-note {{instantiation of}}
   }
 }
+
+namespace TestAsmCleanup {
+struct S {
+  operator int() const { return 1; }
+  ~S();
+};
+
+template 
+void foo() {
+  __asm__ __volatile__("%[i]"
+   :
+   : [i] "r"(-S()));
+}
+
+void test() { foo(); }
+} // namespace TestAsmCleanup
Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -15745,7 +15745,9 @@
 
   if (hasAnyUnrecoverableErrorsInThisFunction())
 DiscardCleanupsInEvaluationContext();
-  assert(!Cleanup.exprNeedsCleanups() &&
+  // Cleanups may be needed if temporaries are created in an AsmStmt.
+  assert((!Cleanup.exprNeedsCleanups() ||
+  (!Compound->body_empty() && isa(Compound->body_front( &&
  "cleanups within StmtExpr not correctly bound!");
   PopExpressionEvaluationContext();
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits