https://github.com/steakhal created https://github.com/llvm/llvm-project/pull/170887
The conservative call invalidation logic is a bit complicated, and would deserve some refactoring. When a call has some arguments, we escape them. Except, if its a pointer to constant storage - because we assume that the program honors const-correctness. In that case, it puts it in the "Preserved" list to keep its contents. However, if we had a constructor call that's job is to initialize an object had a const pointer/reference parameter then the invalidation didn't take place. This meant that if the object was on the stack, that we start warning about uninitialized fields when accessed. (See the example) Similar could be achieved on the heap of course. We should have honored the fact that the constructor should initialize the pointee of "this", thus escape that region regardless (in other words, don't put it on the "preserved" list). rdar://156942972 From 96d5f7c1ab3daf6469203617875f88cb65b170ff Mon Sep 17 00:00:00 2001 From: Balazs Benics <[email protected]> Date: Fri, 5 Dec 2025 17:55:25 +0100 Subject: [PATCH] [analyzer] Invalidate the object in opaque ctor calls regardless if an arg refers to it The conservative call invalidation logic is a bit complicated, and would deserve some refactoring. When a call has some arguments, we escape them. Except, if its a pointer to constant storage - because we assume that the program honors const-correctness. In that case, it puts it in the "Preserved" list to keep its contents. However, if we had a constructor call that's job is to initialize an object had a const pointer/reference parameter then the invalidation didn't take place. This meant that if the object was on the stack, that we start warning about uninitialized fields when accessed. (See the example) Similar could be achieved on the heap of course. We should have honored the fact that the constructor should initialize the pointee of "this", thus escape that region regardless (in other words, don't put it on the "preserved" list). rdar://156942972 --- clang/lib/StaticAnalyzer/Core/CallEvent.cpp | 30 +++++++++++++++++---- clang/test/Analysis/call-invalidation.cpp | 18 +++++++++++++ 2 files changed, 43 insertions(+), 5 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Core/CallEvent.cpp b/clang/lib/StaticAnalyzer/Core/CallEvent.cpp index d04c827ce1391..04b0db0d1ffef 100644 --- a/clang/lib/StaticAnalyzer/Core/CallEvent.cpp +++ b/clang/lib/StaticAnalyzer/Core/CallEvent.cpp @@ -229,6 +229,14 @@ static void findPtrToConstParams(llvm::SmallSet<unsigned, 4> &PreserveArgs, } } +static const MemRegion *getThisRegionBaseOrNull(const CallEvent &Call) { + if (const auto *CtorCall = dyn_cast<CXXConstructorCall>(&Call)) { + if (const MemRegion *R = CtorCall->getCXXThisVal().getAsRegion()) + return R->getBaseRegion(); + } + return nullptr; +} + ProgramStateRef CallEvent::invalidateRegions(unsigned BlockCount, ProgramStateRef State) const { // Don't invalidate anything if the callee is marked pure/const. @@ -246,14 +254,26 @@ ProgramStateRef CallEvent::invalidateRegions(unsigned BlockCount, if (!argumentsMayEscape()) findPtrToConstParams(PreserveArgs, *this); + // We should not preserve the contents of the region pointed by "this" when + // constructing the object, even if an argument refers to it. + const auto *ThisRegionBaseOrNull = getThisRegionBaseOrNull(*this); + for (unsigned Idx = 0, Count = getNumArgs(); Idx != Count; ++Idx) { // Mark this region for invalidation. We batch invalidate regions // below for efficiency. - if (PreserveArgs.count(Idx)) - if (const MemRegion *MR = getArgSVal(Idx).getAsRegion()) - ETraits.setTrait(MR->getBaseRegion(), - RegionAndSymbolInvalidationTraits::TK_PreserveContents); - // TODO: Factor this out + handle the lower level const pointers. + if (PreserveArgs.count(Idx)) { + if (const MemRegion *ArgBaseR = getArgSVal(Idx).getAsRegion()) { + ArgBaseR = ArgBaseR->getBaseRegion(); + + // Preserve the contents of the pointee of the argument - except if it + // refers to the object under construction (ctor call). + if (ArgBaseR != ThisRegionBaseOrNull) { + ETraits.setTrait( + ArgBaseR, RegionAndSymbolInvalidationTraits::TK_PreserveContents); + // TODO: Factor this out + handle the lower level const pointers. + } + } + } ValuesToInvalidate.push_back(getArgSVal(Idx)); diff --git a/clang/test/Analysis/call-invalidation.cpp b/clang/test/Analysis/call-invalidation.cpp index cb503023a1071..84970dcae18af 100644 --- a/clang/test/Analysis/call-invalidation.cpp +++ b/clang/test/Analysis/call-invalidation.cpp @@ -280,3 +280,21 @@ int testNestedStdNamespacesAndRecords() { int y = obj.uninit; // expected-warning {{Assigned value is uninitialized}} return x + y; } + +struct SpecialVector { + SpecialVector(const void *); // Takes a const pointer! + int size() const { + return Size; // no-warning: We should not warn "uninitialized Size" because the ctor might have initialized it. + } + int Size; +}; + +void selfPtrPassedAsConstPointerToOpaqueCtorCall() { + // We construct a "SpecialVector" that takes the address of itself + // (or to a subobject somewhere itself) by a const-pointer. + // Despite the var region "buf" is mentioned via a const argument, the opaque + // ctor cal should still take presecedent and invalidate the underlying object. + SpecialVector buf(&buf); + buf.size(); +} + _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
