[PATCH] Dereference then check

[Anna Zaks]

Anders, 

Here are some comments about the patch. However, I am still to understand and review the core algorithm you are using (asked in another email).

Thanks!
Anna.

Index: include/clang/Analysis/Analyses/TestAfter.h
===================================================================
--- include/clang/Analysis/Analyses/TestAfter.h	(revision 0)
+++ include/clang/Analysis/Analyses/TestAfter.h	(working copy)
@@ -0,0 +1,71 @@
+//===- TestAfter.h - Test after usage analysis for Source CFGs ----*- C++ --*-//
Please, describe the algorithm used and the specifics - test after usage is very generic. (Ex: What is considered a use? What is considered a test?)

+class TestAfter : public ManagedAnalysis {
+  class InvalidatedValues {
The names are off and/or not explained. We have a TestAfter analysis that collects InvalidatedValues.

+    bool isDereferenced(const VarDecl *D) const;
Please, add explanation what this returns. Is this true for the pointer values that have been dereferenced on all paths leading to this block or something else?

--- lib/StaticAnalyzer/Checkers/DereferenceThenCheck.cpp	(revision 0)
+++ lib/StaticAnalyzer/Checkers/DereferenceThenCheck.cpp	(working copy)
@@ -0,0 +1,85 @@
+    BR.EmitBasicReport(AC->getDecl(), Checker, "DerefBug", "DerefBug",
+                       "Possible null pointer dereference", L, R);

Is there a reason why the warning message is different than what the DereferenceChecker produces?


Index: lib/Analysis/TestAfter.cpp
===================================================================
--- lib/Analysis/TestAfter.cpp	(revision 0)
+++ lib/Analysis/TestAfter.cpp	(working copy)
@@ -0,0 +1,165 @@

+  TestAfter::InvalidatedValues runOnBlock(const CFGBlock *Block,
+                                           TestAfter::InvalidatedValues DerefVal,
+                                           TestAfter::Observer *Obs = nullptr);
Please, fix 80 col rule and alignment.

+        DSetFact(false) // This is a *major* performance win.
Is the comment true?


--- test/Analysis/dereference-then-check.c	(revision 0)
+++ test/Analysis/dereference-then-check.c	(working copy)
@@ -0,0 +1,86 @@
+// RUN: %clang_cc1 -analyze -analyzer-checker=core,alpha.core -verify %s
+// RUN: %clang_cc1 -x c++ -analyze -analyzer-checker=core,alpha.core -verify %s

I've noticed that none of the checks, have "if (p)" as a check. It's worth adding it since this is the most common pattern for checking if a pointer is null.

+void foo55(char *p) {
+  if (1)
+    if (*p == 0) {
+    }
+  if (!p) {
+  }
+}

Strange indentation. If possible, please use more descriptive names in false negative tests (hinting on the underlying cause) .

+void f(int **p);
+void foo6() {
+  int *p;
+  f(&p);
+  if (!p) {
+  }
+}
+
+int *w;
+int **ff() { return &w; }
+
+void foo7() {
+  int **p = ff();
+  if (!p) {
+  }
+}
Are these supposed to be false negatives? Looks like normal tests got mixed with false negatives (below as well)..

+int x;
+void foo8(int *p) {
+  if (x)
+    p = 0;
+  else
+    *p = 0; // no-warning
+  if (!p) {
+  }
+}
For the tests where you test for absence of warnings, please, add "// no-warning". This is the case for the test above. 
Also, this test can be renamed into something like "warn_only_if_all_path_dereference".

Let's add this test for completeness:
int inlined_defensive_check(int *p) {
  if (p)
    return 5;
  return 0;
}
int use_inlined_defensive_check(int *p) {
  int x = *p;  // no-warning
  return x + inlined_defensive_check(p);
}

Also, the should produce 2 warnings in this case:
void test_two_dereferences(int *p, char cond) {
  if (cond) {
    *p = 3;  // expected-warning
  } else {
    *p = 1; // expected-warning
  }
  if (p) {
    cond = 1;
  }
}

> On Sep 25, 2014, at 7:30 AM, Anders Rönnholm <Anders.Ronnholm at evidente.se> wrote:
> 
> Hi,
> 
> I have made a new checker similar to the divison by zero i previously submitted.
> 
> This one checks for dereferences that are later compared against zero.
> 
> As requested i have made it CFG-based like how DeadStores is implemented. I hope this is how you meant.
> 
> //Anders<derefthencheck.diff>