[Clang] [Sema] Don't crash on unexpanded pack in invalid block literal #110762
Conversation
Sirraide
added
clang:frontend
|
@llvm/pr-subscribers-clang Author: None (Sirraide) ChangesConsider #109148: template <typename ...Ts>
void f() {
[] {
(^Ts);
};
}When we encounter Side note: lambdas don’t suffer from this problem because they push a new template<typename... Ts>
void f(Ts...ts) {
((^ void (Ts) {} (ts)), ...); // error: block contains unexpanded parameter pack 'Ts'
};I spent some time thinking of a few ways of fixing this (and the author of the issue for this has also proposed some approaches, including the one I decided on); ultimately, I decided on having This feels like a bit of a hack, but the alternatives I’ve seen so far or managed to think of myself are either
Option 1 is dubious at best imo; I have no idea how 2 would work; and 3 seems like it might end up being a lot of work if there’s a reason why blocks don’t act like lambdas wrt unexpanded packs. This fixes #109148. Full diff: https://github.com/llvm/llvm-project/pull/110762.diff 5 Files Affected:
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 34d2b584274a5f..ed4a7affcf6919 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -457,6 +457,8 @@ Bug Fixes to C++ Support
containing outer unexpanded parameters were not correctly expanded. (#GH101754)
- Fixed a bug in constraint expression comparison where the ``sizeof...`` expression was not handled properly
in certain friend declarations. (#GH93099)
+- Clang no longer crashes when a lambda contains an invalid block declaration that contains an unexpanded
+ parameter pack. (#GH109148)
Bug Fixes to AST Handling
^^^^^^^^^^^^^^^^^^^^^^^^^
diff --git a/clang/include/clang/Sema/ScopeInfo.h b/clang/include/clang/Sema/ScopeInfo.h
index 700e361ef83f13..3bad1e3bad4312 100644
--- a/clang/include/clang/Sema/ScopeInfo.h
+++ b/clang/include/clang/Sema/ScopeInfo.h
@@ -793,6 +793,16 @@ class BlockScopeInfo final : public CapturingScopeInfo {
/// Its return type may be BuiltinType::Dependent.
QualType FunctionType;
+ /// We sometimes diagnose unexpanded parameter packs in block literals,
+ /// but an error while the block is parsed can cause it to be discarded,
+ /// in which case we need to reset the enclosing lambda's
+ /// ContainsUnexpandedParameterPack flag.
+ ///
+ /// Note: This issue does not exist with lambdas because they push a new
+ /// LambdaScopeInfo, so if the expression is discarded, the 'enclosing
+ /// lambda' is discarded along with it.
+ bool EnclosingLambdaContainsUnexpandedParameterPack = false;
+
BlockScopeInfo(DiagnosticsEngine &Diag, Scope *BlockScope, BlockDecl *Block)
: CapturingScopeInfo(Diag, ImpCap_Block), TheDecl(Block),
TheScope(BlockScope) {
diff --git a/clang/lib/Sema/Sema.cpp b/clang/lib/Sema/Sema.cpp
index 4be7dfbc293927..1e72910da27d5e 100644
--- a/clang/lib/Sema/Sema.cpp
+++ b/clang/lib/Sema/Sema.cpp
@@ -2187,9 +2187,13 @@ void Sema::PushFunctionScope() {
}
void Sema::PushBlockScope(Scope *BlockScope, BlockDecl *Block) {
- FunctionScopes.push_back(new BlockScopeInfo(getDiagnostics(),
- BlockScope, Block));
+ auto *BSI = new BlockScopeInfo(getDiagnostics(), BlockScope, Block);
+ FunctionScopes.push_back(BSI);
CapturingFunctionScopes++;
+
+ LambdaScopeInfo *Enclosing = getEnclosingLambda();
+ BSI->EnclosingLambdaContainsUnexpandedParameterPack =
+ Enclosing && Enclosing->ContainsUnexpandedParameterPack;
}
LambdaScopeInfo *Sema::PushLambdaScope() {
diff --git a/clang/lib/Sema/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp
index 2db9d1fc69ed1e..d1c91296a9d468 100644
--- a/clang/lib/Sema/SemaExpr.cpp
+++ b/clang/lib/Sema/SemaExpr.cpp
@@ -16187,6 +16187,16 @@ void Sema::ActOnBlockArguments(SourceLocation CaretLoc, Declarator &ParamInfo,
}
void Sema::ActOnBlockError(SourceLocation CaretLoc, Scope *CurScope) {
+ // If the enclosing lambda did not contain any unexpanded parameter
+ // packs before this block, then reset the unexpanded parameter pack
+ // flag, otherwise, we might end up crashing trying to find a pack
+ // that we are about to discard along with the rest of the block.
+ if (!getCurBlock()->EnclosingLambdaContainsUnexpandedParameterPack) {
+ LambdaScopeInfo *L = getEnclosingLambda();
+ if (L)
+ L->ContainsUnexpandedParameterPack = false;
+ }
+
// Leave the expression-evaluation context.
DiscardCleanupsInEvaluationContext();
PopExpressionEvaluationContext();
@@ -16379,6 +16389,8 @@ ExprResult Sema::ActOnBlockStmtExpr(SourceLocation CaretLoc,
if (getCurFunction())
getCurFunction()->addBlock(BD);
+ // This can happen if the block's return type is deduced, but
+ // the return expression is invalid.
if (BD->isInvalidDecl())
return CreateRecoveryExpr(Result->getBeginLoc(), Result->getEndLoc(),
{Result}, Result->getType());
diff --git a/clang/test/SemaCXX/block-unexpanded-pack.cpp b/clang/test/SemaCXX/block-unexpanded-pack.cpp
new file mode 100644
index 00000000000000..c1435b8e314a81
--- /dev/null
+++ b/clang/test/SemaCXX/block-unexpanded-pack.cpp
@@ -0,0 +1,48 @@
+// RUN: %clang_cc1 -fblocks -triple x86_64-apple-darwin -fsyntax-only -verify %s
+// RUN: %clang_cc1 -fblocks -triple x86_64-apple-darwin -fsyntax-only -verify %s -frecovery-ast -frecovery-ast-type
+
+// This checks that when a block is discarded, the enclosing lambda’s
+// unexpanded parameter pack flag is reset to what it was before the
+// block is parsed so we don't crash when trying to diagnose unexpanded
+// parameter packs in the lambda.
+
+template <typename ...Ts>
+void gh109148() {
+ (^Ts); // expected-error {{expected expression}} expected-error {{unexpanded parameter pack 'Ts'}}
+
+ [] {
+ (^Ts); // expected-error {{expected expression}}
+ ^Ts; // expected-error {{expected expression}}
+ ^(Ts); // expected-error {{expected expression}}
+ ^ Ts); // expected-error {{expected expression}}
+ };
+
+ ([] {
+ (^Ts); // expected-error {{expected expression}}
+ ^Ts; // expected-error {{expected expression}}
+ ^(Ts); // expected-error {{expected expression}}
+ ^ Ts); // expected-error {{expected expression}}
+ }, ...); // expected-error {{pack expansion does not contain any unexpanded parameter packs}}
+
+ [] { // expected-error {{unexpanded parameter pack 'Ts'}}
+ ^ (Ts) {};
+ };
+
+ [] { // expected-error {{unexpanded parameter pack 'Ts'}}
+ (void) ^ { Ts x; };
+ };
+
+ [] { // expected-error {{unexpanded parameter pack 'Ts'}}
+ Ts s;
+ (^Ts); // expected-error {{expected expression}}
+ };
+
+ ([] {
+ Ts s;
+ (^Ts); // expected-error {{expected expression}}
+ }, ...)
+
+ [] { // expected-error {{unexpanded parameter pack 'Ts'}}
+ ^ { Ts s; return not_defined; }; // expected-error {{use of undeclared identifier 'not_defined'}}
+ };
+}
|
|
I don't think there's a deep reason blocks and lambdas don't use quite the same scope mechanics in the compiler, so if you wanted to pursue that, it seems reasonable. But this approach also seems viable. I agree that removing the assertion would be the wrong thing to do, and I also don't see a reliable way to include the pack in the AST. |
|
The current solution seems reasonable to me.
|
Yeah, there is a comment about parameter packs in block arguments being a problem somewhere, but that’s from 2012, so that might no longer apply. I might look into this at some point if I have the time, but not for this pr (there are still other issues need to deal with first...) |
I am not sure i understand that. Imo the issue is that Ideally
|
Yeah, that’s also what I figured initially. I just assumed that there was some reason why blocks don’t or can’t work that way (I have never used Objective-C and I candidly don’t know a lot about blocks...), but it seems that there isn’t. So if you think that this is a better idea, I can instead look into refactoring blocks to work the same way as lambdas wrt |
Sirraide
force-pushed
the
block-unexpanded
branch
from
06ac9be
to
eac7b11
Compare
Surprisingly, this turned out to be fairly straight-forward to implement. (I had to fix something in the template instantiator, but I’ve run into the exact same problem before with lambdas, so I knew what to do this time round ;Þ) |
You can test this locally with the following command:git-clang-format --diff 2aa1dbf9c92e3b27954ba9166c927616625073e8 daf41fb0502e54e4ab5c63ed2227b7be88297c74 --extensions cpp,h -- clang/test/SemaCXX/block-packs.cpp clang/include/clang/AST/ComputeDependence.h clang/include/clang/AST/Expr.h clang/include/clang/Sema/ScopeInfo.h clang/include/clang/Sema/Sema.h clang/include/clang/Sema/Template.h clang/lib/AST/ComputeDependence.cpp clang/lib/Sema/Sema.cpp clang/lib/Sema/SemaDecl.cpp clang/lib/Sema/SemaExpr.cpp clang/lib/Sema/SemaLambda.cpp clang/lib/Sema/SemaTemplate.cpp clang/lib/Sema/SemaTemplateInstantiate.cpp clang/lib/Sema/SemaTemplateVariadic.cppView the diff from clang-format here.diff --git a/clang/lib/Sema/SemaTemplateVariadic.cpp b/clang/lib/Sema/SemaTemplateVariadic.cpp
index 19bd454766..42a075ae14 100644
--- a/clang/lib/Sema/SemaTemplateVariadic.cpp
+++ b/clang/lib/Sema/SemaTemplateVariadic.cpp
@@ -8,7 +8,6 @@
// This file implements semantic analysis for C++0x variadic templates.
//===----------------------------------------------------------------------===/
-#include "clang/Sema/Sema.h"
#include "TypeLocBuilder.h"
#include "clang/AST/Expr.h"
#include "clang/AST/RecursiveASTVisitor.h"
@@ -16,6 +15,7 @@
#include "clang/Sema/Lookup.h"
#include "clang/Sema/ParsedTemplate.h"
#include "clang/Sema/ScopeInfo.h"
+#include "clang/Sema/Sema.h"
#include "clang/Sema/SemaInternal.h"
#include "clang/Sema/Template.h"
#include "llvm/Support/SaveAndRestore.h"
|
... I don’t think I changed anything about that include? Or did it just reorder them because I added one? 🤔 |
llvm#110762) Consider llvm#109148: ```c++ template <typename ...Ts> void f() { [] { (^Ts); }; } ``` When we encounter `^Ts`, we try to parse a block and subsequently call `DiagnoseUnexpandedParameterPack()` (in `ActOnBlockArguments()`), which sees `Ts` and sets `ContainsUnexpandedParameterPack` to `true` in the `LambdaScopeInfo` of the enclosing lambda. However, the entire block is subsequently discarded entirely because it isn’t even syntactically well-formed. As a result, `ContainsUnexpandedParameterPack` is `true` despite the lambda’s body no longer containing any unexpanded packs, which causes an assertion the next time `DiagnoseUnexpandedParameterPack()` is called. This pr moves handling of unexpanded parameter packs into `CapturingScopeInfo` instead so that the same logic is used for both blocks and lambdas. This fixes this issue since the `ContainsUnexpandedParameterPack` flag is now part of the block (and before that, its `CapturingScopeInfo`) and no longer affects the surrounding lambda directly when the block is parsed. Moreover, this change makes blocks actually usable with pack expansion. This fixes llvm#109148.
llvm#110762) Consider llvm#109148: ```c++ template <typename ...Ts> void f() { [] { (^Ts); }; } ``` When we encounter `^Ts`, we try to parse a block and subsequently call `DiagnoseUnexpandedParameterPack()` (in `ActOnBlockArguments()`), which sees `Ts` and sets `ContainsUnexpandedParameterPack` to `true` in the `LambdaScopeInfo` of the enclosing lambda. However, the entire block is subsequently discarded entirely because it isn’t even syntactically well-formed. As a result, `ContainsUnexpandedParameterPack` is `true` despite the lambda’s body no longer containing any unexpanded packs, which causes an assertion the next time `DiagnoseUnexpandedParameterPack()` is called. This pr moves handling of unexpanded parameter packs into `CapturingScopeInfo` instead so that the same logic is used for both blocks and lambdas. This fixes this issue since the `ContainsUnexpandedParameterPack` flag is now part of the block (and before that, its `CapturingScopeInfo`) and no longer affects the surrounding lambda directly when the block is parsed. Moreover, this change makes blocks actually usable with pack expansion. This fixes llvm#109148.
Consider #109148:
When we encounter
^Ts, we try to parse a block and subsequently callDiagnoseUnexpandedParameterPack()(inActOnBlockArguments()), which seesTsand setsContainsUnexpandedParameterPacktotruein theLambdaScopeInfoof the enclosing lambda. However, the entire block is subsequently discarded entirely because it isn’t even syntactically well-formed. As a result,ContainsUnexpandedParameterPackistruedespite the lambda’s body no longer containing any unexpanded packs, which causes an assertion the next timeDiagnoseUnexpandedParameterPack()is called.This pr moves handling of unexpanded parameter packs into
CapturingScopeInfoinstead so that the same logic is used for both blocks and lambdas. This fixes this issue since theContainsUnexpandedParameterPackflag is now part of the block (and before that, itsCapturingScopeInfo) and no longer affects the surrounding lambda directly when the block is parsed. Moreover, this change makes blocks actually usable with pack expansion.This fixes #109148.