[analyzer] Refer to macro names in diagnostics for macros representing a literal

[zaks.anna]

Summary

When a macro expending to a literal is used in a comparison, use the macro name in the diagnostic rather than the literal. This improves readability of path notes.

Added tests for various macro literals that could occur. Only BOOl, Int, and NULL tests have changed behavior with this patch.

[dcoughlin]

The diagnostics look great to me and the macro logic seems reasonable.

test/Analysis/diagnostics/macros.cpp

49

Missing newline?

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

public:
   PathDiagnosticPiece *VisitConditionVariable(StringRef LhsString,
                                               const Expr *CondVarExpr,
                                               const bool tookTrue,
                                               BugReporterContext &BRC,
                                               BugReport &R,
                                               const ExplodedNode *N);
 
   bool patternMatch(const Expr *Ex,
+                    const Expr *ParentEx,
                     raw_ostream &Out,
                     BugReporterContext &BRC,
                     BugReport &R,
                     const ExplodedNode *N,
                     Optional<bool> &prunable);
 
   static bool isPieceMessageGeneric(const PathDiagnosticPiece *Piece);
 };

lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

 //  This file defines a set of BugReporter "visitors" which can be used to
 //  enhance the diagnostics reported for a bug.
 //
 //===----------------------------------------------------------------------===//
 #include "clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitor.h"
 #include "clang/AST/Expr.h"
 #include "clang/AST/ExprObjC.h"
 #include "clang/Analysis/CFGStmtMap.h"
+#include "clang/Lex/Lexer.h"
 #include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h"
 #include "clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/ExplodedGraph.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/StringExtras.h"
ConditionBRVisitor::VisitTrueTest(const Expr *Cond,
   if (!Loc.isValid() || !Loc.asLocation().isValid())
     return nullptr;
 
   PathDiagnosticEventPiece *Event = new PathDiagnosticEventPiece(
       Loc, tookTrue ? GenericTrueMessage : GenericFalseMessage);
   return Event;
 }
 
