[PATCH] D77806: [analyzer] Do not report CFError null dereference for nonnull params

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

We're doing much more here than just fixing the CFError behavior; we're actually making the whole analyzer respect these annotations in top frame.

Let's add checker-inspecific tests. We have a test checker `debug.ExprInspection` just for that:

  // RUN: %clang_analyze_cc1 ... -analyzer-checker=debug.ExprInspection ...
  void clang_analyzer_eval(bool);
  
  void foo(void *x) __attribute__((nonnull)) {
    clang_analyzer_eval(x != nullptr); // expected-warning{{TRUE}}
  }



================
Comment at: clang/lib/StaticAnalyzer/Checkers/NonNullParamChecker.cpp:218
 
+/// We want to trust developer annotations and consider all 'nonnul' parameters
+/// as non-null indeed. Each marked parameter will get a corresponding
----------------
Typo :p


================
Comment at: clang/lib/StaticAnalyzer/Checkers/NonNullParamChecker.cpp:234
+/// \endcode
+void NonNullParamChecker::checkBeginFunction(CheckerContext &Context) const {
+  const LocationContext *LocContext = Context.getLocationContext();
----------------
As far as i understand, you should only do all this for //top-level// functions, i.e. the ones from which we've started the analysis. You can skip inlined functions here because a similar assumption is already made for their parameters in `checkPreCall`.

You can figure out if you're in top frame via `Context.inTopFrame()`.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/NonNullParamChecker.cpp:238-239
+  const Decl *FD = LocContext->getDecl();
+  // AnyCall helps us here to avoid checking for FunctionDecl and ObjCMethodDecl
+  // separately and aggregates interfaces of these classes.
+  auto AbstractCall = AnyCall::forDecl(FD);
----------------
Nice! Should we add new tests for ObjC method calls as well then?


================
Comment at: clang/lib/StaticAnalyzer/Checkers/NonNullParamChecker.cpp:253
+    Loc ParameterLoc = State->getLValue(Parameter, LocContext);
+    auto StoredVal = State->getSVal(ParameterLoc).getAs<loc::MemRegionVal>();
+    if (!StoredVal)
----------------
Once you restrict yourself to top frame, you can save two lines of code and one asterisk by doing `.castAs<DefinedOrUnknownSVal>()` here instead. Because we'll never assume that we're being fed an undefined value in an out-of-context top-level function. A much stronger assertion can probably be made.


================
Comment at: clang/test/Analysis/nonnull.cpp:28
+int globalVar = 15;
+void moreParamsThanArgs [[gnu::nonnull(2, 4)]] (int a, int *p, int b = 42, int *q = &globalVar);
+
----------------
C-style variadic functions are the real problem. Variadic templates are easy; they're just duplicated in the AST as many times as necessary and all the parameter declarations are in place. Default arguments are also easy; the argument expression is still present in the AST even if it's not explicitly written down at the call site. C-style variadic functions are hard because they actually have more arguments than they have parameters.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77806