[PATCH] D159549: [analyzer] Fix false negative when accessing a nonnull property from a nullable object

[tripleCC via Phabricator via cfe-commits]

tripleCC created this revision.
Herald added subscribers: steakhal, manas, ASDenysPetrov, martong, dkrupp, 
donat.nagy, Szelethus, mikhail.ramalho, a.sidorin, szepet, baloghadamsoftware, 
xazax.hun.
Herald added a reviewer: NoQ.
Herald added a project: All.
tripleCC requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D159549

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
@@ -55,6 +55,7 @@
 @property(readonly, nullable) void (^propReturnsNullableBlock)(void);
 @property(readonly, nullable) int *propReturnsNullable;
 @property(readonly) int *propReturnsUnspecified;
++ (nullable TestObject *)getNullableObject;
 @end
 
 TestObject * getUnspecifiedTestObject();
@@ -256,6 +257,12 @@
   case 8:
 [o takesNonnullBlock:o.propReturnsNullableBlock]; // expected-warning 
{{Nullable pointer is passed to a callee that requires a non-null 1st 
parameter}}
 break;
+  case 9:
+[o takesNonnull:getNullableTestObject().propReturnsNonnull]; // 
expected-warning {{Nullable pointer is passed to a callee that requires a 
non-null 1st parameter}}
+break;
+  case 10:
+[o takesNonnull:[TestObject getNullableObject].propReturnsNonnull]; // 
expected-warning {{Nullable pointer is passed to a callee that requires a 
non-null 1st parameter}}
+break;
   }
 }
 
Index: clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
@@ -900,7 +900,8 @@
   State->get(Region);
 
   if (!TrackedNullability &&
-  getNullabilityAnnotation(ReturnType) == Nullability::Nullable) {
+  getNullabilityAnnotation(Call.getOriginExpr()->getType()) ==
+  Nullability::Nullable) {
 State = State->set(Region, Nullability::Nullable);
 C.addTransition(State);
   }
@@ -1053,7 +1054,7 @@
   }
 
   // No tracked information. Use static type information for return value.
-  Nullability RetNullability = getNullabilityAnnotation(RetType);
+  Nullability RetNullability = getNullabilityAnnotation(Message->getType());
 
   // Properties might be computed, which means the property value could
   // theoretically change between calls even in commonly-observed cases like


Index: clang/test/Analysis/nullability.mm
===
--- clang/test/Analysis/nullability.mm
+++ clang/test/Analysis/nullability.mm
@@ -55,6 +55,7 @@
 @property(readonly, nullable) void (^propReturnsNullableBlock)(void);
 @property(readonly, nullable) int *propReturnsNullable;
 @property(readonly) int *propReturnsUnspecified;
++ (nullable TestObject *)getNullableObject;
 @end
 
 TestObject * getUnspecifiedTestObject();
@@ -256,6 +257,12 @@
   case 8:
 [o takesNonnullBlock:o.propReturnsNullableBlock]; // expected-warning {{Nullable pointer is passed to a callee that requires a non-null 1st parameter}}
 break;
+  case 9:
+[o takesNonnull:getNullableTestObject().propReturnsNonnull]; // expected-warning {{Nullable pointer is passed to a callee that requires a non-null 1st parameter}}
+break;
+  case 10:
+[o takesNonnull:[TestObject getNullableObject].propReturnsNonnull]; // expected-warning {{Nullable pointer is passed to a callee that requires a non-null 1st parameter}}
+break;
   }
 }
 
Index: clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
@@ -900,7 +900,8 @@
   State->get(Region);
 
   if (!TrackedNullability &&
-  getNullabilityAnnotation(ReturnType) == Nullability::Nullable) {
+  getNullabilityAnnotation(Call.getOriginExpr()->getType()) ==
+  Nullability::Nullable) {
 State = State->set(Region, Nullability::Nullable);
 C.addTransition(State);
   }
@@ -1053,7 +1054,7 @@
   }
 
   // No tracked information. Use static type information for return value.
-  Nullability RetNullability = getNullabilityAnnotation(RetType);
+  Nullability RetNullability = getNullabilityAnnotation(Message->getType());
 
   // Properties might be computed, which means the property value could
   // theoretically change between calls even in commonly-observed cases like
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D159549: [analyzer] Fix false negative when accessing a nonnull property from a nullable object

[tripleCC via Phabricator via cfe-commits]

tripleCC updated this revision to Diff 557405.
tripleCC added a comment.

adjust return type after checking getOriginExpr is non-null


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159549

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
@@ -55,6 +55,7 @@
 @property(readonly, nullable) void (^propReturnsNullableBlock)(void);
 @property(readonly, nullable) int *propReturnsNullable;
 @property(readonly) int *propReturnsUnspecified;
++ (nullable TestObject *)getNullableObject;
 @end
 
 TestObject * getUnspecifiedTestObject();
@@ -256,6 +257,12 @@
   case 8:
 [o takesNonnullBlock:o.propReturnsNullableBlock]; // expected-warning 
{{Nullable pointer is passed to a callee that requires a non-null 1st 
parameter}}
 break;
+  case 9:
+[o takesNonnull:getNullableTestObject().propReturnsNonnull]; // 
expected-warning {{Nullable pointer is passed to a callee that requires a 
non-null 1st parameter}}
+break;
+  case 10:
+[o takesNonnull:[TestObject getNullableObject].propReturnsNonnull]; // 
expected-warning {{Nullable pointer is passed to a callee that requires a 
non-null 1st parameter}}
+break;
   }
 }
 
Index: clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
@@ -899,6 +899,9 @@
   const NullabilityState *TrackedNullability =
   State->get(Region);
 
+  if (const Expr *E = Call.getOriginExpr())
+ReturnType = E->getType();
+
   if (!TrackedNullability &&
   getNullabilityAnnotation(ReturnType) == Nullability::Nullable) {
 State = State->set(Region, Nullability::Nullable);
@@ -1053,7 +1056,7 @@
   }
 
   // No tracked information. Use static type information for return value.
-  Nullability RetNullability = getNullabilityAnnotation(RetType);
+  Nullability RetNullability = getNullabilityAnnotation(Message->getType());
 
   // Properties might be computed, which means the property value could
   // theoretically change between calls even in commonly-observed cases like


Index: clang/test/Analysis/nullability.mm
===
--- clang/test/Analysis/nullability.mm
+++ clang/test/Analysis/nullability.mm
@@ -55,6 +55,7 @@
 @property(readonly, nullable) void (^propReturnsNullableBlock)(void);
 @property(readonly, nullable) int *propReturnsNullable;
 @property(readonly) int *propReturnsUnspecified;
++ (nullable TestObject *)getNullableObject;
 @end
 
 TestObject * getUnspecifiedTestObject();
@@ -256,6 +257,12 @@
   case 8:
 [o takesNonnullBlock:o.propReturnsNullableBlock]; // expected-warning {{Nullable pointer is passed to a callee that requires a non-null 1st parameter}}
 break;
+  case 9:
+[o takesNonnull:getNullableTestObject().propReturnsNonnull]; // expected-warning {{Nullable pointer is passed to a callee that requires a non-null 1st parameter}}
+break;
+  case 10:
+[o takesNonnull:[TestObject getNullableObject].propReturnsNonnull]; // expected-warning {{Nullable pointer is passed to a callee that requires a non-null 1st parameter}}
+break;
   }
 }
 
Index: clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
@@ -899,6 +899,9 @@
   const NullabilityState *TrackedNullability =
   State->get(Region);
 
+  if (const Expr *E = Call.getOriginExpr())
+ReturnType = E->getType();
+
   if (!TrackedNullability &&
   getNullabilityAnnotation(ReturnType) == Nullability::Nullable) {
 State = State->set(Region, Nullability::Nullable);
@@ -1053,7 +1056,7 @@
   }
 
   // No tracked information. Use static type information for return value.
-  Nullability RetNullability = getNullabilityAnnotation(RetType);
+  Nullability RetNullability = getNullabilityAnnotation(Message->getType());
 
   // Properties might be computed, which means the property value could
   // theoretically change between calls even in commonly-observed cases like
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D159549: [analyzer] Fix false negative when accessing a nonnull property from a nullable object

[tripleCC via Phabricator via cfe-commits]

