[PATCH] D54017: [analyzer] NullabilityChecker: Invariant violation should only be triggered for symbols.

[Phabricator via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL348188: Re-apply r347954 [analyzer] Nullability: 
Dont detect post factum violation... (authored by dergachev, 
committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D54017?vs=176029=176461#toc

Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D54017/new/

https://reviews.llvm.org/D54017

Files:
  cfe/trunk/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
  cfe/trunk/test/Analysis/nullability-arc.mm
  cfe/trunk/test/Analysis/nullability.mm

Index: cfe/trunk/test/Analysis/nullability-arc.mm
===
--- cfe/trunk/test/Analysis/nullability-arc.mm
+++ cfe/trunk/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
+
Index: cfe/trunk/test/Analysis/nullability.mm
===
--- cfe/trunk/test/Analysis/nullability.mm
+++ cfe/trunk/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"
 
Index: cfe/trunk/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
===
--- cfe/trunk/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
@@ -329,8 +329,8 @@
  

[PATCH] D54017: [analyzer] NullabilityChecker: Invariant violation should only be triggered for symbols.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

Wasn't me: rC347970 .


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D54017/new/

https://reviews.llvm.org/D54017



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D54017: [analyzer] NullabilityChecker: Invariant violation should only be triggered for symbols.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ reopened this revision.
NoQ added a comment.
This revision is now accepted and ready to land.

Reverted as rC347956  due to 
http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-fast/builds/26658 - 
will have a look tomorrow.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D54017/new/

https://reviews.llvm.org/D54017



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D54017: [analyzer] NullabilityChecker: Invariant violation should only be triggered for symbols.

[Phabricator via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC347954: [analyzer] Nullability: Dont detect post 
factum violation on concrete values. (authored by dergachev, committed by ).

Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D54017/new/

https://reviews.llvm.org/D54017

Files:
  lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
  test/Analysis/nullability-arc.mm
  test/Analysis/nullability.mm

Index: test/Analysis/nullability.mm
===
--- test/Analysis/nullability.mm
+++ 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"
 
Index: test/Analysis/nullability-arc.mm
===
--- test/Analysis/nullability-arc.mm
+++ 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
+
Index: lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
+++ lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
@@ -329,8 +329,8 @@
 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 

[PATCH] D54017: [analyzer] NullabilityChecker: Invariant violation should only be triggered for symbols.

[Umann Kristóf via Phabricator via cfe-commits]

Szelethus added a comment.

I have no other objections, looks great!




Comment at: test/Analysis/nullability.mm:3-4
 // 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 
-analyzer-checker=core,nullability.NullPassedToNonnull,nullability.NullReturnedFromNonnull,nullability.NullablePassedToNonnull,nullability.NullableReturnedFromNonnull,nullability.NullableDereferenced
 -DNOSYSTEMHEADERS=0 -verify %s -fobjc-arc
+// 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 -fobjc-arc
 

NoQ wrote:
> Szelethus wrote:
> > These too. Especially these :D
> These two are in fact a bit annoying because the space after `// RUN: ` is 
> part of the run-line, so you can't break a single long flag into multiple 
> lines without getting involved in an aesthetically unpleasant indentation.
Oh, this looks amazing now. :)


https://reviews.llvm.org/D54017



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D54017: [analyzer] NullabilityChecker: Invariant violation should only be triggered for symbols.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: test/Analysis/nullability.mm:3-4
 // 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 
-analyzer-checker=core,nullability.NullPassedToNonnull,nullability.NullReturnedFromNonnull,nullability.NullablePassedToNonnull,nullability.NullableReturnedFromNonnull,nullability.NullableDereferenced
 -DNOSYSTEMHEADERS=0 -verify %s -fobjc-arc
+// 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 -fobjc-arc
 

Szelethus wrote:
> These too. Especially these :D
These two are in fact a bit annoying because the space after `// RUN: ` is part 
of the run-line, so you can't break a single long flag into multiple lines 
without getting involved in an aesthetically unpleasant indentation.


https://reviews.llvm.org/D54017



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D54017: [analyzer] NullabilityChecker: Invariant violation should only be triggered for symbols.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ updated this revision to Diff 172645.
NoQ marked 3 inline comments as done.
NoQ added a comment.

Fix comments, update comments.

Before i forget - also add invariant violation markers to the exploded graph, 
which helped me a lot with debugging this bug.


https://reviews.llvm.org/D54017

Files:
  lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
  test/Analysis/nullability-arc.mm
  test/Analysis/nullability.mm

Index: test/Analysis/nullability.mm
===
--- test/Analysis/nullability.mm
+++ 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"
 
Index: test/Analysis/nullability-arc.mm
===
--- /dev/null
+++ 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
+
Index: lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
+++ lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
@@ -329,8 +329,8 @@
 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 

[PATCH] D54017: [analyzer] NullabilityChecker: Invariant violation should only be triggered for symbols.

[Umann Kristóf via Phabricator via cfe-commits]

Szelethus added a comment.

Thanks for summarizing the problem so well in the summary! I should start doing 
that too.




Comment at: lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp:343-346
+  // There should have originally been a symbol. If it was a concrete value
+  // to begin with, the violation should have been handled immediately
+  // rather than delayed until constraints are updated. Why "symbol" rather 
than
+  // "region"? Because only symbolic regions can be null.

To me, "should've been handled" sounds like that could be translated to an 
assert.

Also, this doc seems confusing to me, but I might be wrong. You referenced the 
term "symbol" a couple types, but I don't see `SymbolVal` anywhere. Could you 
reword it a bit?



Comment at: test/Analysis/nullability-arc.mm:1-2
+// RUN: %clang_analyze_cc1 -w -analyzer-checker=core,nullability 
-analyzer-output=text -verify %s
+// RUN: %clang_analyze_cc1 -w -analyzer-checker=core,nullability 
-analyzer-output=text -verify %s -fobjc-arc
+

Let's organize this into multiple lines!

```
// RUN: %clang_analyze_cc1 -w -verify %s -fobjc-arc \
// RUN:   -analyzer-checker=core,nullability -analyzer-output=text
```



Comment at: test/Analysis/nullability.mm:3-4
 // 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 
-analyzer-checker=core,nullability.NullPassedToNonnull,nullability.NullReturnedFromNonnull,nullability.NullablePassedToNonnull,nullability.NullableReturnedFromNonnull,nullability.NullableDereferenced
 -DNOSYSTEMHEADERS=0 -verify %s -fobjc-arc
+// 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 -fobjc-arc
 

These too. Especially these :D


Repository:
  rC Clang

https://reviews.llvm.org/D54017



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D54017: [analyzer] NullabilityChecker: Invariant violation should only be triggered for symbols.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

It also seems that we still have a false negative under ARC, but i didn't 
bother investigating it. Just in case, i added `-fobjc-arc` run-lines to older 
tests to see if there's any difference, and it turned out that there's no 
difference.


Repository:
  rC Clang

https://reviews.llvm.org/D54017



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D54017: [analyzer] NullabilityChecker: Invariant violation should only be triggered for symbols.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ created this revision.
NoQ added reviewers: dcoughlin, xazax.hun, george.karpenkov.
Herald added subscribers: cfe-commits, dkrupp, donat.nagy, Szelethus, 
mikhail.ramalho, a.sidorin, rnkovacs, szepet, baloghadamsoftware.

This false negative bug was exposed by https://reviews.llvm.org/D18860. The 
checker tries to detect the situation when a symbol previously passed through a 
`_Nonnull`-annotated function parameter is constrained to null later on the 
path, and suppress its warnings on such paths by setting a state-wide flag 
``. The detection for symbols being constrained to null is 
done, in particular, in `checkDeadSymbols()`, because that's the last moment 
when we see the symbol and therefore we have perfect knowledge of its 
constraints at this moment. The detection works by ascending the stack frame 
chain and seeing if any of the `_Nonnull` parameters has a null value in the 
current program state.

It turned out that such invariant violation detection in checkDeadSymbols() 
races with checkPreCall(). Suppose that we're stuffing a concrete null (rather 
than a symbol constrained to null) into the function. Depending on the result 
of the race, there would be two possible scenarios:

1. `checkPreCall()` fires first. In this case a valid warning will be issued 
that null is being passed through a parameter that is annotated as `_Nonnull`.
2. `checkDeadSymbols()` fires first. In this case it'll notice that a 
`_Nonnull` parameter of the call that we almost entered is equal to null and 
raise the `` flag, suppressing all nullability warnings.

Which means that depending on "something" we either see or don't see a warning.

What is this "something"? Why are callbacks were called in different order? 
Easy: it's because `checkDeadSymbols()` weren't firing at all when there were 
no dead symbols. Now that zombie symbol bugs are fixed, we realize that it's 
always important to call `checkDeadSymbols()`, because we've no idea what 
symbols the checker may track. So after fixing zombie symbols in 
https://reviews.llvm.org/D18860, the race started resolving to scenario 2, 
resulting in more false negatives.

The bug is fixed by restricting the check to work with symbols only, not with 
concrete values. This works because by the time we reach the call, symbolic 
values should be either not constrained to null, or already replaced with 
concrete `0 (Loc)` values. Though generally the code is a bit weird and might 
require more thought.


Repository:
  rC Clang

https://reviews.llvm.org/D54017

Files:
  lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
  test/Analysis/nullability-arc.mm
  test/Analysis/nullability.mm


Index: test/Analysis/nullability.mm
===
--- test/Analysis/nullability.mm
+++ test/Analysis/nullability.mm
@@ -1,5 +1,7 @@
 // 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 
-analyzer-checker=core,nullability.NullPassedToNonnull,nullability.NullReturnedFromNonnull,nullability.NullablePassedToNonnull,nullability.NullableReturnedFromNonnull,nullability.NullableDereferenced
 -DNOSYSTEMHEADERS=0 -verify %s -fobjc-arc
+// 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 -fobjc-arc
 
 #include "Inputs/system-header-simulator-for-nullability.h"
 
Index: test/Analysis/nullability-arc.mm
===
--- /dev/null
+++ test/Analysis/nullability-arc.mm
@@ -0,0 +1,37 @@
+// RUN: %clang_analyze_cc1 -w -analyzer-checker=core,nullability 
-analyzer-output=text -verify %s
+// RUN: %clang_analyze_cc1 -w -analyzer-checker=core,nullability 
-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