[analyzer] pr41335: NoStoreFuncVisitor: Fix crash when no-store event is in a body-farmed function.

[NoQ]

Summary

It is getting increasingly annoying that it's so hard to construct a PathDiagnosticLocation correctly. I think we should eventually make the API more defensive to invalid source locations.

[NoQ]

Fix comments.

[NoQ]

Hmm, i think i'd love to know why doesn't the uninitialized variable checker fire on the if-statement as farmed by the body farm:

592   // Signature:
593   // _Bool OSAtomicCompareAndSwapPtr(void *__oldValue,
594   //                                 void *__newValue,
595   //                                 void * volatile *__theValue)
596   // Generate body:
597   //   if (oldValue == *theValue) {
598   //    *theValue = newValue;
599   //    return YES;
600   //   }
601   //   else return NO;

(closing brace accidentally omitted in the original comment as well)

[alexfh]

LG with a couple of nits.

clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

582 ↗

(On Diff #195497)

Should this be marked with a FIXME?

clang/test/Analysis/diagnostics/body-farm-crashes.c

1 ↗

(On Diff #195497)

Add a space before the backslash.

[NoQ]

In D60808#1469734, @NoQ wrote:

Hmm, i think i'd love to know why doesn't the uninitialized variable checker fire on the if-statement as farmed by the body farm:

Passing arguments to this whole body farm thing doesn't work. It builds the body for the declaration on line 4 but then calls the declaration on line 5, and parameter variables in the synthesized body don't match parameter variables of the call, so it cannot read argument values :/

[NoQ]

Don't canonicalize the decl in the body farm. The decl supplied by the AnalysisDeclContext is already the correct one (and not necessarily the canonical one).

Keep the defensive behavior for NoStoreFuncVisitor because it's generally the right thing to do for future-proofness.

[NoQ]

(also fix typo in the function name)

[NoQ]

Separate the canonicalization change that unbreaks body farms into a separate patch.

[NoQ]

Add space before \.

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

if (!isValid())
       return nullptr;
     return asStmt();
   }
 
   const Decl *asDecl() const { assert(isValid()); return D; }
 
   bool hasRange() const { return K == StmtK || K == RangeK || K == DeclK; }
 
+  bool hasValidLocation() const { return asLocation().isValid(); }
+
   void invalidate() {
     *this = PathDiagnosticLocation();
   }
 
   void flatten();
 
   const SourceManager& getManager() const { assert(isValid()); return *SM; }
 
private:
   PathDiagnosticLocation Pos;
 
 public:
   PathDiagnosticSpotPiece(const PathDiagnosticLocation &pos,
                           StringRef s,
                           PathDiagnosticPiece::Kind k,
                           bool addPosRange = true)
       : PathDiagnosticPiece(s, k), Pos(pos) {
-    assert(Pos.isValid() && Pos.asLocation().isValid() &&
+    assert(Pos.isValid() && Pos.hasValidLocation() &&
            "PathDiagnosticSpotPiece's must have a valid location.");
     if (addPosRange && Pos.hasRange()) addRange(Pos.asRange());
   }
 
   PathDiagnosticLocation getLocation() const override { return Pos; }
   void flattenLocations() override { Pos.flatten(); }
 
   void Profile(llvm::FoldingSetNodeID &ID) const override;

lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

std::shared_ptr<PathDiagnosticPiece> VisitNode(const ExplodedNode *N,
     // Region of interest corresponds to an IVar, exiting a method
     // which could have written into that IVar, but did not.
     if (const auto *MC = dyn_cast<ObjCMethodCall>(Call)) {
       if (const auto *IvarR = dyn_cast<ObjCIvarRegion>(RegionOfInterest)) {
         const MemRegion *SelfRegion = MC->getReceiverSVal().getAsRegion();
         if (RegionOfInterest->isSubRegionOf(SelfRegion) &&
             potentiallyWritesIntoIvar(Call->getRuntimeDefinition().getDecl(),
                                       IvarR->getDecl()))
-          return maybeEmitNode(R, *Call, N, {}, SelfRegion, "self",
+          return maybeEmitNote(R, *Call, N, {}, SelfRegion, "self",
                                /*FirstIsReferenceType=*/false, 1);
       }
     }
 
     if (const auto *CCall = dyn_cast<CXXConstructorCall>(Call)) {
       const MemRegion *ThisR = CCall->getCXXThisVal().getAsRegion();
       if (RegionOfInterest->isSubRegionOf(ThisR)
           && !CCall->getDecl()->isImplicit())
-        return maybeEmitNode(R, *Call, N, {}, ThisR, "this",
+        return maybeEmitNote(R, *Call, N, {}, ThisR, "this",
                              /*FirstIsReferenceType=*/false, 1);
 
       // Do not generate diagnostics for not modified parameters in
       // constructors.
       return nullptr;
     }
 
     ArrayRef<ParmVarDecl *> parameters = getCallParameters(Call);
     for (unsigned I = 0; I < Call->getNumArgs() && I < parameters.size(); ++I) {
       const ParmVarDecl *PVD = parameters[I];
       SVal V = Call->getArgSVal(I);
       bool ParamIsReferenceType = PVD->getType()->isReferenceType();
       std::string ParamName = PVD->getNameAsString();
 
       int IndirectionLevel = 1;
       QualType T = PVD->getType();
       while (const MemRegion *MR = V.getAsRegion()) {
         if (RegionOfInterest->isSubRegionOf(MR) && !isPointerToConst(T))
-          return maybeEmitNode(R, *Call, N, {}, MR, ParamName,
+          return maybeEmitNote(R, *Call, N, {}, MR, ParamName,
                                ParamIsReferenceType, IndirectionLevel);
 
         QualType PT = T->getPointeeType();
         if (PT.isNull() || PT->isVoidType()) break;
 
         if (const RecordDecl *RD = PT->getAsRecordDecl())
           if (auto P = findRegionOfInterestInRecord(RD, State, MR))
-            return maybeEmitNode(R, *Call, N, *P, RegionOfInterest, ParamName,
+            return maybeEmitNote(R, *Call, N, *P, RegionOfInterest, ParamName,
                                  ParamIsReferenceType, IndirectionLevel);
 
         V = State->getSVal(MR, PT);
         T = PT;
         IndirectionLevel++;
       }
     }
 
return !Ty->getPointeeType().isNull() &&
            Ty->getPointeeType().getCanonicalType().isConstQualified();
   }
 
   /// Consume the information on the no-store stack frame in order to
   /// either emit a note or suppress the report enirely.
   /// \return Diagnostics piece for region not modified in the current function,
   /// if it decides to emit one.
   std::shared_ptr<PathDiagnosticPiece>
-  maybeEmitNode(BugReport &R, const CallEvent &Call, const ExplodedNode *N,
+  maybeEmitNote(BugReport &R, const CallEvent &Call, const ExplodedNode *N,
                 const RegionVector &FieldChain, const MemRegion *MatchedRegion,
                 StringRef FirstElement, bool FirstIsReferenceType,
                 unsigned IndirectionLevel) {
     // Optimistically suppress uninitialized value bugs that result
     // from system headers having a chance to initialize the value
     // but failing to do so. It's too unlikely a system header's fault.
     // It's much more likely a situation in which the function has a failure
     // mode that the user decided not to check. If we want to hunt such
if (Call.isInSystemHeader()) {
       if (N->getStackFrame()->getCFG()->size() > 3)
         R.markInvalid(getTag(), nullptr);
       return nullptr;
     }
 
     PathDiagnosticLocation L =
         PathDiagnosticLocation::create(N->getLocation(), SM);
 
+    if (!L.hasValidLocation())
+      return nullptr;
+
     SmallString<256> sbuf;
     llvm::raw_svector_ostream os(sbuf);
     os << "Returning without writing to '";
 
     // Do not generate the note if failed to pretty-print.
     if (!prettyPrintRegionName(FirstElement, FirstIsReferenceType,
                                MatchedRegion, FieldChain, IndirectionLevel, os))
       return nullptr;

test/Analysis/OSAtomic_mac.c

+// RUN: %clang_analyze_cc1 -w -analyzer-checker=core,debug.ExprInspection \
+// RUN:                    -analyzer-output=text -verify %s
+
+int OSAtomicCompareAndSwapPtrBarrier(*, *, **);
+int OSAtomicCompareAndSwapPtrBarrier() {
+  // There is some body in the actual header,
+  // but we should trust our BodyFarm instead.
+}
+
+int *invalidSLocOnRedecl() {
+  int *b; // expected-note{{'b' declared without an initial value}}
+
+  OSAtomicCompareAndSwapPtrBarrier(0, 0, &b); // no-crash
+  // FIXME: We don't really need these notes.
+  // expected-note@-2{{Calling 'OSAtomicCompareAndSwapPtrBarrier'}}
+  // expected-note@-3{{Returning from 'OSAtomicCompareAndSwapPtrBarrier'}}
+
+  return b; // expected-warning{{Undefined or garbage value returned to caller}}
+            // expected-note@-1{{Undefined or garbage value returned to caller}}
+}