[analyzer] suppress nullability inference from a macro when result is used in another macro

[george.karpenkov]

Summary

The current code used to not suppress the report, if the dereference was performed in a macro, assuming it is that same macro.
However, the assumption might not be correct, and XNU has quite a bit of code where dereference is actually performed in a different macro.

As the code uses macro name and not a unique identifier it might be fragile, but in a worst-case scenario we would simply emit an extra diagnostic.

rdar://36160245

[NoQ]

This makes sense. @dcoughlin: does this harmonize with your original intent for adding this suppression in the first place?

[dcoughlin]

Yes, this looks great!

lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

void SuppressInlineDefensiveChecksVisitor::Profile(FoldingSetNodeID &ID) const {
   ID.AddPointer(&id);
   ID.Add(V);
 }
 
 const char *SuppressInlineDefensiveChecksVisitor::getTag() {
   return "IDCVisitor";
 }
 
+/// \return name of the macro inside the location \p Loc.
+static StringRef getMacroName(SourceLocation Loc,
+    BugReporterContext &BRC) {
+  return Lexer::getImmediateMacroName(
+      Loc, BRC.getSourceManager(), BRC.getASTContext().getLangOpts());
+}
+
 std::shared_ptr<PathDiagnosticPiece>
 SuppressInlineDefensiveChecksVisitor::VisitNode(const ExplodedNode *Succ,
                                                 const ExplodedNode *Pred,
                                                 BugReporterContext &BRC,
                                                 BugReport &BR) {
   if (IsSatisfied)
     return nullptr;
 
if (!Pred->getState()->isNull(V).isConstrainedTrue()) {
     // defensive check. If the bug location is not in a macro and the
     // terminator for the current location is in a macro then suppress the
     // warning.
     auto BugPoint = BR.getErrorNode()->getLocation().getAs<StmtPoint>();
 
     if (!BugPoint)
       return nullptr;
 
-    SourceLocation BugLoc = BugPoint->getStmt()->getLocStart();
-    if (BugLoc.isMacroID())
-      return nullptr;
 
     ProgramPoint CurPoint = Succ->getLocation();
     const Stmt *CurTerminatorStmt = nullptr;
     if (auto BE = CurPoint.getAs<BlockEdge>()) {
       CurTerminatorStmt = BE->getSrc()->getTerminator().getStmt();
     } else if (auto SP = CurPoint.getAs<StmtPoint>()) {
       const Stmt *CurStmt = SP->getStmt();
       if (!CurStmt->getLocStart().isMacroID())
if (!Pred->getState()->isNull(V).isConstrainedTrue()) {
 
     SourceLocation TerminatorLoc = CurTerminatorStmt->getLocStart();
     if (TerminatorLoc.isMacroID()) {
       const SourceManager &SMgr = BRC.getSourceManager();
       std::pair<FileID, unsigned> TLInfo = SMgr.getDecomposedLoc(TerminatorLoc);
       SrcMgr::SLocEntry SE = SMgr.getSLocEntry(TLInfo.first);
       const SrcMgr::ExpansionInfo &EInfo = SE.getExpansion();
       if (EInfo.isFunctionMacroExpansion()) {
+        SourceLocation BugLoc = BugPoint->getStmt()->getLocStart();
+
+        // Suppress reports unless we are in that same macro.
+        if (!BugLoc.isMacroID() ||
+            getMacroName(BugLoc, BRC) != getMacroName(TerminatorLoc, BRC)) {
           BR.markInvalid("Suppress Macro IDC", CurLC);
+        }
         return nullptr;
       }
     }
   }
   return nullptr;
 }
 
 static const MemRegion *getLocationRegionIfReference(const Expr *E,

test/Analysis/inlining/false-positive-suppression.c

if (ISEQUAL(p,q)) {
     return;
   }
 
   *p = 1; // no-warning
   *q = 1; // no-warning
 }
 
 
+
 // Here the check is entirely in non-macro code even though the code itself
 // is a macro argument.
 #define MACRO_DO_IT(a) (a)
 void testErrorInArgument(int *p) {
   int i = MACRO_DO_IT((p ? 0 : *p)); // expected-warning {{Dereference of null pointer (loaded from variable 'p')}}c
   (void)i;
 }
 
+// No warning should be emitted if dereference is performed from a different
+// macro.
+#define MACRO_CHECK(a) if (a) {}
+#define MACRO_DEREF(a) (*a)
+int testDifferentMacro(int *p) {
+  MACRO_CHECK(p);
+  return MACRO_DEREF(p); // no-warning
+}
+
 // --------------------------
 // "Suppression suppression"
 // --------------------------
 
 void testDynCastOrNullOfNull() {
   // Don't suppress when one of the arguments is NULL.
   int *casted = dynCastOrNull(0);
   *casted = 1;