r174447 - Change subexpressions to be visited in the CFG from left-to-right.

[Jordan Rose]

On Feb 5, 2013, at 14:00 , Ted Kremenek <kremenek at apple.com> wrote:

> Author: kremenek
> Date: Tue Feb  5 16:00:19 2013
> New Revision: 174447
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=174447&view=rev
> Log:
> Change subexpressions to be visited in the CFG from left-to-right.
> 
> This is a more natural order of evaluation, and it is very important
> for visualization in the static analyzer.  Within Xcode, the arrows
> will not jump from right to left, which looks very visually jarring.
> It also provides a more natural location for dataflow-based diagnostics.
> 
> Along the way, we found a case in the analyzer diagnostics where we
> needed to indicate that a variable was "captured" by a block.
> 
> -fsyntax-only timings on sqlite3.c show no visible performance change,
> although this is just one test case.
> 
> Fixes <rdar://problem/13016513>
> 
> Modified:
>    cfe/trunk/include/clang/AST/Expr.h
>    cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h
>    cfe/trunk/lib/Analysis/CFG.cpp
>    cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
>    cfe/trunk/lib/StaticAnalyzer/Core/MemRegion.cpp
>    cfe/trunk/test/Analysis/temp-obj-dtors-cfg-output.cpp
>    cfe/trunk/test/Analysis/unix-fns.c
>    cfe/trunk/test/Sema/warn-unreachable.c
> 
> Modified: cfe/trunk/include/clang/AST/Expr.h
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/Expr.h?rev=174447&r1=174446&r2=174447&view=diff
> ==============================================================================
> --- cfe/trunk/include/clang/AST/Expr.h (original)
> +++ cfe/trunk/include/clang/AST/Expr.h Tue Feb  5 16:00:19 2013
> @@ -2207,6 +2207,15 @@ public:
>     return SubExprs+PREARGS_START+getNumPreArgs()+getNumArgs();
>   }
> 
> +  /// This method provides fast access to all the subexpressions of
> +  /// a CallExpr without going through the slower virtual child_iterator
> +  /// interface.  This provides efficient reverse iteration of the
> +  /// subexpressions.  This is currently used for CFG construction.
> +  ArrayRef<Stmt*> getRawSubExprs() {
> +    return ArrayRef<Stmt*>(SubExprs,
> +                           getNumPreArgs() + PREARGS_START + getNumArgs());
> +  }
> +
>   /// getNumCommas - Return the number of commas that must have been present in
>   /// this function call.
>   unsigned getNumCommas() const { return NumArgs ? NumArgs - 1 : 0; }
> 
> Modified: cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h?rev=174447&r1=174446&r2=174447&view=diff
> ==============================================================================
> --- cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h (original)
> +++ cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h Tue Feb  5 16:00:19 2013
> @@ -662,6 +662,10 @@ public:
>       return *this;
>     }
>   };
> +
> +  /// Return the original region for a captured region, if
> +  /// one exists.
> +  const VarRegion *getOriginalRegion(const VarRegion *VR) const;
> 
>   referenced_vars_iterator referenced_vars_begin() const;
>   referenced_vars_iterator referenced_vars_end() const;  
> 
> Modified: cfe/trunk/lib/Analysis/CFG.cpp
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Analysis/CFG.cpp?rev=174447&r1=174446&r2=174447&view=diff
> ==============================================================================
> --- cfe/trunk/lib/Analysis/CFG.cpp (original)
> +++ cfe/trunk/lib/Analysis/CFG.cpp Tue Feb  5 16:00:19 2013
> @@ -233,6 +233,43 @@ public:
>   }
> };
> 
> +class reverse_children {
> +  llvm::SmallVector<Stmt *, 12> childrenBuf;
> +  ArrayRef<Stmt*> children;
> +public:
> +  reverse_children(Stmt *S);
> +
> +  typedef ArrayRef<Stmt*>::reverse_iterator iterator;
> +  iterator begin() const { return children.rbegin(); }
> +  iterator end() const { return children.rend(); }
> +};
> +
> +
> +reverse_children::reverse_children(Stmt *S) {
> +  if (CallExpr *CE = dyn_cast<CallExpr>(S)) {
> +    children = CE->getRawSubExprs();
> +    return;
> +  }
> +  switch (S->getStmtClass()) {
> +    case Stmt::InitListExprClass: {
> +      InitListExpr *IE = cast<InitListExpr>(S);
> +      children = llvm::makeArrayRef(reinterpret_cast<Stmt**>(IE->getInits()),
> +                                    IE->getNumInits());
> +      return;
> +    }
> +    default:
> +      break;
> +  }
> +
> +  // Default case for all other statements.
> +  for (Stmt::child_range I = S->children(); I; ++I) {
> +    childrenBuf.push_back(*I);
> +  }
> +
> +  // This needs to be done *after* childrenBuf has been populated.
> +  children = childrenBuf;
> +}
> +
> /// CFGBuilder - This class implements CFG construction from an AST.
> ///   The builder is stateful: an instance of the builder should be used to only
> ///   construct a single CFG.
> @@ -1166,14 +1203,19 @@ CFGBlock *CFGBuilder::VisitStmt(Stmt *S,
> }
> 
> /// VisitChildren - Visit the children of a Stmt.
> -CFGBlock *CFGBuilder::VisitChildren(Stmt *Terminator) {
> -  CFGBlock *lastBlock = Block;
> -  for (Stmt::child_range I = Terminator->children(); I; ++I)
> -    if (Stmt *child = *I)
> -      if (CFGBlock *b = Visit(child))
> -        lastBlock = b;
> +CFGBlock *CFGBuilder::VisitChildren(Stmt *S) {
> +  CFGBlock *B = Block;
> 
> -  return lastBlock;
> +  // Visit the children in their reverse order so that they appear in
> +  // left-to-right (natural) order in the CFG.
> +  reverse_children RChildren(S);
> +  for (reverse_children::iterator I = RChildren.begin(), E = RChildren.end();
> +       I != E; ++I) {
> +    if (Stmt *Child = *I)
> +      if (CFGBlock *R = Visit(Child))
> +        B = R;
> +  }
> +  return B;
> }
> 
> CFGBlock *CFGBuilder::VisitAddrLabelExpr(AddrLabelExpr *A,
> @@ -3093,19 +3135,14 @@ tryAgain:
> 
> CFGBlock *CFGBuilder::VisitChildrenForTemporaryDtors(Stmt *E) {
>   // When visiting children for destructors we want to visit them in reverse
> -  // order. Because there's no reverse iterator for children must to reverse
> -  // them in helper vector.
> -  typedef SmallVector<Stmt *, 4> ChildrenVect;
> -  ChildrenVect ChildrenRev;
> -  for (Stmt::child_range I = E->children(); I; ++I) {
> -    if (*I) ChildrenRev.push_back(*I);
> -  }
> -
> +  // order that they will appear in the CFG.  Because the CFG is built
> +  // bottom-up, this means we visit them in their natural order, which
> +  // reverses them in the CFG.
>   CFGBlock *B = Block;
> -  for (ChildrenVect::reverse_iterator I = ChildrenRev.rbegin(),
> -      L = ChildrenRev.rend(); I != L; ++I) {
> -    if (CFGBlock *R = VisitForTemporaryDtors(*I))
> -      B = R;
> +  for (Stmt::child_range I = E->children(); I; ++I) {
> +    if (Stmt *Child = *I)
> +      if (CFGBlock *R = VisitForTemporaryDtors(Child))
> +        B = R;
>   }
>   return B;
> }

This is wrong. Destructors must be visited in reverse order of initializers, as required by the standard.

Jordan

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130205/4de4332f/attachment.html>