From: Artem Dergachev Date: Fri, 30 Nov 2018 03:39:58 +0000 (+0000) Subject: [analyzer] Nullability: Don't detect post factum violation on concrete values. X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=4d1d8f287a82495809b15297ddd7115bffb31037;p=clang [analyzer] Nullability: Don't detect post factum violation on concrete values. The checker suppresses warnings on paths on which a nonnull value is assumed to be nullable. This probably deserves a warning, but it's a separate story. Now, because dead symbol collection fires in pretty random moments, there sometimes was a situation when dead symbol collection fired after computing a parameter but before actually evaluating call enter into the function, which triggered the suppression when the argument was null in the first place earlier than the obvious warning for null-to-nonnull was emitted, causing false negatives. Only trigger the suppression for symbols, not for concrete values. It is impossible to constrain a concrete value post-factum because it is impossible to constrain a concrete value at all. This covers all the necessary cases because by the time we reach the call, symbolic values should be either not constrained to null, or already collapsed into concrete null values. Which in turn happens because they are passed through the Store, and the respective collapse is implemented as part of getSVal(), which is also weird. Differential Revision: https://reviews.llvm.org/D54017 git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@347954 91177308-0d34-0410-b5e6-96231b3b80d8 --- diff --git a/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp b/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp index eae0a974ce..38c694d5a2 100644 --- a/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp @@ -329,8 +329,8 @@ NullabilityChecker::NullabilityBugVisitor::VisitNode(const ExplodedNode *N, nullptr); } -/// Returns true when the value stored at the given location is null -/// and the passed in type is nonnnull. +/// Returns true when the value stored at the given location has been +/// constrained to null after being passed through an object of nonnnull type. static bool checkValueAtLValForInvariantViolation(ProgramStateRef State, SVal LV, QualType T) { if (getNullabilityAnnotation(T) != Nullability::Nonnull) @@ -340,9 +340,14 @@ static bool checkValueAtLValForInvariantViolation(ProgramStateRef State, if (!RegionVal) return false; - auto StoredVal = - State->getSVal(RegionVal->getRegion()).getAs(); - if (!StoredVal) + // If the value was constrained to null *after* it was passed through that + // location, it could not have been a concrete pointer *when* it was passed. + // In that case we would have handled the situation when the value was + // bound to that location, by emitting (or not emitting) a report. + // Therefore we are only interested in symbolic regions that can be either + // null or non-null depending on the value of their respective symbol. + auto StoredVal = State->getSVal(*RegionVal).getAs(); + if (!StoredVal || !isa(StoredVal->getRegion())) return false; if (getNullConstraint(*StoredVal, State) == NullConstraint::IsNull) @@ -1170,10 +1175,15 @@ void NullabilityChecker::printState(raw_ostream &Out, ProgramStateRef State, NullabilityMapTy B = State->get(); + if (State->get()) + Out << Sep << NL + << "Nullability invariant was violated, warnings suppressed." << NL; + if (B.isEmpty()) return; - Out << Sep << NL; + if (!State->get()) + Out << Sep << NL; for (NullabilityMapTy::iterator I = B.begin(), E = B.end(); I != E; ++I) { Out << I->first << " : "; diff --git a/test/Analysis/nullability-arc.mm b/test/Analysis/nullability-arc.mm new file mode 100644 index 0000000000..5c68dda42e --- /dev/null +++ b/test/Analysis/nullability-arc.mm @@ -0,0 +1,39 @@ +// RUN: %clang_analyze_cc1 -w -analyzer-checker=core,nullability\ +// RUN: -analyzer-output=text -verify %s +// RUN: %clang_analyze_cc1 -w -analyzer-checker=core,nullability\ +// RUN: -analyzer-output=text -verify %s -fobjc-arc + +#if !__has_feature(objc_arc) +// expected-no-diagnostics +#endif + + +#define nil ((id)0) + +@interface Param +@end + +@interface Base +- (void)foo:(Param *_Nonnull)param; +@end + +@interface Derived : Base +@end + +@implementation Derived +- (void)foo:(Param *)param { + // FIXME: Why do we not emit the warning under ARC? + [super foo:param]; +#if __has_feature(objc_arc) + // expected-warning@-2{{nil passed to a callee that requires a non-null 1st parameter}} + // expected-note@-3 {{nil passed to a callee that requires a non-null 1st parameter}} +#endif + + [self foo:nil]; +#if __has_feature(objc_arc) + // expected-note@-2{{Calling 'foo:'}} + // expected-note@-3{{Passing nil object reference via 1st parameter 'param'}} +#endif +} +@end + diff --git a/test/Analysis/nullability.mm b/test/Analysis/nullability.mm index 2278efff3e..c3f27e4d22 100644 --- a/test/Analysis/nullability.mm +++ b/test/Analysis/nullability.mm @@ -1,5 +1,36 @@ -// RUN: %clang_analyze_cc1 -fblocks -analyzer-checker=core,nullability.NullPassedToNonnull,nullability.NullReturnedFromNonnull,nullability.NullablePassedToNonnull,nullability.NullableReturnedFromNonnull,nullability.NullableDereferenced -DNOSYSTEMHEADERS=0 -verify %s -// RUN: %clang_analyze_cc1 -fblocks -analyzer-checker=core,nullability.NullPassedToNonnull,nullability.NullReturnedFromNonnull,nullability.NullablePassedToNonnull,nullability.NullableReturnedFromNonnull,nullability.NullableDereferenced -analyzer-config nullability:NoDiagnoseCallsToSystemHeaders=true -DNOSYSTEMHEADERS=1 -verify %s +// RUN: %clang_analyze_cc1 -fblocks -verify %s -analyzer-checker=core \ +// RUN: -analyzer-checker=nullability.NullPassedToNonnull \ +// RUN: -analyzer-checker=nullability.NullReturnedFromNonnull \ +// RUN: -analyzer-checker=nullability.NullablePassedToNonnull \ +// RUN: -analyzer-checker=nullability.NullableReturnedFromNonnull \ +// RUN: -analyzer-checker=nullability.NullableDereferenced \ +// RUN: -DNOSYSTEMHEADERS=0 + +// RUN: %clang_analyze_cc1 -fblocks -verify %s -analyzer-checker=core \ +// RUN: -analyzer-checker=nullability.NullPassedToNonnull \ +// RUN: -analyzer-checker=nullability.NullReturnedFromNonnull \ +// RUN: -analyzer-checker=nullability.NullablePassedToNonnull \ +// RUN: -analyzer-checker=nullability.NullableReturnedFromNonnull \ +// RUN: -analyzer-checker=nullability.NullableDereferenced \ +// RUN: -DNOSYSTEMHEADERS=1 \ +// RUN: -analyzer-config nullability:NoDiagnoseCallsToSystemHeaders=true + +// RUN: %clang_analyze_cc1 -fblocks -verify %s -analyzer-checker=core\ +// RUN: -analyzer-checker=nullability.NullPassedToNonnull\ +// RUN: -analyzer-checker=nullability.NullReturnedFromNonnull\ +// RUN: -analyzer-checker=nullability.NullablePassedToNonnull\ +// RUN: -analyzer-checker=nullability.NullableReturnedFromNonnull\ +// RUN: -analyzer-checker=nullability.NullableDereferenced\ +// RUN: -DNOSYSTEMHEADERS=0 -fobjc-arc + +// RUN: %clang_analyze_cc1 -fblocks -verify %s -analyzer-checker=core\ +// RUN: -analyzer-checker=nullability.NullPassedToNonnull\ +// RUN: -analyzer-checker=nullability.NullReturnedFromNonnull\ +// RUN: -analyzer-checker=nullability.NullablePassedToNonnull\ +// RUN: -analyzer-checker=nullability.NullableReturnedFromNonnull\ +// RUN: -analyzer-checker=nullability.NullableDereferenced\ +// RUN: -DNOSYSTEMHEADERS=1 -fobjc-arc\ +// RUN: -analyzer-config nullability:NoDiagnoseCallsToSystemHeaders=true #include "Inputs/system-header-simulator-for-nullability.h"