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

[Artem Dergachev via Phabricator via cfe-commits]

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::invalidateRegions()` on it, which should be relatively easy given that we don't need a pointee destructor expression for this to work (because destructors don't typically have expressions anyway; so this will be much harder in case of `make_unique` and the constructor as the constructor would demand a construct-expression).

Also it makes sense to omit the invalidation when the pointee type doesn't have a destructor.

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 would do that properly for us anyway (well, it doesn't really dodge any problems such as the absence of the necessary AST so we'll probably have to solve all these problems anyway, just in a different setting). So it's great that we've fixed `evalCall` for destructors, this could definitely land as a separate patch (tested via `debug.AnalysisOrder`), but we really need to think what to do next here. So I recommend gathering some data to see if proper destructor evaluation is actually a real problem.



================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:375
+    State = State->remove<TrackedRegionMap>(ThisRegion);
+    State = State->invalidateRegions({*InnerPtrVal}, nullptr, C.blockCount(),
+                                     C.getLocationContext(), true);
----------------
`{}` are unnecessary because `llvm::ArrayRef` is [[ https://llvm.org/doxygen/classllvm_1_1ArrayRef.html#a3b1f44186f9787d7ffacb54b62d6798c | implicitly constructible out of a single value ]].

The null expression situation shouldn't be too harmful given that we've already doing this for conservatively evaluated destructors outside of `evalCall()`. That said, it's still actively incorrect because given that it's part of the symbol's identity, it causes us to use the same abstract symbol for different actual runtime values. I guess a proper fix would involve updating the identity of a `SymbolConjured` to include a `CFGElementRef` instead of a statement. Or, well, building a better `SVal` kind (or maybe even a non-value "marker") specifically for invalidation purposes, which would capture an explanation for invalidation and the role that the value played in it (was it an unknown return value? an unknown out-parameter value? a default value covering invalidated globals? a checker-specific value?) which we could introspect later (say, for suppression purposes). This doesn't seem to be urgent though.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D105821/new/

https://reviews.llvm.org/D105821