[PATCH] D105821: [analyzer] [WIP] Model destructor for std::unique_ptr

NoQ added a comment. The code looks great, I don't see any major problems. We still need tests, I can't stress this enough. All the real-world cornercases you've covered here as you updated the patch deserve a test case. Some of these changes should probably be separated into other patches, eg.

[PATCH] D105821: [analyzer] [WIP] Model destructor for std::unique_ptr

RedDocMD updated this revision to Diff 366558. RedDocMD added a comment. Connecting to MallocChecker Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D105821/new/ https://reviews.llvm.org/D105821 Files: clang/lib/StaticAnalyzer/Checkers/SmartPtrMode

[PATCH] D105821: [analyzer] [WIP] Model destructor for std::unique_ptr

NoQ added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:440 + State, {std::make_pair(CC->getCXXThisVal(), ArgVal)}, + C.getLocationContext(), PSK_DirectEscapeOnCall, &Call); RedDocMD wrote: > It seems to m

[PATCH] D105821: [analyzer] [WIP] Model destructor for std::unique_ptr

RedDocMD added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:440 + State, {std::make_pair(CC->getCXXThisVal(), ArgVal)}, + C.getLocationContext(), PSK_DirectEscapeOnCall, &Call); It seems to me that this p

[PATCH] D105821: [analyzer] [WIP] Model destructor for std::unique_ptr

RedDocMD updated this revision to Diff 365031. RedDocMD added a comment. Further pointer escape Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D105821/new/ https://reviews.llvm.org/D105821 Files: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.

[PATCH] D105821: [analyzer] [WIP] Model destructor for std::unique_ptr

RedDocMD updated this revision to Diff 364490. RedDocMD added a comment. Never gonna give you up. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D105821/new/ https://reviews.llvm.org/D105821 Files: clang/lib/StaticAnalyzer/Checkers/SmartPtrModelin

[PATCH] D105821: [analyzer] [WIP] Model destructor for std::unique_ptr

RedDocMD updated this revision to Diff 364486. RedDocMD added a comment. Bug fix in modelling Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D105821/new/ https://reviews.llvm.org/D105821 Files: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cp

[PATCH] D105821: [analyzer] [WIP] Model destructor for std::unique_ptr

NoQ added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:298 + } + return {State, true}; +} Something's not right. Returning `true` here would discard the state and terminate `evalCall` as failed. Why compute the invalidate

[PATCH] D105821: [analyzer] [WIP] Model destructor for std::unique_ptr

RedDocMD added a comment. I have incorporated the bug-fixes suggested last meeting (except the pointer escape one). And it seems to have had dramatic results - now the only extra errors being reported are the pointer escape ones (5 of them, from 3 different projects). Some projects are actually

[PATCH] D105821: [analyzer] [WIP] Model destructor for std::unique_ptr

RedDocMD updated this revision to Diff 364074. RedDocMD added a comment. Better modelling, bug fixes Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D105821/new/ https://reviews.llvm.org/D105821 Files: clang/lib/StaticAnalyzer/Checkers/SmartPtrMode

[PATCH] D105821: [analyzer] [WIP] Model destructor for std::unique_ptr

