[PATCH] D158156: [analyzer] Add C++ array delete checker

This revision was automatically updated to reflect the committed changes. Closed by commit rG71ae858c079f: [clang][analyzer] Rename DeleteWithNonVirtualDtorChecker to CXXDeleteChecker (authored by Viktor Cseh ). Changed prior to commit: https://reviews.llvm.org/D15815

[PATCH] D158156: [analyzer] Add C++ array delete checker

steakhal accepted this revision. steakhal added a comment. This revision is now accepted and ready to land. It's good enough already, given you apply my last suggestion. References would be nice to support for notes, but not a blocker. Comment at: clang/lib/StaticAnalyzer/Check

[PATCH] D158156: [analyzer] Add C++ array delete checker

Discookie marked 5 inline comments as done. Discookie added a comment. Ran the checker on a couple larger projects, but no real-world reports found so far (same as DeleteWithNonVirtualDtor). Can't tell if it's an engine limitation or just the bug being uncommon in general. Co

[PATCH] D158156: [analyzer] Add C++ array delete checker

Discookie updated this revision to Diff 557439. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D158156/new/ https://reviews.llvm.org/D158156 Files: clang/docs/analyzer/checkers.rst clang/include/clang/StaticAnalyzer/Checkers/Checkers.td clang/lib/StaticAnalyzer/Checkers/CMakeLists.tx

[PATCH] D158156: [analyzer] Add C++ array delete checker

Discookie updated this revision to Diff 557438. Discookie added a comment. Replaced backticks with single quotes. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D158156/new/ https://reviews.llvm.org/D158156 Files: clang/docs/analyzer/checkers.rst clang/include/clang/StaticAnalyzer/Ch

[PATCH] D158156: [analyzer] Add C++ array delete checker

steakhal added a comment. The code looks good now. Let's have one last round. Do you have real-world reports? Comment at: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td:762-763 +def CXXArrayDeleteChecker : Checker<"ArrayDelete">, + HelpText<"Reports destructions of a

[PATCH] D158156: [analyzer] Add C++ array delete checker

Discookie added a comment. @steakhal gentle ping for one last round of reviews CHANGES SINCE LAST ACTION https://reviews.llvm.org/D158156/new/ https://reviews.llvm.org/D158156 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.ll

[PATCH] D158156: [analyzer] Add C++ array delete checker

