Re: [PATCH] D11433: [Static Analyzer] Make NonNullParamChecker emit implicit null dereference events.

[Gábor Horváth via cfe-commits]

xazax.hun updated this revision to Diff 5.
xazax.hun added a comment.

- Updated to latest trunk.
- Added a new field to the event with documentation.
- Rebased the patch on the top of nullability checker.
- Added covered cases to the nullability checker tests.


http://reviews.llvm.org/D11433

Files:
  include/clang/StaticAnalyzer/Core/Checker.h
  lib/StaticAnalyzer/Checkers/DereferenceChecker.cpp
  lib/StaticAnalyzer/Checkers/NonNullParamChecker.cpp
  lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
  test/Analysis/nullability.mm

Index: test/Analysis/nullability.mm
===
--- test/Analysis/nullability.mm
+++ test/Analysis/nullability.mm
@@ -50,6 +50,8 @@
 
 template typename T T *eraseNullab(T *p) { return p; }
 
+void takesAttrNonnull(Dummy *p) __attribute((nonnull(1)));
+
 void testBasicRules() {
   Dummy *p = returnsNullable();
   int *ptr = returnsNullableInt();
@@ -73,10 +75,8 @@
 Dummy dd(d);
 break;
   }
-  // Here the copy constructor is called, so a reference is initialized with the
-  // value of p. No ImplicitNullDereference event will be dispatched for this
-  // case. A followup patch is expected to fix this in NonNullParamChecker.
-  default: { Dummy d = *p; } break; // No warning.
+  case 5: takesAttrNonnull(p); break; // expected-warning {{Nullable pointer is passed to}}
+  default: { Dummy d = *p; } break; // expected-warning {{Nullable pointer is dereferenced}}
   }
   if (p) {
 takesNonnull(p);
Index: lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
+++ lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
@@ -335,7 +335,10 @@
   if (Filter.CheckNullableDereferenced 
   TrackedNullability-getValue() == Nullability::Nullable) {
 BugReporter BR = *Event.BR;
-reportBug(ErrorKind::NullableDereferenced, Event.SinkNode, Region, BR);
+if (Event.IsDirectDereference)
+  reportBug(ErrorKind::NullableDereferenced, Event.SinkNode, Region, BR);
+else
+  reportBug(ErrorKind::NullablePassedToNonnull, Event.SinkNode, Region, BR);
   }
 }
 
