[PATCH] D60899: [analyzer] Unbreak body farms in presence of multiple declarations.

[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, JDevlieghere, szepet, kristof.beyls, javed.absar.
Herald added a project: clang.
NoQ added a parent revision: D60808: [analyzer] pr41335: NoStoreFuncVisitor: Fix crash when no-store event is in a body-farmed function..

Split out of D60808 <https://reviews.llvm.org/D60808>.

When growing a body on a body farm, it's essential to use the same redeclaration of the function that's going to be used during analysis. Otherwise our `ParmVarDecl`s won't match the ones that are used to identify argument regions. This boils down to trusting the reasoning in `AnalysisDeclContext`. We shouldn't canonicalize the declaration before farming the body because it makes us not obey the sophisticated decision-making process of `AnalysisDeclContext`.


Repository:
  rC Clang

https://reviews.llvm.org/D60899

Files:
  clang/lib/Analysis/BodyFarm.cpp
  clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
  clang/test/Analysis/OSAtomic_mac.c


Index: clang/test/Analysis/OSAtomic_mac.c
===================================================================
--- clang/test/Analysis/OSAtomic_mac.c
+++ clang/test/Analysis/OSAtomic_mac.c
@@ -8,13 +8,20 @@
 }
 
 int *invalidSLocOnRedecl() {
-  int *b; // expected-note{{'b' declared without an initial value}}
-
+  // Was crashing when trying to throw a report about returning an uninitialized
+  // value to the caller. FIXME: We should probably still throw that report,
+  // something like "The "compare" part of CompareAndSwap depends on an
+  // undefined value".
+  int *b;
   OSAtomicCompareAndSwapPtrBarrier(0, 0, &b); // no-crash
-  // FIXME: We don't really need these notes.
-  // expected-note at -2{{Calling 'OSAtomicCompareAndSwapPtrBarrier'}}
-  // expected-note at -3{{Returning from 'OSAtomicCompareAndSwapPtrBarrier'}}
+  return b;
+}
 
-  return b; // expected-warning{{Undefined or garbage value returned to caller}}
-            // expected-note at -1{{Undefined or garbage value returned to caller}}
+void testThatItActuallyWorks() {
+  void *x = 0;
+  int res = OSAtomicCompareAndSwapPtrBarrier(0, &x, &x);
+  clang_analyzer_eval(res); // expected-warning{{TRUE}}
+                            // expected-note at -1{{TRUE}}
+  clang_analyzer_eval(x == &x); // expected-warning{{TRUE}}
+                                // expected-note at -1{{TRUE}}
 }
Index: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
===================================================================
--- clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -579,6 +579,9 @@
     PathDiagnosticLocation L =
         PathDiagnosticLocation::create(N->getLocation(), SM);
 
+    // For now this shouldn't trigger, but once it does (as we add more
+    // functions to the body farm), we'll need to decide if these reports
+    // are worth suppressing as well.
     if (!L.hasValidLocation())
       return nullptr;
 
Index: clang/lib/Analysis/BodyFarm.cpp
===================================================================
--- clang/lib/Analysis/BodyFarm.cpp
+++ clang/lib/Analysis/BodyFarm.cpp
@@ -665,8 +665,6 @@
 }
 
 Stmt *BodyFarm::getBody(const FunctionDecl *D) {
-  D = D->getCanonicalDecl();
-
   Optional<Stmt *> &Val = Bodies[D];
   if (Val.hasValue())
     return Val.getValue();


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D60899.195852.patch
Type: text/x-patch
Size: 2341 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20190419/eda54da3/attachment-0001.bin>