tripleCC added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp:903-904
   if (!TrackedNullability &&
-  getNullabilityAnnotation(ReturnType) == Nullability::Nullable) {
+  getNullabilityAnnotation(Call.getOriginExpr()->getType()) ==
+  Nullability::Nullable) {
 State = State->set(Region, Nullability::Nullable);

steakhal wrote:
> I'm not sure if `getOriginExpr` is always non-null. How do you know?
> I'm not sure if `getOriginExpr` is always non-null. How do you know?
The getOriginExpr my be null ,  I optimized this logic to only perform the 
operation if getOriginExpr is not null.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159549

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

[PATCH] D159549: [analyzer] Fix false negative when accessing a nonnull property from a nullable object

[tripleCC via Phabricator via cfe-commits]

tripleCC added a comment.

In D159549#4651290 , @steakhal wrote:

> For new revisions, prefer creating a PR at GitHub.

ok,

In D159549#4651290 , @steakhal wrote:

> For new revisions, prefer creating a PR at GitHub.

ok, I will create a github pull request for the patch


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159549

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

[PATCH] D159549: [analyzer] Fix false negative when accessing a nonnull property from a nullable object

[tripleCC via Phabricator via cfe-commits]

tripleCC added a comment.

merged in https://github.com/llvm/llvm-project/pull/67563#event-10544755217


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159549

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

[PATCH] D154221: [analyzer] Fix false negative when pass implicit cast nil to nonnull

[tripleCC via Phabricator via cfe-commits]

tripleCC added a comment.

I don't have commit access, would you mind committing the revision for me ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154221

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

[PATCH] D153017: [analyzer] Fix false negative when using a nullable parameter directly without binding to a variable

[tripleCC via Phabricator via cfe-commits]

tripleCC marked an inline comment as done.
tripleCC added a comment.

In D153017#4428432 , @steakhal wrote:

> LGTM; I'll commit this on Monday.

Hi steakhal again, sorry to bother you. Do you mind committing the revision for 
me ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153017

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

[PATCH] D153017: [analyzer] Fix false negative when using a nullable parameter directly without binding to a variable

[tripleCC via Phabricator via cfe-commits]

tripleCC added a comment.

In D153017#4428432 , @steakhal wrote:

> LGTM; I'll commit this on Monday.

Oops, It seems that I missed the invitation, I just noticed the invitation 
email in my gmail now ...
I will request Chris to send the invitation again.
Thank you for your recommendation.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153017

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

[PATCH] D154221: [analyzer] Fix false negative when pass implicit cast nil to nonnull

[tripleCC via Phabricator via cfe-commits]

tripleCC updated this revision to Diff 537110.
tripleCC added a comment.

try to fix nullability-arc.mm test case error


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154221

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


Index: clang/test/Analysis/nullability.mm
===
--- clang/test/Analysis/nullability.mm
+++ clang/test/Analysis/nullability.mm
@@ -69,6 +69,7 @@
 void takesNonnull(Dummy *_Nonnull);
 void takesUnspecified(Dummy *);
 void takesNonnullBlock(void (^ _Nonnull)(void));
+void takesNonnullObject(NSObject *_Nonnull);
 
 Dummy *_Nullable returnsNullable();
 Dummy *_Nonnull returnsNonnull();
@@ -264,6 +265,17 @@
   return (Dummy * _Nonnull)0; // no-warning
 }
 
+void testImplicitCastNilToNonnull() {
+  id obj = nil;
+  takesNonnullObject(obj); // expected-warning {{nil passed to a callee that 
requires a non-null 1st parameter}}
+}
+
+void testImplicitCastNullableArgToNonnull(TestObject *_Nullable obj) {
+  if (!obj) {
+takesNonnullObject(obj); // expected-warning {{nil passed to a callee that 
requires a non-null 1st parameter}}
+  }
+}
+
 void testIndirectCastNilToNonnullAndPass() {
   Dummy *p = (Dummy * _Nonnull)0;
   // FIXME: Ideally the cast above would suppress this warning.
Index: clang/test/Analysis/nullability-arc.mm
===
--- clang/test/Analysis/nullability-arc.mm
+++ clang/test/Analysis/nullability-arc.mm
@@ -3,9 +3,6 @@
 // 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)
@@ -24,16 +21,10 @@
 - (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
+  // expected-warning@-1{{nil passed to a callee that requires a non-null 1st 
parameter}}
+  // expected-note@-2   {{nil passed to a callee that requires a non-null 1st 
parameter}}
 }
 @end
 
Index: clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
@@ -767,7 +767,7 @@
 Nullability RequiredNullability =
 getNullabilityAnnotation(Param->getType());
 Nullability ArgExprTypeLevelNullability =
-getNullabilityAnnotation(ArgExpr->getType());
+getNullabilityAnnotation(lookThroughImplicitCasts(ArgExpr)->getType());
 
 unsigned ParamIdx = Param->getFunctionScopeIndex() + 1;
 


Index: clang/test/Analysis/nullability.mm
===
--- clang/test/Analysis/nullability.mm
+++ clang/test/Analysis/nullability.mm
@@ -69,6 +69,7 @@
 void takesNonnull(Dummy *_Nonnull);
 void takesUnspecified(Dummy *);
 void takesNonnullBlock(void (^ _Nonnull)(void));
+void takesNonnullObject(NSObject *_Nonnull);
 
 Dummy *_Nullable returnsNullable();
 Dummy *_Nonnull returnsNonnull();
@@ -264,6 +265,17 @@
   return (Dummy * _Nonnull)0; // no-warning
 }
 
+void testImplicitCastNilToNonnull() {
+  id obj = nil;
+  takesNonnullObject(obj); // expected-warning {{nil passed to a callee that requires a non-null 1st parameter}}
+}
+
+void testImplicitCastNullableArgToNonnull(TestObject *_Nullable obj) {
+  if (!obj) {
+takesNonnullObject(obj); // expected-warning {{nil passed to a callee that requires a non-null 1st parameter}}
+  }
+}
+
 void testIndirectCastNilToNonnullAndPass() {
   Dummy *p = (Dummy * _Nonnull)0;
   // FIXME: Ideally the cast above would suppress this warning.
Index: clang/test/Analysis/nullability-arc.mm
===
--- clang/test/Analysis/nullability-arc.mm
+++ clang/test/Analysis/nullability-arc.mm
@@ -3,9 +3,6 @@
 // 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)
@@ -24,16 +21,10 @@
 - (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

[PATCH] D154221: [analyzer] Fix false negative when pass implicit cast nil to nonnull

[tripleCC via Phabricator via cfe-commits]

tripleCC added inline comments.



Comment at: clang/test/Analysis/nullability-arc.mm:26
   [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
+  // expected-warning@-1{{nil passed to a callee that requires a non-null 1st 
parameter}}
+  // expected-note@-2   {{nil passed to a callee that requires a non-null 1st 
parameter}}

I'm not sure if this change fixes the issue in [[ 
https://reviews.llvm.org/D54017?id=172274#inline-477102 | D54017 ]] , and may 
require some further discussion


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154221

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

[PATCH] D154221: [analyzer] Fix false negative when pass implicit cast nil to nonnull

[tripleCC via Phabricator via cfe-commits]

tripleCC added inline comments.



Comment at: clang/test/Analysis/nullability-arc.mm:25
 
   [self foo:nil];
+  // expected-warning@-1{{nil passed to a callee that requires a non-null 1st 
parameter}}

I think there should be a warning when we call the foo: method


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154221

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

[PATCH] D154221: [analyzer] Fix false negative when pass implicit cast nil to nonnull

[tripleCC via Phabricator via cfe-commits]

tripleCC added a comment.

In D154221#4472112 , @steakhal wrote:

> @tripleCC Could you clarify the status. Do you need some help or review? Do 
> you have concerns?
> You made some inline comments that I couldn't put into context.

@steakhal  Yes, I need some review, especially concerning the modifications 
made to file nullability-arc.mm.

If @NoQ could join in the review, that would be even better , as he created 
this file in D54017 .


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154221

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

[PATCH] D154221: Fix false negative when implicit cast nil to nonnull

[tripleCC via Phabricator via cfe-commits]

tripleCC created this revision.
Herald added subscribers: steakhal, martong.
Herald added a reviewer: NoQ.
Herald added a project: All.
tripleCC requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D154221

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
@@ -69,6 +69,7 @@
 void takesNonnull(Dummy *_Nonnull);
 void takesUnspecified(Dummy *);
 void takesNonnullBlock(void (^ _Nonnull)(void));
+void takesNonnullObject(TestObject *_Nonnull);
 
 Dummy *_Nullable returnsNullable();
 Dummy *_Nonnull returnsNonnull();
@@ -264,6 +265,11 @@
   return (Dummy * _Nonnull)0; // no-warning
 }
 
+void testImplicitCastNilToNonnull() {
+  id obj = nil;
+  takesNonnullObject(obj); // expected-warning {{nil passed to a callee that 
requires a non-null 1st parameter}}
+}
+
 void testIndirectCastNilToNonnullAndPass() {
   Dummy *p = (Dummy * _Nonnull)0;
   // FIXME: Ideally the cast above would suppress this warning.
Index: clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
@@ -767,7 +767,7 @@
 Nullability RequiredNullability =
 getNullabilityAnnotation(Param->getType());
 Nullability ArgExprTypeLevelNullability =
-getNullabilityAnnotation(ArgExpr->getType());
+getNullabilityAnnotation(lookThroughImplicitCasts(ArgExpr)->getType());
 
 unsigned ParamIdx = Param->getFunctionScopeIndex() + 1;
 


Index: clang/test/Analysis/nullability.mm
===
--- clang/test/Analysis/nullability.mm
+++ clang/test/Analysis/nullability.mm
@@ -69,6 +69,7 @@
 void takesNonnull(Dummy *_Nonnull);
 void takesUnspecified(Dummy *);
 void takesNonnullBlock(void (^ _Nonnull)(void));
+void takesNonnullObject(TestObject *_Nonnull);
 
 Dummy *_Nullable returnsNullable();
 Dummy *_Nonnull returnsNonnull();
@@ -264,6 +265,11 @@
   return (Dummy * _Nonnull)0; // no-warning
 }
 
