r176144 - [analyzer] If a struct has a partial lazy binding, its fields aren't Undef.
Jordan Rose
jordan_rose at apple.com
Tue Feb 26 16:05:29 PST 2013
Author: jrose
Date: Tue Feb 26 18:05:29 2013
New Revision: 176144
URL: http://llvm.org/viewvc/llvm-project?rev=176144&view=rev
Log:
[analyzer] If a struct has a partial lazy binding, its fields aren't Undef.
This is essentially the same problem as r174031: a lazy binding for the first
field of a struct may stomp on an existing default binding for the
entire struct. Because of the way RegionStore is set up, we can't help
but lose the top-level binding, but then we need to make sure that accessing
one of the other fields doesn't come back as Undefined.
In this case, RegionStore is now correctly detecting that the lazy binding
we have isn't the right type, but then failing to follow through on the
implications of that: we don't know anything about the other fields in the
aggregate. This fix adds a test when searching for other kinds of default
values to see if there's a lazy binding we rejected, and if so returns
a symbolic value instead of Undefined.
The long-term fix for this is probably a new Store model; see
<rdar://problem/12701038>.
Fixes <rdar://problem/13292559>.
Modified:
cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
cfe/trunk/lib/StaticAnalyzer/Core/RegionStore.cpp
cfe/trunk/test/Analysis/uninit-vals.m
Modified: cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp?rev=176144&r1=176143&r2=176144&view=diff
==============================================================================
--- cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp (original)
+++ cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp Tue Feb 26 18:05:29 2013
@@ -222,6 +222,11 @@ public:
// Don't print any more notes after this one.
Mode = Satisfied;
+ // Ignore aggregate rvalues.
+ if (V.getAs<nonloc::LazyCompoundVal>() ||
+ V.getAs<nonloc::CompoundVal>())
+ return 0;
+
const Expr *RetE = Ret->getRetValue();
assert(RetE && "Tracking a return value for a void function");
RetE = RetE->IgnoreParenCasts();
Modified: cfe/trunk/lib/StaticAnalyzer/Core/RegionStore.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Core/RegionStore.cpp?rev=176144&r1=176143&r2=176144&view=diff
==============================================================================
--- cfe/trunk/lib/StaticAnalyzer/Core/RegionStore.cpp (original)
+++ cfe/trunk/lib/StaticAnalyzer/Core/RegionStore.cpp Tue Feb 26 18:05:29 2013
@@ -487,7 +487,10 @@ public: // Part of public interface to c
NonLoc createLazyBinding(RegionBindingsConstRef B, const TypedValueRegion *R);
/// Used to lazily generate derived symbols for bindings that are defined
- /// implicitly by default bindings in a super region.
+ /// implicitly by default bindings in a super region.
+ ///
+ /// Note that callers may need to specially handle LazyCompoundVals, which
+ /// are returned as is in case the caller needs to treat them differently.
Optional<SVal> getBindingForDerivedDefaultValue(RegionBindingsConstRef B,
const MemRegion *superR,
const TypedValueRegion *R,
@@ -1443,9 +1446,10 @@ RegionStoreManager::getBindingForDerived
if (val.isUnknownOrUndef())
return val;
- // Lazy bindings are handled later.
+ // Lazy bindings are usually handled through getExistingLazyBinding().
+ // We should unify these two code paths at some point.
if (val.getAs<nonloc::LazyCompoundVal>())
- return None;
+ return val;
llvm_unreachable("Unknown default value");
}
@@ -1462,7 +1466,7 @@ SVal RegionStoreManager::getLazyBinding(
Result = getBindingForField(LazyBinding,
cast<FieldRegion>(LazyBindingRegion));
- // This is a hack to deal with RegionStore's inability to distinguish a
+ // FIXME: This is a hack to deal with RegionStore's inability to distinguish a
// default value for /part/ of an aggregate from a default value for the
// /entire/ aggregate. The most common case of this is when struct Outer
// has as its first member a struct Inner, which is copied in from a stack
@@ -1503,12 +1507,34 @@ RegionStoreManager::getBindingForFieldOr
// be out of scope of our lookup.
bool hasSymbolicIndex = false;
- while (superR) {
- if (const Optional<SVal> &D =
- getBindingForDerivedDefaultValue(B, superR, R, Ty))
+ // FIXME: This is a hack to deal with RegionStore's inability to distinguish a
+ // default value for /part/ of an aggregate from a default value for the
+ // /entire/ aggregate. The most common case of this is when struct Outer
+ // has as its first member a struct Inner, which is copied in from a stack
+ // variable. In this case, even if the Outer's default value is symbolic, 0,
+ // or unknown, it gets overridden by the Inner's default value of undefined.
+ //
+ // This is a general problem -- if the Inner is zero-initialized, the Outer
+ // will now look zero-initialized. The proper way to solve this is with a
+ // new version of RegionStore that tracks the extent of a binding as well
+ // as the offset.
+ //
+ // This hack only takes care of the undefined case because that can very
+ // quickly result in a warning.
+ bool hasPartialLazyBinding = false;
+
+ const SubRegion *Base = dyn_cast<SubRegion>(superR);
+ while (Base) {
+ if (Optional<SVal> D = getBindingForDerivedDefaultValue(B, Base, R, Ty)) {
+ if (D->getAs<nonloc::LazyCompoundVal>()) {
+ hasPartialLazyBinding = true;
+ break;
+ }
+
return *D;
+ }
- if (const ElementRegion *ER = dyn_cast<ElementRegion>(superR)) {
+ if (const ElementRegion *ER = dyn_cast<ElementRegion>(Base)) {
NonLoc index = ER->getIndex();
if (!index.isConstant())
hasSymbolicIndex = true;
@@ -1516,11 +1542,7 @@ RegionStoreManager::getBindingForFieldOr
// If our super region is a field or element itself, walk up the region
// hierarchy to see if there is a default value installed in an ancestor.
- if (const SubRegion *SR = dyn_cast<SubRegion>(superR)) {
- superR = SR->getSuperRegion();
- continue;
- }
- break;
+ Base = dyn_cast<SubRegion>(Base->getSuperRegion());
}
if (R->hasStackNonParametersStorage()) {
@@ -1540,8 +1562,9 @@ RegionStoreManager::getBindingForFieldOr
// a symbolic offset.
if (hasSymbolicIndex)
return UnknownVal();
-
- return UndefinedVal();
+
+ if (!hasPartialLazyBinding)
+ return UndefinedVal();
}
// All other values are symbolic.
@@ -1620,8 +1643,10 @@ SVal RegionStoreManager::getBindingForVa
if (isa<StaticGlobalSpaceRegion>(MS))
return svalBuilder.makeZeroVal(T);
- if (Optional<SVal> V = getBindingForDerivedDefaultValue(B, MS, R, T))
+ if (Optional<SVal> V = getBindingForDerivedDefaultValue(B, MS, R, T)) {
+ assert(!V->getAs<nonloc::LazyCompoundVal>());
return V.getValue();
+ }
return svalBuilder.getRegionValueSymbolVal(R);
}
Modified: cfe/trunk/test/Analysis/uninit-vals.m
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/uninit-vals.m?rev=176144&r1=176143&r2=176144&view=diff
==============================================================================
--- cfe/trunk/test/Analysis/uninit-vals.m (original)
+++ cfe/trunk/test/Analysis/uninit-vals.m Tue Feb 26 18:05:29 2013
@@ -89,3 +89,14 @@ void PR14765_incorrectBehavior(Circle *t
free(testObj);
}
+void rdar13292559(Circle input) {
+ extern void useCircle(Circle);
+
+ Circle obj = input;
+ useCircle(obj); // no-warning
+
+ // This generated an "uninitialized 'size' field" warning for a (short) while.
+ obj.origin = makePoint(0.0, 0.0);
+ useCircle(obj); // no-warning
+}
+
More information about the cfe-commits
mailing list