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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
