================
@@ -369,24 +393,48 @@ void StackAddrEscapeChecker::checkEndFunction(const
ReturnStmt *RS,
const auto *ReferrerStackSpace =
ReferrerMemSpace->getAs<StackSpaceRegion>();
+
if (!ReferrerStackSpace)
return false;
- if (ReferredMemSpace->getStackFrame() == PoppedFrame &&
- ReferrerStackSpace->getStackFrame()->isParentOf(PoppedFrame)) {
+ if (const auto *ReferredFrame = ReferredMemSpace->getStackFrame();
+ ReferredFrame != PoppedFrame) {
+ return false;
+ }
+
+ if (ReferrerStackSpace->getStackFrame()->isParentOf(PoppedFrame)) {
+ V.emplace_back(Referrer, Referred);
+ return true;
+ }
+ if (isa<StackArgumentsSpaceRegion>(ReferrerMemSpace) &&
+ ReferrerStackSpace->getStackFrame() == PoppedFrame && TopFrame) {
+ // Output parameter of a top-level function
V.emplace_back(Referrer, Referred);
return true;
}
return false;
}
+ void updateInvalidatedRegions(const MemRegion *Region) {
+ if (const auto *SymReg = Region->getAs<SymbolicRegion>()) {
+ SymbolRef Symbol = SymReg->getSymbol();
+ if (const auto *DerS = dyn_cast<SymbolDerived>(Symbol);
+ DerS && isa_and_nonnull<SymbolConjured>(DerS->getParentSymbol())) {
+
InvalidatedRegions.insert(Symbol->getOriginRegion()->getBaseRegion());
+ }
+ }
+ }
----------------
NagyDonat wrote:
I'm uncomfortable with the idea of using `getOriginRegion()` to propagate
invalidation. I can accept using `getOriginRegion()` to insert a variable name
into a bug report error message, because there it has limited potential to
cause harm: it only affects the message.
However, if you accidentally invalidate the wrong variable, then you discard
relevant information from the analyzer state, and that can cause hard-to-debug
false positives. (In general an invalidation FP looks like (1) the analyzer
enters an `if (ptr)` branch, then (2) `ptr` gets invalidated accidentally, then
(3) the analyzer enters an `if (!ptr)` branch, because it forgot its earlier
assumption.)
It is especially problematic to connect this (as far as I see widely applied)
behavior change as the side effect of enabling an innocent-looking checker. The
users would be very surprised if enabling this checker "magically" introduced a
few hard-to-debug confusing false positives (which are emitted by totally
unrelated checkers).
If you really want to propagate invalidation to origin regions, you should
place this in the general invalidation logic of the engine (probably behind an
analyzer config option). However if this is just a "nice to have" extra feature
for your commit, then I'd suggest just skipping it.
(Disclaimer: I'm basing this review on a general "extra invalidations are
problematic" principle, did not exactly analyze all the potential consequences
of this particular source of invalidations. Perhaps @haoNoQ or somebody else
can say more.)
https://github.com/llvm/llvm-project/pull/105648
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