Index: lib/StaticAnalyzer/Checkers/NonNullParamChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/NonNullParamChecker.cpp
+++ lib/StaticAnalyzer/Checkers/NonNullParamChecker.cpp
@@ -28,7 +28,7 @@
 
 namespace {
 class NonNullParamChecker
-  : public Checker check::PreCall  {
+  : public Checker check::PreCall, EventDispatcherImplicitNullDerefEvent  {
   mutable std::unique_ptrBugType BTAttrNonNull;
   mutable std::unique_ptrBugType BTNullRefArg;
 
@@ -139,26 +139,34 @@
 ProgramStateRef stateNotNull, stateNull;
 std::tie(stateNotNull, stateNull) = CM.assumeDual(state, *DV);
 
-if (stateNull  !stateNotNull) {
-  // Generate an error node.  Check for a null node in case
-  // we cache out.
-  if (ExplodedNode *errorNode = C.generateSink(stateNull)) {
+if (stateNull) {
+  if (!stateNotNull) {
+// Generate an error node.  Check for a null node in case
+// we cache out.
+if (ExplodedNode *errorNode = C.generateSink(stateNull)) {
 
-std::unique_ptrBugReport R;
-if (haveAttrNonNull)
-  R = genReportNullAttrNonNull(errorNode, ArgE);
-else if (haveRefTypeParam)
-  R = genReportReferenceToNullPointer(errorNode, ArgE);
+  std::unique_ptrBugReport R;
+  if (haveAttrNonNull)
+R = genReportNullAttrNonNull(errorNode, ArgE);
+  else if (haveRefTypeParam)
+R = genReportReferenceToNullPointer(errorNode, ArgE);
 
-// Highlight the range of the argument that was null.
-R-addRange(Call.getArgSourceRange(idx));
+  // Highlight the range of the argument that was null.
+  R-addRange(Call.getArgSourceRange(idx));
 
-// Emit the bug report.
-C.emitReport(std::move(R));
-  }
+  // Emit the bug report.
+  C.emitReport(std::move(R));
+}
 
-  // Always return.  Either we cached out or we just emitted an error.
-  return;
+// Always return.  Either we cached out or we just emitted an error.
+return;
+  }
+  if (ExplodedNode *N = C.generateSink(stateNull)) {
+ImplicitNullDerefEvent event = {
+V, false, N, C.getBugReporter(),
+/*IsDirectDereference=*/haveRefTypeParam};
+dispatchEvent(event);
+  }
 }
 
 // If a pointer value passed the check we should assume that it is
Index: lib/StaticAnalyzer/Checkers/DereferenceChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/DereferenceChecker.cpp
+++ lib/StaticAnalyzer/Checkers/DereferenceChecker.cpp
@@ -220,7 +220,8 @@
 // null or not-null.  Record the error node as an implicit null
 // dereference.
 if 

Re: [PATCH] D11433: [Static Analyzer] Make NonNullParamChecker emit implicit null dereference events.

[Anna Zaks via cfe-commits]

zaks.anna accepted this revision.
zaks.anna added a comment.
This revision is now accepted and ready to land.

Thanks!


http://reviews.llvm.org/D11433



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

Re: [PATCH] D11433: [Static Analyzer] Make NonNullParamChecker emit implicit null dereference events.

[Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL246182: [Static Analyzer] Make NonNullParamChecker emit 
implicit null dereference… (authored by xazax).

Changed prior to commit:
  http://reviews.llvm.org/D11433?vs=5id=33344#toc

Repository:
  rL LLVM

http://reviews.llvm.org/D11433

Files:
  cfe/trunk/include/clang/StaticAnalyzer/Core/Checker.h
  cfe/trunk/lib/StaticAnalyzer/Checkers/DereferenceChecker.cpp
  cfe/trunk/lib/StaticAnalyzer/Checkers/NonNullParamChecker.cpp
  cfe/trunk/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
  cfe/trunk/test/Analysis/nullability.mm

Index: cfe/trunk/include/clang/StaticAnalyzer/Core/Checker.h
===
--- cfe/trunk/include/clang/StaticAnalyzer/Core/Checker.h
+++ cfe/trunk/include/clang/StaticAnalyzer/Core/Checker.h
@@ -514,6 +514,10 @@
   bool IsLoad;
   ExplodedNode *SinkNode;
   BugReporter *BR;
+  // When true, the dereference is in the source code directly. When false, the
+  // dereference might happen later (for example pointer passed to a parameter
+  // that is marked with nonnull attribute.)
+  bool IsDirectDereference;
 };
 
 /// \brief A helper class which wraps a boolean value set to false by default.
Index: cfe/trunk/test/Analysis/nullability.mm
===
--- cfe/trunk/test/Analysis/nullability.mm
+++ cfe/trunk/test/Analysis/nullability.mm
@@ -50,6 +50,8 @@
 
 template typename T T *eraseNullab(T *p) { return p; }
 
+void takesAttrNonnull(Dummy *p) __attribute((nonnull(1)));
+
 void testBasicRules() {
   Dummy *p = returnsNullable();
   int *ptr = returnsNullableInt();
@@ -73,10 +75,8 @@
 Dummy dd(d);
 break;
   }
-  // Here the copy constructor is called, so a reference is initialized with the
-  // value of p. No ImplicitNullDereference event will be dispatched for this
-  // case. A followup patch is expected to fix this in NonNullParamChecker.
-  default: { Dummy d = *p; } break; // No warning.
+  case 5: takesAttrNonnull(p); break; // expected-warning {{Nullable pointer is passed to}}
+  default: { Dummy d = *p; } break; // expected-warning {{Nullable pointer is dereferenced}}
   }
   if (p) {
 takesNonnull(p);
Index: cfe/trunk/lib/StaticAnalyzer/Checkers/NonNullParamChecker.cpp
===
--- cfe/trunk/lib/StaticAnalyzer/Checkers/NonNullParamChecker.cpp
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/NonNullParamChecker.cpp
@@ -28,7 +28,7 @@
 
 namespace {
 class NonNullParamChecker
-  : public Checker check::PreCall  {
+  : public Checker check::PreCall, EventDispatcherImplicitNullDerefEvent  {
   mutable std::unique_ptrBugType BTAttrNonNull;
   mutable std::unique_ptrBugType BTNullRefArg;
 
@@ -139,26 +139,34 @@
 ProgramStateRef stateNotNull, stateNull;
 std::tie(stateNotNull, stateNull) = CM.assumeDual(state, *DV);
 
-if (stateNull  !stateNotNull) {
-  // Generate an error node.  Check for a null node in case
-  // we cache out.
-  if (ExplodedNode *errorNode = C.generateSink(stateNull)) {
-
-std::unique_ptrBugReport R;
-if (haveAttrNonNull)
-  R = genReportNullAttrNonNull(errorNode, ArgE);
-else if (haveRefTypeParam)
-  R = genReportReferenceToNullPointer(errorNode, ArgE);
+if (stateNull) {
+  if (!stateNotNull) {
+// Generate an error node.  Check for a null node in case
+// we cache out.
+if (ExplodedNode *errorNode = C.generateSink(stateNull)) {
+
+  std::unique_ptrBugReport R;
+  if (haveAttrNonNull)
+R = genReportNullAttrNonNull(errorNode, ArgE);
+  else if (haveRefTypeParam)
+R = genReportReferenceToNullPointer(errorNode, ArgE);
+
+  // Highlight the range of the argument that was null.
+  R-addRange(Call.getArgSourceRange(idx));
+
+  // Emit the bug report.
+  C.emitReport(std::move(R));
+}
 
-// Highlight the range of the argument that was null.
-R-addRange(Call.getArgSourceRange(idx));
-
-// Emit the bug report.
-C.emitReport(std::move(R));
+// Always return.  Either we cached out or we just emitted an error.
+return;
+  }
+  if (ExplodedNode *N = C.generateSink(stateNull)) {
+ImplicitNullDerefEvent event = {
+V, false, N, C.getBugReporter(),
+/*IsDirectDereference=*/haveRefTypeParam};
+dispatchEvent(event);
   }
-
-  // Always return.  Either we cached out or we just emitted an error.
-  return;
 }
 
 // If a pointer value passed the check we should assume that it is
Index: cfe/trunk/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
===
--- cfe/trunk/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
+++ 

Re: [PATCH] D11433: [Static Analyzer] Make NonNullParamChecker emit implicit null dereference events.

[Anna Zaks via cfe-commits]

zaks.anna requested changes to this revision.
zaks.anna added a comment.
This revision now requires changes to proceed.

I believe we saw a regression in diagnostics with this modification. Gabor, do 
you recall what it was?


http://reviews.llvm.org/D11433



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

Re: [PATCH] D11433: [Static Analyzer] Make NonNullParamChecker emit implicit null dereference events.

[Gábor Horváth via cfe-commits]

xazax.hun added a comment.

The problem here is that, this checker can emit a warning for two cases:
1, Null was bound to a reference which should be reported as a dereference
2, Null was passed to parameter that should be non-null (marked by the 
attribute, not the qualifier) which should be reported appropriately

Right now exactly the same event will be triggered for both cases, so the 
checkers that process this event has no information whether it is a dereference 
or a value passed to a nonnull parameter, so it can not provide appropriate 
diagnostic for both cases.

One possibility would be to have two separate events, the other is to have an 
argument to an event that determines its origin. Probably it would be better to 
have two separate events, since it might be confusing to have dereference in 
the name of the event in the second case.

What do you think?


http://reviews.llvm.org/D11433



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