+void testImplicitCastNilToNonnull() {
+  id obj = nil;
+  takesNonnullObject(obj); // expected-warning {{nil passed to a callee that requires a non-null 1st parameter}}
+}
+
 void testIndirectCastNilToNonnullAndPass() {
   Dummy *p = (Dummy * _Nonnull)0;
   // FIXME: Ideally the cast above would suppress this warning.
Index: clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
@@ -767,7 +767,7 @@
 Nullability RequiredNullability =
 getNullabilityAnnotation(Param->getType());
 Nullability ArgExprTypeLevelNullability =
-getNullabilityAnnotation(ArgExpr->getType());
+getNullabilityAnnotation(lookThroughImplicitCasts(ArgExpr)->getType());
 
 unsigned ParamIdx = Param->getFunctionScopeIndex() + 1;
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D151651: [StaticAnalyzer] Fix block pointer type nullability check

[tripleCC via Phabricator via cfe-commits]

tripleCC created this revision.
Herald added subscribers: steakhal, manas, ASDenysPetrov, martong, dkrupp, 
donat.nagy, Szelethus, a.sidorin, szepet, baloghadamsoftware.
Herald added a reviewer: NoQ.
Herald added a project: All.
tripleCC requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D151651

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
@@ -46,10 +46,13 @@
 - (int *_Nullable)returnsNullable;
 - (int *)returnsUnspecified;
 - (void)takesNonnull:(int *_Nonnull)p;
+- (void)takesNonnullBlock:(void (^ _Nonnull)(void))block;
 - (void)takesNullable:(int *_Nullable)p;
 - (void)takesUnspecified:(int *)p;
 @property(readonly, strong) NSString *stuff;
 @property(readonly, nonnull) int *propReturnsNonnull;
+@property(readonly, nonnull) void (^propReturnsNonnullBlock)(void);
+@property(readonly, nullable) void (^propReturnsNullableBlock)(void);
 @property(readonly, nullable) int *propReturnsNullable;
 @property(readonly) int *propReturnsUnspecified;
 @end
@@ -65,6 +68,7 @@
 void takesNullable(Dummy *_Nullable);
 void takesNonnull(Dummy *_Nonnull);
 void takesUnspecified(Dummy *);
+void takesNonnullBlock(void (^ _Nonnull)(void));
 
 Dummy *_Nullable returnsNullable();
 Dummy *_Nonnull returnsNonnull();
@@ -197,6 +201,7 @@
   switch (getRandom()) {
   case 0:
 [o takesNonnull:o.propReturnsNonnull]; // no-warning
+[o takesNonnullBlock:o.propReturnsNonnullBlock]; // no-warning
 break;
   case 1:
 [o takesNonnull:o.propReturnsUnspecified]; // no-warning
@@ -236,6 +241,9 @@
 assert(o.propReturnsNullable);
 [o takesNonnull:o.propReturnsNullable]; // no-warning
 break;
+  case 8:
+[o takesNonnullBlock:o.propReturnsNullableBlock]; // expected-warning {{Nullable pointer is passed to a callee that requires a non-null 1st parameter}}
+break;
   }
 }
 
@@ -308,6 +316,11 @@
   takesNonnull(p);  // expected-warning {{Null passed to a callee that requires a non-null 1st parameter}}
 }
 
