[analyzer] Fix macro names in diagnostics within bigger macros.

If macro "CHECK_X(x)" expands to something like "if (x != NULL) ...",
the "Assuming..." note no longer says "Assuming 'x' is equal to CHECK_X".

Differential Revision: https://reviews.llvm.org/D59121

llvm-svn: 359037

2 files changed, 41 insertions, 37 deletions

diff --git a/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp b/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
index fc8826567c4..b14f5dd5830 100644
--- a/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ b/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -2002,43 +2002,22 @@ bool ConditionBRVisitor::patternMatch(const Expr *Ex,
   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->getBeginLoc();
-  SourceLocation LocEnd = Ex->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());
-    bool beginAndEndAreTheSameMacro = StartName.equals(EndName);
-
-    bool partOfParentMacro = false;
-    if (ParentEx->getBeginLoc().isMacroID()) {
-      StringRef PName = Lexer::getImmediateMacroNameForDiagnostics(
-          ParentEx->getBeginLoc(), 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);
+  if (isa<GNUNullExpr>(Ex) || isa<ObjCBoolLiteralExpr>(Ex) ||
+      isa<CXXBoolLiteralExpr>(Ex) || isa<IntegerLiteral>(Ex) ||
+      isa<FloatingLiteral>(Ex)) {
+    // Use heuristics to determine if the expression is a macro
+    // expanding to a literal and if so, use the macro's name.
+    SourceLocation BeginLoc = OriginalExpr->getBeginLoc();
+    SourceLocation EndLoc = OriginalExpr->getEndLoc();
+    if (BeginLoc.isMacroID() && EndLoc.isMacroID()) {
+      SourceManager &SM = BRC.getSourceManager();
+      const LangOptions &LO = BRC.getASTContext().getLangOpts();
+      if (Lexer::isAtStartOfMacroExpansion(BeginLoc, SM, LO) &&
+          Lexer::isAtEndOfMacroExpansion(EndLoc, SM, LO)) {
+        CharSourceRange R = Lexer::getAsCharRange({BeginLoc, EndLoc}, SM, LO);
+        Out << Lexer::getSourceText(R, SM, LO);
+        return false;
       }
-      StringRef MacroName = Lexer::getImmediateMacroNameForDiagnostics(
-        Loc, BRC.getSourceManager(), BRC.getASTContext().getLangOpts());
-
-      // Return the macro name.
-      Out << MacroName;
-      return false;
     }
   }
 
diff --git a/clang/test/Analysis/diagnostics/macros.cpp b/clang/test/Analysis/diagnostics/macros.cpp
index 67cdef7f389..b3887b39a44 100644
--- a/clang/test/Analysis/diagnostics/macros.cpp
+++ b/clang/test/Analysis/diagnostics/macros.cpp
@@ -3,7 +3,7 @@
 #include "../Inputs/system-header-simulator.h"
 #include "../Inputs/system-header-simulator-cxx.h"
 
-void testIntMacro(unsigned int i) {
+void testUnsignedIntMacro(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}}
@@ -12,6 +12,20 @@ void testIntMacro(unsigned int i) {
   }
 }
 
+
+// FIXME: 'i' can never be equal to UINT32_MAX - it doesn't even fit into its
+// type ('int'). This should say "Assuming 'i' is equal to -1".
+void testIntMacro(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}}
@@ -47,3 +61,14 @@ void testboolMacro(bool b, int *p) {
                     // expected-note@-1 {{Dereference of null pointer (loaded from variable 'p')}}
   }
 }
+
+#define nested_null_split(x) if ((x) != UINT32_MAX) {}
+
+void testNestedNullSplitMacro(int i, int *p) {
+  nested_null_split(i); // expected-note {{Assuming 'i' is equal to -1}}
+                        // expected-note@-1 {{Taking false branch}}
+  if (!p) // expected-note {{Assuming 'p' is null}}
+          // expected-note@-1 {{Taking true branch}}
+    *p = 1; // expected-warning {{Dereference of null pointer (loaded from variable 'p')}}
+            // expected-note@-1 {{Dereference of null pointer (loaded from variable 'p')}}
+}