NoQ added a comment. > Well some of them are exactly the same type as the `Lame` class example > above. Like: > `simbody/report-TestArray.cpp-testMoveConstructionAndAssignment-27-1.html#EndPath`. > (So the incomplete modelling of the destructor is at least one cause. The > other reason that yo

[PATCH] D105821: [analyzer] [WIP] Model destructor for std::unique_ptr

RedDocMD added a comment. Well some of them are exactly the same type as the `Lame` class example above. Like: `simbody/report-TestArray.cpp-testMoveConstructionAndAssignment-27-1.html#EndPath`. (So the incomplete modelling of the destructor is at least one cause. The other reason that you sug

[PATCH] D105821: [analyzer] [WIP] Model destructor for std::unique_ptr

NoQ added a comment. In D105821#2914082 , @RedDocMD wrote: > On running this patch on the `projects` directory, a bunch of projects emit > false-positives: mostly of the form `Potential memory leak`. This points to > the fact that without calling the de

[PATCH] D105821: [analyzer] [WIP] Model destructor for std::unique_ptr

RedDocMD updated this revision to Diff 363138. RedDocMD added a comment. Invalidating using inner pointer destructor call Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D105821/new/ https://reviews.llvm.org/D105821 Files: clang/lib/StaticAnalyzer/

[PATCH] D105821: [analyzer] [WIP] Model destructor for std::unique_ptr

RedDocMD added a comment. On running this patch on the `projects` directory, a bunch of projects emit false-positives: mostly of the form `Potential memory leak`. This points to the fact that without calling the destructor of the pointee type, we are going to have a lot of false positives (408

[PATCH] D105821: [analyzer] [WIP] Model destructor for std::unique_ptr

RedDocMD updated this revision to Diff 362741. RedDocMD added a comment. Put in a TODO Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D105821/new/ https://reviews.llvm.org/D105821 Files: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp cla

[PATCH] D105821: [analyzer] [WIP] Model destructor for std::unique_ptr

RedDocMD added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:381 +const MemRegion *ThisRegion = DC->getCXXThisVal().getAsRegion(); +assert(ThisRegion && "We do not support explicit calls to destructor"); +const auto *InnerPtrVal =

[PATCH] D105821: [analyzer] [WIP] Model destructor for std::unique_ptr

vsavchenko added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:381 +const MemRegion *ThisRegion = DC->getCXXThisVal().getAsRegion(); +assert(ThisRegion && "We do not support explicit calls to destructor"); +const auto *InnerPtrVal

[PATCH] D105821: [analyzer] [WIP] Model destructor for std::unique_ptr

RedDocMD updated this revision to Diff 362738. RedDocMD added a comment. Bug fixes, some cleanup Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D105821/new/ https://reviews.llvm.org/D105821 Files: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling

[PATCH] D105821: [analyzer] [WIP] Model destructor for std::unique_ptr

RedDocMD added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:143 +llvm::ArrayRef ValidNames(STD_PTR_NAMES); +return llvm::is_contained(ValidNames, Name); } vsavchenko wrote: > And why can't we pass `STD_PTR_NAMES`

[PATCH] D105821: [analyzer] [WIP] Model destructor for std::unique_ptr

RedDocMD added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:421-422 const auto *TrackingExpr = Call.getArgExpr(0); - assert(TrackingExpr->getType()->isPointerType() && - "Adding a non pointer value to TrackedRegionMap

[PATCH] D105821: [analyzer] [WIP] Model destructor for std::unique_ptr

vsavchenko added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:143 +llvm::ArrayRef ValidNames(STD_PTR_NAMES); +return llvm::is_contained(ValidNames, Name); } And why can't we pass `STD_PTR_NAMES` directly to `llvm:

[PATCH] D105821: [analyzer] [WIP] Model destructor for std::unique_ptr

NoQ added a comment. Regardless of the kind of pointer, sounds like we need to do something about that API quirk. Eg., it *must* be possible to model a destructor of a `std::unique_ptr` as a no-op when the tracked raw pointer value is an `UnknownVal`. Repository: rG LLVM Github Monorepo CH

[PATCH] D105821: [analyzer] [WIP] Model destructor for std::unique_ptr

RedDocMD added a comment. Ah I see. As a side note, without the "redundant" invalidation that is being done, the analyzer crashes on `shared_ptr`. (Because the `State` essentially remains the same and that's what causes the crash). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTI

[PATCH] D105821: [analyzer] [WIP] Model destructor for std::unique_ptr

NoQ added a comment. No, that's the wrong destructor. We don't want to invalidate the smart pointer; we've already modeled it precisely. What i meant was construct a new `CallEvent` (through `CallEventManager`) for the destructor of the //pointee// and use that. Repository: rG LLVM Github M

[PATCH] D105821: [analyzer] [WIP] Model destructor for std::unique_ptr

RedDocMD updated this revision to Diff 362310. RedDocMD added a comment. Invalidating via the CallEvent Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D105821/new/ https://reviews.llvm.org/D105821 Files: clang/lib/StaticAnalyzer/Checkers/SmartPtrM

[PATCH] D105821: [analyzer] [WIP] Model destructor for std::unique_ptr

NoQ added a comment. In D105821#2904870 , @NoQ wrote: > `std::make_unique` isn't actually getting modeled but inlined instead (I have > all your patches pulled and this patch applied; and the > object-under-construction seems to be available later, in b

[PATCH] D105821: [analyzer] [WIP] Model destructor for std::unique_ptr

NoQ added a comment. In D105821#2903606 , @RedDocMD wrote: > The following code **emits** a warning for leaked memory: > ... > Why does the following command not display the warnings? Wait, what's the difference between this command and the command that

[PATCH] D105821: [analyzer] [WIP] Model destructor for std::unique_ptr

RedDocMD updated this revision to Diff 361581. RedDocMD added a comment. Removed a fatal bug Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D105821/new/ https://reviews.llvm.org/D105821 Files: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp

[PATCH] D105821: [analyzer] [WIP] Model destructor for std::unique_ptr

RedDocMD added a comment. In D105821#2897006 , @NoQ wrote: >> the following code doesn't emit any warnings > > This code doesn't seem to have any `unique_ptr`s in it? It's not like you're > modeling this custom class as well? Can you try the same with th

[PATCH] D105821: [analyzer] [WIP] Model destructor for std::unique_ptr

NoQ added a comment. > the following code doesn't emit any warnings This code doesn't seem to have any `unique_ptr`s in it? It's not like you're modeling this custom class as well? Can you try the same with the actual `unique_ptr`? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACT

[PATCH] D105821: [analyzer] [WIP] Model destructor for std::unique_ptr

RedDocMD added a comment. > But before we go there we should decide whether we want to actually go for > inlining (or otherwise default-evaluating) these destructors. If we do, we > should probably not spend too much time on improving invalidation in the > checker, because default evaluation wo

[PATCH] D105821: [analyzer] [WIP] Model destructor for std::unique_ptr

NoQ added a comment. Yes, I think this should work. You're invalidating less regions than a normal destructor invalidation would have caused (eg., you're not touching globals). One way to emulate that precisely would be to construct a `CallEvent` for the destructor and invoke `CallEvent::inval

[PATCH] D105821: [analyzer] [WIP] Model destructor for std::unique_ptr

RedDocMD added a comment. This is a minimal model of destructors in smart-ptr. Other than the need to probably model the destructor of the pointee, is there anything else to do? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D105821/new/ https://rev

[PATCH] D105821: [analyzer] [WIP] Model destructor for std::unique_ptr

RedDocMD updated this revision to Diff 360218. RedDocMD added a comment. Minimal modelling of destructor Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D105821/new/ https://reviews.llvm.org/D105821 Files: clang/lib/StaticAnalyzer/Checkers/SmartPtr

[PATCH] D105821: [analyzer] [WIP] Model destructor for std::unique_ptr

RedDocMD updated this revision to Diff 360202. RedDocMD added a comment. Reformat Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D105821/new/ https://reviews.llvm.org/D105821 Files: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp clang/li

[PATCH] D105821: [analyzer] [WIP] Model destructor for std::unique_ptr

RedDocMD updated this revision to Diff 360199. RedDocMD added a comment. Retrieving patch Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D105821/new/ https://reviews.llvm.org/D105821 Files: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp

[PATCH] D105821: [analyzer] [WIP] Model destructor for std::unique_ptr

RedDocMD updated this revision to Diff 358998. RedDocMD added a comment. Removed one bug, many more to go Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D105821/new/ https://reviews.llvm.org/D105821 Files: clang/lib/StaticAnalyzer/Checkers/SmartPt

[PATCH] D105821: [analyzer] [WIP] Model destructor for std::unique_ptr

RedDocMD updated this revision to Diff 358839. RedDocMD added a comment. Cleanup, still doesn't work Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D105821/new/ https://reviews.llvm.org/D105821 Files: clang/lib/StaticAnalyzer/Checkers/SmartPtrMode

[PATCH] D105821: [analyzer] [WIP] Model destructor for std::unique_ptr

RedDocMD created this revision. RedDocMD added reviewers: NoQ, vsavchenko, xazax.hun, teemperor. Herald added subscribers: manas, steakhal, ASDenysPetrov, martong, dkrupp, donat.nagy, Szelethus, mikhail.ramalho, a.sidorin, rnkovacs, szepet, baloghadamsoftware. RedDocMD requested review of this re