[PATCH] D146758: Fix codegen for coroutine with function-try-block
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGe00a8d081d78: Fix codegen for coroutine with
function-try-block (authored by MatzeB).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D146758/new/
https://reviews.llvm.org/D146758
Files:
clang/include/clang/AST/StmtCXX.h
clang/lib/AST/ASTImporter.cpp
clang/lib/AST/StmtCXX.cpp
clang/lib/CodeGen/CGCoroutine.cpp
clang/lib/Sema/SemaStmt.cpp
clang/test/CodeGenCoroutines/coro-function-try-block.cpp
Index: clang/test/CodeGenCoroutines/coro-function-try-block.cpp
===
--- /dev/null
+++ clang/test/CodeGenCoroutines/coro-function-try-block.cpp
@@ -0,0 +1,23 @@
+// RUN: %clang_cc1 -std=c++20 -triple=x86_64-- -emit-llvm -fcxx-exceptions \
+// RUN:-disable-llvm-passes %s -o - | FileCheck %s
+
+#include "Inputs/coroutine.h"
+
+struct task {
+ struct promise_type {
+task get_return_object();
+std::suspend_never initial_suspend();
+std::suspend_never final_suspend() noexcept;
+void return_void();
+void unhandled_exception() noexcept;
+ };
+};
+
+task f() try {
+ co_return;
+} catch(...) {
+}
+
+// CHECK-LABEL: define{{.*}} void @_Z1fv(
+// CHECK: call void @_ZNSt13suspend_never13await_suspendESt16coroutine_handleIvE(
+// CHECK: call void @_ZN4task12promise_type11return_voidEv(
Index: clang/lib/Sema/SemaStmt.cpp
===
--- clang/lib/Sema/SemaStmt.cpp
+++ clang/lib/Sema/SemaStmt.cpp
@@ -4546,7 +4546,8 @@
FSI->setHasCXXTry(TryLoc);
- return CXXTryStmt::Create(Context, TryLoc, TryBlock, Handlers);
+ return CXXTryStmt::Create(Context, TryLoc, cast(TryBlock),
+Handlers);
}
StmtResult Sema::ActOnSEHTryBlock(bool IsCXXTry, SourceLocation TryLoc,
Index: clang/lib/CodeGen/CGCoroutine.cpp
===
--- clang/lib/CodeGen/CGCoroutine.cpp
+++ clang/lib/CodeGen/CGCoroutine.cpp
@@ -593,6 +593,18 @@
CGF.EmitStmt(OnFallthrough);
}
+static CompoundStmt *CoroutineStmtBuilder(ASTContext &Context,
+ const CoroutineBodyStmt &S) {
+ Stmt *Stmt = S.getBody();
+ if (CompoundStmt *Body = dyn_cast(Stmt))
+return Body;
+ // We are about to create a `CXXTryStmt` which requires a `CompoundStmt`.
+ // If the function body is not a `CompoundStmt` yet then we have to create
+ // a new one. This happens for cases like the "function-try-block" syntax.
+ return CompoundStmt::Create(Context, {Stmt}, FPOptionsOverride(),
+ SourceLocation(), SourceLocation());
+}
+
void CodeGenFunction::EmitCoroutineBody(const CoroutineBodyStmt &S) {
auto *NullPtr = llvm::ConstantPointerNull::get(Builder.getInt8PtrTy());
auto &TI = CGM.getContext().getTargetInfo();
@@ -721,8 +733,8 @@
auto Loc = S.getBeginLoc();
CXXCatchStmt Catch(Loc, /*exDecl=*/nullptr,
CurCoro.Data->ExceptionHandler);
- auto *TryStmt =
- CXXTryStmt::Create(getContext(), Loc, S.getBody(), &Catch);
+ CompoundStmt *Body = CoroutineStmtBuilder(getContext(), S);
+ auto *TryStmt = CXXTryStmt::Create(getContext(), Loc, Body, &Catch);
EnterCXXTryStmt(*TryStmt);
emitBodyAndFallthrough(*this, S, TryStmt->getTryBlock());
Index: clang/lib/AST/StmtCXX.cpp
===
--- clang/lib/AST/StmtCXX.cpp
+++ clang/lib/AST/StmtCXX.cpp
@@ -23,7 +23,8 @@
}
CXXTryStmt *CXXTryStmt::Create(const ASTContext &C, SourceLocation tryLoc,
- Stmt *tryBlock, ArrayRef handlers) {
+ CompoundStmt *tryBlock,
+ ArrayRef handlers) {
const size_t Size = totalSizeToAlloc(handlers.size() + 1);
void *Mem = C.Allocate(Size, alignof(CXXTryStmt));
return new (Mem) CXXTryStmt(tryLoc, tryBlock, handlers);
@@ -36,7 +37,7 @@
return new (Mem) CXXTryStmt(Empty, numHandlers);
}
-CXXTryStmt::CXXTryStmt(SourceLocation tryLoc, Stmt *tryBlock,
+CXXTryStmt::CXXTryStmt(SourceLocation tryLoc, CompoundStmt *tryBlock,
ArrayRef handlers)
: Stmt(CXXTryStmtClass), TryLoc(tryLoc), NumHandlers(handlers.size()) {
Stmt **Stmts = getStmts();
Index: clang/lib/AST/ASTImporter.cpp
===
--- clang/lib/AST/ASTImporter.cpp
+++ clang/lib/AST/ASTImporter.cpp
@@ -6793,8 +6793,8 @@
return ToHandlerOrErr.takeError();
}
- return CXXTryStmt::Create(
- Importer.getToContext(), *ToTryLocOrErr,*ToTryBlockOrErr, ToHandlers);
+ return CXXTryStmt::Create(Importer.getToContext(), *ToTryLocOrErr,
+cast(*ToTryBlockOrErr), ToHandlers);
}
ExpectedStmt ASTNodeImporte
[PATCH] D146758: Fix codegen for coroutine with function-try-block
MatzeB added inline comments.
Comment at: clang/lib/CodeGen/CGCoroutine.cpp:724-730
+ Stmt *BodyStmt = S.getBody();
+ CompoundStmt *Body = dyn_cast(BodyStmt);
+ if (Body == nullptr) {
+Body =
+CompoundStmt::Create(getContext(), {BodyStmt}, FPOptionsOverride(),
+ SourceLocation(), SourceLocation());
+ }
ChuanqiXu wrote:
> MatzeB wrote:
> > ChuanqiXu wrote:
> > > MatzeB wrote:
> > > > ChuanqiXu wrote:
> > > > > Can we try to move the logic to `CoroutineStmtBuilder`? That makes me
> > > > > feel better. And it will be helpful to add a comment to tell that
> > > > > we're handling the case the function body is function-try-block.
> > > > I'll add a detailed comment. But would you be fine leaving the
> > > > statements here as-is? The logic only makes sense in the context of
> > > > using the `Body` to create a `CXXTryStmt` below (it's really an effect
> > > > of `CXXTryStmt` only accepting CompountStmt operands).
> > > It looks like you didn't address the comments. Would you like to address
> > > it? I don't mind to address it later myself.
> > Did you mean to create a new function named `CoroutineStmtBuilder` like I
> > did now?
> > Did you mean to create a new function named CoroutineStmtBuilder like I did
> > now?
>
> No, I mean we should construct this in Sema.
>
> > Putting an assert here feels unnecessary and may be in the way if in the
> > future we ever allow other types of single-statement function bodies.
>
> Personally I prefer the more precise style.
> No, I mean we should construct this in Sema.
I'll land the patch as-is then and leave the refactoring to you if necessary.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D146758/new/
https://reviews.llvm.org/D146758
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D146758: Fix codegen for coroutine with function-try-block
ChuanqiXu accepted this revision.
ChuanqiXu added a comment.
This revision is now accepted and ready to land.
Since the patch itself is good and not large. Let me handle the trivial
refactoring later.
Comment at: clang/lib/CodeGen/CGCoroutine.cpp:724-730
+ Stmt *BodyStmt = S.getBody();
+ CompoundStmt *Body = dyn_cast(BodyStmt);
+ if (Body == nullptr) {
+Body =
+CompoundStmt::Create(getContext(), {BodyStmt}, FPOptionsOverride(),
+ SourceLocation(), SourceLocation());
+ }
MatzeB wrote:
> ChuanqiXu wrote:
> > MatzeB wrote:
> > > ChuanqiXu wrote:
> > > > Can we try to move the logic to `CoroutineStmtBuilder`? That makes me
> > > > feel better. And it will be helpful to add a comment to tell that we're
> > > > handling the case the function body is function-try-block.
> > > I'll add a detailed comment. But would you be fine leaving the statements
> > > here as-is? The logic only makes sense in the context of using the `Body`
> > > to create a `CXXTryStmt` below (it's really an effect of `CXXTryStmt`
> > > only accepting CompountStmt operands).
> > It looks like you didn't address the comments. Would you like to address
> > it? I don't mind to address it later myself.
> Did you mean to create a new function named `CoroutineStmtBuilder` like I did
> now?
> Did you mean to create a new function named CoroutineStmtBuilder like I did
> now?
No, I mean we should construct this in Sema.
> Putting an assert here feels unnecessary and may be in the way if in the
> future we ever allow other types of single-statement function bodies.
Personally I prefer the more precise style.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D146758/new/
https://reviews.llvm.org/D146758
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D146758: Fix codegen for coroutine with function-try-block
MatzeB added inline comments.
Comment at: clang/lib/CodeGen/CGCoroutine.cpp:724-730
+ Stmt *BodyStmt = S.getBody();
+ CompoundStmt *Body = dyn_cast(BodyStmt);
+ if (Body == nullptr) {
+Body =
+CompoundStmt::Create(getContext(), {BodyStmt}, FPOptionsOverride(),
+ SourceLocation(), SourceLocation());
+ }
ChuanqiXu wrote:
> Can we try to move the logic to `CoroutineStmtBuilder`? That makes me feel
> better. And it will be helpful to add a comment to tell that we're handling
> the case the function body is function-try-block.
I'll add a detailed comment. But would you be fine leaving the statements here
as-is? The logic only makes sense in the context of using the `Body` to create
a `CXXTryStmt` below (it's really an effect of `CXXTryStmt` only accepting
CompountStmt operands).
Comment at: clang/lib/CodeGen/CGCoroutine.cpp:724-730
+ Stmt *BodyStmt = S.getBody();
+ CompoundStmt *Body = dyn_cast(BodyStmt);
+ if (Body == nullptr) {
+Body =
+CompoundStmt::Create(getContext(), {BodyStmt}, FPOptionsOverride(),
+ SourceLocation(), SourceLocation());
+ }
ChuanqiXu wrote:
> MatzeB wrote:
> > ChuanqiXu wrote:
> > > Can we try to move the logic to `CoroutineStmtBuilder`? That makes me
> > > feel better. And it will be helpful to add a comment to tell that we're
> > > handling the case the function body is function-try-block.
> > I'll add a detailed comment. But would you be fine leaving the statements
> > here as-is? The logic only makes sense in the context of using the `Body`
> > to create a `CXXTryStmt` below (it's really an effect of `CXXTryStmt` only
> > accepting CompountStmt operands).
> It looks like you didn't address the comments. Would you like to address it?
> I don't mind to address it later myself.
Did you mean to create a new function named `CoroutineStmtBuilder` like I did
now?
Comment at: clang/lib/CodeGen/CGCoroutine.cpp:726
+ CompoundStmt *Body = dyn_cast(BodyStmt);
+ if (Body == nullptr) {
+Body =
ChuanqiXu wrote:
> It reads better to specify the potential type for Body.
I will mention a single-statement body as an example in the comment now.
Putting an `assert` here feels unnecessary and may be in the way if in the
future we ever allow other types of single-statement function bodies.
Comment at: clang/test/CodeGenCoroutines/coro-function-try-block.cpp:21-23
+// CHECK-LABEL: define{{.*}} void @_Z1fv(
+// CHECK: call void
@_ZNSt13suspend_never13await_suspendESt16coroutine_handleIvE(
+// CHECK: call void @_ZN4task12promise_type11return_voidEv(
ChuanqiXu wrote:
> I expect to see the nested try statements in the case.
This was mostly meant as a test for "does not crash the compiler", so it is
just checking some parts of the output without caring too much what they are
specifically.
I cannot test for nested try statements, because in llvm-ir those are already
lowered to unwind tables and landing pad blocks, but there's neither a "try"
statement nor an immediate nesting I could test for.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D146758/new/
https://reviews.llvm.org/D146758
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D146758: Fix codegen for coroutine with function-try-block
MatzeB updated this revision to Diff 509173.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D146758/new/
https://reviews.llvm.org/D146758
Files:
clang/include/clang/AST/StmtCXX.h
clang/lib/AST/ASTImporter.cpp
clang/lib/AST/StmtCXX.cpp
clang/lib/CodeGen/CGCoroutine.cpp
clang/lib/Sema/SemaStmt.cpp
clang/test/CodeGenCoroutines/coro-function-try-block.cpp
Index: clang/test/CodeGenCoroutines/coro-function-try-block.cpp
===
--- /dev/null
+++ clang/test/CodeGenCoroutines/coro-function-try-block.cpp
@@ -0,0 +1,23 @@
+// RUN: %clang_cc1 -std=c++20 -triple=x86_64-- -emit-llvm -fcxx-exceptions \
+// RUN:-disable-llvm-passes %s -o - | FileCheck %s
+
+#include "Inputs/coroutine.h"
+
+struct task {
+ struct promise_type {
+task get_return_object();
+std::suspend_never initial_suspend();
+std::suspend_never final_suspend() noexcept;
+void return_void();
+void unhandled_exception() noexcept;
+ };
+};
+
+task f() try {
+ co_return;
+} catch(...) {
+}
+
+// CHECK-LABEL: define{{.*}} void @_Z1fv(
+// CHECK: call void @_ZNSt13suspend_never13await_suspendESt16coroutine_handleIvE(
+// CHECK: call void @_ZN4task12promise_type11return_voidEv(
Index: clang/lib/Sema/SemaStmt.cpp
===
--- clang/lib/Sema/SemaStmt.cpp
+++ clang/lib/Sema/SemaStmt.cpp
@@ -4546,7 +4546,8 @@
FSI->setHasCXXTry(TryLoc);
- return CXXTryStmt::Create(Context, TryLoc, TryBlock, Handlers);
+ return CXXTryStmt::Create(Context, TryLoc, cast(TryBlock),
+Handlers);
}
StmtResult Sema::ActOnSEHTryBlock(bool IsCXXTry, SourceLocation TryLoc,
Index: clang/lib/CodeGen/CGCoroutine.cpp
===
--- clang/lib/CodeGen/CGCoroutine.cpp
+++ clang/lib/CodeGen/CGCoroutine.cpp
@@ -593,6 +593,18 @@
CGF.EmitStmt(OnFallthrough);
}
+static CompoundStmt *CoroutineStmtBuilder(ASTContext &Context,
+ const CoroutineBodyStmt &S) {
+ Stmt *Stmt = S.getBody();
+ if (CompoundStmt *Body = dyn_cast(Stmt))
+return Body;
+ // We are about to create a `CXXTryStmt` which requires a `CompoundStmt`.
+ // If the function body is not a `CompoundStmt` yet then we have to create
+ // a new one. This happens for cases like the "function-try-block" syntax.
+ return CompoundStmt::Create(Context, {Stmt}, FPOptionsOverride(),
+ SourceLocation(), SourceLocation());
+}
+
void CodeGenFunction::EmitCoroutineBody(const CoroutineBodyStmt &S) {
auto *NullPtr = llvm::ConstantPointerNull::get(Builder.getInt8PtrTy());
auto &TI = CGM.getContext().getTargetInfo();
@@ -721,8 +733,8 @@
auto Loc = S.getBeginLoc();
CXXCatchStmt Catch(Loc, /*exDecl=*/nullptr,
CurCoro.Data->ExceptionHandler);
- auto *TryStmt =
- CXXTryStmt::Create(getContext(), Loc, S.getBody(), &Catch);
+ CompoundStmt *Body = CoroutineStmtBuilder(getContext(), S);
+ auto *TryStmt = CXXTryStmt::Create(getContext(), Loc, Body, &Catch);
EnterCXXTryStmt(*TryStmt);
emitBodyAndFallthrough(*this, S, TryStmt->getTryBlock());
Index: clang/lib/AST/StmtCXX.cpp
===
--- clang/lib/AST/StmtCXX.cpp
+++ clang/lib/AST/StmtCXX.cpp
@@ -23,7 +23,8 @@
}
CXXTryStmt *CXXTryStmt::Create(const ASTContext &C, SourceLocation tryLoc,
- Stmt *tryBlock, ArrayRef handlers) {
+ CompoundStmt *tryBlock,
+ ArrayRef handlers) {
const size_t Size = totalSizeToAlloc(handlers.size() + 1);
void *Mem = C.Allocate(Size, alignof(CXXTryStmt));
return new (Mem) CXXTryStmt(tryLoc, tryBlock, handlers);
@@ -36,7 +37,7 @@
return new (Mem) CXXTryStmt(Empty, numHandlers);
}
-CXXTryStmt::CXXTryStmt(SourceLocation tryLoc, Stmt *tryBlock,
+CXXTryStmt::CXXTryStmt(SourceLocation tryLoc, CompoundStmt *tryBlock,
ArrayRef handlers)
: Stmt(CXXTryStmtClass), TryLoc(tryLoc), NumHandlers(handlers.size()) {
Stmt **Stmts = getStmts();
Index: clang/lib/AST/ASTImporter.cpp
===
--- clang/lib/AST/ASTImporter.cpp
+++ clang/lib/AST/ASTImporter.cpp
@@ -6793,8 +6793,8 @@
return ToHandlerOrErr.takeError();
}
- return CXXTryStmt::Create(
- Importer.getToContext(), *ToTryLocOrErr,*ToTryBlockOrErr, ToHandlers);
+ return CXXTryStmt::Create(Importer.getToContext(), *ToTryLocOrErr,
+cast(*ToTryBlockOrErr), ToHandlers);
}
ExpectedStmt ASTNodeImporter::VisitCXXForRangeStmt(CXXForRangeStmt *S) {
Index: clang/include/clang/AST/StmtCXX.h
===
--- clang/include/clang/AST/StmtCXX.
[PATCH] D146758: Fix codegen for coroutine with function-try-block
ChuanqiXu added inline comments.
Comment at: clang/lib/CodeGen/CGCoroutine.cpp:724-730
+ Stmt *BodyStmt = S.getBody();
+ CompoundStmt *Body = dyn_cast(BodyStmt);
+ if (Body == nullptr) {
+Body =
+CompoundStmt::Create(getContext(), {BodyStmt}, FPOptionsOverride(),
+ SourceLocation(), SourceLocation());
+ }
ChuanqiXu wrote:
> Can we try to move the logic to `CoroutineStmtBuilder`? That makes me feel
> better. And it will be helpful to add a comment to tell that we're handling
> the case the function body is function-try-block.
It looks like you didn't address the comments. Would you like to address it? I
don't mind to address it later myself.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D146758/new/
https://reviews.llvm.org/D146758
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D146758: Fix codegen for coroutine with function-try-block
MatzeB updated this revision to Diff 508792.
MatzeB marked an inline comment as done.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D146758/new/
https://reviews.llvm.org/D146758
Files:
clang/include/clang/AST/StmtCXX.h
clang/lib/AST/ASTImporter.cpp
clang/lib/AST/StmtCXX.cpp
clang/lib/CodeGen/CGCoroutine.cpp
clang/lib/Sema/SemaStmt.cpp
clang/test/CodeGenCoroutines/coro-function-try-block.cpp
Index: clang/test/CodeGenCoroutines/coro-function-try-block.cpp
===
--- /dev/null
+++ clang/test/CodeGenCoroutines/coro-function-try-block.cpp
@@ -0,0 +1,23 @@
+// RUN: %clang_cc1 -std=c++20 -triple=x86_64-- -emit-llvm -fcxx-exceptions \
+// RUN:-disable-llvm-passes %s -o - | FileCheck %s
+
+#include "Inputs/coroutine.h"
+
+struct task {
+ struct promise_type {
+task get_return_object();
+std::suspend_never initial_suspend();
+std::suspend_never final_suspend() noexcept;
+void return_void();
+void unhandled_exception() noexcept;
+ };
+};
+
+task f() try {
+ co_return;
+} catch(...) {
+}
+
+// CHECK-LABEL: define{{.*}} void @_Z1fv(
+// CHECK: call void @_ZNSt13suspend_never13await_suspendESt16coroutine_handleIvE(
+// CHECK: call void @_ZN4task12promise_type11return_voidEv(
Index: clang/lib/Sema/SemaStmt.cpp
===
--- clang/lib/Sema/SemaStmt.cpp
+++ clang/lib/Sema/SemaStmt.cpp
@@ -4546,7 +4546,8 @@
FSI->setHasCXXTry(TryLoc);
- return CXXTryStmt::Create(Context, TryLoc, TryBlock, Handlers);
+ return CXXTryStmt::Create(Context, TryLoc, cast(TryBlock),
+Handlers);
}
StmtResult Sema::ActOnSEHTryBlock(bool IsCXXTry, SourceLocation TryLoc,
Index: clang/lib/CodeGen/CGCoroutine.cpp
===
--- clang/lib/CodeGen/CGCoroutine.cpp
+++ clang/lib/CodeGen/CGCoroutine.cpp
@@ -721,8 +721,17 @@
auto Loc = S.getBeginLoc();
CXXCatchStmt Catch(Loc, /*exDecl=*/nullptr,
CurCoro.Data->ExceptionHandler);
- auto *TryStmt =
- CXXTryStmt::Create(getContext(), Loc, S.getBody(), &Catch);
+ Stmt *BodyStmt = S.getBody();
+ // We are about to create a `CXXTryStmt` which requires a `CompoundStmt`.
+ // We have to create a new CompoundStmt if the function body is not a
+ // `CompoundStmt` yet in cases like the "function-try-block" syntax.
+ CompoundStmt *Body = dyn_cast(BodyStmt);
+ if (Body == nullptr) {
+Body =
+CompoundStmt::Create(getContext(), {BodyStmt}, FPOptionsOverride(),
+ SourceLocation(), SourceLocation());
+ }
+ auto *TryStmt = CXXTryStmt::Create(getContext(), Loc, Body, &Catch);
EnterCXXTryStmt(*TryStmt);
emitBodyAndFallthrough(*this, S, TryStmt->getTryBlock());
Index: clang/lib/AST/StmtCXX.cpp
===
--- clang/lib/AST/StmtCXX.cpp
+++ clang/lib/AST/StmtCXX.cpp
@@ -23,7 +23,8 @@
}
CXXTryStmt *CXXTryStmt::Create(const ASTContext &C, SourceLocation tryLoc,
- Stmt *tryBlock, ArrayRef handlers) {
+ CompoundStmt *tryBlock,
+ ArrayRef handlers) {
const size_t Size = totalSizeToAlloc(handlers.size() + 1);
void *Mem = C.Allocate(Size, alignof(CXXTryStmt));
return new (Mem) CXXTryStmt(tryLoc, tryBlock, handlers);
@@ -36,7 +37,7 @@
return new (Mem) CXXTryStmt(Empty, numHandlers);
}
-CXXTryStmt::CXXTryStmt(SourceLocation tryLoc, Stmt *tryBlock,
+CXXTryStmt::CXXTryStmt(SourceLocation tryLoc, CompoundStmt *tryBlock,
ArrayRef handlers)
: Stmt(CXXTryStmtClass), TryLoc(tryLoc), NumHandlers(handlers.size()) {
Stmt **Stmts = getStmts();
Index: clang/lib/AST/ASTImporter.cpp
===
--- clang/lib/AST/ASTImporter.cpp
+++ clang/lib/AST/ASTImporter.cpp
@@ -6793,8 +6793,8 @@
return ToHandlerOrErr.takeError();
}
- return CXXTryStmt::Create(
- Importer.getToContext(), *ToTryLocOrErr,*ToTryBlockOrErr, ToHandlers);
+ return CXXTryStmt::Create(Importer.getToContext(), *ToTryLocOrErr,
+cast(*ToTryBlockOrErr), ToHandlers);
}
ExpectedStmt ASTNodeImporter::VisitCXXForRangeStmt(CXXForRangeStmt *S) {
Index: clang/include/clang/AST/StmtCXX.h
===
--- clang/include/clang/AST/StmtCXX.h
+++ clang/include/clang/AST/StmtCXX.h
@@ -75,7 +75,8 @@
unsigned NumHandlers;
size_t numTrailingObjects(OverloadToken) const { return NumHandlers; }
- CXXTryStmt(SourceLocation tryLoc, Stmt *tryBlock, ArrayRef handlers);
+ CXXTryStmt(SourceLocation tryLoc, CompoundStmt *tryBlock,
+ ArrayRef handlers);
CXXTryStmt(EmptyShell Empty,
[PATCH] D146758: Fix codegen for coroutine with function-try-block
ChuanqiXu requested changes to this revision.
ChuanqiXu added a comment.
This revision now requires changes to proceed.
Thanks for finding this! We didn't notice this before.
Comment at: clang/lib/CodeGen/CGCoroutine.cpp:724-730
+ Stmt *BodyStmt = S.getBody();
+ CompoundStmt *Body = dyn_cast(BodyStmt);
+ if (Body == nullptr) {
+Body =
+CompoundStmt::Create(getContext(), {BodyStmt}, FPOptionsOverride(),
+ SourceLocation(), SourceLocation());
+ }
Can we try to move the logic to `CoroutineStmtBuilder`? That makes me feel
better. And it will be helpful to add a comment to tell that we're handling the
case the function body is function-try-block.
Comment at: clang/lib/CodeGen/CGCoroutine.cpp:726
+ CompoundStmt *Body = dyn_cast(BodyStmt);
+ if (Body == nullptr) {
+Body =
It reads better to specify the potential type for Body.
Comment at: clang/test/CodeGenCoroutines/coro-function-try-block.cpp:21-23
+// CHECK-LABEL: define{{.*}} void @_Z1fv(
+// CHECK: call void
@_ZNSt13suspend_never13await_suspendESt16coroutine_handleIvE(
+// CHECK: call void @_ZN4task12promise_type11return_voidEv(
I expect to see the nested try statements in the case.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D146758/new/
https://reviews.llvm.org/D146758
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D146758: Fix codegen for coroutine with function-try-block
MatzeB added inline comments. Comment at: clang/test/CodeGenCoroutines/coro-function-try-block.cpp:1 +// RUN: %clang_cc1 -std=c++20 -triple=x86_64-- -emit-llvm -fcxx-exceptions \ +// RUN:-disable-llvm-passes %s -o - | FileCheck %s bruno wrote: > Space after `-triple=x86_64` it is `-triple=x86_64--` (lazy mans way of writing `-triple=x86_64-unknown-unknown`) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D146758/new/ https://reviews.llvm.org/D146758 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D146758: Fix codegen for coroutine with function-try-block
bruno accepted this revision. bruno added a comment. This revision is now accepted and ready to land. LGTM! @ChuanqiXu wdyt? Comment at: clang/test/CodeGenCoroutines/coro-function-try-block.cpp:1 +// RUN: %clang_cc1 -std=c++20 -triple=x86_64-- -emit-llvm -fcxx-exceptions \ +// RUN:-disable-llvm-passes %s -o - | FileCheck %s Space after `-triple=x86_64` Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D146758/new/ https://reviews.llvm.org/D146758 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D146758: Fix codegen for coroutine with function-try-block
MatzeB created this revision.
MatzeB added reviewers: GorNishanov, EricWF, ChuanqiXu, bruno.
Herald added subscribers: modimo, wenlei, martong, mcrosier.
Herald added a reviewer: shafik.
Herald added a project: All.
MatzeB requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
This fixes an assertion error when writing a coroutine with a
function-try-block. In this case the function body is not a `CompoundStmt` so
the code constructing an artificial CXXTryStmt must also construct a
`CompoundStmt` for it.
While on it adjust the `CXXStmt::Create` function to only accept
`CompoundStmt*`.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D146758
Files:
clang/include/clang/AST/StmtCXX.h
clang/lib/AST/ASTImporter.cpp
clang/lib/AST/StmtCXX.cpp
clang/lib/CodeGen/CGCoroutine.cpp
clang/lib/Sema/SemaStmt.cpp
clang/test/CodeGenCoroutines/coro-function-try-block.cpp
Index: clang/test/CodeGenCoroutines/coro-function-try-block.cpp
===
--- /dev/null
+++ clang/test/CodeGenCoroutines/coro-function-try-block.cpp
@@ -0,0 +1,23 @@
+// RUN: %clang_cc1 -std=c++20 -triple=x86_64-- -emit-llvm -fcxx-exceptions \
+// RUN:-disable-llvm-passes %s -o - | FileCheck %s
+
+#include "Inputs/coroutine.h"
+
+struct task {
+ struct promise_type {
+task get_return_object();
+std::suspend_never initial_suspend();
+std::suspend_never final_suspend() noexcept;
+void return_void();
+void unhandled_exception() noexcept;
+ };
+};
+
+task f() try {
+ co_return;
+} catch(...) {
+}
+
+// CHECK-LABEL: define{{.*}} void @_Z1fv(
+// CHECK: call void @_ZNSt13suspend_never13await_suspendESt16coroutine_handleIvE(
+// CHECK: call void @_ZN4task12promise_type11return_voidEv(
Index: clang/lib/Sema/SemaStmt.cpp
===
--- clang/lib/Sema/SemaStmt.cpp
+++ clang/lib/Sema/SemaStmt.cpp
@@ -4546,7 +4546,8 @@
FSI->setHasCXXTry(TryLoc);
- return CXXTryStmt::Create(Context, TryLoc, TryBlock, Handlers);
+ return CXXTryStmt::Create(Context, TryLoc, cast(TryBlock),
+Handlers);
}
StmtResult Sema::ActOnSEHTryBlock(bool IsCXXTry, SourceLocation TryLoc,
Index: clang/lib/CodeGen/CGCoroutine.cpp
===
--- clang/lib/CodeGen/CGCoroutine.cpp
+++ clang/lib/CodeGen/CGCoroutine.cpp
@@ -721,8 +721,14 @@
auto Loc = S.getBeginLoc();
CXXCatchStmt Catch(Loc, /*exDecl=*/nullptr,
CurCoro.Data->ExceptionHandler);
- auto *TryStmt =
- CXXTryStmt::Create(getContext(), Loc, S.getBody(), &Catch);
+ Stmt *BodyStmt = S.getBody();
+ CompoundStmt *Body = dyn_cast(BodyStmt);
+ if (Body == nullptr) {
+Body =
+CompoundStmt::Create(getContext(), {BodyStmt}, FPOptionsOverride(),
+ SourceLocation(), SourceLocation());
+ }
+ auto *TryStmt = CXXTryStmt::Create(getContext(), Loc, Body, &Catch);
EnterCXXTryStmt(*TryStmt);
emitBodyAndFallthrough(*this, S, TryStmt->getTryBlock());
Index: clang/lib/AST/StmtCXX.cpp
===
--- clang/lib/AST/StmtCXX.cpp
+++ clang/lib/AST/StmtCXX.cpp
@@ -23,7 +23,8 @@
}
CXXTryStmt *CXXTryStmt::Create(const ASTContext &C, SourceLocation tryLoc,
- Stmt *tryBlock, ArrayRef handlers) {
+ CompoundStmt *tryBlock,
+ ArrayRef handlers) {
const size_t Size = totalSizeToAlloc(handlers.size() + 1);
void *Mem = C.Allocate(Size, alignof(CXXTryStmt));
return new (Mem) CXXTryStmt(tryLoc, tryBlock, handlers);
@@ -36,7 +37,7 @@
return new (Mem) CXXTryStmt(Empty, numHandlers);
}
-CXXTryStmt::CXXTryStmt(SourceLocation tryLoc, Stmt *tryBlock,
+CXXTryStmt::CXXTryStmt(SourceLocation tryLoc, CompoundStmt *tryBlock,
ArrayRef handlers)
: Stmt(CXXTryStmtClass), TryLoc(tryLoc), NumHandlers(handlers.size()) {
Stmt **Stmts = getStmts();
Index: clang/lib/AST/ASTImporter.cpp
===
--- clang/lib/AST/ASTImporter.cpp
+++ clang/lib/AST/ASTImporter.cpp
@@ -6777,8 +6777,8 @@
return ToHandlerOrErr.takeError();
}
- return CXXTryStmt::Create(
- Importer.getToContext(), *ToTryLocOrErr,*ToTryBlockOrErr, ToHandlers);
+ return CXXTryStmt::Create(Importer.getToContext(), *ToTryLocOrErr,
+cast(*ToTryBlockOrErr), ToHandlers);
}
ExpectedStmt ASTNodeImporter::VisitCXXForRangeStmt(CXXForRangeStmt *S) {
Index: clang/include/clang/AST/StmtCXX.h
===
--- clang/include/clang/AST/StmtCXX.h
+++ clang/include/clang/AST/StmtCXX.h
@@ -75,7 +75,8 @@
unsigned NumHandlers;
size_t nu