donat.nagy added a comment. Looks good, thanks for the improvements! One minor remark: your commit uses backticks (`) around the class names in the bug reports, which is commonly used to mark code fragments e.g. in commit messages; however, in the bug reports IIRC most other checkers use apostr

[PATCH] D158156: [analyzer] Add C++ array delete checker

Discookie marked 6 inline comments as done. Discookie added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/CXXDeleteChecker.cpp:220 N->getLocationContext()); - return std::make_shared(Pos, OS.str(), true); + return std::make_shared(

[PATCH] D158156: [analyzer] Add C++ array delete checker

Discookie marked 12 inline comments as done. Discookie added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/CXXDeleteChecker.cpp:192 + + const Stmt *S = N->getStmtForDiagnostics(); + if (!S) steakhal wrote: > Discookie wrote: > > steakhal wrote:

[PATCH] D158156: [analyzer] Add C++ array delete checker

Discookie updated this revision to Diff 556933. Discookie added a comment. Updated the visitor to track all conversions, and have type names for clarity. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D158156/new/ https://reviews.llvm.org/D158156 Files: clang/docs/analyzer/checkers.rst

[PATCH] D158156: [analyzer] Add C++ array delete checker

donat.nagy added inline comments. Comment at: clang/test/Analysis/ArrayDelete.cpp:85-88 +Derived *d3 = new DoubleDerived[10]; +Base *b3 = d3; // expected-note{{Conversion from derived to base happened here}} +delete[] b3; // expected-warning{{Deleting an array of pol

[PATCH] D158156: [analyzer] Add C++ array delete checker

steakhal added inline comments. Comment at: clang/test/Analysis/ArrayDelete.cpp:85-88 +Derived *d3 = new DoubleDerived[10]; +Base *b3 = d3; // expected-note{{Conversion from derived to base happened here}} +delete[] b3; // expected-warning{{Deleting an array of polym

[PATCH] D158156: [analyzer] Add C++ array delete checker

donat.nagy added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/CXXDeleteChecker.cpp:47 +protected: + class PtrCastVisitor : public BugReporterVisitor { public: I like that yo switched to a more descriptive class name :) Com

[PATCH] D158156: [analyzer] Add C++ array delete checker

steakhal added a comment. I have concerns mostly about the cast visitor. Comment at: clang/lib/StaticAnalyzer/Checkers/CXXDeleteChecker.cpp:192 + + const Stmt *S = N->getStmtForDiagnostics(); + if (!S) Discookie wrote: > steakhal wrote: > > Aren't you actuall

[PATCH] D158156: [analyzer] Add C++ array delete checker

Discookie added inline comments. Comment at: clang/docs/analyzer/checkers.rst:1793-1803 +.. code-block:: cpp + + Base *create() { + Base *x = new Derived[10]; // note: conversion from derived to base + // happened here + return x; + } --

[PATCH] D158156: [analyzer] Add C++ array delete checker

Discookie updated this revision to Diff 556402. Discookie marked 8 inline comments as done. Discookie added a comment. Added a test for taking last upcast only for the note tag. For now the visitor matches all explicit casts, because there are too many edge cases to count for now wrt. explicit u

[PATCH] D158156: [analyzer] Add C++ array delete checker

donat.nagy added inline comments. Comment at: clang/docs/analyzer/checkers.rst:1793-1803 +.. code-block:: cpp + + Base *create() { + Base *x = new Derived[10]; // note: conversion from derived to base + // happened here + return x; + } -

[PATCH] D158156: [analyzer] Add C++ array delete checker

steakhal added a comment. Add a test where we have multiple imp derived to base casts but only one leads to a bug. Do it for both ways, once that the first is the offending, and once the second. Comment at: clang/docs/analyzer/checkers.rst:1793-1803 +.. code-block:: cpp + + B

[PATCH] D158156: [analyzer] Add C++ array delete checker

Discookie marked 8 inline comments as done. Discookie added a comment. Fixed the formatting issues as well. Comment at: clang/docs/analyzer/checkers.rst:1787-1804 +.. _alpha-cplusplus-ArrayDelete: + +alpha.cplusplus.ArrayDelete (C++) +"""

[PATCH] D158156: [analyzer] Add C++ array delete checker

Discookie updated this revision to Diff 555821. Discookie added a comment. Refactored the checkers to use a shared base class, otherwise I kept the same functionality. I'll push this diff in a way where the blame transfers between the files, but I don't think keeping the file name the same make

[PATCH] D158156: [analyzer] Add C++ array delete checker

donat.nagy added a comment. In D158156#4604242 , @steakhal wrote: > One more thing to mention. Its usually illadvised to rename files or do > changes that would seriously impact git blame, unless we have a really good > reason doing so. > Aesthetics usu

[PATCH] D158156: [analyzer] Add C++ array delete checker

steakhal added a comment. One more thing to mention. Its usually illadvised to rename files or do changes that would seriously impact git blame, unless we have a really good reason doing so. Aesthetics usually don't count one, especially if the name is not user-facing. However, maintainability

[PATCH] D158156: [analyzer] Add C++ array delete checker

steakhal added a comment. I share the raised concerns. And I think we are aligned. PS: sorry if any of my comments are dups, or outdated. I only had a little time last week, and things have progressed since then I noticed. I still decided to submit my possibly outdated and partial review commen

[PATCH] D158156: [analyzer] Add C++ array delete checker

balazske added a comment. I found that this concept of "fake" checkers (that change only options of a common checker instance) has more problems (checker dependencies, callback order, separation of "modeling" and bug report generation independently), it is better to avoid this. It is really the

[PATCH] D158156: [analyzer] Add C++ array delete checker

Discookie planned changes to this revision. Discookie added a comment. Indeed it would make more sense to separate checkers into their own classes, but using a shared base. This checker doesn't have quite as advanced logic as MallocChecker, so there's not that much of a need for a single class m

[PATCH] D158156: [analyzer] Add C++ array delete checker

donat.nagy added a comment. By the way, what would you think about putting the implementation of the two checkers (DeleteWithNonVirtualDtor, ArrayDelete) into two separate checker classes and placing the shared code into a common base class of these classes? I think this could be a significant

[PATCH] D158156: [analyzer] Add C++ array delete checker

donat.nagy added a comment. This checker is a straightforward, clean implementation of the SEI-CERT rule EXP51-CPP, it'll be a good addition to Static Analyzer. I'm satisfied with the implementation, although there might be others who have a more refined taste in C++ coding style. The only issu

[PATCH] D158156: [analyzer] Add C++ array delete checker

Discookie created this revision. Discookie added reviewers: NoQ, donat.nagy, balazske. Discookie added projects: clang, All. Herald added subscribers: steakhal, manas, ASDenysPetrov, martong, dkrupp, Szelethus, mikhail.ramalho, a.sidorin, szepet, baloghadamsoftware, xazax.hun. Discookie requested