[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>.

git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@176144 91177308-0d34-0410-b5e6-96231b3b80d8

diff --git a/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp b/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
index be7a401e3ab9978ed73bee8430a79a8fb1d7d74f..8dbfc8b61d3a9d806320bfcee278c3502c7770e6 100644 (file)
--- a/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ b/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -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();

diff --git a/lib/StaticAnalyzer/Core/RegionStore.cpp b/lib/StaticAnalyzer/Core/RegionStore.cpp
index acda9e0af321d8efd1db717f5980c41bc43c629d..53ecb2f539e96825eaf22bc9eb09f24193954705 100644 (file)
--- a/lib/StaticAnalyzer/Core/RegionStore.cpp
+++ b/lib/StaticAnalyzer/Core/RegionStore.cpp
@@ -487,7 +487,10 @@ public: // Part of public interface to class.
   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::getBindingForDerivedDefaultValue(RegionBindingsConstRef B,
     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(const SubRegion *LazyBindingRegion,
     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::getBindingForFieldOrElementCommon(RegionBindingsConstRef B,
   // 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::getBindingForFieldOrElementCommon(RegionBindingsConstRef B,
     
     // 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::getBindingForFieldOrElementCommon(RegionBindingsConstRef B,
     // a symbolic offset.
     if (hasSymbolicIndex)
       return UnknownVal();
-    
-    return UndefinedVal();
+
+    if (!hasPartialLazyBinding)
+      return UndefinedVal();
   }
 
   // All other values are symbolic.
@@ -1620,8 +1643,10 @@ SVal RegionStoreManager::getBindingForVar(RegionBindingsConstRef B,
     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);
   }

diff --git a/test/Analysis/uninit-vals.m b/test/Analysis/uninit-vals.m
index 57e83e3f9754ae06157e66e5a0e8fa41911e2677..6813b8ebf8d5670ce4e0b8c191ad05c9bb496a5b 100644 (file)
--- a/test/Analysis/uninit-vals.m
+++ b/test/Analysis/uninit-vals.m
@@ -89,3 +89,14 @@ void PR14765_incorrectBehavior(Circle *testObj) {
   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
+}
+