NoQ updated this revision to Diff 195810.
NoQ added a comment.
This revision is now accepted and ready to land.
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.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D60808/new/
https://reviews.llvm.org/D60808
Files:
clang/include/clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h
clang/lib/Analysis/BodyFarm.cpp
clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
clang/test/Analysis/OSAtomic_mac.c
Index: clang/test/Analysis/OSAtomic_mac.c
===================================================================
--- /dev/null
+++ clang/test/Analysis/OSAtomic_mac.c
@@ -0,0 +1,27 @@
+// 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() {
+ // 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
+ 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}}
+}
Index: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
===================================================================
--- clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -335,7 +335,7 @@
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);
}
}
@@ -344,7 +344,7 @@
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
@@ -363,7 +363,7 @@
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();
@@ -371,7 +371,7 @@
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);
@@ -549,7 +549,7 @@
/// \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) {
@@ -579,6 +579,11 @@
PathDiagnosticLocation L =
PathDiagnosticLocation::create(N->getLocation(), SM);
+ // For now this shouldn't trigger, but once it does, 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 '";
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();
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());
}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