-bool ConditionBRVisitor::patternMatch(const Expr *Ex, raw_ostream &Out,
+bool ConditionBRVisitor::patternMatch(const Expr *Ex,
+                                      const Expr *ParentEx,
+                                      raw_ostream &Out,
                                       BugReporterContext &BRC,
                                       BugReport &report,
                                       const ExplodedNode *N,
                                       Optional<bool> &prunable) {
   const Expr *OriginalExpr = Ex;
   Ex = Ex->IgnoreParenCasts();
 
+  // Use heuristics to determine if Ex is a macro expending to a literal and
+  // if so, use the macro's name.
+  SourceLocation LocStart = Ex->getLocStart();
+  SourceLocation LocEnd = Ex->getLocEnd();
+  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());
+    bool beginAndEndAreTheSameMacro = StartName.equals(EndName);
+
+    bool partOfParentMacro = false;
+    if (ParentEx->getLocStart().isMacroID()) {
+      StringRef PName = Lexer::getImmediateMacroNameForDiagnostics(
+        ParentEx->getLocStart(), BRC.getSourceManager(),
+        BRC.getASTContext().getLangOpts());
+      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);
+      }
+      StringRef MacroName = Lexer::getImmediateMacroNameForDiagnostics(
+        Loc, BRC.getSourceManager(), BRC.getASTContext().getLangOpts());
+
+      // Return the macro name.
+      Out << MacroName;
+      return false;
+    }
+  }
+
   if (const DeclRefExpr *DR = dyn_cast<DeclRefExpr>(Ex)) {
     const bool quotes = isa<VarDecl>(DR->getDecl());
     if (quotes) {
       Out << '\'';
       const LocationContext *LCtx = N->getLocationContext();
       const ProgramState *state = N->getState().get();
       if (const MemRegion *R = state->getLValue(cast<VarDecl>(DR->getDecl()),
                                                 LCtx).getAsRegion()) {
ConditionBRVisitor::VisitTrueTest(const Expr *Cond,
                                   const ExplodedNode *N) {
 
   bool shouldInvert = false;
   Optional<bool> shouldPrune;
 
   SmallString<128> LhsString, RhsString;
   {
     llvm::raw_svector_ostream OutLHS(LhsString), OutRHS(RhsString);
-    const bool isVarLHS = patternMatch(BExpr->getLHS(), OutLHS, BRC, R, N,
-                                       shouldPrune);
-    const bool isVarRHS = patternMatch(BExpr->getRHS(), OutRHS, BRC, R, N,
-                                       shouldPrune);
+    const bool isVarLHS = patternMatch(BExpr->getLHS(), BExpr, OutLHS,
+                                       BRC, R, N, shouldPrune);
+    const bool isVarRHS = patternMatch(BExpr->getRHS(), BExpr, OutRHS,
+                                       BRC, R, N, shouldPrune);
 
     shouldInvert = !isVarLHS && isVarRHS;
   }
 
   BinaryOperator::Opcode Op = BExpr->getOpcode();
 
   if (BinaryOperator::isAssignmentOp(Op)) {
     // For assignment operators, all that we care about is that the LHS

test/Analysis/Inputs/system-header-simulator-objc.h

 typedef signed char BOOL;
 #define YES ((BOOL)1)
 #define NO ((BOOL)0)
 
 typedef unsigned long NSUInteger;
 typedef unsigned short unichar;
 typedef UInt16 UniChar;
 
-#define NULL ((void *)0)
+#ifndef NULL
+#define __DARWIN_NULL ((void *)0)
+#define NULL __DARWIN_NULL
+#endif
+
 #define nil ((id)0)
 
 enum {
     NSASCIIStringEncoding = 1,
     NSNEXTSTEPStringEncoding = 2,
     NSJapaneseEUCStringEncoding = 3,
     NSUTF8StringEncoding = 4,
     NSISOLatin1StringEncoding = 5,

 @class NSInvocation, NSMethodSignature, NSCoder, NSString, NSEnumerator;
 @protocol NSObject
 - (BOOL)isEqual:(id)object;
 - (id)retain;
 - (id)copy;
 - (oneway void)release;
 - (id)autorelease;
 - (id)init;
+@property (readonly, copy) NSString *description;
 @end  @protocol NSCopying  - (id)copyWithZone:(NSZone *)zone;
 @end  @protocol NSMutableCopying  - (id)mutableCopyWithZone:(NSZone *)zone;
 @end  @protocol NSCoding  - (void)encodeWithCoder:(NSCoder *)aCoder;
 @end
 @interface NSObject <NSObject> {}
 + (id)allocWithZone:(NSZone *)zone;
 + (id)alloc;
 - (void)dealloc;

test/Analysis/Inputs/system-header-simulator.h

 pid_t fork(void);
 pid_t vfork(void);
 int execl(const char *path, const char *arg, ...);
 
 void exit(int status) __attribute__ ((__noreturn__));
 void _exit(int status) __attribute__ ((__noreturn__));
 void _Exit(int status) __attribute__ ((__noreturn__));
 
+#define UINT32_MAX        4294967295U
+#define INT64_MIN        (-INT64_MAX-1)
+#define __DBL_MAX__ 1.7976931348623157e+308
+#define DBL_MAX __DBL_MAX__
+#ifndef NULL
+#define __DARWIN_NULL 0
+#define NULL __DARWIN_NULL
+#endif
 No newline at end of file

test/Analysis/diagnostics/macros.cpp

+// RUN: %clang_cc1 -analyze -std=c++11 -analyzer-checker=core,osx -analyzer-output=text -verify %s
+
+#include "../Inputs/system-header-simulator.h"
+#include "../Inputs/system-header-simulator-cxx.h"
+
+void testIntMacro(unsigned int i) {
+  if (i == UINT32_MAX) { // expected-note {{Assuming 'i' is equal to UINT32_MAX}}
+                         // expected-note@-1 {{Taking true branch}}
+    char *p = NULL; // expected-note {{'p' initialized to a null pointer value}}
+    *p = 7;  // expected-warning {{Dereference of null pointer (loaded from variable 'p')}}
+             // expected-note@-1 {{Dereference of null pointer (loaded from variable 'p')}}
+  }
+}
+
+void testNULLMacro(int *p) {
+  if (p == NULL) { // expected-note {{Assuming 'p' is equal to NULL}}
+                   // expected-note@-1 {{Taking true branch}}
+    *p = 7;  // expected-warning {{Dereference of null pointer (loaded from variable 'p')}}
+             // expected-note@-1 {{Dereference of null pointer (loaded from variable 'p')}}
+  }
+}
+
+void testnullptrMacro(int *p) {
+  if (p == nullptr) { // expected-note {{Assuming pointer value is null}}
+                      // expected-note@-1 {{Taking true branch}}
+    *p = 7;  // expected-warning {{Dereference of null pointer (loaded from variable 'p')}}
+             // expected-note@-1 {{Dereference of null pointer (loaded from variable 'p')}}
+  }
+}
+
+// There are no path notes on the comparison to float types.
+void testDoubleMacro(double d) {
+  if (d == DBL_MAX) { // expected-note {{Taking true branch}}
+
+    char *p = NULL; // expected-note {{'p' initialized to a null pointer value}}
+    *p = 7;         // expected-warning {{Dereference of null pointer (loaded from variable 'p')}}
+                    // expected-note@-1 {{Dereference of null pointer (loaded from variable 'p')}}
+  }
+}
+
+void testboolMacro(bool b, int *p) {
+  p = nullptr;      // expected-note {{Null pointer value stored to 'p'}}
+  if (b == false) { // expected-note {{Assuming the condition is true}}
+                    // expected-note@-1 {{Taking true branch}}
+    *p = 7;         // expected-warning {{Dereference of null pointer (loaded from variable 'p')}}
+                    // expected-note@-1 {{Dereference of null pointer (loaded from variable 'p')}}
+  }
+}
 No newline at end of file

test/Analysis/diagnostics/macros.m

+// RUN: %clang_cc1 -analyze -analyzer-checker=core,osx -fblocks -analyzer-output=text -verify %s
+
+#include "../Inputs/system-header-simulator-objc.h"
+
+@interface NSDictionary : NSObject
+- (NSUInteger)count;
+- (id)objectForKey:(id)aKey;
+- (NSEnumerator *)keyEnumerator;
+@end
+@interface NSMutableDictionary : NSDictionary
+- (void)setObject:(id)anObject forKey:(id <NSCopying>)aKey;
+@end
+
+void testBOOLMacro(BOOL b) {
+  if (b == YES) { // expected-note {{Assuming 'b' is equal to YES}}
+                  // expected-note@-1 {{Taking true branch}}
+    char *p = NULL;// expected-note {{'p' initialized to a null pointer value}}
+    *p = 7;  // expected-warning {{Dereference of null pointer (loaded from variable 'p')}}
+             // expected-note@-1 {{Dereference of null pointer (loaded from variable 'p')}}
+  }
+}
+
+void testNilMacro(NSMutableDictionary *d, NSObject *o) {
+  if (o == nil) // expected-note {{Assuming 'o' is equal to nil}}
+                // expected-note@-1 {{Taking true branch}}
+    [d setObject:o forKey:[o description]]; // expected-warning {{Key argument to 'setObject:forKey:' cannot be nil}}
+                                            // expected-note@-1 {{'description' not called because the receiver is nil}}
+                                            // expected-note@-2 {{Key argument to 'setObject:forKey:' cannot be nil}}
+
+  return;
+}