PathDiagnostics:

- PathDiagnosticControlFlowPiece now consists of a "start" and "end" location
  to indicating the branch location and where the branch goes.

BugReporter:
- Updated BugReporter to construct PathDiagnosticControlFlowPiece objects with
  "end" locations.

PlistDiagnostics:
- Plists now contain the bug "type" (not just bug "category")
- Plists now encode control-flow pieces differently than events; now the
  "start" and "end" locations are recorded

llvm-svn: 66818

clang/include/clang/Analysis/PathDiagnostic.h

@@ -186,7 +186,6 @@ private:
   const Kind kind;
   const DisplayHint Hint;
   std::vector<SourceRange> ranges;
-  std::vector<PathDiagnosticPiece*> SubPieces;
   
   // Do not implement:
   PathDiagnosticPiece();
@@ -195,10 +194,12 @@ private:
 
 protected:
   PathDiagnosticPiece(FullSourceLoc pos, const std::string& s,
-                      Kind k = Event, DisplayHint hint = Below);
+                      Kind k, DisplayHint hint = Below);
   
   PathDiagnosticPiece(FullSourceLoc pos, const char* s,
-                      Kind k = Event, DisplayHint hint = Below);
+                      Kind k, DisplayHint hint = Below);
+
+  PathDiagnosticPiece(FullSourceLoc pos, Kind k, DisplayHint hint = Below);
   
 public:
   virtual ~PathDiagnosticPiece();
