rnk created this revision.
rnk added a reviewer: rsmith.
Before this patch, Sema pre-allocated a FunctionScopeInfo and kept it in
the first, always present element of the FunctionScopes stack. This
meant that Sema::getCurFunction would return a pointer to this
pre-allocated object when parsing code outside a function body. This is
pretty much always a bug, so this patch moves the pre-allocated object
into a separate unique_ptr. This should make bugs like PR36536 a lot
more obvious.
As you can see from this patch, there were a number of places that
unconditionally assumed they were always called inside a function.
However, there are also many places that null checked the result of
getCurFunction(), so I think this is a reasonable direction.
https://reviews.llvm.org/D44039
Files:
clang/include/clang/Sema/Sema.h
clang/lib/Sema/AnalysisBasedWarnings.cpp
clang/lib/Sema/Sema.cpp
clang/lib/Sema/SemaDecl.cpp
clang/lib/Sema/SemaExprCXX.cpp
Index: clang/lib/Sema/SemaExprCXX.cpp
===================================================================
--- clang/lib/Sema/SemaExprCXX.cpp
+++ clang/lib/Sema/SemaExprCXX.cpp
@@ -1114,8 +1114,9 @@
assert((!ByCopy || Explicit) && "cannot implicitly capture *this by value");
- const unsigned MaxFunctionScopesIndex = FunctionScopeIndexToStopAt ?
- *FunctionScopeIndexToStopAt : FunctionScopes.size() - 1;
+ const int MaxFunctionScopesIndex = FunctionScopeIndexToStopAt
+ ? *FunctionScopeIndexToStopAt
+ : FunctionScopes.size() - 1;
// Check that we can capture the *enclosing object* (referred to by '*this')
// by the capturing-entity/closure (lambda/block/etc) at
@@ -1141,7 +1142,7 @@
unsigned NumCapturingClosures = 0;
- for (unsigned idx = MaxFunctionScopesIndex; idx != 0; idx--) {
+ for (int idx = MaxFunctionScopesIndex; idx >= 0; idx--) {
if (CapturingScopeInfo *CSI =
dyn_cast<CapturingScopeInfo>(FunctionScopes[idx])) {
if (CSI->CXXThisCaptureIndex != 0) {
@@ -1196,8 +1197,8 @@
// FIXME: We need to delay this marking in PotentiallyPotentiallyEvaluated
// contexts.
QualType ThisTy = getCurrentThisType();
- for (unsigned idx = MaxFunctionScopesIndex; NumCapturingClosures;
- --idx, --NumCapturingClosures) {
+ for (int idx = MaxFunctionScopesIndex; NumCapturingClosures;
+ --idx, --NumCapturingClosures) {
CapturingScopeInfo *CSI = cast<CapturingScopeInfo>(FunctionScopes[idx]);
Expr *ThisExpr = nullptr;
@@ -7176,9 +7177,6 @@
const bool IsFullExprInstantiationDependent = FE->isInstantiationDependent();
- ArrayRef<const FunctionScopeInfo *> FunctionScopesArrayRef(
- S.FunctionScopes.data(), S.FunctionScopes.size());
-
// All the potentially captureable variables in the current nested
// lambda (within a generic outer lambda), must be captured by an
// outer lambda that is enclosed within a non-dependent context.
@@ -7207,7 +7205,7 @@
// capture the variable in that lambda (and all its enclosing lambdas).
if (const Optional<unsigned> Index =
getStackIndexOfNearestEnclosingCaptureCapableLambda(
- FunctionScopesArrayRef, Var, S)) {
+ S.FunctionScopes, Var, S)) {
const unsigned FunctionScopeIndexOfCapturableLambda = Index.getValue();
MarkVarDeclODRUsed(Var, VarExpr->getExprLoc(), S,
&FunctionScopeIndexOfCapturableLambda);
@@ -7243,7 +7241,7 @@
// 'this' in that lambda (and all its enclosing lambdas).
if (const Optional<unsigned> Index =
getStackIndexOfNearestEnclosingCaptureCapableLambda(
- FunctionScopesArrayRef, /*0 is 'this'*/ nullptr, S)) {
+ S.FunctionScopes, /*0 is 'this'*/ nullptr, S)) {
const unsigned FunctionScopeIndexOfCapturableLambda = Index.getValue();
S.CheckCXXThisCapture(CurrentLSI->PotentialThisCaptureLocation,
/*Explicit*/ false, /*BuildAndDiagnose*/ true,
Index: clang/lib/Sema/SemaDecl.cpp
===================================================================
--- clang/lib/Sema/SemaDecl.cpp
+++ clang/lib/Sema/SemaDecl.cpp
@@ -5747,7 +5747,8 @@
TypeSourceInfo *TInfo = NewTD->getTypeSourceInfo();
QualType T = TInfo->getType();
if (T->isVariablyModifiedType()) {
- getCurFunction()->setHasBranchProtectedScope();
+ if (getCurFunction())
+ getCurFunction()->setHasBranchProtectedScope();
if (S->getFnParent() == nullptr) {
bool SizeIsNegative;
@@ -7407,7 +7408,8 @@
bool isVM = T->isVariablyModifiedType();
if (isVM || NewVD->hasAttr<CleanupAttr>() ||
NewVD->hasAttr<BlocksAttr>())
- getCurFunction()->setHasBranchProtectedScope();
+ if (getCurFunction())
+ getCurFunction()->setHasBranchProtectedScope();
if ((isVM && NewVD->hasLinkage()) ||
(T->isVariableArrayType() && NewVD->hasGlobalStorage())) {
@@ -10643,7 +10645,7 @@
return;
}
- if (VDecl->hasLocalStorage())
+ if (VDecl->hasLocalStorage() && getCurFunction())
getCurFunction()->setHasBranchProtectedScope();
if (DiagnoseUnexpandedParameterPack(Init, UPPC_Initializer)) {
@@ -11181,7 +11183,7 @@
// Mark the function for further checking even if the looser rules of
// C++11 do not require such checks, so that we can diagnose
// incompatibilities with C++98.
- if (!CXXRecord->isPOD())
+ if (!CXXRecord->isPOD() && getCurFunction())
getCurFunction()->setHasBranchProtectedScope();
}
}
@@ -11323,7 +11325,8 @@
}
}
- if (var->getType().isDestructedType() == QualType::DK_nontrivial_c_struct)
+ if (var->hasLocalStorage() &&
+ var->getType().isDestructedType() == QualType::DK_nontrivial_c_struct)
getCurFunction()->setHasBranchProtectedScope();
// Warn about externally-visible variables being defined without a
Index: clang/lib/Sema/Sema.cpp
===================================================================
--- clang/lib/Sema/Sema.cpp
+++ clang/lib/Sema/Sema.cpp
@@ -162,7 +162,7 @@
ExpressionEvaluationContext::PotentiallyEvaluated, 0, CleanupInfo{},
nullptr, false);
- FunctionScopes.push_back(new FunctionScopeInfo(Diags));
+ PreallocatedFunctionScope.reset(new FunctionScopeInfo(Diags));
// Initilization of data sharing attributes stack for OpenMP
InitDataSharingAttributesStack();
@@ -332,11 +332,14 @@
Sema::~Sema() {
if (VisContext) FreeVisContext();
+
// Kill all the active scopes.
- for (unsigned I = 1, E = FunctionScopes.size(); I != E; ++I)
- delete FunctionScopes[I];
- if (FunctionScopes.size() == 1)
- delete FunctionScopes[0];
+ for (sema::FunctionScopeInfo *FSI : FunctionScopes) {
+ if (FSI != PreallocatedFunctionScope.get())
+ delete FSI;
+ }
+ FunctionScopes.clear();
+ PreallocatedFunctionScope.reset();
// Tell the SemaConsumer to forget about us; we're going out of scope.
if (SemaConsumer *SC = dyn_cast<SemaConsumer>(&Consumer))
@@ -1340,17 +1343,13 @@
/// \brief Enter a new function scope
void Sema::PushFunctionScope() {
- if (FunctionScopes.size() == 1) {
- // Use the "top" function scope rather than having to allocate
- // memory for a new scope.
- FunctionScopes.back()->Clear();
- FunctionScopes.push_back(FunctionScopes.back());
- if (LangOpts.OpenMP)
- pushOpenMPFunctionRegion();
- return;
+ if (FunctionScopes.empty()) {
+ // Use PreallocatedFunctionScope to avoid allocating memory when possible.
+ PreallocatedFunctionScope->Clear();
+ FunctionScopes.push_back(PreallocatedFunctionScope.get());
+ } else {
+ FunctionScopes.push_back(new FunctionScopeInfo(getDiagnostics()));
}
-
- FunctionScopes.push_back(new FunctionScopeInfo(getDiagnostics()));
if (LangOpts.OpenMP)
pushOpenMPFunctionRegion();
}
@@ -1370,15 +1369,15 @@
if (LambdaScopeInfo *const LSI = getCurLambda()) {
LSI->AutoTemplateParameterDepth = Depth;
return;
- }
- llvm_unreachable(
+ }
+ llvm_unreachable(
"Remove assertion if intentionally called in a non-lambda context.");
}
void Sema::PopFunctionScopeInfo(const AnalysisBasedWarnings::Policy *WP,
const Decl *D, const BlockExpr *blkExpr) {
- FunctionScopeInfo *Scope = FunctionScopes.pop_back_val();
assert(!FunctionScopes.empty() && "mismatched push/pop!");
+ FunctionScopeInfo *Scope = FunctionScopes.pop_back_val();
if (LangOpts.OpenMP)
popOpenMPFunctionRegion(Scope);
@@ -1390,7 +1389,8 @@
for (const auto &PUD : Scope->PossiblyUnreachableDiags)
Diag(PUD.Loc, PUD.PD);
- if (FunctionScopes.back() != Scope)
+ // Delete the scope unless its our preallocated scope.
+ if (Scope != PreallocatedFunctionScope.get())
delete Scope;
}
Index: clang/lib/Sema/AnalysisBasedWarnings.cpp
===================================================================
--- clang/lib/Sema/AnalysisBasedWarnings.cpp
+++ clang/lib/Sema/AnalysisBasedWarnings.cpp
@@ -632,18 +632,19 @@
} // anonymous namespace
-/// CheckFallThroughForFunctionDef - Check that we don't fall off the end of a
+/// CheckFallThroughForBody - Check that we don't fall off the end of a
/// function that should return a value. Check that we don't fall off the end
/// of a noreturn function. We assume that functions and blocks not marked
/// noreturn will return.
static void CheckFallThroughForBody(Sema &S, const Decl *D, const Stmt *Body,
const BlockExpr *blkExpr,
- const CheckFallThroughDiagnostics& CD,
- AnalysisDeclContext &AC) {
+ const CheckFallThroughDiagnostics &CD,
+ AnalysisDeclContext &AC,
+ sema::FunctionScopeInfo *FSI) {
bool ReturnsVoid = false;
bool HasNoReturn = false;
- bool IsCoroutine = S.getCurFunction() && S.getCurFunction()->isCoroutine();
+ bool IsCoroutine = FSI->isCoroutine();
if (const auto *FD = dyn_cast<FunctionDecl>(D)) {
if (const auto *CBody = dyn_cast<CoroutineBodyStmt>(Body))
@@ -675,7 +676,7 @@
SourceLocation LBrace = Body->getLocStart(), RBrace = Body->getLocEnd();
auto EmitDiag = [&](SourceLocation Loc, unsigned DiagID) {
if (IsCoroutine)
- S.Diag(Loc, DiagID) << S.getCurFunction()->CoroutinePromise->getType();
+ S.Diag(Loc, DiagID) << FSI->CoroutinePromise->getType();
else
S.Diag(Loc, DiagID);
};
@@ -2143,7 +2144,7 @@
: (fscope->isCoroutine()
? CheckFallThroughDiagnostics::MakeForCoroutine(D)
: CheckFallThroughDiagnostics::MakeForFunction(D)));
- CheckFallThroughForBody(S, D, Body, blkExpr, CD, AC);
+ CheckFallThroughForBody(S, D, Body, blkExpr, CD, AC, fscope);
}
// Warning: check for unreachable code
Index: clang/include/clang/Sema/Sema.h
===================================================================
--- clang/include/clang/Sema/Sema.h
+++ clang/include/clang/Sema/Sema.h
@@ -529,12 +529,10 @@
/// full expression.
llvm::SmallPtrSet<Expr*, 2> MaybeODRUseExprs;
+ std::unique_ptr<sema::FunctionScopeInfo> PreallocatedFunctionScope;
+
/// \brief Stack containing information about each of the nested
/// function, block, and method scopes that are currently active.
- ///
- /// This array is never empty. Clients should ignore the first
- /// element, which is used to cache a single FunctionScopeInfo
- /// that's used to parse every top-level function.
SmallVector<sema::FunctionScopeInfo *, 4> FunctionScopes;
typedef LazyVector<TypedefNameDecl *, ExternalSemaSource,
@@ -1319,7 +1317,7 @@
const BlockExpr *blkExpr = nullptr);
sema::FunctionScopeInfo *getCurFunction() const {
- return FunctionScopes.back();
+ return FunctionScopes.empty() ? nullptr : FunctionScopes.back();
}
sema::FunctionScopeInfo *getEnclosingFunction() const {
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
