[PATCH] D120992: [analyzer] ReverseNull: New checker to warn for pointer value conditions, if the pointer value is unconditionally non-null

[Balázs Benics via Phabricator via cfe-commits]

steakhal added a comment.

Looks great. I'm loving it!
BTW what is the semantics of `[p retain]` in ObjC? Can `p` be null in that context? Or does it count as a dereferences, hence it constraints the pointer?

Why did you touch the `AnalysisOrderChecker`, should we separate those changes?

Additionally, why do you think it needs to be in the `alpha.core` package instead of the `core`?
What sort of false-positives have you seen in the wild justifying that classification?



================
Comment at: clang/docs/analyzer/checkers.rst:1703
+alpha.core.ReverseNull (C)
+"""""""""""""""""""""""""
+Checks whether a dereferenced (and as such, assumed to be non-null) pointer is
----------------



================
Comment at: clang/lib/StaticAnalyzer/Checkers/NullPtrInterferenceChecker.cpp:196-198
+    const MemRegion *MR = Cond->getAsRegion();
+    if (!MR)
+      return;
----------------
I think additionally to this, you should also check for the type of the `Condition` expression. It's gotta be a pointer.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/NullPtrInterferenceChecker.cpp:211-216
+      // We want to be sure that the constraint and the condition are in the
+      // same stackframe. Caller and callee functions' pre/post conditions may
+      // not apply to the caller stackframe. A similar issue is discussed here:
+      // https://discourse.llvm.org/t/static-analyzer-query-why-is-suppress-null-return-paths-enabled-by-default/
+      if (NBeforeConstraint->getLocationContext() != Ctx.getLocationContext())
+        return;
----------------
IMO we should catch these as well:
```lang=C++
void store(int *q, int value) {
  *q = value;
}

void top(int *p) {
  store(p, 5);
  if (!p)
    return;
}
```

In this case, the stack-frame in which the pointer gets constrained is not the same as where the redundant check resides - rather a child frame of that.


================
Comment at: clang/test/Analysis/null-pointer-interference.c:18
+  // expected-note at -1{{Pointer assumed non-null here}}
+  // expected-note at -2{{Consider moving the condition here}}
+  if (p)
----------------
"before this expression"?
The term `here` is not well defined IMO.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120992