@@ -269,15 +270,24 @@ public:
 };
   
 class PathDiagnosticControlFlowPiece : public PathDiagnosticPiece {
+  const SourceLocation EndPos;
 public:
-  PathDiagnosticControlFlowPiece(FullSourceLoc pos, const std::string& s)
-    : PathDiagnosticPiece(pos, s, ControlFlow) {}
+  PathDiagnosticControlFlowPiece(FullSourceLoc startPos, SourceLocation endPos,
+                                 const std::string& s)
+    : PathDiagnosticPiece(startPos, s, ControlFlow), EndPos(endPos) {}
   
-  PathDiagnosticControlFlowPiece(FullSourceLoc pos, const char* s)
-    : PathDiagnosticPiece(pos, s, ControlFlow) {}
+  PathDiagnosticControlFlowPiece(FullSourceLoc startPos, SourceLocation endPos,
+                                 const char* s)
+    : PathDiagnosticPiece(startPos, s, ControlFlow), EndPos(endPos) {}
+  
+  PathDiagnosticControlFlowPiece(FullSourceLoc startPos, SourceLocation endPos)
+    : PathDiagnosticPiece(startPos, ControlFlow), EndPos(endPos) {}  
   
   ~PathDiagnosticControlFlowPiece();
   
+  SourceLocation getStartLocation() const { return getLocation(); }
+  SourceLocation getEndLocation() const { return EndPos; }  
+  
   static inline bool classof(const PathDiagnosticPiece* P) {
     return P->getKind() == ControlFlow;
   }

clang/lib/Analysis/BugReporter.cpp

@@ -84,21 +84,40 @@ static inline Stmt* GetCurrentOrNextStmt(const ExplodedNode<GRState>* N) {
 // Diagnostics for 'execution continues on line XXX'.
 //===----------------------------------------------------------------------===//
 
-static inline void ExecutionContinues(llvm::raw_string_ostream& os,
-                                      SourceManager& SMgr,
-                                      const ExplodedNode<GRState>* N,
-                                      const Decl& D) {
+static SourceLocation ExecutionContinues(SourceManager& SMgr,
+                                         const ExplodedNode<GRState>* N,
+                                         const Decl& D,
+                                         bool* OutHasStmt = 0) {
+  if (Stmt *S = GetNextStmt(N)) {
+    if (OutHasStmt) *OutHasStmt = true;
+    return S->getLocStart();
+  }
+  else {
+    if (OutHasStmt) *OutHasStmt = false;
+    return D.getBody()->getRBracLoc();
+  }
+}
+  
+static SourceLocation ExecutionContinues(llvm::raw_string_ostream& os,
+                                         SourceManager& SMgr,
+                                         const ExplodedNode<GRState>* N,
+                                         const Decl& D) {
   
   // Slow, but probably doesn't matter.
   if (os.str().empty())
     os << ' ';
   
-  if (Stmt *S = GetNextStmt(N))
+  bool hasStmt;
+  SourceLocation Loc = ExecutionContinues(SMgr, N, D, &hasStmt);
+  
+  if (hasStmt)
     os << "Execution continues on line "
-    << SMgr.getInstantiationLineNumber(S->getLocStart()) << '.';
+    << SMgr.getInstantiationLineNumber(Loc) << '.';
   else
     os << "Execution jumps to the end of the "
        << (isa<ObjCMethodDecl>(D) ? "method" : "function") << '.';
+  
+  return Loc;
 }
 
 //===----------------------------------------------------------------------===//
@@ -706,36 +725,33 @@ void GRBugReporter::GeneratePathDiagnostic(PathDiagnostic& PD,
     ProgramPoint P = N->getLocation();
     
     if (const BlockEdge* BE = dyn_cast<BlockEdge>(&P)) {
-      
       CFGBlock* Src = BE->getSrc();
       CFGBlock* Dst = BE->getDst();
-      
       Stmt* T = Src->getTerminator();
       
       if (!T)
         continue;
       
-      FullSourceLoc L(T->getLocStart(), SMgr);
+      FullSourceLoc Start(T->getLocStart(), SMgr);
       
       switch (T->getStmtClass()) {
         default:
           break;
           
         case Stmt::GotoStmtClass:
-        case Stmt::IndirectGotoStmtClass: {
-          
+        case Stmt::IndirectGotoStmtClass: {          
           Stmt* S = GetNextStmt(N);
           
           if (!S)
             continue;
           
           std::string sbuf;
-          llvm::raw_string_ostream os(sbuf);
+          llvm::raw_string_ostream os(sbuf);          
+          SourceLocation End = S->getLocStart();
           
-          os << "Control jumps to line "
-             << SMgr.getInstantiationLineNumber(S->getLocStart()) << ".\n";
-          
-          PD.push_front(new PathDiagnosticControlFlowPiece(L, os.str()));
+          os << "Control jumps to line " << SMgr.getInstantiationLineNumber(End);
+          PD.push_front(new PathDiagnosticControlFlowPiece(Start, End,
+                                                           os.str()));
           break;
         }
           
@@ -743,67 +759,70 @@ void GRBugReporter::GeneratePathDiagnostic(PathDiagnostic& PD,
           // Figure out what case arm we took.
           std::string sbuf;
           llvm::raw_string_ostream os(sbuf);
-
-          if (Stmt* S = Dst->getLabel())
+          SourceLocation End;
+          
+          if (Stmt* S = Dst->getLabel()) {
+            End = S->getLocStart();
+          
             switch (S->getStmtClass()) {
-            default:
-              os << "No cases match in the switch statement. "
-                    "Control jumps to line "
-                 << SMgr.getInstantiationLineNumber(S->getLocStart()) << ".\n";
-              break;
-            case Stmt::DefaultStmtClass:
-              os << "Control jumps to the 'default' case at line "
-                 << SMgr.getInstantiationLineNumber(S->getLocStart()) << ".\n";
-              break;
-              
-            case Stmt::CaseStmtClass: {
-              os << "Control jumps to 'case ";
-              
-              CaseStmt* Case = cast<CaseStmt>(S);              
-              Expr* LHS = Case->getLHS()->IgnoreParenCasts();
-              
-              // Determine if it is an enum.
-              
-              bool GetRawInt = true;
-              
-              if (DeclRefExpr* DR = dyn_cast<DeclRefExpr>(LHS)) {
+              default:
+                os << "No cases match in the switch statement. "
+                      "Control jumps to line "
+                   << SMgr.getInstantiationLineNumber(End);
+                break;
+              case Stmt::DefaultStmtClass:
+                os << "Control jumps to the 'default' case at line "
+                   << SMgr.getInstantiationLineNumber(End);
+                break;
+                
+              case Stmt::CaseStmtClass: {
+                os << "Control jumps to 'case ";              
+                CaseStmt* Case = cast<CaseStmt>(S);              
+                Expr* LHS = Case->getLHS()->IgnoreParenCasts();
+                
+                // Determine if it is an enum.              
+                bool GetRawInt = true;
+                
+                if (DeclRefExpr* DR = dyn_cast<DeclRefExpr>(LHS)) {
+                  // FIXME: Maybe this should be an assertion.  Are there cases
+                  // were it is not an EnumConstantDecl?
+                  EnumConstantDecl* D =
+                    dyn_cast<EnumConstantDecl>(DR->getDecl());
 
-                // FIXME: Maybe this should be an assertion.  Are there cases
-                // were it is not an EnumConstantDecl?
-                EnumConstantDecl* D = dyn_cast<EnumConstantDecl>(DR->getDecl());
-                if (D) {
-                  GetRawInt = false;
-                  os << D->getNameAsString();
+                  if (D) {
+                    GetRawInt = false;
+                    os << D->getNameAsString();
+                  }
                 }
+
+                if (GetRawInt) {
+                
+                  // Not an enum.
+                  Expr* CondE = cast<SwitchStmt>(T)->getCond();
+                  unsigned bits = Ctx.getTypeSize(CondE->getType());
+                  llvm::APSInt V(bits, false);
+                  
+                  if (!LHS->isIntegerConstantExpr(V, Ctx, 0, true)) {
+                    assert (false && "Case condition must be constant.");
+                    continue;
+                  }
+                  
+                  os << V;
+                }       
+                
+                os << ":'  at line " << SMgr.getInstantiationLineNumber(End);
+                break;
               }
-
-              if (GetRawInt) {
-              
-                // Not an enum.
-                Expr* CondE = cast<SwitchStmt>(T)->getCond();
-                unsigned bits = Ctx.getTypeSize(CondE->getType());
-                llvm::APSInt V(bits, false);
-                
-                if (!LHS->isIntegerConstantExpr(V, Ctx, 0, true)) {
-                  assert (false && "Case condition must be constant.");
-                  continue;
-                }
-                
-                os << V;
-              }       
-              
-              os << ":'  at line " 
-                 << SMgr.getInstantiationLineNumber(S->getLocStart()) << ".\n";
-              
-              break;
             }
           }
           else {
             os << "'Default' branch taken. ";
-            ExecutionContinues(os, SMgr, N, getStateManager().getCodeDecl());
+            End = ExecutionContinues(os, SMgr, N,
+                                     getStateManager().getCodeDecl());
           }
           
-          PD.push_front(new PathDiagnosticControlFlowPiece(L, os.str()));
+          PD.push_front(new PathDiagnosticControlFlowPiece(Start, End,
+                                                           os.str()));
           break;
         }
           
@@ -811,8 +830,10 @@ void GRBugReporter::GeneratePathDiagnostic(PathDiagnostic& PD,
         case Stmt::ContinueStmtClass: {
           std::string sbuf;
           llvm::raw_string_ostream os(sbuf);
-          ExecutionContinues(os, SMgr, N, getStateManager().getCodeDecl());
-          PD.push_front(new PathDiagnosticControlFlowPiece(L, os.str()));
+          SourceLocation End = ExecutionContinues(os, SMgr, N,
+                                                  getStateManager().getCodeDecl());
+          PD.push_front(new PathDiagnosticControlFlowPiece(Start, End,
+                                                           os.str()));
           break;
         }
 
@@ -822,58 +843,73 @@ void GRBugReporter::GeneratePathDiagnostic(PathDiagnostic& PD,
           os << "'?' condition evaluates to ";
 
           if (*(Src->succ_begin()+1) == Dst)
-            os << "false.";
+            os << "false";
           else
-            os << "true.";
+            os << "true";
           
-          PD.push_front(new PathDiagnosticControlFlowPiece(L, os.str()));
+          SourceLocation End =
+            ExecutionContinues(SMgr, N, getStateManager().getCodeDecl());
+          
+          PD.push_front(new PathDiagnosticControlFlowPiece(Start, End,
+                                                           os.str()));
           break;
         }
           
-        case Stmt::DoStmtClass:  {
-          
+        case Stmt::DoStmtClass:  {          
           if (*(Src->succ_begin()) == Dst) {
             std::string sbuf;
             llvm::raw_string_ostream os(sbuf);
             
             os << "Loop condition is true. ";
-            ExecutionContinues(os, SMgr, N, getStateManager().getCodeDecl());
-            
-            PD.push_front(new PathDiagnosticControlFlowPiece(L, os.str()));
+            SourceLocation End =
+              ExecutionContinues(os, SMgr, N, getStateManager().getCodeDecl());            
+            PD.push_front(new PathDiagnosticControlFlowPiece(Start, End,
+                                                             os.str()));
+          }
+          else {
+            SourceLocation End =
+              ExecutionContinues(SMgr, N, getStateManager().getCodeDecl());            
+            PD.push_front(new PathDiagnosticControlFlowPiece(Start, End,
+                                    "Loop condition is false.  Exiting loop"));
           }
-          else
-            PD.push_front(new PathDiagnosticControlFlowPiece(L,
-                                    "Loop condition is false.  Exiting loop."));
           
           break;
         }
           
         case Stmt::WhileStmtClass:
-        case Stmt::ForStmtClass: {
-          
+        case Stmt::ForStmtClass: {          
           if (*(Src->succ_begin()+1) == Dst) {
             std::string sbuf;
             llvm::raw_string_ostream os(sbuf);
 
             os << "Loop condition is false. ";
-            ExecutionContinues(os, SMgr, N, getStateManager().getCodeDecl());
+            SourceLocation End = 
+              ExecutionContinues(os, SMgr, N, getStateManager().getCodeDecl());
 
-            PD.push_front(new PathDiagnosticControlFlowPiece(L, os.str()));
+            PD.push_front(new PathDiagnosticControlFlowPiece(Start, End,
+                                                             os.str()));
+          }
+          else {
+            SourceLocation End =
+              ExecutionContinues(SMgr, N, getStateManager().getCodeDecl());
+            
+            PD.push_front(new PathDiagnosticControlFlowPiece(Start, End,
+                               "Loop condition is true.  Entering loop body"));
           }
-          else
-            PD.push_front(new PathDiagnosticControlFlowPiece(L,
-                                                             "Loop condition is true.  Entering loop body."));
           
           break;
         }
           
-        case Stmt::IfStmtClass: {          
+        case Stmt::IfStmtClass: {
+          SourceLocation End =
+            ExecutionContinues(SMgr, N, getStateManager().getCodeDecl());
+          
           if (*(Src->succ_begin()+1) == Dst)
-            PD.push_front(new PathDiagnosticControlFlowPiece(L,
-                                                       "Taking false branch."));
+            PD.push_front(new PathDiagnosticControlFlowPiece(Start, End,
+                                                       "Taking false branch"));
           else  
-            PD.push_front(new PathDiagnosticControlFlowPiece(L,
-                                                       "Taking true branch."));
+            PD.push_front(new PathDiagnosticControlFlowPiece(Start, End,
+                                                       "Taking true branch"));
           
           break;
         }

clang/lib/Analysis/PathDiagnostic.cpp

@@ -65,6 +65,13 @@ PathDiagnosticPiece::PathDiagnosticPiece(FullSourceLoc pos,
          "PathDiagnosticPiece's must have a valid location.");
 }
 
+PathDiagnosticPiece::PathDiagnosticPiece(FullSourceLoc pos, Kind k,
+                                         DisplayHint hint)
+  : Pos(pos), kind(k), Hint(hint) {
+  assert(Pos.isValid() &&
+         "PathDiagnosticPiece's must have a valid location.");
+}  
+
 PathDiagnosticPiece::~PathDiagnosticPiece() {}
 PathDiagnosticEventPiece::~PathDiagnosticEventPiece() {}
 PathDiagnosticControlFlowPiece::~PathDiagnosticControlFlowPiece() {}

clang/lib/Frontend/PlistDiagnostics.cpp

@@ -17,10 +17,12 @@
 #include "clang/Basic/FileManager.h"
 #include "llvm/Support/Compiler.h"
 #include "llvm/Support/raw_ostream.h"
+#include "llvm/Support/Casting.h"
 #include "llvm/System/Path.h"
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/SmallVector.h"
 using namespace clang;
+using llvm::cast;
 
 typedef llvm::DenseMap<FileID, unsigned> FIDMap;
 
@@ -67,8 +69,7 @@ static unsigned GetFID(const FIDMap& FIDs, SourceManager* SM, SourceLocation L){
 }
 
 static llvm::raw_ostream& Indent(llvm::raw_ostream& o, const unsigned indent) {
-  for (unsigned i = 0; i < indent; ++i)
-    o << ' ';
+  for (unsigned i = 0; i < indent; ++i) o << ' ';
   return o;
 }
 
@@ -95,22 +96,49 @@ static void EmitRange(llvm::raw_ostream& o, SourceManager* SM, SourceRange R,
   Indent(o, indent) << "</array>\n";
 }
 
-static void ReportDiag(llvm::raw_ostream& o, const PathDiagnosticPiece& P, 
-                       const FIDMap& FM, SourceManager* SM) {
+static void ReportControlFlow(llvm::raw_ostream& o,
+                              const PathDiagnosticControlFlowPiece& P,
+                              const FIDMap& FM, SourceManager *SM,
+                              unsigned indent) {
   
-  unsigned indent = 4;
   Indent(o, indent) << "<dict>\n";
   ++indent;
   
+  Indent(o, indent) << "<key>kind</key><string>control</string>\n";
+  
+  // Output the start and end locations.
+  Indent(o, indent) << "<key>start</key>\n";
+  EmitLocation(o, SM, P.getStartLocation(), FM, indent);
+  Indent(o, indent) << "<key>end</key>\n";
+  EmitLocation(o, SM, P.getEndLocation(), FM, indent);
+  
+  // Output any helper text.
+  const std::string& s = P.getString();
+  if (!s.empty()) {
+    Indent(o, indent) << "<key>alternate</key><string>" << s << "</string>\n";
+  }
+  
+  --indent;
+  Indent(o, indent) << "</dict>\n";  
+}
+
+static void ReportEvent(llvm::raw_ostream& o, const PathDiagnosticPiece& P, 
+                        const FIDMap& FM, SourceManager* SM, unsigned indent) {
+  
+  Indent(o, indent) << "<dict>\n";
+  ++indent;
+
+  Indent(o, indent) << "<key>kind</key><string>event</string>\n";
+  
   // Output the location.
   FullSourceLoc L = P.getLocation();
-
+  
   Indent(o, indent) << "<key>location</key>\n";
   EmitLocation(o, SM, L, FM, indent);
-
+  
   // Output the ranges (if any).
   PathDiagnosticPiece::range_iterator RI = P.ranges_begin(),
-                                      RE = P.ranges_end();
+  RE = P.ranges_end();
   
   if (RI != RE) {
     Indent(o, indent) << "<key>ranges</key>\n";
@@ -122,35 +150,55 @@ static void ReportDiag(llvm::raw_ostream& o, const PathDiagnosticPiece& P,
   }
   
   // Output the text.
+  assert(!P.getString().empty());
   Indent(o, indent) << "<key>message</key>\n";
   Indent(o, indent) << "<string>" << P.getString() << "</string>\n";
-  
-  // Output the hint.
-#if 0
-  // Disable the display hint until we clear up its meaning.
-  Indent(o, indent) << "<key>displayhint</key>\n";
-  Indent(o, indent) << "<string>"
-                    << (P.getDisplayHint() == PathDiagnosticPiece::Above 
-                        ? "above" : "below")
-                    << "</string>\n";
-#endif
-  // Output the PathDiagnosticPiece::Kind.
-  Indent(o, indent) << "<key>kind</key>\n";
-  Indent(o, indent) << "<string>";
-  
-  switch (P.getKind()) {
-    case PathDiagnosticPiece::Event: o << "Event"; break;
-    case PathDiagnosticPiece::ControlFlow: o << "ControlFlow"; break;
-    case PathDiagnosticPiece::Macro: o << "Macro"; break;
-  }
-  o << "</string>\n";
-  
-  
+
   // Finish up.
   --indent;
   Indent(o, indent); o << "</dict>\n";
 }
 
+static void ReportMacro(llvm::raw_ostream& o,
+                        const PathDiagnosticMacroPiece& P,
+                        const FIDMap& FM, SourceManager *SM,
+                        unsigned indent) {
+  
+  for (PathDiagnosticMacroPiece::const_iterator I=P.begin(), E=P.end();
+       I!=E; ++I) {
+    
+    switch ((*I)->getKind()) {
+      default:
+        break;
+      case PathDiagnosticPiece::Event:
+        ReportEvent(o, cast<PathDiagnosticEventPiece>(**I), FM, SM, indent);
+        break;
+      case PathDiagnosticPiece::Macro:
+        ReportMacro(o, cast<PathDiagnosticMacroPiece>(**I), FM, SM, indent);
+        break;
+    }      
+  }    
+}
+
+static void ReportDiag(llvm::raw_ostream& o, const PathDiagnosticPiece& P, 
+                       const FIDMap& FM, SourceManager* SM) {
+
+  unsigned indent = 4;
+  
+  switch (P.getKind()) {
+    case PathDiagnosticPiece::ControlFlow:
+      ReportControlFlow(o, cast<PathDiagnosticControlFlowPiece>(P), FM, SM,
+                        indent);
+      break;
+    case PathDiagnosticPiece::Event:
+      ReportEvent(o, cast<PathDiagnosticEventPiece>(P), FM, SM, indent);
+      break;
+    case PathDiagnosticPiece::Macro:
+      ReportMacro(o, cast<PathDiagnosticMacroPiece>(P), FM, SM, indent);
+      break;
+  }
+}
+
 void PlistDiagnostics::HandlePathDiagnostic(const PathDiagnostic* D) {
   if (!D)
     return;
@@ -238,11 +286,14 @@ PlistDiagnostics::~PlistDiagnostics() {
     o << "   </array>\n";
     
     // Output the bug type and bug category.  
-    o << "   <key>description</key>\n   <string>" << D->getDescription()
+    o << "   <key>description</key><string>" << D->getDescription()
       << "</string>\n"
-      << "   <key>category</key>\n   <string>" << D->getCategory()
+      << "   <key>category</key><string>" << D->getCategory()
       << "</string>\n"
-      << "  </dict>\n";    
+      << "   <key>type</key><string>" << D->getBugType()
+      << "</string>\n"
+      << "  </dict>\n";
+
   }
 
   o << " </array>\n";