NoQ created this revision.
NoQ added reviewers: dcoughlin, xazax.hun, a_sidorin, rnkovacs, Szelethus,
baloghadamsoftware, Charusso.
Herald added subscribers: cfe-commits, dkrupp, donat.nagy, mikhail.ramalho,
a.sidorin, szepet.
Herald added a project: clang.
Enabling pointer escape notification for the return value invalidation when
conservatively evaluating calls that return C++ objects by value was supposed
to be pointless. Indeed, we're looking at a freshly conjured value; why would
any checker already track it at this point? Yet it does cause a change in
results, so i decided to reduce and investigate a reproducer @Charusso sent me.
When i was thinking about it previously (as of D44131
<https://reviews.llvm.org/D44131>), i was imagining a function that constructs
a temporary that would later be copied to its target destination. In this case
there's indeed no need to notify checkers of pointer escape.
However, once RVO was implemented (D47671 <https://reviews.llvm.org/D47671>),
the target region is no longer necessarily a temporary; it may be an arbitrary
memory region. In particular, it may be a non-base region, such as
`FieldRegion` if the construction context is a
`ConstructorInitializerConstructionContext`. In this case the invalidation
covers not only the target region but its whole base region that may already
contain some values that might as well be tracked by the checkers.
The right solution, therefore, is to restrict invalidation so that it didn't
propagate to the base region, but only wipe the target region itself. It
probably doesn't work perfectly because RegionStore doesn't enjoy this sort of
stuff, but it should be something.
In the newly added test case the previous behavior was to immediately forget
about `b1.p` and `b2.p`, therefore evals on lines 21 and 23 yielded both `TRUE`
and `FALSE` each (due to eagerly-assume) and the leak was diagnosed on line 22
(even though the pointer is used later).
Repository:
rC Clang
https://reviews.llvm.org/D63968
Files:
clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
clang/test/Analysis/rvo.cpp
Index: clang/test/Analysis/rvo.cpp
===================================================================
--- /dev/null
+++ clang/test/Analysis/rvo.cpp
@@ -0,0 +1,25 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker core,cplusplus \
+// RUN: -analyzer-checker debug.ExprInspection -verify %s
+
+void clang_analyzer_eval(bool);
+
+struct A {
+ int x;
+};
+
+A getA();
+
+struct B {
+ int *p;
+ A a;
+
+ B(int *p) : p(p), a(getA()) {}
+};
+
+void foo() {
+ B b1(nullptr);
+ clang_analyzer_eval(b1.p == nullptr); // expected-warning{{TRUE}}
+ B b2(new int); // No leak yet!
+ clang_analyzer_eval(b2.p == nullptr); // expected-warning{{FALSE}}
+ // expected-warning@-1{{Potential leak of memory pointed to by 'b2.p'}}
+}
Index: clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
===================================================================
--- clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
+++ clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
@@ -634,12 +634,19 @@
std::tie(State, Target) =
prepareForObjectConstruction(Call.getOriginExpr(), State, LCtx,
RTC->getConstructionContext(), CallOpts);
- assert(Target.getAsRegion());
- // Invalidate the region so that it didn't look uninitialized. Don't notify
- // the checkers.
- State = State->invalidateRegions(Target.getAsRegion(), E, Count, LCtx,
+ const MemRegion *TargetR = Target.getAsRegion();
+ assert(TargetR);
+ // Invalidate the region so that it didn't look uninitialized. If this is
+ // a field or element constructor, we do not want to invalidate
+ // the whole structure. Pointer escape is meaningless because
+ // the structure is a product of conservative evaluation
+ // and therefore contains nothing interesting at this point.
+ RegionAndSymbolInvalidationTraits ITraits;
+ ITraits.setTrait(TargetR,
+ RegionAndSymbolInvalidationTraits::TK_DoNotInvalidateSuperRegion);
+ State = State->invalidateRegions(TargetR, E, Count, LCtx,
/* CausedByPointerEscape=*/false, nullptr,
- &Call, nullptr);
+ &Call, &ITraits);
R = State->getSVal(Target.castAs<Loc>(), E->getType());
} else {
Index: clang/test/Analysis/rvo.cpp
===================================================================
--- /dev/null
+++ clang/test/Analysis/rvo.cpp
@@ -0,0 +1,25 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker core,cplusplus \
+// RUN: -analyzer-checker debug.ExprInspection -verify %s
+
+void clang_analyzer_eval(bool);
+
+struct A {
+ int x;
+};
+
+A getA();
+
+struct B {
+ int *p;
+ A a;
+
+ B(int *p) : p(p), a(getA()) {}
+};
+
+void foo() {
+ B b1(nullptr);
+ clang_analyzer_eval(b1.p == nullptr); // expected-warning{{TRUE}}
+ B b2(new int); // No leak yet!
+ clang_analyzer_eval(b2.p == nullptr); // expected-warning{{FALSE}}
+ // expected-warning@-1{{Potential leak of memory pointed to by 'b2.p'}}
+}
Index: clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
===================================================================
--- clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
+++ clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
@@ -634,12 +634,19 @@
std::tie(State, Target) =
prepareForObjectConstruction(Call.getOriginExpr(), State, LCtx,
RTC->getConstructionContext(), CallOpts);
- assert(Target.getAsRegion());
- // Invalidate the region so that it didn't look uninitialized. Don't notify
- // the checkers.
- State = State->invalidateRegions(Target.getAsRegion(), E, Count, LCtx,
+ const MemRegion *TargetR = Target.getAsRegion();
+ assert(TargetR);
+ // Invalidate the region so that it didn't look uninitialized. If this is
+ // a field or element constructor, we do not want to invalidate
+ // the whole structure. Pointer escape is meaningless because
+ // the structure is a product of conservative evaluation
+ // and therefore contains nothing interesting at this point.
+ RegionAndSymbolInvalidationTraits ITraits;
+ ITraits.setTrait(TargetR,
+ RegionAndSymbolInvalidationTraits::TK_DoNotInvalidateSuperRegion);
+ State = State->invalidateRegions(TargetR, E, Count, LCtx,
/* CausedByPointerEscape=*/false, nullptr,
- &Call, nullptr);
+ &Call, &ITraits);
R = State->getSVal(Target.castAs<Loc>(), E->getType());
} else {
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
