[PATCH] D118880: [analyzer] Improve NoOwnershipChangeVisitor's understanding of deallocators

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

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

The problem with leak bug reports is that the most interesting event in the code is likely the one that did //not// happen -- lack of ownership change and lack of deallocation, which is often present within the same function that the analyzer inlined anyway, but not on the path of execution on which the bug occured. We struggle to understand that a function was responsible for freeing the memory, but failed.

D105819 <https://reviews.llvm.org/D105819> added a new visitor to improve memory leak bug reports. In addition to inspecting the ExplodedNodes of the bug pat, the visitor tries to guess whether the function was supposed to free memory, but failed to. Initially (in D108753 <https://reviews.llvm.org/D108753>), this was done by checking whether a `CXXDeleteExpr` is present in the function. If so, we assume that the function was at least party responsible, and prevent the analyzer from pruning bug report notes in it. This patch improves this heuristic by recognizing all deallocator functions that MallocChecker itself recognizes, by reusing `MallocChecker::isFreeingCall`.

However, we are only able to match a `CallEvent` against a `CallDescription`. `CallEvent`s are created during symbolic execution, providing a lot of additional information about the call, but the grand idea behind this visitor that it checks code that was //not// executed smybolically (well, not on the path of execution on which the bug was found). For this reason, I added a set of functions to allow matching a `CallExpr` against a `CallDescription`.

I am aware that this idea may induce strong opinions -- after all, I'm adding a a new interface called `.*Imprecise`, discourage people from using it in the comments, while we already have one too many confusing interfaces in the analyzer, not to mention that `CallDescription` was meant to be among the better documented, newcomer friendlier and more intuitive tools we have. Also, its possible that matching function calls in syntactic-only cases should be done by `StdLibraryFunctionChecker::Signature` instead.

I argue for this patch because

- A lot of checkers use `CallDescriptionMap` already, and we either never intend to change to `Signature`, or it would be an enormous effort upfront
- Leak checkers like `StreamChecker` could benefit from this as well
- The comments I believe resolve the unintuitiveness of the new interface well
- In the case of this patch, matching `CallEvent`s for plain C function calls is pretty much matching against a `CallExpr` anyways.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D118880

Files:
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h
  clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
  clang/lib/StaticAnalyzer/Core/CallDescription.cpp
  clang/test/Analysis/NewDeleteLeaks.cpp

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D118880.405541.patch
Type: text/x-patch
Size: 11507 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20220203/ec5fdda1/attachment-0001.bin>