[analyzer] Do not infer nullability inside function-like macros, even when macro is explicitly returning NULL

[george.karpenkov]

Summary

We already suppress such reports for inlined functions, we should then get the same behavior for macros.
The underlying reason is that the same macro, can be called from many different contexts, and nullability can only be expected in _some_ of them.
Assuming that the macro can return null in _all_ of them sometimes leads to a large number of false positives.

E.g. consider the test case for the dynamic cast implementation in macro: in such cases, the bug report is unwanted.

Tracked in rdar://36304776

[NoQ]

lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

218

This hides the other local variable.

241–254

I wonder if at least some of this code can be re-used across visitors. Finding assignments sounds very common.

[NoQ]

Also, hmm, for plain functions we only make this suppression when the value is a null pointer, not when it's a null integer (see ReturnVisitor::addVisitorIfNecessary). I suspect it we might want to behave identically(?)

[george.karpenkov]

lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

218

yeah other one is not doing anything, should be removed.

[NoQ]

lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

242

Actually i suspect that it'd always be a single decl. Because for the purposes of analysis, during CFG construction, we modify the AST to split out declstmts of multiple variables into multiple single-variable declstmts. Could you see if dyn_cast<VarDecl>(DS->getSingleDecl()) works?

[george.karpenkov]

lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

241–254

@NoQ haven't really seen anything useful apart from isInitializationOfVar, but that requires knowing in advance the required region (assuming you mean matching assignment together with declaration)

[george.karpenkov]

One issue not done is *not* suppressing reports for division by zero in same scenarios.
I don't see an easy way to do this without adding quite a bit of complexity (additional parsing of statement...)
Do we really want to do this? Is the complexity worse it? Why do we even want a different behavior for those?

[NoQ]

This sounds like a tiny loss of coverage for division by zero checker. I guess it's an unfortunate side effect that indeed rather deserves a FIXME than blocking the patch on it.

[george.karpenkov]

Updated a fix for division-by-zero

[NoQ]

Woohoo even better.

lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

std::unique_ptr<PathDiagnosticPiece> BugReporterVisitor::getDefaultEndPath(
   auto P = llvm::make_unique<PathDiagnosticEventPiece>(
       L, BR.getDescription(), Ranges.begin() == Ranges.end());
   for (SourceRange Range : Ranges)
     P->addRange(Range);
 
   return std::move(P);
 }
 
