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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
