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

[NoQ]

Summary

On the newly included test case the previous behavior was

Line 53: note: Assuming 'i' is equal to nested_null_split

which is meh.

I tried to make this piece of logic slightly more correct, but it's most likely still completely incorrect.

[Szelethus]

Uhh, it's such a chore to work on these things -- You can really feel that the preprocessor must've been the first thing implemented in clang. Unfortunately, you really have to counterweight it's shortcomings with excessive amount of comments. I can see an infinite loop and a lot of string comparisons, but someone without having this particular test case as a context, it's very hard to follow what's happening.

Could you please add some code examples in the comments too?

clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

1972–1987

Gets the location of the immediate macro caller, one level up the stack toward the initial macro typed into the source.

Hmm, is there a guarantee that at the end of the stack is not a macro location?

clang/test/Analysis/diagnostics/macros.cpp

54

Ah, that looks pretty :D

[Charusso]

clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

1972

ParentName?

1972

I was never cool in algorithms, but I think it is over-complicated. In Hungary we are about to build a new nuclear power plant so here it is really emphasized: never-ever create an infinite loop.
What about the following?:

while (LocStart.isMacroID()) {                                             
  StringRef CurrentMacroName = Lexer::getImmediateMacroNameForDiagnostics( 
      LocStart, BRC.getSourceManager(),                                    
      BRC.getASTContext().getLangOpts());                                  
  LocStart = BRC.getSourceManager().getImmediateMacroCallerLoc(LocStart);  
  if (CurrentMacroName != ParentName)                                           
    MacroName = CurrentMacroName;                                          
}

1978

There is too much duplication and it is difficult to understand what is going on. May here is the time for deduplicating?
Please note that, there is a function called getMacroName(SourceLocation, BugReporterContext &), may you would put your own helper-function above that.

[NoQ]

An elegant solution for a more civilized age. Unfortunately, doesn't preserve the UINT32_MAX macro in the newly added test - i'll try a bit harder to preserve it. Relies on D59977 to work.

[NoQ]

I think the right thing to do here is "look at the immediate macro; while it expands exactly to our original expression, look at what it is an expansion of; write down the last macro we've reached". My code now gives up whenever we stop expanding to the original expression, but it should write down the highest macro it has reached instead.

[NoQ]

On second thought, dunno. In the scan-build macro preview it wouldn't show you UINT32_MAX anyway. Maybe let's keep this behavior.

Cleaned up the patch a little bit.

[Charusso]

In D59121#1448367, @NoQ wrote:

On second thought, dunno. In the scan-build macro preview it wouldn't show you UINT32_MAX anyway. Maybe let's keep this behavior.

Cleaned up the patch a little bit.

Somehow on the Assuming ... pieces we write out the macro name, and my little code did that too. We could obtain the value but I think your first intention is the correct behaviour. Why are you not using my working example?

[NoQ]

Mmm, that *is* an Assuming... piece, i.e., this is the same code, just the structure of macros is more complicated than usual.

[Charusso]

In D59121#1448386, @NoQ wrote:

Mmm, that *is* an Assuming... piece, i.e., this is the same code, just the structure of macros is more complicated than usual.

You told me we would like to see a value when we hover over a name, which is cool. If I think about C and they do not C# over the complicated macros, I would like to C names of macros. It is up to you, but your first intention working with my code very well.

[NoQ]

Also, this is kinda weird. According to my logic, we should have written Assuming 'i' is equal to 4294967295 because that's what the user will see in the macro popup. However, that's incorrect for the same reason: i is an int, while 4294967295 doesn't fit into an int, so they can never be equal. Writing Assuming 'i' is equal to UINT32_MAX is incorrect for the same reason, and the current message is the only one that's technically the truth.

Writing what exactly we're assuming is, in my opinion, more important than repeating what's literally written in the code.

Added a FIXME test to document this problem.

[Charusso]

Nice catch!

[Szelethus]

LGTM! Thanks!

clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

bool ConditionBRVisitor::patternMatch(const Expr *Ex,
                                       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->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);
-      }
-      StringRef MacroName = Lexer::getImmediateMacroNameForDiagnostics(
-        Loc, BRC.getSourceManager(), BRC.getASTContext().getLangOpts());
-
-      // Return the macro name.
-      Out << MacroName;
+  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 LocStart = OriginalExpr->getBeginLoc();
+    SourceLocation LocEnd = OriginalExpr->getEndLoc();
+    if (LocStart.isMacroID() && LocEnd.isMacroID()) {
+      SourceManager &SM = BRC.getSourceManager();
+      const LangOptions &LO = BRC.getASTContext().getLangOpts();
+      if (Lexer::isAtStartOfMacroExpansion(LocStart, SM, LO) &&
+          Lexer::isAtEndOfMacroExpansion(LocEnd, SM, LO)) {
+        SourceRange R{LocStart, LocEnd};
+        Out << Lexer::getSourceText(Lexer::getAsCharRange(R, SM, LO), SM, LO);
         return false;
       }
     }
+  }
 
   if (const auto *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()),

clang/test/Analysis/diagnostics/macros.cpp

 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')}}
   }
 }
+
+#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')}}
+}