[PATCH] D71612: [analyzer] Add PlacementNewChecker

martong marked 2 inline comments as done. martong added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp:1 +#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h" +#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h" --

[PATCH] D71612: [analyzer] Add PlacementNewChecker

Szelethus added inline comments. Herald added subscribers: ASDenysPetrov, steakhal. Comment at: clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp:1 +#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h" +#include "clang/StaticAnalyzer/Core/BugReporter/BugTyp

[PATCH] D71612: [analyzer] Add PlacementNewChecker

NoQ added a comment. 🎉🎉🎉 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71612/new/ https://reviews.llvm.org/D71612 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/

[PATCH] D71612: [analyzer] Add PlacementNewChecker

martong added a comment. In D71612#1828059 , @NoQ wrote: > This looks fantastic. Let's enable by it default! Ok, I've done that: https://github.com/llvm/llvm-project/commit/bc29069dc40 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D71612: [analyzer] Add PlacementNewChecker

NoQ added a comment. This looks fantastic. Let's enable by it default! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71612/new/ https://reviews.llvm.org/D71612 ___ cfe-commits mailing list cfe-commits@

[PATCH] D71612: [analyzer] Add PlacementNewChecker

martong added a comment. In D71612#1812476 , @NoQ wrote: > In D71612#1812036 , @martong wrote: > > > In D71612#1790045 , @NoQ wrote: > > > > > I wonder if this checker will f

[PATCH] D71612: [analyzer] Add PlacementNewChecker

martong added a comment. Related commit that moves to alpha: https://github.com/llvm/llvm-project/commit/13ec473b9d4bd4f7a558272932b7c0806171c666 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71612/new/ https://reviews.llvm.org/D71612 __

[PATCH] D71612: [analyzer] Add PlacementNewChecker

NoQ added a comment. In D71612#1814410 , @martong wrote: > Ups, I am sorry. Now I am creating another commit which moves it to alpha. > (So then, If I understand it correctly then the `cplusplus` is enabled by > default, but `alpha` is not.) Aha, yup.

[PATCH] D71612: [analyzer] Add PlacementNewChecker

martong added a comment. In D71612#1814225 , @NoQ wrote: > In D71612#1813937 , @martong wrote: > > > I am going to execute the analysis on LLVM/Clang as you suggested, so then > > we'll see what the experiment will

[PATCH] D71612: [analyzer] Add PlacementNewChecker

This revision was automatically updated to reflect the committed changes. Closed by commit rG5e7beb0a4146: [analyzer] Add PlacementNewChecker (authored by martong). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71612/new/ https://reviews.llvm.org/D7

[PATCH] D71612: [analyzer] Add PlacementNewChecker

NoQ added a comment. In D71612#1813937 , @martong wrote: > I am going to execute the analysis on LLVM/Clang as you suggested, so then > we'll see what the experiment will bring us. And then we could enable it by > default I think. BTW how can we do that?

[PATCH] D71612: [analyzer] Add PlacementNewChecker

martong marked 2 inline comments as done. martong added a comment. > Can we enable the check by default then? What pieces are missing? Like, the > symbolic extent/offset case is not a must. I think we have everything except > maybe some deeper testing. I'd rather spend some time on the above exe

[PATCH] D71612: [analyzer] Add PlacementNewChecker

martong updated this revision to Diff 237290. martong added a comment. - Declare ElementCountNL in if stmt - Use makeArrayIndex instead makeIntVal Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71612/new/ https://reviews.llvm.org/D71612 Files: cl

[PATCH] D71612: [analyzer] Add PlacementNewChecker

NoQ added a comment. In D71612#1812036 , @martong wrote: > In D71612#1790045 , @NoQ wrote: > > > I wonder if this checker will find any misuses of placement into > > `llvm::TrailingObjects`. > > > I've evaluated th

[PATCH] D71612: [analyzer] Add PlacementNewChecker

xazax.hun accepted this revision. xazax.hun added inline comments. This revision is now accepted and ready to land. Comment at: clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp:91 + SVal SizeOfPlace = getExtentSizeOfPlace(C, Place, State); + const auto SizeOfTargetCI = S

[PATCH] D71612: [analyzer] Add PlacementNewChecker

NoQ added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp:91 + SVal SizeOfPlace = getExtentSizeOfPlace(C, Place, State); + const auto SizeOfTargetCI = SizeOfTarget.getAs(); + if (!SizeOfTargetCI) xazax.hun wrote: > martong

