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

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.

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

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

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

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 encl

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

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/l

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

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

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

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

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

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

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

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 ___

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

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

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

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://review

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

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

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

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

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

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 ini

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

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/cg

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

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

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

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/in

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

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

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

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 i

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

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

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

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.

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

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 Mon

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

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

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

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