+/// \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());
+}
+
+/// \return Whether given spelling location corresponds to an expansion
+/// of a function-like macro.
+static bool isFunctionMacroExpansion(SourceLocation Loc,
+                                const SourceManager &SM) {
+  if (!Loc.isMacroID())
+    return false;
+  while (SM.isMacroArgExpansion(Loc))
+    Loc = SM.getImmediateExpansionRange(Loc).first;
+  std::pair<FileID, unsigned> TLInfo = SM.getDecomposedLoc(Loc);
+  SrcMgr::SLocEntry SE = SM.getSLocEntry(TLInfo.first);
+  const SrcMgr::ExpansionInfo &EInfo = SE.getExpansion();
+  return EInfo.isFunctionMacroExpansion();
+}
 
 namespace {
+
+class MacroNullReturnSuppressionVisitor final
+    : public BugReporterVisitorImpl<MacroNullReturnSuppressionVisitor> {
+
+  const SubRegion *RegionOfInterest;
+
+public:
+  MacroNullReturnSuppressionVisitor(const SubRegion *R) : RegionOfInterest(R) {}
+
+  static void *getTag() {
+    static int Tag = 0;
+    return static_cast<void *>(&Tag);
+  }
+
+  void Profile(llvm::FoldingSetNodeID &ID) const override {
+    ID.AddPointer(getTag());
+  }
+
+  std::shared_ptr<PathDiagnosticPiece> VisitNode(const ExplodedNode *N,
+                                                 const ExplodedNode *PrevN,
+                                                 BugReporterContext &BRC,
+                                                 BugReport &BR) override {
+    auto BugPoint = BR.getErrorNode()->getLocation().getAs<StmtPoint>();
+    if (!BugPoint)
+      return nullptr;
+
+    const SourceManager &SMgr = BRC.getSourceManager();
+    if (auto Loc = matchAssignment(N, BRC)) {
+      if (isFunctionMacroExpansion(*Loc, SMgr)) {
+        std::string MacroName = getMacroName(*Loc, BRC);
+        SourceLocation BugLoc = BugPoint->getStmt()->getLocStart();
+        if (!BugLoc.isMacroID() || getMacroName(BugLoc, BRC) != MacroName)
+          BR.markInvalid(getTag(), MacroName.c_str());
+      }
+    }
+    return nullptr;
+  }
+
+  static void addMacroVisitorIfNecessary(
+        const ExplodedNode *N, const MemRegion *R,
+        bool EnableNullFPSuppression, BugReport &BR,
+        const SVal V) {
+    AnalyzerOptions &Options = N->getState()->getStateManager()
+        .getOwningEngine()->getAnalysisManager().options;
+    if (EnableNullFPSuppression && Options.shouldSuppressNullReturnPaths()
+          && V.getAs<Loc>())
+      BR.addVisitor(llvm::make_unique<MacroNullReturnSuppressionVisitor>(
+              R->getAs<SubRegion>()));
+  }
+
+private:
+  /// \return Source location of right hand side of an assignment
+  /// into \c RegionOfInterest, empty optional if none found.
+  Optional<SourceLocation> matchAssignment(const ExplodedNode *N,
+                                           BugReporterContext &BRC) {
+    const Stmt *S = PathDiagnosticLocation::getStmt(N);
+    ProgramStateRef State = N->getState();
+    auto *LCtx = N->getLocationContext();
+    if (!S)
+      return None;
+
+    if (auto *DS = dyn_cast<DeclStmt>(S)) {
+      if (const VarDecl *VD = dyn_cast<VarDecl>(DS->getSingleDecl()))
+        if (const Expr *RHS = VD->getInit())
+          if (RegionOfInterest->isSubRegionOf(
+                  State->getLValue(VD, LCtx).getAsRegion()))
+            return RHS->getLocStart();
+    } else if (auto *BO = dyn_cast<BinaryOperator>(S)) {
+      const MemRegion *R = N->getSVal(BO->getLHS()).getAsRegion();
+      const Expr *RHS = BO->getRHS();
+      if (BO->isAssignmentOp() && RegionOfInterest->isSubRegionOf(R)) {
+        return RHS->getLocStart();
+      }
+    }
+    return None;
+  }
+};
+
 /// Emits an extra note at the return statement of an interesting stack frame.
 ///
 /// The returned value is marked as an interesting value, and if it's null,
 /// adds a visitor to track where it became null.
 ///
 /// This visitor is intended to be used when another visitor discovers that an
 /// interesting value comes from an inlined function call.
 class ReturnVisitor : public BugReporterVisitorImpl<ReturnVisitor> {
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 (Inner && ExplodedGraph::isInterestingLValueExpr(Inner)) {
     // For those, we want to track the location of the reference.
     const MemRegion *R = (RR && LVIsNull) ? RR :
         LVNode->getSVal(Inner).getAsRegion();
 
     if (R) {
       // Mark both the variable region and its contents as interesting.
       SVal V = LVState->getRawSVal(loc::MemRegionVal(R));
 
+      MacroNullReturnSuppressionVisitor::addMacroVisitorIfNecessary(
+          N, R, EnableNullFPSuppression, report, V);
+
       report.markInteresting(R);
       report.markInteresting(V);
       report.addVisitor(llvm::make_unique<UndefOrNullArgVisitor>(R));
 
       // If the contents are symbolic, find out when they became null.
       if (V.getAsLocSymbol(/*IncludeBaseRegions*/ true))
         report.addVisitor(llvm::make_unique<TrackConstraintBRVisitor>(
               V.castAs<DefinedSVal>(), false));

test/Analysis/diagnostics/macro-null-return-suppression.cpp

+// RUN: %clang_analyze_cc1 -x c -analyzer-checker=core -analyzer-output=text -verify %s
+
+#define NULL 0
+
+int test_noparammacro() {
+  int *x = NULL; // expected-note{{'x' initialized to a null pointer value}}
+  return *x; // expected-warning{{Dereference of null pointer (loaded from variable 'x')}}
+             // expected-note@-1{{Dereference of null pointer (loaded from variable 'x')}}
+}
+
+#define DYN_CAST(X) (X ? (char*)X : 0)
+#define GENERATE_NUMBER(X) (0)
+
+char test_assignment(int *param) {
+  char *param2;
+  param2 = DYN_CAST(param);
+  return *param2;
+}
+
+char test_declaration(int *param) {
+  char *param2 = DYN_CAST(param);
+  return *param2;
+}
+
+int coin();
+
+int test_multi_decl(int *paramA, int *paramB) {
+  char *param1 = DYN_CAST(paramA), *param2 = DYN_CAST(paramB);
+  if (coin())
+    return *param1;
+  return *param2;
+}
+
+int testDivision(int a) {
+  int divider = GENERATE_NUMBER(2); // expected-note{{'divider' initialized to 0}}
+  return 1/divider; // expected-warning{{Division by zero}}
+                    // expected-note@-1{{Division by zero}}
+}
+
+// Warning should not be suppressed if it happens in the same macro.
+#define DEREF_IN_MACRO(X) int fn() {int *p = 0; return *p; }
+
+DEREF_IN_MACRO(0) // expected-warning{{Dereference of null pointer}}
+                  // expected-note@-1{{'p' initialized to a null}}
+                  // expected-note@-2{{Dereference of null pointer}}