+void testBlockIndirectNilPassToNonnull() {
+  void (^p)(void) = nil;
+  takesNonnullBlock(p);  // expected-warning {{Null passed to a callee that requires a non-null 1st parameter}}
+}
+
 void testConditionalNilPassToNonnull(Dummy *p) {
   if (!p) {
 takesNonnull(p);  // expected-warning {{Null passed to a callee that requires a non-null 1st parameter}}
Index: clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
@@ -306,6 +306,10 @@
   return NullConstraint::Unknown;
 }
 
+static bool isValidPointerType(QualType T) {
+  return T->isAnyPointerType() || T->isBlockPointerType();
+}
+
 const SymbolicRegion *
 NullabilityChecker::getTrackRegion(SVal Val, bool CheckSuperRegion) const {
   if (!NeedTracking)
@@ -621,7 +625,7 @@
   if (!RetExpr)
 return;
 
-  if (!RetExpr->getType()->isAnyPointerType())
+  if (!isValidPointerType(RetExpr->getType()))
 return;
 
   ProgramStateRef State = C.getState();
@@ -754,7 +758,7 @@
 if (!ArgSVal)
   continue;
 
-if (!Param->getType()->isAnyPointerType() &&
+if (!isValidPointerType(Param->getType()) &&
 !Param->getType()->isReferenceType())
   continue;
 
@@ -841,7 +845,7 @@
   if (!FuncType)
 return;
   QualType ReturnType = FuncType->getReturnType();
-  if (!ReturnType->isAnyPointerType())
+  if (!isValidPointerType(ReturnType))
 return;
   ProgramStateRef State = C.getState();
   if (State->get())
@@ -935,7 +939,7 @@
   if (!Decl)
 return;
   QualType RetType = Decl->getReturnType();
-  if (!RetType->isAnyPointerType())
+  if (!isValidPointerType(RetType))
 return;
 
   ProgramStateRef State = C.getState();
@@ -1089,9 +1093,9 @@
CheckerContext &C) const {
   QualType OriginType = CE->getSubExpr()->getType();
   QualType DestType = CE->getType();
-  if (!OriginType->isAnyPointerType())
+  if (!isValidPointerType(OriginType))
 return;
-  if (!DestType->isAnyPointerType())
+  if (!isValidPointerType(DestType))
 return;
 
   ProgramStateRef State = C.getState();
@@ -1215,7 +1219,7 @@
 return;
 
   QualType LocType = TVR->getValueType();
-  if (!LocType->isAnyPointerType())
+  if (!isValidPointerType(LocType))
 return;
 
   ProgramStateRef State = C.getState();
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D151651: [StaticAnalyzer] Fix block pointer type nullability check

[tripleCC via Phabricator via cfe-commits]

tripleCC added a comment.

In D151651#4379144 , @steakhal wrote:

> Looks good.
> That pesky ObjC :D

I don't have commit access, could you help me to commit it? thanks
--author "tripleCC "


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151651

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

[PATCH] D152269: [StaticAnalyzer] Fix false negtive on NilArgChecker when creating literal object

[tripleCC via Phabricator via cfe-commits]

tripleCC created this revision.
Herald added subscribers: steakhal, manas, ASDenysPetrov, martong, dkrupp, 
donat.nagy, Szelethus, a.sidorin, szepet, baloghadamsoftware.
Herald added a reviewer: NoQ.
Herald added a project: All.
tripleCC requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D152269

Files:
  clang/lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp
  clang/test/Analysis/NSContainers.m


Index: clang/test/Analysis/NSContainers.m
===
--- clang/test/Analysis/NSContainers.m
+++ clang/test/Analysis/NSContainers.m
@@ -1,4 +1,4 @@
-// RUN: %clang_analyze_cc1  -Wno-objc-literal-conversion 
-analyzer-checker=core,osx.cocoa.NonNilReturnValue,osx.cocoa.NilArg,osx.cocoa.Loops,debug.ExprInspection
 -verify -Wno-objc-root-class %s
+// RUN: %clang_analyze_cc1 -Wno-objc-literal-conversion 
-analyzer-checker=core,osx.cocoa.NonNilReturnValue,osx.cocoa.NilArg,osx.cocoa.Loops,debug.ExprInspection,nullability
 -verify -Wno-objc-root-class %s
 
 void clang_analyzer_eval(int);
 
@@ -323,3 +323,16 @@
   // that 'obj' can be nil in this context.
   dict[obj] = getStringFromString(obj); // no-warning
 }
+
+Foo * _Nonnull getNonnullFoo();
+Foo * _Nullable getNullableFoo();
+
+void testCreateDictionaryLiteralWithNullableArg() {
+  (void)@{@"abc" : getNonnullFoo()}; // no warning
+  (void)@{@"abc" : getNullableFoo()}; // expected-warning {{Nullable pointer 
is passed to a callee that requires a non-null}}
+}
+
+void testCreateArrayLiteralWithNullableArg() {
+  (void)@[getNonnullFoo()]; // no warning
+  (void)@[getNullableFoo()]; // expected-warning {{Nullable pointer is passed 
to a callee that requires a non-null}}
+}
Index: clang/lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp
+++ clang/lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp
@@ -97,7 +97,8 @@
 namespace {
   class NilArgChecker : public Checker,
-   check::PostStmt > {
+   check::PostStmt,
+   
EventDispatcher> {
 mutable std::unique_ptr BT;
 
 mutable llvm::SmallDenseMap StringSelectors;
@@ -141,11 +142,31 @@
   CheckerContext &C) const {
   ProgramStateRef State = C.getState();
   if (State->isNull(C.getSVal(E)).isConstrainedTrue()) {
-
 if (ExplodedNode *N = C.generateErrorNode()) {
   generateBugReport(N, Msg, E->getSourceRange(), E, C);
+  return;
 }
   }
+
+  auto ArgSVal = C.getSVal(E).getAs();
+  if (!ArgSVal)
+return;
+
+  ProgramStateRef StNonNull, StNull;
+  std::tie(StNonNull, StNull) = State->assume(*ArgSVal);
+
+  if (StNull) {
+if (ExplodedNode *N = C.generateSink(StNull, C.getPredecessor())) {
+  ImplicitNullDerefEvent event = {*ArgSVal, false, N, &C.getBugReporter(),
+  /*IsDirectDereference=*/false};
+  dispatchEvent(event);
+}
+  }
+
+  if (StNonNull)
+State = StNonNull;
+
+  C.addTransition(State);
 }
 
 void NilArgChecker::warnIfNilArg(CheckerContext &C,


Index: clang/test/Analysis/NSContainers.m
===
--- clang/test/Analysis/NSContainers.m
+++ clang/test/Analysis/NSContainers.m
@@ -1,4 +1,4 @@
-// RUN: %clang_analyze_cc1  -Wno-objc-literal-conversion -analyzer-checker=core,osx.cocoa.NonNilReturnValue,osx.cocoa.NilArg,osx.cocoa.Loops,debug.ExprInspection -verify -Wno-objc-root-class %s
+// RUN: %clang_analyze_cc1 -Wno-objc-literal-conversion -analyzer-checker=core,osx.cocoa.NonNilReturnValue,osx.cocoa.NilArg,osx.cocoa.Loops,debug.ExprInspection,nullability -verify -Wno-objc-root-class %s
 
 void clang_analyzer_eval(int);
 
@@ -323,3 +323,16 @@
   // that 'obj' can be nil in this context.
   dict[obj] = getStringFromString(obj); // no-warning
 }
+
+Foo * _Nonnull getNonnullFoo();
+Foo * _Nullable getNullableFoo();
+
+void testCreateDictionaryLiteralWithNullableArg() {
+  (void)@{@"abc" : getNonnullFoo()}; // no warning
+  (void)@{@"abc" : getNullableFoo()}; // expected-warning {{Nullable pointer is passed to a callee that requires a non-null}}
+}
+
+void testCreateArrayLiteralWithNullableArg() {
+  (void)@[getNonnullFoo()]; // no warning
+  (void)@[getNullableFoo()]; // expected-warning {{Nullable pointer is passed to a callee that requires a non-null}}
+}
Index: clang/lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp
+++ clang/lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp
@@ -97,7 +97,8 @@
 namespace {
   class NilArgChecker : public Checker,
-  

[PATCH] D152269: [StaticAnalyzer] Fix false negative on NilArgChecker when creating literal object

[tripleCC via Phabricator via cfe-commits]

tripleCC updated this revision to Diff 529116.
tripleCC added a comment.

[StaticAnalyzer] add might be null test case for NilArgChecker


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152269

Files:
  clang/lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp
  clang/test/Analysis/NSContainers.m


Index: clang/test/Analysis/NSContainers.m
===
--- clang/test/Analysis/NSContainers.m
+++ clang/test/Analysis/NSContainers.m
@@ -1,4 +1,4 @@
-// RUN: %clang_analyze_cc1  -Wno-objc-literal-conversion 
-analyzer-checker=core,osx.cocoa.NonNilReturnValue,osx.cocoa.NilArg,osx.cocoa.Loops,debug.ExprInspection
 -verify -Wno-objc-root-class %s
+// RUN: %clang_analyze_cc1 -Wno-objc-literal-conversion 
-analyzer-checker=core,osx.cocoa.NonNilReturnValue,osx.cocoa.NilArg,osx.cocoa.Loops,debug.ExprInspection,nullability
 -verify -Wno-objc-root-class %s
 
 void clang_analyzer_eval(int);
 
@@ -323,3 +323,19 @@
   // that 'obj' can be nil in this context.
   dict[obj] = getStringFromString(obj); // no-warning
 }
+
+Foo * _Nonnull getNonnullFoo();
+Foo * _Nullable getNullableFoo();
+Foo * getMightBeNullFoo();
+
+void testCreateDictionaryLiteralWithNullableArg() {
+  (void)@{@"abc" : getMightBeNullFoo()}; // no warning
+  (void)@{@"abc" : getNonnullFoo()}; // no warning
+  (void)@{@"abc" : getNullableFoo()}; // expected-warning {{Nullable pointer 
is passed to a callee that requires a non-null}}
+}
+
+void testCreateArrayLiteralWithNullableArg() {
+  (void)@[getMightBeNullFoo()]; // no warning
+  (void)@[getNonnullFoo()]; // no warning
+  (void)@[getNullableFoo()]; // expected-warning {{Nullable pointer is passed 
to a callee that requires a non-null}}
+}
Index: clang/lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp
+++ clang/lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp
@@ -97,7 +97,8 @@
 namespace {
   class NilArgChecker : public Checker,
-   check::PostStmt > {
+   check::PostStmt,
+   
EventDispatcher> {
 mutable std::unique_ptr BT;
 
 mutable llvm::SmallDenseMap StringSelectors;
@@ -141,11 +142,31 @@
   CheckerContext &C) const {
   ProgramStateRef State = C.getState();
   if (State->isNull(C.getSVal(E)).isConstrainedTrue()) {
-
 if (ExplodedNode *N = C.generateErrorNode()) {
   generateBugReport(N, Msg, E->getSourceRange(), E, C);
+  return;
 }
   }
+
+  auto ArgSVal = C.getSVal(E).getAs();
+  if (!ArgSVal)
+return;
+
+  ProgramStateRef StNonNull, StNull;
+  std::tie(StNonNull, StNull) = State->assume(*ArgSVal);
+
+  if (StNull) {
+if (ExplodedNode *N = C.generateSink(StNull, C.getPredecessor())) {
+  ImplicitNullDerefEvent event = {*ArgSVal, false, N, &C.getBugReporter(),
+  /*IsDirectDereference=*/false};
+  dispatchEvent(event);
+}
+  }
+
+  if (StNonNull)
+State = StNonNull;
+
+  C.addTransition(State);
 }
 
 void NilArgChecker::warnIfNilArg(CheckerContext &C,


Index: clang/test/Analysis/NSContainers.m
===
--- clang/test/Analysis/NSContainers.m
+++ clang/test/Analysis/NSContainers.m
@@ -1,4 +1,4 @@
-// RUN: %clang_analyze_cc1  -Wno-objc-literal-conversion -analyzer-checker=core,osx.cocoa.NonNilReturnValue,osx.cocoa.NilArg,osx.cocoa.Loops,debug.ExprInspection -verify -Wno-objc-root-class %s
+// RUN: %clang_analyze_cc1 -Wno-objc-literal-conversion -analyzer-checker=core,osx.cocoa.NonNilReturnValue,osx.cocoa.NilArg,osx.cocoa.Loops,debug.ExprInspection,nullability -verify -Wno-objc-root-class %s
 
 void clang_analyzer_eval(int);
 
@@ -323,3 +323,19 @@
   // that 'obj' can be nil in this context.
   dict[obj] = getStringFromString(obj); // no-warning
 }
+
+Foo * _Nonnull getNonnullFoo();
+Foo * _Nullable getNullableFoo();
+Foo * getMightBeNullFoo();
+
+void testCreateDictionaryLiteralWithNullableArg() {
+  (void)@{@"abc" : getMightBeNullFoo()}; // no warning
+  (void)@{@"abc" : getNonnullFoo()}; // no warning
+  (void)@{@"abc" : getNullableFoo()}; // expected-warning {{Nullable pointer is passed to a callee that requires a non-null}}
+}
+
+void testCreateArrayLiteralWithNullableArg() {
+  (void)@[getMightBeNullFoo()]; // no warning
+  (void)@[getNonnullFoo()]; // no warning
+  (void)@[getNullableFoo()]; // expected-warning {{Nullable pointer is passed to a callee that requires a non-null}}
+}
Index: clang/lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp
+++ clang/lib/StaticAnalyzer/Checkers/Ba

[PATCH] D152269: [StaticAnalyzer] Fix false negative on NilArgChecker when creating literal object

[tripleCC via Phabricator via cfe-commits]

tripleCC added inline comments.



Comment at: clang/test/Analysis/NSContainers.m:336-337
+void testCreateArrayLiteralWithNullableArg() {
+  (void)@[getNonnullFoo()]; // no warning
+  (void)@[getNullableFoo()]; // expected-warning {{Nullable pointer is passed 
to a callee that requires a non-null}}
+}