[PATCH] D71612: [analyzer] Add PlacementNewChecker

xazax.hun added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp:91 + SVal SizeOfPlace = getExtentSizeOfPlace(C, Place, State); + const auto SizeOfTargetCI = SizeOfTarget.getAs(); + if (!SizeOfTargetCI) martong wrote: > xaza

[PATCH] D71612: [analyzer] Add PlacementNewChecker

martong added a comment. In D71612#1790045 , @NoQ wrote: > I wonder if this checker will find any misuses of placement into > `llvm::TrailingObjects`. I've evaluated this checker on LLVM/Clang source and it did not find any results, though there are ma

[PATCH] D71612: [analyzer] Add PlacementNewChecker

martong updated this revision to Diff 237059. martong marked 11 inline comments as done. martong added a comment. - Add documentation to checkers.rst - Pass CheckerContext as last param - Use BugType and default member initializer Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D71612: [analyzer] Add PlacementNewChecker

martong added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp:91 + SVal SizeOfPlace = getExtentSizeOfPlace(C, Place, State); + const auto SizeOfTargetCI = SizeOfTarget.getAs(); + if (!SizeOfTargetCI) xazax.hun wrote: > Here

[PATCH] D71612: [analyzer] Add PlacementNewChecker

NoQ added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp:32 + const MemRegion *BaseRegion = MRegion->getBaseRegion(); + assert(BaseRegion == Offset.getRegion()); + martong wrote: > This assertion fails on real code quite of

[PATCH] D71612: [analyzer] Add PlacementNewChecker

xazax.hun added inline comments. Comment at: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td:472 +def PlacementNewChecker : Checker<"PlacementNew">, + HelpText<"Check if default placement new is provided with pointers to " + "sufficient storage capacity">,

[PATCH] D71612: [analyzer] Add PlacementNewChecker

martong updated this revision to Diff 234901. martong added a comment. - Better handling of unknown values Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71612/new/ https://reviews.llvm.org/D71612 Files: clang/include/clang/StaticAnalyzer/Checker

[PATCH] D71612: [analyzer] Add PlacementNewChecker

martong marked an inline comment as done. martong added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp:32 + const MemRegion *BaseRegion = MRegion->getBaseRegion(); + assert(BaseRegion == Offset.getRegion()); + This assertio

[PATCH] D71612: [analyzer] Add PlacementNewChecker

martong marked 22 inline comments as done. martong added a comment. Thank you guys for the assiduous review! Comment at: clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp:22 +ProgramStateRef State) const; + mutable std::unique_ptr BT_Placement

[PATCH] D71612: [analyzer] Add PlacementNewChecker

martong updated this revision to Diff 234857. martong marked 8 inline comments as done. martong added a comment. - Bugtype by value - Decompose Place to base region and offset - Add test for type hierarchy - Remove State check - Return directly with the size of the type in case of non-array new -

[PATCH] D71612: [analyzer] Add PlacementNewChecker

NoQ added a comment. I wonder if this checker will find any misuses of placement into `llvm::TrailingObjects`. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71612/new/ https://reviews.llvm.org/D71612

[PATCH] D71612: [analyzer] Add PlacementNewChecker

NoQ added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp:40 +if (const auto *ERegion = dyn_cast(TVRegion)) { + const auto *SRegion = cast(ERegion->getSuperRegion()); + DefinedOrUnknownSVal Extent = SRegion->getExtent(svalBuilder

[PATCH] D71612: [analyzer] Add PlacementNewChecker

xazax.hun added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp:22 +ProgramStateRef State) const; + mutable std::unique_ptr BT_Placement; +}; I think now it is safe to have the bugtype by value and

[PATCH] D71612: [analyzer] Add PlacementNewChecker

whisperity added a comment. Are buffers otherwise modelled by the Analyser, such as results of the `malloc` family and `alloca` intentionally not matched, or are there some tests missing regarding this? Comment at: clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp:115-1

[PATCH] D71612: [analyzer] Add PlacementNewChecker

martong created this revision. martong added reviewers: NoQ, Szelethus. Herald added subscribers: cfe-commits, Charusso, gamesh411, dkrupp, donat.nagy, mikhail.ramalho, a.sidorin, rnkovacs, szepet, baloghadamsoftware, xazax.hun, whisperity, mgorny. Herald added a project: clang. This checker ver