[cfe-commits] [PATCH] CFG automatic object destructors
Ted Kremenek
kremenek at apple.com
Mon Sep 27 18:31:03 PDT 2010
On Sep 27, 2010, at 3:55 PM, Marcin Ĺwiderski wrote:
> Patch adds implicit destructors generation for objects with automatic storage duration. Patch is rather big, but it contains all cases (that I thought of), as it will be easier to review as a whole IMO. While commiting I can divide it to smaller chuncks.
Overall this looks great. There are a few misspelled words in the comments resulting from typos (e.g., 'Elememnts'), so it's probably worth running a spell-checker.
Besides specific comments (below), I'm concerned about the impact on CFG-construction time when scope building is *not* enabled. This is currently the common case for uses of the CFG in Sema. I think there shouldn't be any noticeable penalty since most of the logic is guarded with a flag, but it is worth verifying.
>
> I've added some test cases. They can be later used to create regresion tests.
For the test cases, how do you plan on testing it? Also, since these are all small tests, please include them in one file (if possible).
A few specific comments inline:
Index: include/clang/Analysis/CFG.h
===================================================================
--- include/clang/Analysis/CFG.h (revision 114789)
+++ include/clang/Analysis/CFG.h (working copy)
@@ -221,6 +221,11 @@
typedef ImplTy::const_iterator const_reverse_iterator;
void push_back(CFGElement e, BumpVectorContext &C) { Impl.push_back(e, C); }
+ reverse_iterator insert(reverse_iterator I, size_t Cnt, CFGElement E,
+ BumpVectorContext& C) {
+ return Impl.insert(I, Cnt, E, C);
+ }
+
CFGElement front() const { return Impl.back(); }
CFGElement back() const { return Impl.front(); }
@@ -427,6 +432,18 @@
void appendStmt(Stmt* Statement, BumpVectorContext &C, bool asLValue) {
Elements.push_back(CFGStmt(Statement, asLValue), C);
}
+
+ // Destructors must be inserted in reversed order. So insertion is in two
+ // steps. First we prepare space for some number of elements, then we insert
+ // the elements begining at the last position in prepared space.
+ iterator beginAutomaticObjDtorsInsert(iterator I, size_t Cnt,
+ BumpVectorContext& C) {
+ return iterator(Elements.insert(I.base(), Cnt, CFGElement(), C));
+ }
+ iterator insertAutomaticObjDtor(iterator I, VarDecl* VD, Stmt* S) {
+ *I = CFGAutomaticObjDtor(VD, S);
+ return ++I;
+ }
};
/// CFG - Represents a source-level, intra-procedural CFG that represents the
Index: lib/Analysis/CFG.cpp
===================================================================
--- lib/Analysis/CFG.cpp (revision 114790)
+++ lib/Analysis/CFG.cpp (working copy)
@@ -122,6 +122,11 @@
*this = Scope->Prev;
return *this;
}
+ const_iterator operator++(int) {
+ const_iterator P = *this;
+ ++*this;
+ return P;
+ }
bool operator==(const const_iterator& rhs) const {
return Scope == rhs.Scope && VarIter == rhs.VarIter;
@@ -129,6 +134,13 @@
bool operator!=(const const_iterator& rhs) const {
return !(*this == rhs);
}
+
+ operator bool() const {
+ return *this != const_iterator();
+ }
+
+ int distance(const_iterator L);
+ friend int distance(const_iterator F, const_iterator L);
Does this need to be a friend function?
};
friend class const_iterator;
@@ -148,8 +160,32 @@
/// Begin of scope in direction of CFG building (backwards).
const_iterator begin() const { return const_iterator(*this, Vars.size()); }
+
+ void addVar(VarDecl* VD) {
+ Vars.push_back(VD);
+ }
};
+/// distance - Calculates distance from F to L. L must be reachable from F (with
+/// use of ++ operator). Cost of calculating the distance is linear w.r.t.
+/// number of scopes between F and L.
+int distance(LocalScope::const_iterator F, LocalScope::const_iterator L) {
+ return F.distance(L);
+}
+
+int LocalScope::const_iterator::distance(LocalScope::const_iterator L) {
+ int D = 0;
+ const_iterator F = *this;
+ while (F.Scope != L.Scope) {
+ assert (F != const_iterator()
+ && "L iterator is not reachable from F iterator.");
+ D += F.VarIter;
+ F = F.Scope->Prev;
+ }
+ D += F.VarIter - L.VarIter;
+ return D;
+}
Can this be made into a static function?
+
/// BlockScopePosPair - Structure for specifing position in CFG during its build
/// proces. It consists of CFGBlock that specifies position in CFG graph and
/// LocalScope::const_iterator that specifies position in LocalScope graph.
@@ -207,6 +243,9 @@
typedef llvm::SmallPtrSet<LabelStmt*,5> LabelSetTy;
LabelSetTy AddressTakenLabels;
+ // Allocator to use for LocalScope objects.
+ llvm::BumpPtrAllocator ScopesAlloc;
+
You probably should just reuse the BumpPtrAllocator associated with the CFGBuilder.
bool badCFG;
CFG::BuildOptions BuildOpts;
@@ -273,12 +312,31 @@
CFGBlock *addStmt(Stmt *S) {
return Visit(S, AddStmtChoice::AlwaysAdd);
}
+ CFGBlock *addAutomaticObjDtors(LocalScope::const_iterator B,
+ LocalScope::const_iterator E, Stmt* S);
+ // Local scopes creation.
+ LocalScope* createOrReuseLocalScope(LocalScope* Scope);
+
+ LocalScope* addLocalScopeForStmt(Stmt* S, LocalScope* Scope = NULL);
+ LocalScope* addLocalScopeForDeclStmt(DeclStmt* DS, LocalScope* Scope = NULL);
+ LocalScope* addLocalScopeForVarDecl(VarDecl* VD, LocalScope* Scope = NULL);
+
+ LocalScope* addLocalScopeAndDtors(Stmt* S, LocalScope* Scope = NULL);
+
+ // Interface to CFGBlock - adding CFGElements.
void AppendStmt(CFGBlock *B, Stmt *S,
AddStmtChoice asc = AddStmtChoice::AlwaysAdd) {
B->appendStmt(S, cfg->getBumpVectorContext(), asc.asLValue());
}
+ void insertAutomaticObjDtors(CFGBlock* Blk, CFGBlock::iterator I,
+ LocalScope::const_iterator B, LocalScope::const_iterator E, Stmt* S);
+ void appendAutomaticObjDtors(CFGBlock* Blk, LocalScope::const_iterator B,
+ LocalScope::const_iterator E, Stmt* S);
+ void prependAutomaticObjDtorsWithTerminator(CFGBlock* Blk,
+ LocalScope::const_iterator B, LocalScope::const_iterator E);
+
void AddSuccessor(CFGBlock *B, CFGBlock *S) {
B->addSuccessor(S, cfg->getBumpVectorContext());
}
@@ -383,6 +441,7 @@
if (LI == LabelMap.end()) continue;
JumpTarget JT = LI->second;
+ prependAutomaticObjDtorsWithTerminator(B, I->ScopePos, JT.ScopePos);
AddSuccessor(B, JT.Block);
}
@@ -416,6 +475,155 @@
return B;
}
+/// addAutomaticObjDtors - Add to current block automatic objects destructors
+/// for objects in range of local scope positions. Use S as trigger statement
+/// for destructors.
+CFGBlock* CFGBuilder::addAutomaticObjDtors(LocalScope::const_iterator B,
+ LocalScope::const_iterator E, Stmt* S) {
+ if (!BuildOpts.AddImplicitDtors)
+ return Block;
+ if (B == E)
+ return Block;
+
+ autoCreateBlock();
+ appendAutomaticObjDtors(Block, B, E, S);
+ return Block;
+}
+
+/// createOrReuseLocalScope - If Scope is NULL create new LocalScope. Either
+/// way return valid LocalScope object.
+LocalScope* CFGBuilder::createOrReuseLocalScope(LocalScope* Scope) {
+ if (!Scope) {
+ Scope = ScopesAlloc.Allocate<LocalScope>();
+ new (Scope) LocalScope(ScopePos);
+ }
+ return Scope;
+}
Looks good.
+
+/// addLocalScopeForStmt - Add LocalScope to local scopes tree for statement
+/// that should create implicit scope (e.g. if/else substatements). Will resuse
+/// Scope if not NULL.
+LocalScope* CFGBuilder::addLocalScopeForStmt(Stmt* S, LocalScope* Scope) {
+ if (!BuildOpts.AddImplicitDtors)
+ return Scope;
+
+ // For compound statement we will be creating explicit scope.
+ if (CompoundStmt* CS = dyn_cast<CompoundStmt>(S)) {
+ for (CompoundStmt::body_iterator BI = CS->body_begin(), BE = CS->body_end()
+ ; BI != BE; ++BI) {
+ Stmt* SI = *BI;
+ if (LabelStmt* LS = dyn_cast<LabelStmt>(SI))
+ SI = LS->getSubStmt();
+ if (DeclStmt* DS = dyn_cast<DeclStmt>(SI))
+ Scope = addLocalScopeForDeclStmt(DS, Scope);
+ }
+ return Scope;
+ }
Looks good.
+
+ // For any other statement scope will be implicit and as such will be
+ // interesting only for DeclStmt.
+ if (LabelStmt* LS = dyn_cast<LabelStmt>(S))
+ S = LS->getSubStmt();
+ if (DeclStmt* DS = dyn_cast<DeclStmt>(S))
+ Scope = addLocalScopeForDeclStmt(DS, Scope);
+ return Scope;
+}
Looks good.
+
+/// addLocalScopeForDeclStmt - Add LocalScope for declaration statement. Will
+/// reuse Scope if not NULL.
+LocalScope* CFGBuilder::addLocalScopeForDeclStmt(DeclStmt* DS,
+ LocalScope* Scope) {
+ if (!BuildOpts.AddImplicitDtors)
+ return Scope;
+
+ for (DeclStmt::decl_iterator DI = DS->decl_begin(), DE = DS->decl_end()
+ ; DI != DE; ++DI) {
+ if (VarDecl* VD = dyn_cast<VarDecl>(*DI))
+ Scope = addLocalScopeForVarDecl(VD, Scope);
+ }
+ return Scope;
+}
Looks good.
+
+/// addLocalScopeForVarDecl - Add LocalScope for variable declaration. It will
+/// create add scope for automatic objects and temporary objects bound to
+/// const reference. Will reuse Scope if not NULL.
+LocalScope* CFGBuilder::addLocalScopeForVarDecl(VarDecl* VD,
+ LocalScope* Scope) {
+ if (!BuildOpts.AddImplicitDtors)
+ return Scope;
+
+ // Check if variable is local.
+ switch (VD->getStorageClass()) {
+ case SC_None:
+ case SC_Auto:
+ case SC_Register:
+ break;
+ default: return Scope;
+ }
+
+ // Check for const references bound to temporary. Set type to pointee.
+ QualType QT = VD->getType();
+ if (const ReferenceType* RT = QT.getTypePtr()->getAs<ReferenceType>()) {
+ QT = RT->getPointeeType();
+ if (!QT.isConstQualified())
+ return Scope;
+ if (!VD->getInit() || !VD->getInit()->Classify(*Context).isRValue())
+ return Scope;
+ }
+
+ // Check if type is a C++ class with non-trivial destructor.
+ const RecordType* RT = QT.getTypePtr()->getAs<RecordType>();
+ if (!RT || cast<CXXRecordDecl>(RT->getDecl())->hasTrivialDestructor())
+ return Scope;
+
+ // Add the variable to scope
+ Scope = createOrReuseLocalScope(Scope);
+ Scope->addVar(VD);
+ ScopePos = Scope->begin();
+ return Scope;
+}
+
+/// addLocalScopeAndDtors - For given statement add local scope for it and
+/// add destructors that will cleanup the scope. Will reuse Scope if not NULL.
+LocalScope* CFGBuilder::addLocalScopeAndDtors(Stmt* S, LocalScope* Scope) {
+ if (!BuildOpts.AddImplicitDtors)
+ return Scope;
+
+ LocalScope::const_iterator scopeBeginPos = ScopePos;
+ Scope = addLocalScopeForStmt(S, Scope);
+ addAutomaticObjDtors(ScopePos, scopeBeginPos, S);
+ return Scope;
+}
+
+/// insertAutomaticObjDtors - Insert destructor CFGElements for variables with
+/// automatic storage duration to CFGBlock's elements vector. Insertion will be
+/// performed in place specified with iterator.
+void CFGBuilder::insertAutomaticObjDtors(CFGBlock* Blk, CFGBlock::iterator I,
+ LocalScope::const_iterator B, LocalScope::const_iterator E, Stmt* S) {
+ BumpVectorContext& C = cfg->getBumpVectorContext();
+ I = Blk->beginAutomaticObjDtorsInsert(I, distance(B, E), C);
+ while (B != E)
+ I = Blk->insertAutomaticObjDtor(I, *B++, S);
+}
+
+/// appendAutomaticObjDtors - Append destructor CFGElements for variables with
+/// automatic storage duration to CFGBlock's elements vector. Elements will be
+/// appended to physical end of the vector which happens to be logical begining.
+void CFGBuilder::appendAutomaticObjDtors(CFGBlock* Blk,
+ LocalScope::const_iterator B, LocalScope::const_iterator E, Stmt* S) {
+ insertAutomaticObjDtors(Blk, Blk->begin(), B, E, S);
+}
+
+/// prependAutomaticObjDtorsWithTerminator - Prepend destructor CFGElements for
+/// variables with automatic storage duration to CFGBlock's elements vector.
+/// Elememnts will be prepended to physical begining of the vector which happens
+/// to be logical end. Use blocks terminator as statement that specifies
+/// destructors call site.
+void CFGBuilder::prependAutomaticObjDtorsWithTerminator(CFGBlock* Blk,
+ LocalScope::const_iterator B, LocalScope::const_iterator E) {
+ insertAutomaticObjDtors(Blk, Blk->end(), B, E, Blk->getTerminator());
+}
+
/// Visit - Walk the subtree of a statement and add extra
/// blocks for ternary operators, &&, and ||. We also process "," and
/// DeclStmts (which may contain nested control-flow).
@@ -666,6 +874,7 @@
// If there is no target for the break, then we are looking at an incomplete
// AST. This means that the CFG cannot be constructed.
if (BreakJumpTarget.Block) {
+ addAutomaticObjDtors(ScopePos, BreakJumpTarget.ScopePos, B);
AddSuccessor(Block, BreakJumpTarget.Block);
} else
badCFG = true;
@@ -779,6 +988,7 @@
CFGBlock* CFGBuilder::VisitCompoundStmt(CompoundStmt* C) {
+ addLocalScopeAndDtors(C);
CFGBlock* LastBlock = Block;
for (CompoundStmt::reverse_body_iterator I=C->body_rbegin(), E=C->body_rend();
@@ -918,6 +1128,10 @@
VA = FindVA(VA->getElementType().getTypePtr()))
Block = addStmt(VA->getSizeExpr());
+ // Remove variable from local scope.
+ if (ScopePos && VD == *ScopePos)
+ ++ScopePos;
+
return Block;
}
@@ -929,6 +1143,18 @@
// middle of a block, we stop processing that block. That block is then the
// implicit successor for the "then" and "else" clauses.
+ // Save local scope position because in case of condition variable ScopePos
+ // won't be restored when traversing AST.
+ SaveAndRestore<LocalScope::const_iterator> save_scope_pos(ScopePos);
+
+ // Create local scope for possible condition variable.
+ // Store scope position. Add implicit destructor.
+ if (VarDecl* VD = I->getConditionVariable()) {
+ LocalScope::const_iterator BeginScopePos = ScopePos;
+ addLocalScopeForVarDecl(VD);
+ addAutomaticObjDtors(ScopePos, BeginScopePos, I);
+ }
+
// The block we were proccessing is now finished. Make it the successor
// block.
if (Block) {
@@ -946,6 +1172,12 @@
// NULL out Block so that the recursive call to Visit will
// create a new basic block.
Block = NULL;
+
+ // If branch is not a compound statement create implicit scope
+ // and add destructors.
+ if (!isa<CompoundStmt>(Else))
+ addLocalScopeAndDtors(Else);
+
ElseBlock = addStmt(Else);
if (!ElseBlock) // Can occur when the Else body has all NullStmts.
@@ -963,6 +1195,12 @@
assert(Then);
SaveAndRestore<CFGBlock*> sv(Succ);
Block = NULL;
+
+ // If branch is not a compound statement create implicit scope
+ // and add destructors.
+ if (!isa<CompoundStmt>(Then))
+ addLocalScopeAndDtors(Then);
+
ThenBlock = addStmt(Then);
if (!ThenBlock) {
@@ -1021,6 +1259,7 @@
Block = createBlock(false);
// The Exit block is the only successor.
+ addAutomaticObjDtors(ScopePos, LocalScope::const_iterator(), R);
AddSuccessor(Block, &cfg->getExit());
// Add the return statement to the block. This may create new blocks if R
@@ -1071,6 +1310,7 @@
BackpatchBlocks.push_back(JumpSource(Block, ScopePos));
else {
JumpTarget JT = I->second;
+ addAutomaticObjDtors(ScopePos, JT.ScopePos, G);
AddSuccessor(Block, JT.Block);
}
@@ -1080,8 +1320,23 @@
CFGBlock* CFGBuilder::VisitForStmt(ForStmt* F) {
CFGBlock* LoopSuccessor = NULL;
+ // Save local scope position because in case of condition variable ScopePos
+ // won't be restored when traversing AST.
+ SaveAndRestore<LocalScope::const_iterator> save_scope_pos(ScopePos);
+
+ // Create local scope for init statement and possible condition variable.
+ // Add destructor for init statement and condition variable.
+ // Store scope position for continue statement.
+ if (Stmt* Init = F->getInit())
+ addLocalScopeForStmt(Init);
LocalScope::const_iterator LoopBeginScopePos = ScopePos;
+ if (VarDecl* VD = F->getConditionVariable())
+ addLocalScopeForVarDecl(VD);
+ LocalScope::const_iterator ContinueScopePos = ScopePos;
+
+ addAutomaticObjDtors(ScopePos, save_scope_pos.get(), F);
+
// "for" is a control-flow statement. Thus we stop processing the current
// block.
if (Block) {
@@ -1094,7 +1349,7 @@
// Save the current value for the break targets.
// All breaks should go to the code following the loop.
SaveAndRestore<JumpTarget> save_break(BreakJumpTarget);
- BreakJumpTarget = JumpTarget(LoopSuccessor, LoopBeginScopePos);
+ BreakJumpTarget = JumpTarget(LoopSuccessor, ScopePos);
// Because of short-circuit evaluation, the condition of the loop can span
// multiple basic blocks. Thus we need the "Entry" and "Exit" blocks that
@@ -1150,6 +1405,9 @@
// Create a new block to contain the (bottom) of the loop body.
Block = NULL;
+
+ // Loop body should end with destructor of Condition variable (if any).
+ addAutomaticObjDtors(ScopePos, LoopBeginScopePos, F);
if (Stmt* I = F->getInc()) {
// Generate increment code in its own basic block. This is the target of
@@ -1159,7 +1417,7 @@
// No increment code. Create a special, empty, block that is used as the
// target block for "looping back" to the start of the loop.
assert(Succ == EntryConditionBlock);
- Succ = createBlock();
+ Succ = Block ? Block : createBlock();
Why this line change? The "loop back" block is very important for analyzer diagnostics. Is this because 'addAutomaticObjDtors' might create a block?
}
// Finish up the increment (or empty) block if it hasn't been already.
@@ -1170,12 +1428,17 @@
Block = 0;
}
- ContinueJumpTarget = JumpTarget(Succ, LoopBeginScopePos);
+ ContinueJumpTarget = JumpTarget(Succ, ContinueScopePos);
// The starting block for the loop increment is the block that should
// represent the 'loop target' for looping back to the start of the loop.
ContinueJumpTarget.Block->setLoopTarget(F);
+ // If body is not a compound statement create implicit scope
+ // and add destructors.
+ if (!isa<CompoundStmt>(F->getBody()))
+ addLocalScopeAndDtors(F->getBody());
+
// Now populate the body block, and in the process create new blocks as we
// walk the body of the loop.
CFGBlock* BodyBlock = addStmt(F->getBody());
@@ -1351,7 +1614,17 @@
CFGBlock* CFGBuilder::VisitWhileStmt(WhileStmt* W) {
CFGBlock* LoopSuccessor = NULL;
+ // Save local scope position because in case of condition variable ScopePos
+ // won't be restored when traversing AST.
+ SaveAndRestore<LocalScope::const_iterator> save_scope_pos(ScopePos);
+
+ // Create local scope for possible condition variable.
+ // Store scope position for continue statement.
LocalScope::const_iterator LoopBeginScopePos = ScopePos;
+ if (VarDecl* VD = W->getConditionVariable()) {
+ addLocalScopeForVarDecl(VD);
+ addAutomaticObjDtors(ScopePos, LoopBeginScopePos, W);
+ }
// "while" is a control-flow statement. Thus we stop processing the current
// block.
@@ -1421,11 +1694,19 @@
ContinueJumpTarget = JumpTarget(Succ, LoopBeginScopePos);
// All breaks should go to the code following the loop.
- BreakJumpTarget = JumpTarget(LoopSuccessor, LoopBeginScopePos);
+ BreakJumpTarget = JumpTarget(LoopSuccessor, ScopePos);
// NULL out Block to force lazy instantiation of blocks for the body.
Block = NULL;
+ // Loop body should end with destructor of Condition variable (if any).
+ addAutomaticObjDtors(ScopePos, LoopBeginScopePos, W);
+
+ // If body is not a compound statement create implicit scope
+ // and add destructors.
+ if (!isa<CompoundStmt>(W->getBody()))
+ addLocalScopeAndDtors(W->getBody());
+
// Create the body. The returned block is the entry to the loop body.
CFGBlock* BodyBlock = addStmt(W->getBody());
@@ -1556,6 +1837,11 @@
// NULL out Block to force lazy instantiation of blocks for the body.
Block = NULL;
+ // If body is not a compound statement create implicit scope
+ // and add destructors.
+ if (!isa<CompoundStmt>(D->getBody()))
+ addLocalScopeAndDtors(D->getBody());
+
// Create the body. The returned block is the entry to the loop body.
BodyBlock = addStmt(D->getBody());
@@ -1610,6 +1896,7 @@
// If there is no target for the continue, then we are looking at an
// incomplete AST. This means the CFG cannot be constructed.
if (ContinueJumpTarget.Block) {
+ addAutomaticObjDtors(ScopePos, ContinueJumpTarget.ScopePos, C);
AddSuccessor(Block, ContinueJumpTarget.Block);
} else
badCFG = true;
@@ -1650,6 +1937,18 @@
// block.
CFGBlock* SwitchSuccessor = NULL;
+ // Save local scope position because in case of condition variable ScopePos
+ // won't be restored when traversing AST.
+ SaveAndRestore<LocalScope::const_iterator> save_scope_pos(ScopePos);
+
+ // Create local scope for possible condition variable.
+ // Store scope position. Add implicit destructor.
+ if (VarDecl* VD = Terminator->getConditionVariable()) {
+ LocalScope::const_iterator SwitchBeginScopePos = ScopePos;
+ addLocalScopeForVarDecl(VD);
+ addAutomaticObjDtors(ScopePos, SwitchBeginScopePos, Terminator);
+ }
+
if (Block) {
if (badCFG)
return 0;
@@ -1679,7 +1978,14 @@
// control-flow from the switch goes to case/default statements.
assert(Terminator->getBody() && "switch must contain a non-NULL body");
Block = NULL;
+
+ // If body is not a compound statement create implicit scope
+ // and add destructors.
+ if (!isa<CompoundStmt>(Terminator->getBody()))
+ addLocalScopeAndDtors(Terminator->getBody());
+
addStmt(Terminator->getBody());
+
if (Block) {
if (badCFG)
return 0;
@@ -1856,6 +2162,18 @@
// CXXCatchStmt are treated like labels, so they are the first statement in a
// block.
+ // Save local scope position because in case of exception variable ScopePos
+ // won't be restored when traversing AST.
+ SaveAndRestore<LocalScope::const_iterator> save_scope_pos(ScopePos);
+
+ // Create local scope for possible exception variable.
+ // Store scope position. Add implicit destructor.
+ if (VarDecl* VD = CS->getExceptionDecl()) {
+ LocalScope::const_iterator BeginScopePos = ScopePos;
+ addLocalScopeForVarDecl(VD);
+ addAutomaticObjDtors(ScopePos, BeginScopePos, CS);
+ }
+
if (CS->getHandlerBlock())
addStmt(CS->getHandlerBlock());
More information about the cfe-commits
mailing list