steakhal wrote:
> How about the case when it calls a `Foo * getMightBeNullFoo();`? I guess, it 
> would still raise an issue, even though we couldn't prove that it must be 
> null.
I have added the `Foo * getMightBeNullFoo();` test case. It would not raise an 
issue actually because without tracked nullability, the NullabilityChecker does 
not produce warnings. You can check the logic in the  checkEvent function for 
this part


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152269

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

[PATCH] D152269: [StaticAnalyzer] Fix false negative on NilArgChecker when creating literal object

[tripleCC via Phabricator via cfe-commits]

tripleCC added inline comments.



Comment at: 
clang/lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp:151-164
+  auto ArgSVal = C.getSVal(E).getAs();
+  if (!ArgSVal)
+return;
+
+  ProgramStateRef StNonNull, StNull;
+  std::tie(StNonNull, StNull) = State->assume(*ArgSVal);
+

steakhal wrote:
> steakhal wrote:
> > This means that we would raise an issue if `ArgSVal` might be null. We 
> > usually warn if we are sure there is a bug, aka. if it must be null. 
> > Consequently, the condition should be rather `StNull && !StNonNull` instead 
> > of just `StNull`.
> Ah I see now. My bad, thats the whole point of this :D
Not exactly, this meas that if ArgSVal might be null, we dispatch an "implicit" 
null event to NullabilityChecker. NullabilityChecker would emit a warning if 
the pointer is nullable  in `checkEvent` function:

