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

[NoQ]

Summary

Split out of 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 ParmVarDecls 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.

[a_sidorin]

Hi Artem,
This looks good to me. However, it seems to me that the correct solution is to synthesize a shining new function and include it into the redeclaration chain. This requires much more work, however.

[NoQ]

In D60899#1473412, @a_sidorin wrote:

the correct solution is to synthesize a shining new function and include it into the redeclaration chain.

Mmm, what are the pros and cons?

[a_sidorin]

Mmm, what are the pros and cons?

This is how we do it in the ASTImporter. It allows us to correctly handle redeclarations with and without bodies - the declarations in the current AST remain unchanged but a new declaration appears. But it can be a kind of a professional deformation :) BodyFarm is a much lighter facility and I won't argue that my approach is better - just an idea.

[NoQ]

With body farms, i think, the current declaration also remains unchanged, we're just getting a shining new CompoundStmt as a body in the middle of nowhere, i.e., ADC->getDecl()->getBody() isn't the same as ADC->getBody(). Which is, yeah, still weird :)

clang/lib/Analysis/BodyFarm.cpp

auto *If = IfStmt::Create(C, SourceLocation(),
                             /* init=*/nullptr,
                             /* var=*/nullptr, Comparison, Body,
                             SourceLocation(), Else);
 
   return If;
 }
 
 Stmt *BodyFarm::getBody(const FunctionDecl *D) {
-  D = D->getCanonicalDecl();
-
   Optional<Stmt *> &Val = Bodies[D];
   if (Val.hasValue())
     return Val.getValue();
 
   Val = nullptr;
 
   if (D->getIdentifier() == nullptr)
     return nullptr;

clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

if (Call.isInSystemHeader()) {
       if (N->getStackFrame()->getCFG()->size() > 3)
         R.markInvalid(getTag(), nullptr);
       return nullptr;
     }
 
     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;
 
     SmallString<256> sbuf;
     llvm::raw_svector_ostream os(sbuf);
     os << "Returning without writing to '";
 
     // Do not generate the note if failed to pretty-print.

clang/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}}
-
+  // 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@-2{{Calling 'OSAtomicCompareAndSwapPtrBarrier'}}
-  // expected-note@-3{{Returning from 'OSAtomicCompareAndSwapPtrBarrier'}}
+  return b;
+}
 
-  return b; // expected-warning{{Undefined or garbage value returned to caller}}
-            // expected-note@-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@-1{{TRUE}}
+  clang_analyzer_eval(x == &x); // expected-warning{{TRUE}}
+                                // expected-note@-1{{TRUE}}
 }