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

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko marked 4 inline comments as done.
vsavchenko added a comment.

In D77806#1973616 <https://reviews.llvm.org/D77806#1973616>, @NoQ wrote:

> 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}}
>   }
>


Cool, I didn't know about that! I'll add a group of tests for user annotations then.



================
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
----------------
NoQ wrote:
> Typo :p
Whoops! It even got me a full minute or two to spot it now!


================
Comment at: clang/lib/StaticAnalyzer/Checkers/NonNullParamChecker.cpp:234
+/// \endcode
+void NonNullParamChecker::checkBeginFunction(CheckerContext &Context) const {
+  const LocationContext *LocContext = Context.getLocationContext();
----------------
NoQ wrote:
> 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()`.
Gotcha!


================
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);
----------------
NoQ wrote:
> Nice! Should we add new tests for ObjC method calls as well then?
Good catch!


================
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);
+
----------------
NoQ wrote:
> 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.
C-style variadic functions are already covered in `nonnull.m`. I added here C++-specific cases.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77806