NoQ updated this revision to Diff 452893.
NoQ added a comment.
Remove a redundant `C.addTransition(State)`. This caused us to run both the old
mode and the new mode by producing a state split on an otherwise linear path.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D131655/new/
https://reviews.llvm.org/D131655
Files:
clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
clang/test/Analysis/nullability.mm
Index: clang/test/Analysis/nullability.mm
===================================================================
--- clang/test/Analysis/nullability.mm
+++ clang/test/Analysis/nullability.mm
@@ -34,6 +34,13 @@
#include "Inputs/system-header-simulator-for-nullability.h"
+extern void __assert_fail(__const char *__assertion, __const char *__file,
+ unsigned int __line, __const char *__function)
+ __attribute__((__noreturn__));
+
+#define assert(expr) \
+ ((expr) ? (void)(0) : __assert_fail(#expr, __FILE__, __LINE__, __func__))
+
@interface TestObject : NSObject
- (int *_Nonnull)returnsNonnull;
- (int *_Nullable)returnsNullable;
@@ -42,6 +49,9 @@
- (void)takesNullable:(int *_Nullable)p;
- (void)takesUnspecified:(int *)p;
@property(readonly, strong) NSString *stuff;
+@property(readonly, nonnull) int *propReturnsNonnull;
+@property(readonly, nullable) int *propReturnsNullable;
+@property(readonly) int *propReturnsUnspecified;
@end
TestObject * getUnspecifiedTestObject();
@@ -182,6 +192,53 @@
}
}
+void testObjCPropertyReadNullability() {
+ TestObject *o = getNonnullTestObject();
+ switch (getRandom()) {
+ case 0:
+ [o takesNonnull:o.propReturnsNonnull]; // no-warning
+ break;
+ case 1:
+ [o takesNonnull:o.propReturnsUnspecified]; // no-warning
+ break;
+ case 2:
+ [o takesNonnull:o.propReturnsNullable]; // expected-warning {{Nullable pointer is passed to a callee that requires a non-null 1st parameter}}
+ break;
+ case 3:
+ // If a property is constrained nonnull, assume it remains nonnull
+ if (o.propReturnsNullable) {
+ [o takesNonnull:o.propReturnsNullable]; // no-warning
+ [o takesNonnull:o.propReturnsNullable]; // no-warning
+ }
+ break;
+ case 4:
+ if (!o.propReturnsNullable) {
+ [o takesNonnull:o.propReturnsNullable]; // expected-warning {{Nullable pointer is passed to a callee that requires a non-null 1st parameter}}
+ }
+ break;
+ case 5:
+ if (!o.propReturnsNullable) {
+ if (o.propReturnsNullable) {
+ // Nonnull constraint from the more recent call wins
+ [o takesNonnull:o.propReturnsNullable]; // no-warning
+ }
+ }
+ break;
+ case 6:
+ // Constraints on property return values are receiver-qualified
+ if (o.propReturnsNullable) {
+ [o takesNonnull:o.propReturnsNullable]; // no-warning
+ [o takesNonnull:getNonnullTestObject().propReturnsNullable]; // expected-warning {{Nullable pointer is passed to a callee that requires a non-null 1st parameter}}
+ }
+ break;
+ case 7:
+ // Assertions must be effective at suppressing warnings
+ assert(o.propReturnsNullable);
+ [o takesNonnull:o.propReturnsNullable]; // no-warning
+ break;
+ }
+}
+
Dummy * _Nonnull testDirectCastNullableToNonnull() {
Dummy *p = returnsNullable();
takesNonnull((Dummy * _Nonnull)p); // no-warning
Index: clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
===================================================================
--- clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
@@ -80,7 +80,7 @@
class NullabilityChecker
: public Checker<check::Bind, check::PreCall, check::PreStmt<ReturnStmt>,
check::PostCall, check::PostStmt<ExplicitCastExpr>,
- check::PostObjCMessage, check::DeadSymbols,
+ check::PostObjCMessage, check::DeadSymbols, eval::Assume,
check::Location, check::Event<ImplicitNullDerefEvent>> {
public:
@@ -102,6 +102,8 @@
void checkEvent(ImplicitNullDerefEvent Event) const;
void checkLocation(SVal Location, bool IsLoad, const Stmt *S,
CheckerContext &C) const;
+ ProgramStateRef evalAssume(ProgramStateRef State, SVal Cond,
+ bool Assumption) const;
void printState(raw_ostream &Out, ProgramStateRef State, const char *NL,
const char *Sep) const override;
@@ -129,7 +131,7 @@
// When set to false no nullability information will be tracked in
// NullabilityMap. It is possible to catch errors like passing a null pointer
// to a callee that expects nonnull argument without the information that is
- // stroed in the NullabilityMap. This is an optimization.
+ // stored in the NullabilityMap. This is an optimization.
bool NeedTracking = false;
private:
@@ -230,10 +232,41 @@
Lhs.getNullabilitySource() == Rhs.getNullabilitySource();
}
+// For the purpose of tracking historical property accesses, the key for lookup
+// is an object pointer (could be an instance or a class) paired with the unique
+// identifier for the property being invoked on that object.
+using ObjectPropPair = std::pair<const MemRegion *, const IdentifierInfo *>;
+
+// Metadata associated with the return value from a recorded property access.
+struct ConstrainedPropertyVal {
+ // This will reference the conjured return SVal for some call
+ // of the form [object property]
+ DefinedOrUnknownSVal Value;
+
+ // If the SVal has been determined to be nonnull, that is recorded here
+ bool isConstrainedNonnull;
+
+ ConstrainedPropertyVal(DefinedOrUnknownSVal SV)
+ : Value(SV), isConstrainedNonnull(false) {}
+
+ void Profile(llvm::FoldingSetNodeID &ID) const {
+ Value.Profile(ID);
+ ID.AddInteger(isConstrainedNonnull ? 1 : 0);
+ }
+};
+
+bool operator==(const ConstrainedPropertyVal &Lhs,
+ const ConstrainedPropertyVal &Rhs) {
+ return Lhs.Value == Rhs.Value &&
+ Lhs.isConstrainedNonnull == Rhs.isConstrainedNonnull;
+}
+
} // end anonymous namespace
REGISTER_MAP_WITH_PROGRAMSTATE(NullabilityMap, const MemRegion *,
NullabilityState)
+REGISTER_MAP_WITH_PROGRAMSTATE(PropertyAccessesMap, ObjectPropPair,
+ ConstrainedPropertyVal)
// We say "the nullability type invariant is violated" when a location with a
// non-null type contains NULL or a function with a non-null return type returns
@@ -464,6 +497,19 @@
State = State->remove<NullabilityMap>(I->first);
}
}
+
+ // When an object goes out of scope, we can free the history associated
+ // with any property accesses on that object
+ PropertyAccessesMapTy PropertyAccesses = State->get<PropertyAccessesMap>();
+ for (PropertyAccessesMapTy::iterator I = PropertyAccesses.begin(),
+ E = PropertyAccesses.end();
+ I != E; ++I) {
+ const MemRegion *ReceiverRegion = I->first.first;
+ if (!SR.isLiveRegion(ReceiverRegion)) {
+ State = State->remove<PropertyAccessesMap>(I->first);
+ }
+ }
+
// When one of the nonnull arguments are constrained to be null, nullability
// preconditions are violated. It is not enough to check this only when we
// actually report an error, because at that time interesting symbols might be
@@ -851,6 +897,32 @@
return Nullability::Unspecified;
}
+// The return value of a property access is typically a temporary value which
+// will not be tracked in a persistent manner by the analyzer. We use
+// evalAssume() in order to immediately record constraints on those temporaries
+// at the time they are imposed (e.g. by a nil-check conditional).
+ProgramStateRef NullabilityChecker::evalAssume(ProgramStateRef State, SVal Cond,
+ bool Assumption) const {
+ PropertyAccessesMapTy PropertyAccesses = State->get<PropertyAccessesMap>();
+ for (PropertyAccessesMapTy::iterator I = PropertyAccesses.begin(),
+ E = PropertyAccesses.end();
+ I != E; ++I) {
+ if (!I->second.isConstrainedNonnull) {
+ ConditionTruthVal IsNonNull = State->isNonNull(I->second.Value);
+ if (IsNonNull.isConstrainedTrue()) {
+ ConstrainedPropertyVal Replacement = I->second;
+ Replacement.isConstrainedNonnull = true;
+ State = State->set<PropertyAccessesMap>(I->first, Replacement);
+ } else if (IsNonNull.isConstrainedFalse()) {
+ // Space optimization: no point in tracking constrained-null cases
+ State = State->remove<PropertyAccessesMap>(I->first);
+ }
+ }
+ }
+
+ return State;
+}
+
/// Calculate the nullability of the result of a message expr based on the
/// nullability of the receiver, the nullability of the return value, and the
/// constraints.
@@ -947,12 +1019,53 @@
// No tracked information. Use static type information for return value.
Nullability RetNullability = getNullabilityAnnotation(RetType);
- // Properties might be computed. For this reason the static analyzer creates a
- // new symbol each time an unknown property is read. To avoid false pozitives
- // do not treat unknown properties as nullable, even when they explicitly
- // marked nullable.
- if (M.getMessageKind() == OCM_PropertyAccess && !C.wasInlined)
- RetNullability = Nullability::Nonnull;
+ // Properties might be computed, which means the property value could
+ // theoretically change between calls even in commonly-observed cases like
+ // this:
+ //
+ // if (foo.prop) { // ok, it's nonnull here...
+ // [bar doStuffWithNonnullVal:foo.prop]; // ...but what about
+ // here?
+ // }
+ //
+ // If the property is nullable-annotated, a naive analysis would lead to many
+ // false positives despite the presence of probably-correct nil-checks. To
+ // reduce the false positive rate, we maintain a history of the most recently
+ // observed property value. For each property access, if the prior value has
+ // been constrained to be not nil then we will conservatively assume that the
+ // next access can be inferred as nonnull.
+ if (RetNullability != Nullability::Nonnull &&
+ M.getMessageKind() == OCM_PropertyAccess && !C.wasInlined) {
+ bool LookupResolved = false;
+ if (const MemRegion *ReceiverRegion = getTrackRegion(M.getReceiverSVal())) {
+ if (IdentifierInfo *Ident = M.getSelector().getIdentifierInfoForSlot(0)) {
+ LookupResolved = true;
+ ObjectPropPair Key = std::make_pair(ReceiverRegion, Ident);
+ const ConstrainedPropertyVal *PrevPropVal =
+ State->get<PropertyAccessesMap>(Key);
+ if (PrevPropVal && PrevPropVal->isConstrainedNonnull) {
+ RetNullability = Nullability::Nonnull;
+ } else {
+ // If a previous property access was constrained as nonnull, we hold
+ // on to that constraint (effectively inferring that all subsequent
+ // accesses on that code path can be inferred as nonnull). If the
+ // previous property access was *not* constrained as nonnull, then
+ // let's throw it away in favor of keeping the SVal associated with
+ // this more recent access.
+ if (auto ReturnSVal =
+ M.getReturnValue().getAs<DefinedOrUnknownSVal>()) {
+ State = State->set<PropertyAccessesMap>(
+ Key, ConstrainedPropertyVal(*ReturnSVal));
+ }
+ }
+ }
+ }
+
+ if (!LookupResolved) {
+ // Fallback: err on the side of suppressing the false positive.
+ RetNullability = Nullability::Nonnull;
+ }
+ }
Nullability ComputedNullab = getMostNullable(RetNullability, SelfNullability);
if (ComputedNullab == Nullability::Nullable) {
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
