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

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

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

Loosely tied to EXP34-C.

Suppose we do a pointer operation, such as dereferencing, or passing it to a function whose parameter is attributed with non null (like `_Nonnull` or `clang::nonnull`). Then, we write an if branch whose condition is this very pointer.

  void tp1(int *p) {
    *p = 5;
    // expected-note at -1{{Pointer assumed non-null here}}
    // expected-note at -2{{Consider moving the condition here}}
    if (p)
      // expected-note at -1{{Pointer is unconditionally non-null here}}
      // expected-warning at -2{{Pointer is unconditionally non-null here [alpha.core.ReverseNull]}}
      return;
  }

This is a sign of code smell: We either inteded to place the condition before the operation that already expects the pointer to be non-null, or there are some other high level issues in our code. The patch adds a new checker to find and warn for similar cases.

One of the fundamental ideas here is that this is an interprocedural checker. We want to ensure that the derefence (or similar operation) unconditionally leads to the condition without interleaving assignments. This sounds a lot like a dataflow analysis! Indeed, it would be great if we had a dataflow framework already in the bag, and although there is a work on it (a lot of patches can be seen on @ymandel's profile: https://reviews.llvm.org/people/revisions/17749/), its not quite there yet.

Another thought could be, that this checker is (somewhat) redundant with DeadCodeChecker, because this checker also finds dead code, but restricted to pointers. This wasn't a deterrent in developing this alpha checker, because if an 'else' branch is not present, the dead code will not be found (despite the sode smell being there), and restricting ourselves to pointer analysis takes a lot of weight off from our constraint solvers.

I've ran the checker on a lot of projects, and the results so far look promising, and I'm looking forward to publishing them ASAP.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D120992

Files:
  clang/docs/analyzer/checkers.rst
  clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
  clang/lib/StaticAnalyzer/Checkers/AnalysisOrderChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
  clang/lib/StaticAnalyzer/Checkers/NullPtrInterferenceChecker.cpp
  clang/test/Analysis/NSString.m
  clang/test/Analysis/analyzer-config.c
  clang/test/Analysis/null-pointer-interference.c
  clang/test/Analysis/null-pointer-interference.m

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D120992.412988.patch
Type: text/x-patch
Size: 20988 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20220304/b5abb1d2/attachment-0001.bin>