From e185d73101dc6b1637ee103133cd6b07057661da Mon Sep 17 00:00:00 2001
From: Jordan Rose <jordan_rose@apple.com>
Date: Fri, 1 Mar 2013 23:03:17 +0000
Subject: [PATCH] [analyzer] Special-case bitfields when finding sub-region
 bindings.

Previously we were assuming that we'd never ask for the sub-region bindings
of a bitfield, since a bitfield cannot have subregions. However,
unification of code paths has made that assumption invalid. While we could
take advantage of this by just checking for the single possible binding,
it's probably better to do the right thing, so that if/when we someday
support unions we'll do the right thing there, too.

This fixes a handful of false positives in analyzing LLVM.

<rdar://problem/13325522>

llvm-svn: 176388
---
 clang/lib/StaticAnalyzer/Core/MemRegion.cpp   |  4 ++
 clang/lib/StaticAnalyzer/Core/RegionStore.cpp |  7 ++-
 clang/test/Analysis/fields.c                  | 87 +++++++++++++++++++++++++++
 3 files changed, 95 insertions(+), 3 deletions(-)

diff --git a/clang/lib/StaticAnalyzer/Core/MemRegion.cpp b/clang/lib/StaticAnalyzer/Core/MemRegion.cpp
index 12e4353..b3a1e65 100644
--- a/clang/lib/StaticAnalyzer/Core/MemRegion.cpp
+++ b/clang/lib/StaticAnalyzer/Core/MemRegion.cpp
@@ -195,6 +195,10 @@ DefinedOrUnknownSVal TypedValueRegion::getExtent(SValBuilder &svalBuilder) const
 }
 
 DefinedOrUnknownSVal FieldRegion::getExtent(SValBuilder &svalBuilder) const {
+  // Force callers to deal with bitfields explicitly.
+  if (getDecl()->isBitField())
+    return UnknownVal();
+
   DefinedOrUnknownSVal Extent = DeclRegion::getExtent(svalBuilder);
 
   // A zero-length array at the end of a struct often stands for dynamically-
diff --git a/clang/lib/StaticAnalyzer/Core/RegionStore.cpp b/clang/lib/StaticAnalyzer/Core/RegionStore.cpp
index b93c10d..82db23d 100644
--- a/clang/lib/StaticAnalyzer/Core/RegionStore.cpp
+++ b/clang/lib/StaticAnalyzer/Core/RegionStore.cpp
@@ -757,9 +757,7 @@ collectSubRegionBindings(SmallVectorImpl<BindingPair> &Bindings,
     TopKey = BindingKey::Make(Top, BindingKey::Default);
   }
 
-  // This assumes the region being invalidated is char-aligned. This isn't
-  // true for bitfields, but since bitfields have no subregions they shouldn't
-  // be using this function anyway.
+  // Find the length (in bits) of the region being invalidated.
   uint64_t Length = UINT64_MAX;
   SVal Extent = Top->getExtent(SVB);
   if (Optional<nonloc::ConcreteInt> ExtentCI =
@@ -768,6 +766,9 @@ collectSubRegionBindings(SmallVectorImpl<BindingPair> &Bindings,
     assert(ExtentInt.isNonNegative() || ExtentInt.isUnsigned());
     // Extents are in bytes but region offsets are in bits. Be careful!
     Length = ExtentInt.getLimitedValue() * SVB.getContext().getCharWidth();
+  } else if (const FieldRegion *FR = dyn_cast<FieldRegion>(Top)) {
+    if (FR->getDecl()->isBitField())
+      Length = FR->getDecl()->getBitWidthValue(SVB.getContext());
   }
 
   for (ClusterBindings::iterator I = Cluster.begin(), E = Cluster.end();
diff --git a/clang/test/Analysis/fields.c b/clang/test/Analysis/fields.c
index 12e8bbf..863a21a 100644
--- a/clang/test/Analysis/fields.c
+++ b/clang/test/Analysis/fields.c
@@ -29,6 +29,10 @@ void test() {
   (void)(p = getit()).x;
 }
 
+#define true ((bool)1)
+#define false ((bool)0)
+typedef _Bool bool;
+
 
 void testLazyCompoundVal() {
   Point p = {42, 0};
@@ -36,3 +40,86 @@ void testLazyCompoundVal() {
   clang_analyzer_eval((q = p).x == 42); // expected-warning{{TRUE}}
   clang_analyzer_eval(q.x == 42); // expected-warning{{TRUE}}
 }
+
+
+struct Bits {
+  unsigned a : 1;
+  unsigned b : 2;
+  unsigned c : 1;
+
+  bool x;
+
+  struct InnerBits {
+    bool y;
+
+    unsigned d : 16;
+    unsigned e : 6;
+    unsigned f : 2;
+  } inner;
+};
+
+void testBitfields() {
+  struct Bits bits;
+
+  if (foo() && bits.b) // expected-warning {{garbage}}
+    return;
+  if (foo() && bits.inner.e) // expected-warning {{garbage}}
+    return;
+
+  bits.c = 1;
+  clang_analyzer_eval(bits.c == 1); // expected-warning {{TRUE}}
+
+  if (foo() && bits.b) // expected-warning {{garbage}}
+    return;
+  if (foo() && bits.x) // expected-warning {{garbage}}
+    return;
+
+  bits.x = true;
+  clang_analyzer_eval(bits.x == true); // expected-warning{{TRUE}}
+  bits.b = 2;
+  clang_analyzer_eval(bits.x == true); // expected-warning{{TRUE}}
+  if (foo() && bits.c) // no-warning
+    return;
+
+  bits.inner.e = 50;
+  if (foo() && bits.inner.e) // no-warning
+    return;
+  if (foo() && bits.inner.y) // expected-warning {{garbage}}
+    return;
+  if (foo() && bits.inner.f) // expected-warning {{garbage}}
+    return;
+
+  extern struct InnerBits getInner();
+  bits.inner = getInner();
+  
+  if (foo() && bits.inner.e) // no-warning
+    return;
+  if (foo() && bits.inner.y) // no-warning
+    return;
+  if (foo() && bits.inner.f) // no-warning
+    return;
+
+  bits.inner.f = 1;
+  
+  if (foo() && bits.inner.e) // no-warning
+    return;
+  if (foo() && bits.inner.y) // no-warning
+    return;
+  if (foo() && bits.inner.f) // no-warning
+    return;
+
+  if (foo() && bits.a) // expected-warning {{garbage}}
+    return;
+}
+
+
+//-----------------------------------------------------------------------------
+// Incorrect behavior
+//-----------------------------------------------------------------------------
+
+void testTruncation() {
+  struct Bits bits;
+  bits.c = 0x11; // expected-warning{{implicit truncation}}
+  // FIXME: We don't model truncation of bitfields.
+  clang_analyzer_eval(bits.c == 1); // expected-warning {{FALSE}}
+}
-- 
2.7.4