```c++
/// This callback triggers when a pointer is dereferenced and the analyzer does
/// not know anything about the value of that pointer. When that pointer is
/// nullable, this code emits a warning.
void NullabilityChecker::checkEvent(ImplicitNullDerefEvent Event) const {
  if (Event.SinkNode->getState()->get())
return;

  const MemRegion *Region =
  getTrackRegion(Event.Location, /*CheckSuperRegion=*/true);
  if (!Region)
return;

  ProgramStateRef State = Event.SinkNode->getState();
  const NullabilityState *TrackedNullability =
  State->get(Region);

  if (!TrackedNullability)
return;

  if (ChecksEnabled[CK_NullableDereferenced] &&
  TrackedNullability->getValue() == Nullability::Nullable) {
BugReporter &BR = *Event.BR;
// Do not suppress errors on defensive code paths, because dereferencing
// a nullable pointer is always an error.
if (Event.IsDirectDereference)
  reportBug("Nullable pointer is dereferenced",
ErrorKind::NullableDereferenced, CK_NullableDereferenced,
Event.SinkNode, Region, BR);
else {
  reportBug("Nullable pointer is passed to a callee that requires a "
"non-null",
ErrorKind::NullablePassedToNonnull, CK_NullableDereferenced,
Event.SinkNode, Region, BR);
}
  }
}
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152269

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

[PATCH] D152269: [StaticAnalyzer] Fix false negative on NilArgChecker when creating literal object

[tripleCC via Phabricator via cfe-commits]

tripleCC added a comment.

In D152269#4402593 , @steakhal wrote:

> I had some improvement opportunities in mind scattered, so I decided to do 
> them, and here is how the diff looks for me now: F27853795: 
> recommendation.patch 
>
> Basically, I reworked the two branches a bit to express the intent more 
> strait forward.
> I also enable all `osx.cocoa` checkers, as this file is all about objc stuff 
> anyway - this also meant to diagnose two non-fatal leak errors, which are not 
> tied to this patch otherwise.
>
> I'm also testing that the "assumption" after the objc call thingie (message 
> passing?) the pointer is assumed to be "nonnull". For this, I'm using the 
> `eagerly-assume=false` to ensure we don't split null and non-null states 
> eagerly when we encounter the `p == nil` inside `clang_analyzer_eval()` call 
> argument.
>
> I think all my changes are aligned with your intent, right?
>
> One more thing I was thinking of was to make this ObjC null checking checker 
> depend on the "nullability" checker package, but it turns out we can only 
> depend on individual checkers as of now. I tried some ideas there, e.g. 
> depending on the base checker of that package but it didn't work, so I 
> dropped the idea. (`clang/include/clang/StaticAnalyzer/Checkers/Checkers.td`)
>
> Do you plan to contribute more ObjC-related fixes in the future?
> Please note that I have absolutely no experience with ObjC stuff.
>
> And I also wanted to thank you for the high-quality patches you submitted so 
> far!

Yes, your changes are aligned with my intent. It seems like you have made 
excellent optimizations to this patch. 
To eliminate the following two warnings, I add the `-fobjc-arc` compilation 
option for NSContainers test .

  +  MyView *view = b ? [[MyView alloc] init] : 0; // expected-warning 
{{Potential leak of an object stored into 'view'}}
  +  NSMutableArray *subviews = [[view subviews] mutableCopy]; // 
expected-warning {{Potential leak of an object stored into 'subviews'}}

I will continue to contribute more ObjC-related issue fixes in the future, and 
currently, my work is also related to this. 
Thank you very much for your review. Do you mind if I merge your 
recommendations?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152269

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

[PATCH] D152269: [StaticAnalyzer] Fix false negative on NilArgChecker when creating literal object

[tripleCC via Phabricator via cfe-commits]

tripleCC added a comment.

In D152269#4403404 , @steakhal wrote:

> In D152269#4403282 , @tripleCC 
> wrote:
>
>> Yes, your changes are aligned with my intent. It seems like you have made 
>> excellent optimizations to this patch. 
>> To eliminate the following two warnings, I add the `-fobjc-arc`(enable 
>> Automatic Reference Counting) compilation option for NSContainers test .
>>
>>   +  MyView *view = b ? [[MyView alloc] init] : 0; // expected-warning 
>> {{Potential leak of an object stored into 'view'}}
>>   +  NSMutableArray *subviews = [[view subviews] mutableCopy]; // 
>> expected-warning {{Potential leak of an object stored into 'subviews'}}
>
> I think we shouldn't add the `-fobj-arc` as these two new issues are 
> considered TPs, and meaningful for the test file they are part of. It's just 
> that they are not that meaningful in the scope of this patch, but that 
> shouldn't hold us back from improving what the test covers and demonstrates, 
> so I'm fine if those two appear as part of this patch.
>
>> I will continue to contribute more ObjC-related fixes in the future, and 
>> currently, my work is also related to this.
>
> Sounds good. Thanks for clarifying.
> If that's the case, after a few more quality patches I think it would make 
> sense to request you commit access. Let's keep this in mind now.
>
>> Thank you very much for your review. Would you mind if I merge your 
>> recommendations?
>
> I don't mind. You don't need to give attribution. Keep parts of the whole as 
> you wish.

Thanks.

Additionally , there are already other unit tests focus on leak scenarios, such 
as the retain-release*.m test cases. Should we focus on testing container 
issues here? Currently, Objective-C code almost always uses ARC (Automatic 
Reference Counting).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152269

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

[PATCH] D152269: [StaticAnalyzer] Fix false negative on NilArgChecker when creating literal object

[tripleCC via Phabricator via cfe-commits]

tripleCC updated this revision to Diff 529583.
tripleCC added a comment.

taking steakhal's advice


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152269

Files:
  clang/lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp
  clang/test/Analysis/NSContainers.m

Index: clang/test/Analysis/NSContainers.m
===
--- clang/test/Analysis/NSContainers.m
+++ clang/test/Analysis/NSContainers.m
@@ -1,4 +1,7 @@
-// RUN: %clang_analyze_cc1  -Wno-objc-literal-conversion -analyzer-checker=core,osx.cocoa.NonNilReturnValue,osx.cocoa.NilArg,osx.cocoa.Loops,debug.ExprInspection -verify -Wno-objc-root-class %s
+// RUN: %clang_analyze_cc1 -Wno-objc-literal-conversion -Wno-objc-root-class -fobjc-arc \
+// RUN:   -analyzer-checker=core,osx.cocoa,nullability \
+// RUN:   -analyzer-config eagerly-assume=false \
+// RUN:   -analyzer-checker=debug.ExprInspection -verify %s
 
 void clang_analyzer_eval(int);
 
@@ -31,13 +34,13 @@
 @end
 
 typedef struct {
-  unsigned long state;
-  id *itemsPtr;
-  unsigned long *mutationsPtr;
-  unsigned long extra[5];
+unsigned long state;
+id __unsafe_unretained _Nullable * _Nullable itemsPtr;
+unsigned long * _Nullable mutationsPtr;
+unsigned long extra[5];
 } NSFastEnumerationState;
 @protocol NSFastEnumeration
-- (NSUInteger)countByEnumeratingWithState:(NSFastEnumerationState *)state objects:(id [])buffer count:(NSUInteger)len;
+- (NSUInteger)countByEnumeratingWithState:(NSFastEnumerationState *)state objects:(id __unsafe_unretained _Nullable [_Nonnull])buffer count:(NSUInteger)len;
 @end
 
 @interface NSArray : NSObject 
@@ -323,3 +326,43 @@
   // that 'obj' can be nil in this context.
   dict[obj] = getStringFromString(obj); // no-warning
 }
+
+Foo * getMightBeNullFoo();
+Foo * _Nonnull getNonnullFoo();
+Foo * _Nullable getNullableFoo();
+
+void testCreateDictionaryLiteralWithNullableArg() {
+  Foo *p1 = getMightBeNullFoo();
+  Foo *p2 = getNonnullFoo();
+  Foo *p3 = getNullableFoo();
+
+  clang_analyzer_eval(p1 == nil); // expected-warning {{UNKNOWN}}
+  clang_analyzer_eval(p2 == nil); // expected-warning {{UNKNOWN}}
+  clang_analyzer_eval(p3 == nil); // expected-warning {{UNKNOWN}}
+
+  (void)@{@"abc" : p1}; // no-warning
+  (void)@{@"abc" : p2}; // no-warning
+  (void)@{@"abc" : p3}; // expected-warning {{Nullable pointer is passed to a callee that requires a non-null}}
+
+  clang_analyzer_eval(p1 == nil); // expected-warning {{FALSE}}
+  clang_analyzer_eval(p2 == nil); // expected-warning {{FALSE}}
+  clang_analyzer_eval(p3 == nil); // expected-warning {{FALSE}}
+}
+
+void testCreateArrayLiteralWithNullableArg() {
+  Foo *p1 = getMightBeNullFoo();
+  Foo *p2 = getNonnullFoo();
+  Foo *p3 = getNullableFoo();
+
+  clang_analyzer_eval(p1 == nil); // expected-warning {{UNKNOWN}}
+  clang_analyzer_eval(p2 == nil); // expected-warning {{UNKNOWN}}
+  clang_analyzer_eval(p3 == nil); // expected-warning {{UNKNOWN}}
+
+  (void)@[p1]; // no-warning
+  (void)@[p2]; // no-warning
+  (void)@[p3]; // expected-warning {{Nullable pointer is passed to a callee that requires a non-null}}
+
+  clang_analyzer_eval(p1 == nil); // expected-warning {{FALSE}}
+  clang_analyzer_eval(p2 == nil); // expected-warning {{FALSE}}
+  clang_analyzer_eval(p3 == nil); // expected-warning {{FALSE}}
+}
\ No newline at end of file
Index: clang/lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp
+++ clang/lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp
@@ -97,7 +97,8 @@
 namespace {
   class NilArgChecker : public Checker,
-   check::PostStmt > {
+   check::PostStmt,
+   EventDispatcher> {
 mutable std::unique_ptr BT;
 
 mutable llvm::SmallDenseMap StringSelectors;
@@ -139,12 +140,28 @@
 void NilArgChecker::warnIfNilExpr(const Expr *E,
   const char *Msg,
   CheckerContext &C) const {
-  ProgramStateRef State = C.getState();
-  if (State->isNull(C.getSVal(E)).isConstrainedTrue()) {
-
+  auto Location = C.getSVal(E).getAs();
+  if (!Location)
+return;
+  
+  auto [NonNull, Null] = C.getState()->assume(*Location);
+  
+  // If it's known to be null.
+  if (!NonNull && Null) {
 if (ExplodedNode *N = C.generateErrorNode()) {
   generateBugReport(N, Msg, E->getSourceRange(), E, C);
+  return;
+}
+  }
+  
+  // If it might be null, assume that it cannot after this operation.
+  if (Null) {
+// One needs to make sure the pointer is non-null to be used here.
+if (ExplodedNode *N = C.generateSink(Null, C.getPredecessor())) {
+  dispatchEvent({*Location, /*IsLoad=*/false, N, &C.getBugReporter(),
+   

[PATCH] D152269: [StaticAnalyzer] Fix false negative on NilArgChecker when creating literal object

[tripleCC via Phabricator via cfe-commits]

tripleCC updated this revision to Diff 529584.
tripleCC added a comment.

clang-format BasicObjCFoundationChecks


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152269

Files:
  clang/lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp
  clang/test/Analysis/NSContainers.m

Index: clang/test/Analysis/NSContainers.m
===
--- clang/test/Analysis/NSContainers.m
+++ clang/test/Analysis/NSContainers.m
@@ -1,4 +1,7 @@
-// RUN: %clang_analyze_cc1  -Wno-objc-literal-conversion -analyzer-checker=core,osx.cocoa.NonNilReturnValue,osx.cocoa.NilArg,osx.cocoa.Loops,debug.ExprInspection -verify -Wno-objc-root-class %s
+// RUN: %clang_analyze_cc1 -Wno-objc-literal-conversion -Wno-objc-root-class -fobjc-arc \
+// RUN:   -analyzer-checker=core,osx.cocoa,nullability \
+// RUN:   -analyzer-config eagerly-assume=false \
+// RUN:   -analyzer-checker=debug.ExprInspection -verify %s
 
 void clang_analyzer_eval(int);
 
@@ -31,13 +34,13 @@
 @end
 
 typedef struct {
-  unsigned long state;
-  id *itemsPtr;
-  unsigned long *mutationsPtr;
-  unsigned long extra[5];
+unsigned long state;
+id __unsafe_unretained _Nullable * _Nullable itemsPtr;
+unsigned long * _Nullable mutationsPtr;
+unsigned long extra[5];
 } NSFastEnumerationState;
 @protocol NSFastEnumeration
-- (NSUInteger)countByEnumeratingWithState:(NSFastEnumerationState *)state objects:(id [])buffer count:(NSUInteger)len;
+- (NSUInteger)countByEnumeratingWithState:(NSFastEnumerationState *)state objects:(id __unsafe_unretained _Nullable [_Nonnull])buffer count:(NSUInteger)len;
 @end
 
 @interface NSArray : NSObject 
@@ -323,3 +326,43 @@
   // that 'obj' can be nil in this context.
   dict[obj] = getStringFromString(obj); // no-warning
 }
+
+Foo * getMightBeNullFoo();
+Foo * _Nonnull getNonnullFoo();
+Foo * _Nullable getNullableFoo();
+
+void testCreateDictionaryLiteralWithNullableArg() {
+  Foo *p1 = getMightBeNullFoo();
+  Foo *p2 = getNonnullFoo();
+  Foo *p3 = getNullableFoo();
+
+  clang_analyzer_eval(p1 == nil); // expected-warning {{UNKNOWN}}
+  clang_analyzer_eval(p2 == nil); // expected-warning {{UNKNOWN}}
+  clang_analyzer_eval(p3 == nil); // expected-warning {{UNKNOWN}}
+
+  (void)@{@"abc" : p1}; // no-warning
+  (void)@{@"abc" : p2}; // no-warning
+  (void)@{@"abc" : p3}; // expected-warning {{Nullable pointer is passed to a callee that requires a non-null}}
+
+  clang_analyzer_eval(p1 == nil); // expected-warning {{FALSE}}
+  clang_analyzer_eval(p2 == nil); // expected-warning {{FALSE}}
+  clang_analyzer_eval(p3 == nil); // expected-warning {{FALSE}}
+}
+
+void testCreateArrayLiteralWithNullableArg() {
+  Foo *p1 = getMightBeNullFoo();
+  Foo *p2 = getNonnullFoo();
+  Foo *p3 = getNullableFoo();
+
+  clang_analyzer_eval(p1 == nil); // expected-warning {{UNKNOWN}}
+  clang_analyzer_eval(p2 == nil); // expected-warning {{UNKNOWN}}
+  clang_analyzer_eval(p3 == nil); // expected-warning {{UNKNOWN}}
+
+  (void)@[p1]; // no-warning
+  (void)@[p2]; // no-warning
+  (void)@[p3]; // expected-warning {{Nullable pointer is passed to a callee that requires a non-null}}
+
+  clang_analyzer_eval(p1 == nil); // expected-warning {{FALSE}}
+  clang_analyzer_eval(p2 == nil); // expected-warning {{FALSE}}
+  clang_analyzer_eval(p3 == nil); // expected-warning {{FALSE}}
+}
\ No newline at end of file
Index: clang/lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp
+++ clang/lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp
@@ -95,56 +95,64 @@
 //===--===//
 
 namespace {
-  class NilArgChecker : public Checker,
-   check::PostStmt > {
-mutable std::unique_ptr BT;
-
-mutable llvm::SmallDenseMap StringSelectors;
-mutable Selector ArrayWithObjectSel;
-mutable Selector AddObjectSel;
-mutable Selector InsertObjectAtIndexSel;
-mutable Selector ReplaceObjectAtIndexWithObjectSel;
-mutable Selector SetObjectAtIndexedSubscriptSel;
-mutable Selector ArrayByAddingObjectSel;
-mutable Selector DictionaryWithObjectForKeySel;
-mutable Selector SetObjectForKeySel;
-mutable Selector SetObjectForKeyedSubscriptSel;
-mutable Selector RemoveObjectForKeySel;
-
-void warnIfNilExpr(const Expr *E,
-   const char *Msg,
-   CheckerContext &C) const;
-
-void warnIfNilArg(CheckerContext &C,
-  const ObjCMethodCall &msg, unsigned Arg,
-  FoundationClass Class,
-  bool CanBeSubscript = false) const;
-
-void generateBugReport(ExplodedNode *N,
-   StringRef Msg,
-   SourceRange Range,
-

[PATCH] D153017: [StaticAnalyzer] Fix false negative when using a nullable parameter directly without binding to a variable

[tripleCC via Phabricator via cfe-commits]

tripleCC created this revision.
Herald added subscribers: steakhal, manas, ASDenysPetrov, martong, dkrupp, 
donat.nagy, Szelethus, a.sidorin, szepet, baloghadamsoftware.
Herald added a reviewer: NoQ.
Herald added a project: All.
tripleCC requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D153017

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
@@ -145,6 +145,17 @@
   }
 }
 
+void testArgumentTrackingDirectly(Dummy *_Nonnull nonnull, Dummy *_Nullable 
nullable) {
+  switch(getRandom()) {
+  case 1: testMultiParamChecking(nonnull, nullable, nonnull); break;
+  case 2: testMultiParamChecking(nonnull, nonnull, nonnull); break;
+  case 3: testMultiParamChecking(nonnull, nullable, nullable); break; // 
expected-warning {{Nullable pointer is passed to a callee that requires a 
non-null 3rd parameter}}
+  case 4: testMultiParamChecking(nullable, nullable, nonnull); // 
expected-warning {{Nullable pointer is passed to a callee that requires a 
non-null 1st parameter}}
+  case 5: testMultiParamChecking(nullable, nullable, nullable); // 
expected-warning {{Nullable pointer is passed to a callee that requires a 
non-null 1st parameter}}
+  case 6: testMultiParamChecking((Dummy *_Nonnull)0, nullable, nonnull); break;
+  }
+}
+
 Dummy *_Nonnull testNullableReturn(Dummy *_Nullable a) {
   Dummy *p = a;
   return p; // expected-warning {{Nullable pointer is returned from a function 
that is expected to return a non-null value}}
Index: clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
@@ -26,12 +26,13 @@
 
 #include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
 
+#include "clang/Analysis/AnyCall.h"
 #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
 #include "clang/StaticAnalyzer/Core/Checker.h"
 #include "clang/StaticAnalyzer/Core/CheckerManager.h"
-#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerHelpers.h"
-#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerHelpers.h"
 
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/Support/Path.h"
@@ -81,7 +82,8 @@
 : public Checker,
  check::PostCall, check::PostStmt,
  check::PostObjCMessage, check::DeadSymbols, eval::Assume,
- check::Location, check::Event> {
+ check::Location, check::Event,
+ check::BeginFunction> {
 
 public:
   // If true, the checker will not diagnose nullabilility issues for calls
@@ -102,6 +104,7 @@
   void checkEvent(ImplicitNullDerefEvent Event) const;
   void checkLocation(SVal Location, bool IsLoad, const Stmt *S,
  CheckerContext &C) const;
+  void checkBeginFunction(CheckerContext &Ctx) const;
   ProgramStateRef evalAssume(ProgramStateRef State, SVal Cond,
  bool Assumption) const;
 
@@ -563,6 +566,33 @@
   }
 }
 
+void NullabilityChecker::checkBeginFunction(CheckerContext &C) const {
+  const LocationContext *LCtx = C.getLocationContext();
+  auto AbstractCall = AnyCall::forDecl(LCtx->getDecl());
+  if (!AbstractCall)
+return;
+
+  ProgramStateRef State = C.getState();
+  for (auto Parameter : AbstractCall->parameters()) {
+if (!isValidPointerType(Parameter->getType()))
+  continue;
+
+Nullability RequiredNullability =
+getNullabilityAnnotation(Parameter->getType());
+if (RequiredNullability != Nullability::Nullable)
+  continue;
+
+const VarRegion *ParamRegion = State->getRegion(Parameter, LCtx);
+auto StoredVal = State->getSVal(ParamRegion).getAs();
+if (!StoredVal)
+  continue;
+
+State = C.getState()->set(
+StoredVal->getRegion(), NullabilityState(RequiredNullability));
+C.addTransition(State);
+  }
+}
+
 // Whenever we see a load from a typed memory region that's been annotated as
 // 'nonnull', we want to trust the user on that and assume that it is is indeed
 // non-null.


Index: clang/test/Analysis/nullability.mm
===
--- clang/test/Analysis/nullability.mm
+++ clang/test/Analysis/nullability.mm
@@ -145,6 +145,17 @@
   }
 }
 
+void testArgumentTrackingDirectly(Dummy *_Nonnull nonnull, Dummy *_Nullable nullable) {
+  switch(getRandom()) {
+  case 1: testMultiParamChecking(nonnull,

[PATCH] D153017: [analyzer] Fix false negative when using a nullable parameter directly without binding to a variable

[tripleCC via Phabricator via cfe-commits]

tripleCC updated this revision to Diff 531779.
tripleCC added a comment.

add inTopFrame condition. call the addTransition function only once for all 
params.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153017

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
@@ -145,6 +145,17 @@
   }
 }
 
+void testArgumentTrackingDirectly(Dummy *_Nonnull nonnull, Dummy *_Nullable 
nullable) {
+  switch(getRandom()) {
+  case 1: testMultiParamChecking(nonnull, nullable, nonnull); break;
+  case 2: testMultiParamChecking(nonnull, nonnull, nonnull); break;
+  case 3: testMultiParamChecking(nonnull, nullable, nullable); break; // 
expected-warning {{Nullable pointer is passed to a callee that requires a 
non-null 3rd parameter}}
+  case 4: testMultiParamChecking(nullable, nullable, nonnull); // 
expected-warning {{Nullable pointer is passed to a callee that requires a 
non-null 1st parameter}}
+  case 5: testMultiParamChecking(nullable, nullable, nullable); // 
expected-warning {{Nullable pointer is passed to a callee that requires a 
non-null 1st parameter}}
+  case 6: testMultiParamChecking((Dummy *_Nonnull)0, nullable, nonnull); break;
+  }
+}
+
 Dummy *_Nonnull testNullableReturn(Dummy *_Nullable a) {
   Dummy *p = a;
   return p; // expected-warning {{Nullable pointer is returned from a function 
that is expected to return a non-null value}}
Index: clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
@@ -26,12 +26,13 @@
 
 #include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
 
+#include "clang/Analysis/AnyCall.h"
 #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
 #include "clang/StaticAnalyzer/Core/Checker.h"
 #include "clang/StaticAnalyzer/Core/CheckerManager.h"
-#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerHelpers.h"
-#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerHelpers.h"
 
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/Support/Path.h"
@@ -81,7 +82,8 @@
 : public Checker,
  check::PostCall, check::PostStmt,
  check::PostObjCMessage, check::DeadSymbols, eval::Assume,
- check::Location, check::Event> {
+ check::Location, check::Event,
+ check::BeginFunction> {
 
 public:
   // If true, the checker will not diagnose nullabilility issues for calls
@@ -102,6 +104,7 @@
   void checkEvent(ImplicitNullDerefEvent Event) const;
   void checkLocation(SVal Location, bool IsLoad, const Stmt *S,
  CheckerContext &C) const;
+  void checkBeginFunction(CheckerContext &Ctx) const;
   ProgramStateRef evalAssume(ProgramStateRef State, SVal Cond,
  bool Assumption) const;
 
@@ -563,6 +566,37 @@
   }
 }
 
+void NullabilityChecker::checkBeginFunction(CheckerContext &C) const {
+  if (!C.inTopFrame())
+return;
+
+  const LocationContext *LCtx = C.getLocationContext();
+  auto AbstractCall = AnyCall::forDecl(LCtx->getDecl());
+  if (!AbstractCall || AbstractCall->parameters().empty())
+return;
+
+  ProgramStateRef State = C.getState();
+  for (const ParmVarDecl *Param : AbstractCall->parameters()) {
+if (!isValidPointerType(Param->getType()))
+  continue;
+
+Nullability RequiredNullability =
+getNullabilityAnnotation(Param->getType());
+if (RequiredNullability != Nullability::Nullable)
+  continue;
+
+const VarRegion *ParamRegion = State->getRegion(Param, LCtx);
+const MemRegion *ParamPointeeRegion =
+State->getSVal(ParamRegion).getAsRegion();
+if (!ParamPointeeRegion)
+  continue;
+
+State = C.getState()->set(
+ParamPointeeRegion, NullabilityState(RequiredNullability));
+  }
+  C.addTransition(State);
+}
+
 // Whenever we see a load from a typed memory region that's been annotated as
 // 'nonnull', we want to trust the user on that and assume that it is is indeed
 // non-null.


Index: clang/test/Analysis/nullability.mm
===
--- clang/test/Analysis/nullability.mm
+++ clang/test/Analysis/nullability.mm
@@ -145,6 +145,17 @@
   }
 }
 
+void testArgumentTrackingDirectly(Dummy *_Nonnull nonnull, Dummy *_Nullable nullable) {
+  switch(getRandom()) {
+  case 1: testMultiParamChecking(nonnull, nu

[PATCH] D153017: [analyzer] Fix false negative when using a nullable parameter directly without binding to a variable

[tripleCC via Phabricator via cfe-commits]

tripleCC added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp:569-573
+void NullabilityChecker::checkBeginFunction(CheckerContext &C) const {
+  const LocationContext *LCtx = C.getLocationContext();
+  auto AbstractCall = AnyCall::forDecl(LCtx->getDecl());
+  if (!AbstractCall)
+return;

steakhal wrote:
> steakhal wrote:
> > Uh, the diffing here looks terrible.
> > What you probably want: Fold the `State`s, and if you are done, transition 
> > - but only if we have any parameters.
> > We need to have a single `addTransition()` call if we want a single 
> > execution path modeled in the graph. We probably don't want one path on 
> > which the first parameter's annotation is known; and a separate one where 
> > only the second, etc.
> Shouldn't we only do this for the analysis entrypoints only? (aka. top-level 
> functions)
> I assume this checker already did some modeling of the attributes, hence we 
> have the warnings in the tests.
Thanking for your reviewing. You are correct, I added an `inTopFrame()`  
condition here. It only makes sense for top-level functions.



Comment at: clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp:569-594
+void NullabilityChecker::checkBeginFunction(CheckerContext &C) const {
+  const LocationContext *LCtx = C.getLocationContext();
+  auto AbstractCall = AnyCall::forDecl(LCtx->getDecl());
+  if (!AbstractCall)
+return;
+
+  ProgramStateRef State = C.getState();

tripleCC wrote:
> steakhal wrote:
> > steakhal wrote:
> > > Uh, the diffing here looks terrible.
> > > What you probably want: Fold the `State`s, and if you are done, 
> > > transition - but only if we have any parameters.
> > > We need to have a single `addTransition()` call if we want a single 
> > > execution path modeled in the graph. We probably don't want one path on 
> > > which the first parameter's annotation is known; and a separate one where 
> > > only the second, etc.
> > Shouldn't we only do this for the analysis entrypoints only? (aka. 
> > top-level functions)
> > I assume this checker already did some modeling of the attributes, hence we 
> > have the warnings in the tests.
> Thanking for your reviewing. You are correct, I added an `inTopFrame()`  
> condition here. It only makes sense for top-level functions.
I made a rookie mistake here. I should call the `addTransition` function 
outside the for loop. 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153017

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

[PATCH] D153017: [analyzer] Fix false negative when using a nullable parameter directly without binding to a variable

[tripleCC via Phabricator via cfe-commits]

tripleCC updated this revision to Diff 531784.
tripleCC added a comment.

Do not overwrite state


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153017

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
@@ -145,6 +145,17 @@
   }
 }
 
+void testArgumentTrackingDirectly(Dummy *_Nonnull nonnull, Dummy *_Nullable 
nullable) {
+  switch(getRandom()) {
+  case 1: testMultiParamChecking(nonnull, nullable, nonnull); break;
+  case 2: testMultiParamChecking(nonnull, nonnull, nonnull); break;
+  case 3: testMultiParamChecking(nonnull, nullable, nullable); break; // 
expected-warning {{Nullable pointer is passed to a callee that requires a 
non-null 3rd parameter}}
+  case 4: testMultiParamChecking(nullable, nullable, nonnull); // 
expected-warning {{Nullable pointer is passed to a callee that requires a 
non-null 1st parameter}}
+  case 5: testMultiParamChecking(nullable, nullable, nullable); // 
expected-warning {{Nullable pointer is passed to a callee that requires a 
non-null 1st parameter}}
+  case 6: testMultiParamChecking((Dummy *_Nonnull)0, nullable, nonnull); break;
+  }
+}
+
 Dummy *_Nonnull testNullableReturn(Dummy *_Nullable a) {
   Dummy *p = a;
   return p; // expected-warning {{Nullable pointer is returned from a function 
that is expected to return a non-null value}}
Index: clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
@@ -26,12 +26,13 @@
 
 #include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
 
+#include "clang/Analysis/AnyCall.h"
 #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
 #include "clang/StaticAnalyzer/Core/Checker.h"
 #include "clang/StaticAnalyzer/Core/CheckerManager.h"
-#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerHelpers.h"
-#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerHelpers.h"
 
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/Support/Path.h"
@@ -81,7 +82,8 @@
 : public Checker,
  check::PostCall, check::PostStmt,
  check::PostObjCMessage, check::DeadSymbols, eval::Assume,
- check::Location, check::Event> {
+ check::Location, check::Event,
+ check::BeginFunction> {
 
 public:
   // If true, the checker will not diagnose nullabilility issues for calls
@@ -102,6 +104,7 @@
   void checkEvent(ImplicitNullDerefEvent Event) const;
   void checkLocation(SVal Location, bool IsLoad, const Stmt *S,
  CheckerContext &C) const;
+  void checkBeginFunction(CheckerContext &Ctx) const;
   ProgramStateRef evalAssume(ProgramStateRef State, SVal Cond,
  bool Assumption) const;
 
@@ -563,6 +566,37 @@
   }
 }
 
+void NullabilityChecker::checkBeginFunction(CheckerContext &C) const {
+  if (!C.inTopFrame())
+return;
+
+  const LocationContext *LCtx = C.getLocationContext();
+  auto AbstractCall = AnyCall::forDecl(LCtx->getDecl());
+  if (!AbstractCall || AbstractCall->parameters().empty())
+return;
+
+  ProgramStateRef State = C.getState();
+  for (const ParmVarDecl *Param : AbstractCall->parameters()) {
+if (!isValidPointerType(Param->getType()))
+  continue;
+
+Nullability RequiredNullability =
+getNullabilityAnnotation(Param->getType());
+if (RequiredNullability != Nullability::Nullable)
+  continue;
+
+const VarRegion *ParamRegion = State->getRegion(Param, LCtx);
+const MemRegion *ParamPointeeRegion =
+State->getSVal(ParamRegion).getAsRegion();
+if (!ParamPointeeRegion)
+  continue;
+
+State = State->set(ParamPointeeRegion,
+   NullabilityState(RequiredNullability));
+  }
+  C.addTransition(State);
+}
+
 // Whenever we see a load from a typed memory region that's been annotated as
 // 'nonnull', we want to trust the user on that and assume that it is is indeed
 // non-null.


Index: clang/test/Analysis/nullability.mm
===
--- clang/test/Analysis/nullability.mm
+++ clang/test/Analysis/nullability.mm
@@ -145,6 +145,17 @@
   }
 }
 
+void testArgumentTrackingDirectly(Dummy *_Nonnull nonnull, Dummy *_Nullable nullable) {
+  switch(getRandom()) {
+  case 1: testMultiParamChecking(nonnull, nullable, nonnull); break;
+  case 2: tes

[PATCH] D153017: [analyzer] Fix false negative when using a nullable parameter directly without binding to a variable

[tripleCC via Phabricator via cfe-commits]

tripleCC added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp:569-594
+void NullabilityChecker::checkBeginFunction(CheckerContext &C) const {
+  const LocationContext *LCtx = C.getLocationContext();
+  auto AbstractCall = AnyCall::forDecl(LCtx->getDecl());
+  if (!AbstractCall)
+return;
+
+  ProgramStateRef State = C.getState();

steakhal wrote:
> tripleCC wrote:
> > tripleCC wrote:
> > > steakhal wrote:
> > > > steakhal wrote:
> > > > > Uh, the diffing here looks terrible.
> > > > > What you probably want: Fold the `State`s, and if you are done, 
> > > > > transition - but only if we have any parameters.
> > > > > We need to have a single `addTransition()` call if we want a single 
> > > > > execution path modeled in the graph. We probably don't want one path 
> > > > > on which the first parameter's annotation is known; and a separate 
> > > > > one where only the second, etc.
> > > > Shouldn't we only do this for the analysis entrypoints only? (aka. 
> > > > top-level functions)
> > > > I assume this checker already did some modeling of the attributes, 
> > > > hence we have the warnings in the tests.
> > > Thanking for your reviewing. You are correct, I added an `inTopFrame()`  
> > > condition here. It only makes sense for top-level functions.
> > I made a rookie mistake here. I should call the `addTransition` function 
> > outside the for loop. 
> > 
> Please note that each iteration of the loop will overwrite the state you made 
> in the previous iteration this way.
> Its most likely not what you want.
> Because C.getState() will always return the same state no matter the context 
> here.
Updated


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153017

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