Skip to content

Commit

Permalink
Fix for #1017: Compiler issues an invalid-bounds error message for an…
Browse files Browse the repository at this point in the history
… out-of-scope variable. (#1031)

* Fix for the crash during CFG construction when CFGLifetimeEnds elements are added to the CFG.

* Make VariableOccurrenceCount take a ValueDecl rather than DeclRefExpr argument

* Removing checked pointers that are no longer in scope from ObservedBounds.
Also removing expressions that refer to out-of-scope variables from AvailableExpressions.

* Added a test case to test the modifications to equivalent expressions
when variables go out of scope within a basic block.

* Fix for a crash on Windows caused by an iterator access out of range. If one iterates through a loop and removes elements from it at the same time, the end of the iterator has to be re-evaluated in each iteration.

* Incorporated review comments.

* Incorporated review comments.

Co-authored-by: kakje <[email protected]>
  • Loading branch information
sulekhark and kakje authored Apr 17, 2021
1 parent 1d5b7e0 commit 4b43422
Show file tree
Hide file tree
Showing 10 changed files with 498 additions and 395 deletions.
14 changes: 8 additions & 6 deletions clang/lib/Sema/AvailableFactsAnalysis.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -73,12 +73,14 @@ void AvailableFactsAnalysis::Analyze() {
// Which blocks contain potential pointer assignments?
std::vector<bool> PointerAssignmentInBlocks(Blocks.size(), false);
for (unsigned int Index = 0; Index < Blocks.size(); Index++) {
for (CFGElement Elem : *(Blocks[Index]->Block))
if (const Expr *E = dyn_cast<Expr>(Elem.castAs<CFGStmt>().getStmt()))
if (ContainsPointerAssignment(E)) {
PointerAssignmentInBlocks[Index] = true;
break;
}
for (CFGElement Elem : *(Blocks[Index]->Block)) {
if (Elem.getKind() == CFGElement::Statement)
if (const Expr *E = dyn_cast<Expr>(Elem.castAs<CFGStmt>().getStmt()))
if (ContainsPointerAssignment(E)) {
PointerAssignmentInBlocks[Index] = true;
break;
}
}
}

// Compute Kill Sets
Expand Down
8 changes: 4 additions & 4 deletions clang/lib/Sema/BoundsAnalysis.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -687,13 +687,13 @@ T BoundsAnalysis::Intersect(T &A, T &B) const {
return Ret;
}

for (auto I = Ret.begin(), E = Ret.end(); I != E;) {
// As the container iterated over by Ret is modified within the
// body of the loop, we need to evaluate Ret.end() each time.
for (auto I = Ret.begin(); I != Ret.end();) {
const auto *V = I->first;

if (!B.count(V)) {
auto Next = std::next(I);
Ret.erase(I);
I = Next;
I = Ret.erase(I);
} else {
Ret[V] = std::min(Ret[V], B[V]);
++I;
Expand Down
43 changes: 42 additions & 1 deletion clang/lib/Sema/SemaBounds.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2726,6 +2726,36 @@ namespace {
}
}

// When a variable goes out of scope:
// 1) it has to be removed from ObservedBounds in the CheckingState
// if it is a checked pointer variable because we no longer want
// to validate its bounds, and
// 2) the expressions in EquivExprs that use it have to be removed
// because the expressions are now undefined.
void UpdateStateForVariableOutOfScope(CheckingState &State, VarDecl *V) {
if (V->hasBoundsExpr()) {
const AbstractSet *A = AbstractSetMgr.GetOrCreateAbstractSet(V);
auto I = State.ObservedBounds.find(A);
if (I != State.ObservedBounds.end())
State.ObservedBounds.erase(A);
}

EquivExprSets CrntEquivExprs(State.EquivExprs);
State.EquivExprs.clear();
for (auto I = CrntEquivExprs.begin(), E = CrntEquivExprs.end();
I != E; ++I) {
ExprSetTy ExprList;
for (auto InnerList = (*I).begin(); InnerList != (*I).end();
++InnerList) {
Expr *E = *InnerList;
if (!VariableOccurrenceCount(S, V, E))
ExprList.push_back(E);
}
if (ExprList.size() > 1)
State.EquivExprs.push_back(ExprList);
}
}

// Walk the CFG, traversing basic blocks in reverse post-oder.
// For each element of a block, check bounds declarations. Skip
// CFG elements that are subexpressions of other CFG elements.
Expand Down Expand Up @@ -2845,6 +2875,15 @@ namespace {
// variable should be in effect until the very end of traversing S.
ResetKilledBounds(BA, Block, S, BlockState);
}
else if (Elem.getKind() == CFGElement::LifetimeEnds) {
// Every variable going out of scope is indicated by a LifetimeEnds
// CFGElement. When a variable goes out of scope, ObservedBounds and
// EquivExprs in the CheckingState have to be updated.
CFGLifetimeEnds LE = Elem.castAs<CFGLifetimeEnds>();
VarDecl *V = const_cast<VarDecl *>(LE.getVarDecl());
if (V)
UpdateStateForVariableOutOfScope(BlockState, V);
}
}
if (Block->getBlockID() != Cfg->getEntry().getBlockID())
BlockStates[Block->getBlockID()] = BlockState;
Expand Down Expand Up @@ -6464,7 +6503,9 @@ void Sema::CheckFunctionBodyBoundsDecls(FunctionDecl *FD, Stmt *Body) {
// (if any) that is the first use of the VarDecl.
ComputeBoundsDependencies(Tracker, VarUses, FD, Body);
std::pair<ComparisonSet, ComparisonSet> EmptyFacts;
std::unique_ptr<CFG> Cfg = CFG::buildCFG(nullptr, Body, &getASTContext(), CFG::BuildOptions());
CFG::BuildOptions BO;
BO.AddLifetime = true;
std::unique_ptr<CFG> Cfg = CFG::buildCFG(nullptr, Body, &getASTContext(), BO);
CheckBoundsDeclarations Checker(*this, VarUses, Body, Cfg.get(), FD->getBoundsExpr(), EmptyFacts);
if (Cfg != nullptr) {
AvailableFactsAnalysis Collector(*this, Cfg.get());
Expand Down
Loading

0 comments on commit 4b43422

Please sign in to comment.