[PATCH] D41406: [analyzer] Add a new checker callback, check::NewAllocator.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ created this revision.
NoQ added reviewers: dcoughlin, xazax.hun, a.sidorin, george.karpenkov, szepet.
Herald added a subscriber: rnkovacs.

This patch is roughly based on the discussion we've had in http://lists.llvm.org/pipermail/cfe-dev/2017-December/056314.html about how our support for C++ operator new() should provide useful callbacks to the checkers. As in, this patch tries to do a minimal necessary thing while avoiding all decisions that we didn't have a consensus over.

This patch also absorbs https://reviews.llvm.org/D36708.

In the discussion, we've been mentioning PreAllocator/PostAllocator callbacks. However, we already sort of have them: they are PreCall/PostCall callbacks for the allocator call, which are readily available in the `c++-allocator-inlining` mode.

This patch also causes no change in the behavior of the pre/post `CXXNewExpr` callbacks. They remain broken in the `c++-allocator-inlining` mode for now.

Missing in the current system of callbacks, however, is a callback that would be called before construction while providing the //casted// return value of operator new (in the sense of https://reviews.llvm.org/D41250: operator new returns `void *`, but we need to have `C *`). The user can subscribe on the allocator's `PostCall` and perform the cast manually, but that's an unpleasant thing to do. So it sounds like a good idea to provide a callback that would contain the relevant information.

A similar "Pre" callback doesn't seem necessary - there's not much additional information we can provide compared to PreCall.

In this patch, I add the callback (with a lot of code that is the usual boilerplate around the callback, though there's still a questionable TODO in `CheckNewAllocatorContext::runChecker()`) and modify MallocChecker to make use of it when `c++-allocator-inlining` mode is turned on. Specifically:

- If the allocator (operator new) call is evaluated conservatively, `checkNewAllocator()` provides the return value of the allocator that is semantically equivalent to the value in `checkPostStmt<CXXNewExpr>()` that we've been using earlier. Without the cast, the value is different and the checker crashes.
- If the allocator call is inlined, the checker does not treat it as an allocator, but instead as a simple inlined function. That is, it does not start tracking the return value here, and in particular avoids crashes. This policy has already been there because previously we could inline the operator when it was written without operator syntax - see tests in `test/Analysis/NewDelete-custom.cpp`. Whether this policy was correct or not is a matter of discussion. For now it means that in `c++-allocator-inlining` mode we'd have new negatives (i.e. lost positives) on code that uses custom allocators which do not boil down to simple malloc.

The `std::nothrow_t` tests in `NewDelete-custom.cpp` are affected in a surprising manner. I'd fix them in a follow-up commit.


https://reviews.llvm.org/D41406

Files:
  include/clang/StaticAnalyzer/Core/Checker.h
  include/clang/StaticAnalyzer/Core/CheckerManager.h
  lib/StaticAnalyzer/Checkers/MallocChecker.cpp
  lib/StaticAnalyzer/Core/CheckerManager.cpp
  lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
  test/Analysis/NewDelete-custom.cpp
  test/Analysis/new-ctor-malloc.cpp

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D41406.127569.patch
Type: text/x-patch
Size: 16952 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20171219/1d485e93/attachment.bin>