-
Notifications
You must be signed in to change notification settings - Fork 11.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Diagnose use of VLAs in a coroutine #70341
Conversation
@llvm/pr-subscribers-clang @llvm/pr-subscribers-coroutines Author: Aaron Ballman (AaronBallman) ChangesFixes #65858 Full diff: https://github.com/llvm/llvm-project/pull/70341.diff 5 Files Affected:
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index a673ce726d6c220..453bd8a9a340425 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -166,6 +166,8 @@ def ext_vla_folded_to_constant : ExtWarn<
InGroup<GNUFoldingConstant>;
def err_vla_unsupported : Error<
"variable length arrays are not supported for %select{the current target|'%1'}0">;
+def err_vla_in_coroutine_unsupported : Error<
+ "variable length arrays in a coroutine are not supported">;
def note_vla_unsupported : Note<
"variable length arrays are not supported for the current target">;
diff --git a/clang/include/clang/Sema/ScopeInfo.h b/clang/include/clang/Sema/ScopeInfo.h
index 02b22af89ff035d..b2f6e3289f41fce 100644
--- a/clang/include/clang/Sema/ScopeInfo.h
+++ b/clang/include/clang/Sema/ScopeInfo.h
@@ -189,6 +189,9 @@ class FunctionScopeInfo {
/// First SEH '__try' statement in the current function.
SourceLocation FirstSEHTryLoc;
+ /// First use of a VLA within the current function.
+ SourceLocation FirstVLALoc;
+
private:
/// Used to determine if errors occurred in this function or block.
DiagnosticErrorTrap ErrorTrap;
@@ -473,6 +476,11 @@ class FunctionScopeInfo {
FirstSEHTryLoc = TryLoc;
}
+ void setHasVLA(SourceLocation VLALoc) {
+ if (FirstVLALoc.isInvalid())
+ FirstVLALoc = VLALoc;
+ }
+
bool NeedsScopeChecking() const {
return !HasDroppedStmt && (HasIndirectGoto || HasMustTail ||
(HasBranchProtectedScope && HasBranchIntoScope));
diff --git a/clang/lib/Sema/SemaCoroutine.cpp b/clang/lib/Sema/SemaCoroutine.cpp
index d2b0922a4bb9c4c..e1f3b5acb3cadec 100644
--- a/clang/lib/Sema/SemaCoroutine.cpp
+++ b/clang/lib/Sema/SemaCoroutine.cpp
@@ -1198,6 +1198,11 @@ void Sema::CheckCompletedCoroutineBody(FunctionDecl *FD, Stmt *&Body) {
if (FD->hasAttr<AlwaysInlineAttr>())
Diag(FD->getLocation(), diag::warn_always_inline_coroutine);
+ // We don't allow use of VLAs within a coroutine, so diagnose if we've seen
+ // a VLA in the body of this function.
+ if (Fn->FirstVLALoc.isValid())
+ Diag(Fn->FirstVLALoc, diag::err_vla_in_coroutine_unsupported);
+
// [stmt.return.coroutine]p1:
// A coroutine shall not enclose a return statement ([stmt.return]).
if (Fn->FirstReturnLoc.isValid()) {
diff --git a/clang/lib/Sema/SemaType.cpp b/clang/lib/Sema/SemaType.cpp
index 28b81c1768a3004..dea77fae4cadb59 100644
--- a/clang/lib/Sema/SemaType.cpp
+++ b/clang/lib/Sema/SemaType.cpp
@@ -2706,12 +2706,18 @@ QualType Sema::BuildArrayType(QualType T, ArrayType::ArraySizeModifier ASM,
}
}
- if (T->isVariableArrayType() && !Context.getTargetInfo().isVLASupported()) {
- // CUDA device code and some other targets don't support VLAs.
- bool IsCUDADevice = (getLangOpts().CUDA && getLangOpts().CUDAIsDevice);
- targetDiag(Loc,
- IsCUDADevice ? diag::err_cuda_vla : diag::err_vla_unsupported)
- << (IsCUDADevice ? CurrentCUDATarget() : 0);
+ if (T->isVariableArrayType()) {
+ if (!Context.getTargetInfo().isVLASupported()) {
+ // CUDA device code and some other targets don't support VLAs.
+ bool IsCUDADevice = (getLangOpts().CUDA && getLangOpts().CUDAIsDevice);
+ targetDiag(Loc,
+ IsCUDADevice ? diag::err_cuda_vla : diag::err_vla_unsupported)
+ << (IsCUDADevice ? CurrentCUDATarget() : 0);
+ } else if (sema::FunctionScopeInfo *FSI = getCurFunction()) {
+ // VLAs are supported on this target, but we may need to do delayed
+ // checking that the VLA is not being used within a coroutine.
+ FSI->setHasVLA(Loc);
+ }
}
// If this is not C99, diagnose array size modifiers on non-VLAs.
diff --git a/clang/test/SemaCXX/coroutine-vla.cpp b/clang/test/SemaCXX/coroutine-vla.cpp
new file mode 100644
index 000000000000000..176e35f346e2b45
--- /dev/null
+++ b/clang/test/SemaCXX/coroutine-vla.cpp
@@ -0,0 +1,29 @@
+// RUN: %clang_cc1 %s -std=c++20 -fsyntax-only -Wno-vla-cxx-extension -verify
+#include "Inputs/std-coroutine.h"
+
+struct promise;
+
+struct coroutine : std::coroutine_handle<promise> {
+ using promise_type = ::promise;
+};
+
+struct promise
+{
+ coroutine get_return_object();
+ std::suspend_always initial_suspend() noexcept;
+ std::suspend_always final_suspend() noexcept;
+ void return_void();
+ void unhandled_exception();
+};
+
+coroutine foo(int n) {
+ int array[n]; // expected-error {{variable length arrays in a coroutine are not supported}}
+ co_return;
+}
+
+void lambda() {
+ [](int n) -> coroutine {
+ int array[n]; // expected-error {{variable length arrays in a coroutine are not supported}}
+ co_return;
+ }(10);
+}
|
LGTM but it is missing a release note :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! LGTM with a nit.
clang/lib/Sema/SemaCoroutine.cpp
Outdated
@@ -1198,6 +1198,11 @@ void Sema::CheckCompletedCoroutineBody(FunctionDecl *FD, Stmt *&Body) { | |||
if (FD->hasAttr<AlwaysInlineAttr>()) | |||
Diag(FD->getLocation(), diag::warn_always_inline_coroutine); | |||
|
|||
// We don't allow use of VLAs within a coroutine, so diagnose if we've seen |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// We don't allow use of VLAs within a coroutine, so diagnose if we've seen | |
// VLAs are not allowed within a coroutine, so diagnose if we've seen |
nit: it is not us (clang/LLVM) who decide to not support VLA in coroutines. But it is the C++20 coroutines itself can't support VLA by its design naturally.
it may be fine to not mention it too since VLAs in coroutines never got compiled.. user might meet backend crashes.. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment suggestion makes sense to me, else just needs a release note.
Fixes #65858