Re: [PATCH] D11468: [Static Analyzer] The first implementation of nullability checker.
xazax.hun updated this revision to Diff 33239.
xazax.hun added a comment.
- Fine tuned the heuristics for cocoa APIs.
- Only track nullable and contradicted nullability for symbols. Use the
rest statically.
- Cleanups.
http://reviews.llvm.org/D11468
Files:
docs/analyzer/nullability.rst
lib/StaticAnalyzer/Checkers/CMakeLists.txt
lib/StaticAnalyzer/Checkers/Checkers.td
lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
test/Analysis/nullability.mm
Index: test/Analysis/nullability.mm
===
--- /dev/null
+++ test/Analysis/nullability.mm
@@ -0,0 +1,181 @@
+// RUN: %clang_cc1 -analyze -analyzer-checker=core,alpha.core.nullability -verify %s
+
+#define nil 0
+#define BOOL int
+
+@protocol NSObject
++ (id)alloc;
+- (id)init;
+@end
+
+@protocol NSCopying
+@end
+
+__attribute__((objc_root_class))
+@interface
+NSObjectNSObject
+@end
+
+@interface NSString : NSObjectNSCopying
+- (BOOL)isEqualToString : (NSString *_Nonnull)aString;
+- (NSString *)stringByAppendingString:(NSString *_Nonnull)aString;
+@end
+
+@interface TestObject : NSObject
+- (int *_Nonnull)returnsNonnull;
+- (int *_Nullable)returnsNullable;
+- (int *)returnsUnspecified;
+- (void)takesNonnull:(int *_Nonnull)p;
+- (void)takesNullable:(int *_Nullable)p;
+- (void)takesUnspecified:(int *)p;
+@property(readonly, strong) NSString *stuff;
+@end
+
+TestObject * getUnspecifiedTestObject();
+TestObject *_Nonnull getNonnullTestObject();
+TestObject *_Nullable getNullableTestObject();
+
+int getRandom();
+
+typedef struct Dummy { int val; } Dummy;
+
+void takesNullable(Dummy *_Nullable);
+void takesNonnull(Dummy *_Nonnull);
+void takesUnspecified(Dummy *);
+
+Dummy *_Nullable returnsNullable();
+Dummy *_Nonnull returnsNonnull();
+Dummy *returnsUnspecified();
+int *_Nullable returnsNullableInt();
+
+template typename T T *eraseNullab(T *p) { return p; }
+
+void testBasicRules() {
+ Dummy *p = returnsNullable();
+ int *ptr = returnsNullableInt();
+ // Make every dereference a different path to avoid sinks after errors.
+ switch (getRandom()) {
+ case 0: {
+Dummy r = *p; // expected-warning {{}}
+ } break;
+ case 1: {
+int b = p-val; // expected-warning {{}}
+ } break;
+ case 2: {
+int stuff = *ptr; // expected-warning {{}}
+ } break;
+ case 3:
+takesNonnull(p); // expected-warning {{}}
+break;
+ case 4: {
+Dummy d;
+takesNullable(d);
+Dummy dd(d);
+break;
+ }
+ // Here the copy constructor is called, so a reference is initialized with the
+ // value of p. No ImplicitNullDereference event will be dispatched for this
+ // case. A followup patch is expected to fix this in NonNullParamChecker.
+ default: { Dummy d = *p; } break; // No warning.
+ }
+ if (p) {
+takesNonnull(p);
+if (getRandom()) {
+ Dummy r = *p;
+} else {
+ int b = p-val;
+}
+ }
+ Dummy *q = 0;
+ if (getRandom()) {
+takesNullable(q);
+takesNonnull(q); // expected-warning {{}}
+ }
+ Dummy a;
+ Dummy *_Nonnull nonnull = a;
+ nonnull = q; // expected-warning {{}}
+ q = a;
+ takesNullable(q);
+ takesNonnull(q);
+}
+
+void testMultiParamChecking(Dummy *_Nonnull a, Dummy *_Nullable b,
+Dummy *_Nonnull c);
+
+void testArgumentTracking(Dummy *_Nonnull nonnull, Dummy *_Nullable nullable) {
+ Dummy *p = nullable;
+ Dummy *q = nonnull;
+ switch(getRandom()) {
+ case 1: nonnull = p; break; // expected-warning {{}}
+ case 2: p = 0; break;
+ case 3: q = p; break;
+ case 4: testMultiParamChecking(nonnull, nullable, nonnull); break;
+ case 5: testMultiParamChecking(nonnull, nonnull, nonnull); break;
+ case 6: testMultiParamChecking(nonnull, nullable, nullable); break; // expected-warning {{}}
+ case 7: testMultiParamChecking(nullable, nullable, nonnull); // expected-warning {{}}
+ case 8: testMultiParamChecking(nullable, nullable, nullable); // expected-warning {{}}
+ case 9: testMultiParamChecking((Dummy *_Nonnull)0, nullable, nonnull); break;
+ }
+}
+
+Dummy *_Nonnull testNullableReturn(Dummy *_Nullable a) {
+ Dummy *p = a;
+ return p; // expected-warning {{}}
+}
+
+Dummy *_Nonnull testNullReturn() {
+ Dummy *p = 0;
+ return p; // expected-warning {{}}
+}
+
+void testObjCMessageResultNullability() {
+ // The expected result: the most nullable of self and method return type.
+ TestObject *o = getUnspecifiedTestObject();
+ int *shouldBeNullable = [eraseNullab(getNullableTestObject()) returnsNonnull];
+ switch (getRandom()) {
+ case 0:
+// The core analyzer assumes that the receiver is non-null after a message
+// send. This is to avoid some false positives, and increase performance
+// but it also reduces the coverage and makes this checker unable to reason
+// about the nullness of the receiver.
+[o takesNonnull:shouldBeNullable]; // No warning expected.
+break;
+ case 1:
+shouldBeNullable =
+[eraseNullab(getNullableTestObject()) returnsUnspecified];
+[o
Re: [PATCH] D11468: [Static Analyzer] The first implementation of nullability checker.
This revision was automatically updated to reflect the committed changes.
Closed by commit rL246105: [Static Analyzer] Checks to catch nullability
related issues. (authored by xazax).
Changed prior to commit:
http://reviews.llvm.org/D11468?vs=33239id=33267#toc
Repository:
rL LLVM
http://reviews.llvm.org/D11468
Files:
cfe/trunk/docs/analyzer/nullability.rst
cfe/trunk/lib/StaticAnalyzer/Checkers/CMakeLists.txt
cfe/trunk/lib/StaticAnalyzer/Checkers/Checkers.td
cfe/trunk/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
cfe/trunk/test/Analysis/nullability.mm
Index: cfe/trunk/docs/analyzer/nullability.rst
===
--- cfe/trunk/docs/analyzer/nullability.rst
+++ cfe/trunk/docs/analyzer/nullability.rst
@@ -0,0 +1,92 @@
+
+Nullability Checks
+
+
+This document is a high level description of the nullablility checks.
+These checks intended to use the annotations that is described in this
+RFC: http://lists.cs.uiuc.edu/pipermail/cfe-dev/2015-March/041798.html.
+
+Let's consider the following 2 categories:
+
+1) nullable
+
+
+If a pointer 'p' has a nullable annotation and no explicit null check or assert, we should warn in the following cases:
+- 'p' gets implicitly converted into nonnull pointer, for example, we are passing it to a function that takes a nonnull parameter.
+- 'p' gets dereferenced
+
+Taking a branch on nullable pointers are the same like taking branch on null unspecified pointers.
+
+Explicit cast from nullable to nonnul::
+
+__nullable id foo;
+id bar = foo;
+takesNonNull((_nonnull) bar); — should not warn here (backward compatibility hack)
+anotherTakesNonNull(bar); — would be great to warn here, but not necessary(*)
+
+Because bar corresponds to the same symbol all the time it is not easy to implement the checker that way the cast only suppress the first call but not the second. For this reason in the first implementation after a contradictory cast happens, I will treat bar as nullable unspecified, this way all of the warnings will be suppressed. Treating the symbol as nullable unspecified also has an advantage that in case the takesNonNull function body is being inlined, the will be no warning, when the symbol is dereferenced. In case I have time after the initial version I might spend additional time to try to find a more sophisticated solution, in which we would produce the second warning (*).
+
+2) nonnull
+
+
+- Dereferencing a nonnull, or sending message to it is ok.
+- Converting nonnull to nullable is Ok.
+- When there is an explicit cast from nonnull to nullable I will trust the cast (it is probable there for a reason, because this cast does not suppress any warnings or errors).
+- But what should we do about null checks?::
+
+__nonnull id takesNonnull(__nonnull id x) {
+if (x == nil) {
+// Defensive backward compatible code:
+
+return nil; - Should the analyzer cover this piece of code? Should we require the cast (__nonnull)nil?
+}
+
+}
+
+There are these directions:
+- We can either take the branch; this way the branch is analyzed
+ - Should we not warn about any nullability issues in that branch? Probably not, it is ok to break the nullability postconditions when the nullability preconditions are violated.
+- We can assume that these pointers are not null and we lose coverage with the analyzer. (This can be implemented either in constraint solver or in the checker itself.)
+
+Other Issues to keep in mind/take care of:
+Messaging:
+- Sending a message to a nullable pointer
+ - Even though the method might return a nonnull pointer, when it was sent to a nullable pointer the return type will be nullable.
+ - The result is nullable unless the receiver is known to be non null.
+- Sending a message to a unspecified or nonnull pointer
+ - If the pointer is not assumed to be nil, we should be optimistic and use the nullability implied by the method.
+- This will not happen automatically, since the AST will have null unspecified in this case.
+
+Inlining
+
+
+A symbol may need to be treated differently inside an inlined body. For example, consider these conversions from nonnull to nullable in presence of inlining::
+
+id obj = getNonnull();
+takesNullable(obj);
+takesNonnull(obj);
+
+void takesNullable(nullable id obj) {
+ obj-ivar // we should assume obj is nullable and warn here
+}
+
+With no special treatment, when the takesNullable is inlined the analyzer will not warn when the obj symbol is dereferenced. One solution for this is to reanalyze takesNullable as a top level function to get possible violations. The alternative method, deducing nullability information from the arguments after inlining is not robust enough (for example there might be more parameters with different nullability, but in the given path the
Re: [PATCH] D11468: [Static Analyzer] The first implementation of nullability checker.
xazax.hun updated this revision to Diff 33151.
xazax.hun marked 30 inline comments as done.
xazax.hun added a comment.
Addressed the comments.
Added some more tests.
Added a design document.
http://reviews.llvm.org/D11468
Files:
docs/analyzer/nullability.rst
lib/StaticAnalyzer/Checkers/CMakeLists.txt
lib/StaticAnalyzer/Checkers/Checkers.td
lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
test/Analysis/nullability.mm
Index: test/Analysis/nullability.mm
===
--- /dev/null
+++ test/Analysis/nullability.mm
@@ -0,0 +1,181 @@
+// RUN: %clang_cc1 -analyze -analyzer-checker=core,alpha.core.nullability -verify %s
+
+#define nil 0
+#define BOOL int
+
+@protocol NSObject
++ (id)alloc;
+- (id)init;
+@end
+
+@protocol NSCopying
+@end
+
+__attribute__((objc_root_class))
+@interface
+NSObjectNSObject
+@end
+
+@interface NSString : NSObjectNSCopying
+- (BOOL)isEqualToString : (NSString *_Nonnull)aString;
+- (NSString *)stringByAppendingString:(NSString *_Nonnull)aString;
+@end
+
+@interface TestObject : NSObject
+- (int *_Nonnull)returnsNonnull;
+- (int *_Nullable)returnsNullable;
+- (int *)returnsUnspecified;
+- (void)takesNonnull:(int *_Nonnull)p;
+- (void)takesNullable:(int *_Nullable)p;
+- (void)takesUnspecified:(int *)p;
+@property(readonly, strong) NSString *stuff;
+@end
+
+TestObject * getUnspecifiedTestObject();
+TestObject *_Nonnull getNonnullTestObject();
+TestObject *_Nullable getNullableTestObject();
+
+int getRandom();
+
+typedef struct Dummy { int val; } Dummy;
+
+void takesNullable(Dummy *_Nullable);
+void takesNonnull(Dummy *_Nonnull);
+void takesUnspecified(Dummy *);
+
+Dummy *_Nullable returnsNullable();
+Dummy *_Nonnull returnsNonnull();
+Dummy *returnsUnspecified();
+int *_Nullable returnsNullableInt();
+
+template typename T T *eraseNullab(T *p) { return p; }
+
+void testBasicRules() {
+ Dummy *p = returnsNullable();
+ int *ptr = returnsNullableInt();
+ // Make every dereference a different path to avoid sinks after errors.
+ switch (getRandom()) {
+ case 0: {
+Dummy r = *p; // expected-warning {{}}
+ } break;
+ case 1: {
+int b = p-val; // expected-warning {{}}
+ } break;
+ case 2: {
+int stuff = *ptr; // expected-warning {{}}
+ } break;
+ case 3:
+takesNonnull(p); // expected-warning {{}}
+break;
+ case 4: {
+Dummy d;
+takesNullable(d);
+Dummy dd(d);
+break;
+ }
+ // Here the copy constructor is called, so a reference is initialized with the
+ // value of p. No ImplicitNullDereference event will be dispatched for this
+ // case. A followup patch is expected to fix this in NonNullParamChecker.
+ default: { Dummy d = *p; } break; // No warning.
+ }
+ if (p) {
+takesNonnull(p);
+if (getRandom()) {
+ Dummy r = *p;
+} else {
+ int b = p-val;
+}
+ }
+ Dummy *q = 0;
+ if (getRandom()) {
+takesNullable(q);
+takesNonnull(q); // expected-warning {{}}
+ }
+ Dummy a;
+ Dummy *_Nonnull nonnull = a;
+ nonnull = q; // expected-warning {{}}
+ q = a;
+ takesNullable(q);
+ takesNonnull(q);
+}
+
+void testMultiParamChecking(Dummy *_Nonnull a, Dummy *_Nullable b,
+Dummy *_Nonnull c);
+
+void testArgumentTracking(Dummy *_Nonnull nonnull, Dummy *_Nullable nullable) {
+ Dummy *p = nullable;
+ Dummy *q = nonnull;
+ switch(getRandom()) {
+ case 1: nonnull = p; break; // expected-warning {{}}
+ case 2: p = 0; break;
+ case 3: q = p; break;
+ case 4: testMultiParamChecking(nonnull, nullable, nonnull); break;
+ case 5: testMultiParamChecking(nonnull, nonnull, nonnull); break;
+ case 6: testMultiParamChecking(nonnull, nullable, nullable); break; // expected-warning {{}}
+ case 7: testMultiParamChecking(nullable, nullable, nonnull); // expected-warning {{}}
+ case 8: testMultiParamChecking(nullable, nullable, nullable); // expected-warning {{}}
+ case 9: testMultiParamChecking((Dummy *_Nonnull)0, nullable, nonnull); break;
+ }
+}
+
+Dummy *_Nonnull testNullableReturn(Dummy *_Nullable a) {
+ Dummy *p = a;
+ return p; // expected-warning {{}}
+}
+
+Dummy *_Nonnull testNullReturn() {
+ Dummy *p = 0;
+ return p; // expected-warning {{}}
+}
+
+void testObjCMessageResultNullability() {
+ // The expected result: the most nullable of self and method return type.
+ TestObject *o = getUnspecifiedTestObject();
+ int *shouldBeNullable = [eraseNullab(getNullableTestObject()) returnsNonnull];
+ switch (getRandom()) {
+ case 0:
+// The core analyzer assumes that the receiver is non-null after a message
+// send. This is to avoid some false positives, and increase performance
+// but it also reduces the coverage and makes this checker unable to reason
+// about the nullness of the receiver.
+[o takesNonnull:shouldBeNullable]; // No warning expected.
+break;
+ case 1:
+shouldBeNullable =
+[eraseNullab(getNullableTestObject()) returnsUnspecified];
+[o
Re: [PATCH] D11468: [Static Analyzer] The first implementation of nullability checker.
xazax.hun marked 11 inline comments as done.
xazax.hun added a comment.
Thank you for the review. I will upload the new version of the patch once the
rest of it was reviewed.
Comment at: lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp:12
@@ +11,3 @@
+// checker is that, the user is running this checker after all the nullability
+// warnings that is emitted by the compiler was fixed.
+//
zaks.anna wrote:
is - are
How are we relying on this assumption? What happens if they are not fixed?
Also we should describe what we mean by nullability warnings, maybe refer to
the annotations themselves here? It would be great to give a high level
overflow of the algorithm and maybe even talk about the difference between
the checks?
There are no assumptions like that in the code of the checker right now. I
updated the comment to reflect that.
Comment at: lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp:54
@@ +53,3 @@
+ return static_castNullability(
+ std::min(static_castchar(Lhs), static_castchar(Rhs)));
+}
zaks.anna wrote:
??
Added a comment to clarify what it is and how it is used.
Comment at: lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp:192
@@ +191,3 @@
+
+static bool shouldTrackRegion(const MemRegion *Region,
+ AnalysisDeclContext *DeclContext) {
zaks.anna wrote:
Is this used for optimization purposes? Can you describe the rules we use
here?
What's the benefit of not tracking self?
Initially it was not just an optimization. It was also intended to deal with
the caching out issue, but eventually it turned out that caching out was ok,
and not an error in the checker. I did not erase the code afterwards.
There are several possibilities now:
* Leave this part as it is.
* Only track symbolic regions.
* Delete this function.
Which one do you prefer?
Comment at: lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp:267
@@ +266,3 @@
+return Nullability::Nonnull;
+ return Nullability::Unspecified;
+}
zaks.anna wrote:
shouldn't this be an llvm_unreachable?
There are several AttrKind.
Source:
http://clang.llvm.org/doxygen/classclang_1_1AttributedType.html#ab4901f7a37f5539698cb5ebd706245ed
So that code is not unreachable.
Comment at: lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp:298
@@ +297,3 @@
+TrackedNullability = State-getNullabilityMap(
+Region-getAsSubRegion()-getSuperRegion());
+if (!TrackedNullability)
zaks.anna wrote:
Are we sure that if !TrackedNullability, the event complains about a
dereference on the parent (like field access)?
Sometimes when there is an expression like p[0] or p-field, the event contains
the region for the element or the field region. However the tracked information
is only available for p. For this reason I also check whether the super region
has some tracked nullability and complain about that region in this case.
http://reviews.llvm.org/D11468
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D11468: [Static Analyzer] The first implementation of nullability checker.
zaks.anna added a comment.
Another partial review.
Thanks!
Anna.
Comment at: lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp:312
@@ +311,3 @@
+ CheckerContext C) const {
+ auto RetExpr = S-getRetValue();
+ if (!RetExpr)
Please, explain what this method does and add a TODO comment if this needs got
be improved in the future.
Comment at: lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp:332
@@ +331,3 @@
+ ConditionTruthVal Nullness =
+ State-isNull(RetSVal.castAsDefinedOrUnknownSVal());
+ bool IsNotNull = Nullness.isConstrainedFalse();
You could cast and check if (RetSVal.isUndef()) at the same time.
Comment at: lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp:376
@@ +375,3 @@
+
+void NullabilityChecker::checkPreCall(const CallEvent Call,
+ CheckerContext C) const {
Please, add a high-level comment about what this method does.
Comment at: lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp:398
@@ +397,3 @@
+!Param-getType()-isReferenceType()
+!Param-getType()-isObjCObjectPointerType()) {
+ continue;
Should you use the existing helper function here as well?
Comment at: lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp:402
@@ +401,3 @@
+
+ConditionTruthVal Nullness =
+State-isNull(ArgSVal.castAsDefinedOrUnknownSVal());
A lot of code here looks like a copy and paste from the checkPreStmt(const
ReturnStmt *S, above. Please, factor into a subroutine.
Comment at: lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp:453
@@ +452,3 @@
+// Marking memory regions of variables to be nullable would be a mistake.
+// Marking otherwise is redundant.
+if (!Region-getAsSymbolicRegion())
I do not understand this comment. You are setting the nullability below..
Comment at: lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp:463
@@ +462,3 @@
+ if (State != OrigState)
+C.addTransition(State);
+}
Is calling a function/method with multiple arguments with set nullability
tested?
Looks like testArgumentTracking could be used for this but it is never called.
Comment at: lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp:475
@@ +474,3 @@
+ QualType RetType = FuncType-getReturnType();
+ if (!RetType-isPointerType() !RetType-isObjCObjectPointerType())
+return;
Use a subroutine.
Comment at: lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp:496
@@ +495,3 @@
+ CheckerContext C) const {
+ auto Decl = M.getDecl();
+ if (!Decl)
The next 11 lines could be factored out. They are the same as in the method
above.
Comment at: lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp:513
@@ +512,3 @@
+ auto Name = Interface ? Interface-getName() : ;
+ // Frameworks related heuristics.
+ if (Name.startswith(NS)) {
Please, explain why we are ignoring these.
EX: Users might know that objectForKey and objectForKeyedSubscript of
NSDictionary always return a non-null value. It's dynamic invariant that the
analyzer cannot infer.
Why are we suppressing all methods on NSArray?
Comment at: lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp:524
@@ +523,3 @@
+
+// Ignore the return value of string encoding methods.
+if (Name.find(String) != StringRef::npos) {
Again, explain why we have this heuristic.
Comment at: lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp:540
@@ +539,3 @@
+
+ const ObjCMessageExpr *Message = M.getOriginExpr();
+ Nullability SelfNullability = Nullability::Unspecified;
Maybe getting nullability of the receiver could be split into a separate
function to simplify readability?
Comment at: lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp:542
@@ +541,3 @@
+ Nullability SelfNullability = Nullability::Unspecified;
+ if (Message-getReceiverKind() == ObjCMessageExpr::SuperClass ||
+ Message-getReceiverKind() == ObjCMessageExpr::SuperInstance) {
+comment
// If the receiver is super, assume it's nonnull.
Comment at: lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp:545
@@ +544,3 @@
+SelfNullability = Nullability::Nonnull;
+ } else {
+SVal Receiver = M.getReceiverSVal();
+comment
// Otherwise, look up the nullability info in the state.
Comment at: lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp:558
@@ +557,3 @@
+}
+if (!Receiver.isUndef()) {
+ ConditionTruthVal Nullness =
+ comment:
// If we know that the receiver is constrained to not be null, assume it's
nullability is Nonnull,
