[analyzer] ConditionBRVisitor: Treat enums as unknown

[Charusso]

Summary

• We do not model enums so removed from ConditionBRVisitor::patternMatch().
• Refactored that function.

[Charusso]

This error found in D59155.

clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

2095

That was the point when we catch a DRE and we said that it is a variable.

clang/test/Analysis/keychainAPI-diagnostic-visitor.m

30

DeclRefExpr 0x67b308 'OSStatus':'unsigned int' lvalue Var 0x657aa8 'st' 'OSStatus':'unsigned int'
and
DeclRefExpr 0x67b328 'int' EnumConstant 0x657228 'noErr' 'int'

[Charusso]

@NoQ, this is my last patch, just we bumped into that randomly, it would be cool to fix that.

clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h

public:
   /// \param N The node where we encountered the condition.
   /// \param TookTrue Whether we took the \c true branch of the condition.
   ///
   /// \return Whether the print was successful. (The printing is successful if
   ///         we model the value and we could obtain it.)
   bool printValue(const Expr *CondVarExpr, raw_ostream &Out,
                   const ExplodedNode *N, bool TookTrue, bool IsAssuming);
 
-  bool patternMatch(const Expr *Ex,
-                    const Expr *ParentEx,
-                    raw_ostream &Out,
-                    BugReporterContext &BRC,
-                    BugReport &R,
-                    const ExplodedNode *N,
-                    Optional<bool> &prunable,
+  /// Prints out the name or the value of the given expression.
+  /// We print out the name if we work with a variable or a macro,
+  /// otherwise it is a literal so we pretty-print its value.
+  ///
+  /// \param E The expression to print its name or value.
+  /// \param BOExpr The immediate ancestor of \c E.
+  /// \param Out The stream to print.
+  /// \param BRC The context.
+  /// \param R The report.
+  /// \param N The node where we encountered the condition.
+  /// \param Prunable True if the given expression is not interesting.
+  /// \param IsSameFieldName True if both the LHS and RHS are \c MemberExprs and
+  ///                        the name of the fields are equal.
+  ///
+  /// \return Whether the \c E is refers to a variable.
+  bool printNameOrValue(const Expr *E, const BinaryOperator *BOExpr,
+                        raw_ostream &Out, BugReporterContext &BRC, BugReport &R,
+                        const ExplodedNode *N, Optional<bool> &Prunable,
                         bool IsSameFieldName);
 
   static bool isPieceMessageGeneric(const PathDiagnosticPiece *Piece);
 };
 
 /// Suppress reports that might lead to known false positives.
 ///
 /// Currently this suppresses reports based on locations of bugs.
 class LikelyFalsePositiveSuppressionBRVisitor final

clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

ConditionBRVisitor::VisitTrueTest(const Expr *Cond, BugReporterContext &BRC,
   PathDiagnosticLocation Loc(Cond, BRC.getSourceManager(), LCtx);
   if (!Loc.isValid() || !Loc.asLocation().isValid())
     return nullptr;
 
   return std::make_shared<PathDiagnosticEventPiece>(
       Loc, TookTrue ? GenericTrueMessage : GenericFalseMessage);
 }
 
