From 727d6ca3f0a2fdeddfbe22de6812ab301c6a9da2 Mon Sep 17 00:00:00 2001 From: Artem Dergachev Date: Tue, 23 Apr 2019 02:56:00 +0000 Subject: [PATCH] [analyzer] Unbreak body farms in presence of multiple declarations. 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. Differential Revision: https://reviews.llvm.org/D60899 llvm-svn: 358946 --- clang/lib/Analysis/BodyFarm.cpp | 2 -- .../Core/BugReporterVisitors.cpp | 3 +++ clang/test/Analysis/OSAtomic_mac.c | 23 ++++++++++++------- 3 files changed, 18 insertions(+), 10 deletions(-) diff --git a/clang/lib/Analysis/BodyFarm.cpp b/clang/lib/Analysis/BodyFarm.cpp index a36b0f337cac..bb70795d91a8 100644 --- a/clang/lib/Analysis/BodyFarm.cpp +++ b/clang/lib/Analysis/BodyFarm.cpp @@ -665,8 +665,6 @@ static Stmt *create_OSAtomicCompareAndSwap(ASTContext &C, const FunctionDecl *D) } Stmt *BodyFarm::getBody(const FunctionDecl *D) { - D = D->getCanonicalDecl(); - Optional &Val = Bodies[D]; if (Val.hasValue()) return Val.getValue(); diff --git a/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp b/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp index af92077ef981..fc8826567c42 100644 --- a/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp +++ b/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp @@ -579,6 +579,9 @@ private: 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; diff --git a/clang/test/Analysis/OSAtomic_mac.c b/clang/test/Analysis/OSAtomic_mac.c index a69d98078e17..b09c71f6c6e9 100644 --- a/clang/test/Analysis/OSAtomic_mac.c +++ b/clang/test/Analysis/OSAtomic_mac.c @@ -8,13 +8,20 @@ int OSAtomicCompareAndSwapPtrBarrier() { } 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; // expected-warning{{Undefined or garbage value returned to caller}} - // expected-note@-1{{Undefined or garbage value returned to caller}} + return b; +} + +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}} }