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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ created this revision.
NoQ added reviewers: dcoughlin, xazax.hun, a_sidorin, rnkovacs, mikhail.ramalho, Szelethus, baloghadamsoftware, Charusso.
Herald added subscribers: cfe-commits, dkrupp, donat.nagy, jfb, a.sidorin, szepet, kristof.beyls, javed.absar.
Herald added a project: clang.
NoQ updated this revision to Diff 195497.
NoQ added a comment.

Fix comments.


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.


https://reviews.llvm.org/D60808

Files:
  clang/include/clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h
  clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
  clang/test/Analysis/diagnostics/body-farm-crashes.c


Index: clang/test/Analysis/diagnostics/body-farm-crashes.c
===================================================================
--- /dev/null
+++ clang/test/Analysis/diagnostics/body-farm-crashes.c
@@ -0,0 +1,15 @@
+// RUN: %clang_analyze_cc1 -w -analyzer-checker=core\
+// RUN:                    -analyzer-output=text -verify %s
+
+int OSAtomicCompareAndSwapPtrBarrier(*, *, **);
+int OSAtomicCompareAndSwapPtrBarrier() {}
+int *atomicInvalidSLocOnRedecl() {
+  int *b; // expected-note{{'b' declared without an initial value}}
+
+	// FIXME: These notes shouldn't be there because there's nothing between them.
+  OSAtomicCompareAndSwapPtrBarrier(0, 0, &b); // expected-note{{Calling 'OSAtomicCompareAndSwapPtrBarrier'}}
+																							// expected-note at -1{{Returning from 'OSAtomicCompareAndSwapPtrBarrier'}}
+
+  return b; // expected-warning{{Undefined or garbage value returned to caller}}
+            // expected-note at -1{{Undefined or garbage value returned to caller}}
+}
Index: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
===================================================================
--- clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -578,6 +578,11 @@
 
     PathDiagnosticLocation L =
         PathDiagnosticLocation::create(N->getLocation(), SM);
+    if (!L.hasValidLocation()) {
+      // Do we need to suppress our report for body-farmed functions as well?
+      // Or maybe attach the note to the call site instead?
+      return nullptr;
+    }
 
     SmallString<256> sbuf;
     llvm::raw_svector_ostream os(sbuf);
Index: clang/include/clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h
===================================================================
--- clang/include/clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h
+++ clang/include/clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h
@@ -313,6 +313,8 @@
 
   bool hasRange() const { return K == StmtK || K == RangeK || K == DeclK; }
 
+  bool hasValidLocation() const { return asLocation().isValid(); }
+
   void invalidate() {
     *this = PathDiagnosticLocation();
   }
@@ -468,7 +470,7 @@
                           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());
   }


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D60808.195497.patch
Type: text/x-patch
Size: 2583 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20190417/f9cac646/attachment.bin>