-bool ConditionBRVisitor::patternMatch(const Expr *Ex,
-                                      const Expr *ParentEx,
-                                      raw_ostream &Out,
-                                      BugReporterContext &BRC,
-                                      BugReport &report,
-                                      const ExplodedNode *N,
-                                      Optional<bool> &prunable,
-                                      bool IsSameFieldName) {
-  const Expr *OriginalExpr = Ex;
-  Ex = Ex->IgnoreParenCasts();
+bool ConditionBRVisitor::printNameOrValue(
+    const Expr *E, const BinaryOperator *BOExpr, raw_ostream &Out,
+    BugReporterContext &BRC, BugReport &R, const ExplodedNode *N,
+    Optional<bool> &Prunable, bool IsSameFieldName) {
+  SourceManager &SM = BRC.getSourceManager();
+  const LangOptions &LO = BRC.getASTContext().getLangOpts();
+
+  QualType Ty = E->getType();
+  E = E->IgnoreParenCasts();
 
-  // Use heuristics to determine if Ex is a macro expending to a literal and
+  // Use heuristics to determine if 'E' is a macro expending to a literal and
   // if so, use the macro's name.
-  SourceLocation LocStart = Ex->getBeginLoc();
-  SourceLocation LocEnd = Ex->getEndLoc();
+  SourceLocation LocStart = E->getBeginLoc();
+  SourceLocation LocEnd = E->getEndLoc();
   if (LocStart.isMacroID() && LocEnd.isMacroID() &&
-      (isa<GNUNullExpr>(Ex) ||
-       isa<ObjCBoolLiteralExpr>(Ex) ||
-       isa<CXXBoolLiteralExpr>(Ex) ||
-       isa<IntegerLiteral>(Ex) ||
-       isa<FloatingLiteral>(Ex))) {
-    StringRef StartName = Lexer::getImmediateMacroNameForDiagnostics(LocStart,
-      BRC.getSourceManager(), BRC.getASTContext().getLangOpts());
-    StringRef EndName = Lexer::getImmediateMacroNameForDiagnostics(LocEnd,
-      BRC.getSourceManager(), BRC.getASTContext().getLangOpts());
+      (isa<GNUNullExpr>(E) || isa<ObjCBoolLiteralExpr>(E) ||
+       isa<CXXBoolLiteralExpr>(E) || isa<IntegerLiteral>(E) ||
+       isa<FloatingLiteral>(E))) {
+    StringRef StartName =
+        Lexer::getImmediateMacroNameForDiagnostics(LocStart, SM, LO);
+    StringRef EndName =
+        Lexer::getImmediateMacroNameForDiagnostics(LocEnd, SM, LO);
     bool beginAndEndAreTheSameMacro = StartName.equals(EndName);
 
     bool partOfParentMacro = false;
-    if (ParentEx->getBeginLoc().isMacroID()) {
+    if (BOExpr->getBeginLoc().isMacroID()) {
       StringRef PName = Lexer::getImmediateMacroNameForDiagnostics(
-          ParentEx->getBeginLoc(), BRC.getSourceManager(),
-          BRC.getASTContext().getLangOpts());
+          BOExpr->getBeginLoc(), SM, LO);
       partOfParentMacro = PName.equals(StartName);
     }
 
     if (beginAndEndAreTheSameMacro && !partOfParentMacro ) {
       // Get the location of the macro name as written by the caller.
       SourceLocation Loc = LocStart;
       while (LocStart.isMacroID()) {
         Loc = LocStart;
-        LocStart = BRC.getSourceManager().getImmediateMacroCallerLoc(LocStart);
+        LocStart = SM.getImmediateMacroCallerLoc(LocStart);
       }
-      StringRef MacroName = Lexer::getImmediateMacroNameForDiagnostics(
-        Loc, BRC.getSourceManager(), BRC.getASTContext().getLangOpts());
+      StringRef MacroName =
+          Lexer::getImmediateMacroNameForDiagnostics(Loc, SM, LO);
 
       // Return the macro name.
       Out << MacroName;
       return false;
     }
   }
 
-  if (const auto *DR = dyn_cast<DeclRefExpr>(Ex)) {
-    const bool quotes = isa<VarDecl>(DR->getDecl());
-    if (quotes) {
-      Out << '\'';
+  if (const auto *DRE = dyn_cast<DeclRefExpr>(E)) {
+    if (const auto *VD = dyn_cast<VarDecl>(DRE->getDecl())) {
       const LocationContext *LCtx = N->getLocationContext();
       const ProgramState *state = N->getState().get();
-      if (const MemRegion *R = state->getLValue(cast<VarDecl>(DR->getDecl()),
-                                                LCtx).getAsRegion()) {
-        if (report.isInteresting(R))
-          prunable = false;
-        else {
-          const ProgramState *state = N->getState().get();
-          SVal V = state->getSVal(R);
-          if (report.isInteresting(V))
-            prunable = false;
-        }
-      }
+      if (const MemRegion *MR = state->getLValue(VD, LCtx).getAsRegion()) {
+        if (R.isInteresting(MR)) {
+          Prunable = false;
+        } else {
+          SVal V = state->getSVal(MR);
+          if (R.isInteresting(V))
+            Prunable = false;
         }
-    Out << DR->getDecl()->getDeclName().getAsString();
-    if (quotes)
-      Out << '\'';
-    return quotes;
       }
 
-  if (const auto *IL = dyn_cast<IntegerLiteral>(Ex)) {
-    QualType OriginalTy = OriginalExpr->getType();
-    if (OriginalTy->isPointerType()) {
-      if (IL->getValue() == 0) {
-        Out << "null";
-        return false;
-      }
-    }
-    else if (OriginalTy->isObjCObjectPointerType()) {
-      if (IL->getValue() == 0) {
-        Out << "nil";
-        return false;
-      }
-    }
+      Out << '\'' << VD->getName() << '\'';
+      return true;
 
-    Out << IL->getValue();
-    return false;
-  }
-
-  if (const auto *ME = dyn_cast<MemberExpr>(Ex)) {
+  if (const auto *ME = dyn_cast<MemberExpr>(E)) {
     if (!IsSameFieldName)
       Out << "field '" << ME->getMemberDecl()->getName() << '\'';
     else
       Out << '\''
           << Lexer::getSourceText(
-                 CharSourceRange::getTokenRange(Ex->getSourceRange()),
-                 BRC.getSourceManager(), BRC.getASTContext().getLangOpts(), 0)
+                 CharSourceRange::getTokenRange(E->getSourceRange()), SM, LO, 0)
           << '\'';
+    return true;
+  }
+
+  // Value pretty-printing.
+  if (const auto *IL = dyn_cast<IntegerLiteral>(E)) {
+    if (Ty->isPointerType())
+      Out << "null";
+    else if (Ty->isObjCObjectPointerType())
+      Out << "nil";
+    else
+      Out << IL->getValue();
+
+    return false;
   }
 
   return false;
 }
 
 std::shared_ptr<PathDiagnosticPiece> ConditionBRVisitor::VisitTrueTest(
-    const Expr *Cond, const BinaryOperator *BExpr, BugReporterContext &BRC,
+    const Expr *Cond, const BinaryOperator *BOExpr, BugReporterContext &BRC,
     BugReport &R, const ExplodedNode *N, bool TookTrue, bool IsAssuming) {
   bool shouldInvert = false;
   Optional<bool> shouldPrune;
 
   // Check if the field name of the MemberExprs is ambiguous. Example:
   // " 'a.d' is equal to 'h.d' " in 'test/Analysis/null-deref-path-notes.cpp'.
+  const Expr *LhsExpr = BOExpr->getLHS();
+  const Expr *RhsExpr = BOExpr->getRHS();
   bool IsSameFieldName = false;
-  if (const auto *LhsME =
-          dyn_cast<MemberExpr>(BExpr->getLHS()->IgnoreParenCasts()))
-    if (const auto *RhsME =
-            dyn_cast<MemberExpr>(BExpr->getRHS()->IgnoreParenCasts()))
+  if (const auto *LhsME = dyn_cast<MemberExpr>(LhsExpr->IgnoreImpCasts()))
+    if (const auto *RhsME = dyn_cast<MemberExpr>(RhsExpr->IgnoreImpCasts()))
       IsSameFieldName = LhsME->getMemberDecl()->getName() ==
                         RhsME->getMemberDecl()->getName();
 
   SmallString<128> LhsString, RhsString;
   {
     llvm::raw_svector_ostream OutLHS(LhsString), OutRHS(RhsString);
-    const bool isVarLHS = patternMatch(BExpr->getLHS(), BExpr, OutLHS, BRC, R,
-                                       N, shouldPrune, IsSameFieldName);
-    const bool isVarRHS = patternMatch(BExpr->getRHS(), BExpr, OutRHS, BRC, R,
-                                       N, shouldPrune, IsSameFieldName);
+    const bool isVarLHS = printNameOrValue(LhsExpr, BOExpr, OutLHS, BRC, R, N,
+                                           shouldPrune, IsSameFieldName);
+    const bool isVarRHS = printNameOrValue(RhsExpr, BOExpr, OutRHS, BRC, R, N,
+                                           shouldPrune, IsSameFieldName);
 
     shouldInvert = !isVarLHS && isVarRHS;
   }
 
-  BinaryOperator::Opcode Op = BExpr->getOpcode();
+  BinaryOperator::Opcode Op = BOExpr->getOpcode();
 
   if (BinaryOperator::isAssignmentOp(Op)) {
     // For assignment operators, all that we care about is that the LHS
     // evaluates to "true" or "false".
-    return VisitConditionVariable(LhsString, BExpr->getLHS(), BRC, R, N,
-                                  TookTrue);
+    return VisitConditionVariable(LhsString, LhsExpr, BRC, R, N, TookTrue);
   }
 
   // For non-assignment operations, we require that we can understand
   // both the LHS and RHS.
   if (LhsString.empty() || RhsString.empty() ||
       !BinaryOperator::isComparisonOp(Op) || Op == BO_Cmp)
     return nullptr;
 

clang/test/Analysis/keychainAPI-diagnostic-visitor.m

 void DellocWithCFStringCreate4() {
     unsigned int *ptr = 0;
     OSStatus st = 0;
     UInt32 length;
     char *bytes;
     char *x;
     st = SecKeychainItemCopyContent(2, ptr, ptr, &length, (void **)&bytes); // expected-note {{Data is allocated here}}
     x = bytes;
-    if (st == noErr) // expected-note {{Assuming 'st' is equal to noErr}} // expected-note{{Taking true branch}}
+    if (st == noErr) // expected-note {{Assuming the condition is true}} // expected-note{{Taking true branch}}
         x = bytes;;
   
     length++; // expected-warning {{Allocated data is not released}} // expected-note{{Allocated data is not released}}
 }