From fb29410c85a632e328877a6310b57b33908156ac Mon Sep 17 00:00:00 2001
From: Jordan Rose <jordan_rose@apple.com>
Date: Wed, 10 Oct 2012 21:31:21 +0000
Subject: [PATCH] [analyzer] Treat fields of unions as having symbolic offsets.

This allows only one field to be active at a time in RegionStore.
This isn't quite the correct behavior for unions, but it at least
would handle the case of "value goes in, value comes out" from the
same field.

RegionStore currently has a number of places where any access to a union
results in UnknownVal being returned. However, it is clearly missing
some cases, or the original issue wouldn't have occurred. It is probably
now safe to remove those changes, but that's a potentially destabilizing
change that should wait for more thorough testing.

Fixes PR14054.

llvm-svn: 165660
---
 clang/lib/StaticAnalyzer/Core/MemRegion.cpp |  6 +++-
 clang/test/Analysis/unions.cpp              | 50 +++++++++++++++++++++++++++++
 2 files changed, 55 insertions(+), 1 deletion(-)
 create mode 100644 clang/test/Analysis/unions.cpp

diff --git a/clang/lib/StaticAnalyzer/Core/MemRegion.cpp b/clang/lib/StaticAnalyzer/Core/MemRegion.cpp
index 7c66739..da34a79 100644
--- a/clang/lib/StaticAnalyzer/Core/MemRegion.cpp
+++ b/clang/lib/StaticAnalyzer/Core/MemRegion.cpp
@@ -1168,8 +1168,12 @@ RegionOffset MemRegion::getAsOffset() const {
       R = FR->getSuperRegion();
 
       const RecordDecl *RD = FR->getDecl()->getParent();
-      if (!RD->isCompleteDefinition()) {
+      if (/*RD->isUnion() || */!RD->isCompleteDefinition()) {
         // We cannot compute offset for incomplete type.
+        // For unions, we could treat everything as offset 0, but we'd rather
+        // treat each field as a symbolic offset so they aren't stored on top
+        // of each other, since we depend on things in typed regions actually
+        // matching their types.
         SymbolicOffsetBase = R;
       }
 
diff --git a/clang/test/Analysis/unions.cpp b/clang/test/Analysis/unions.cpp
new file mode 100644
index 0000000..e7671a9
--- /dev/null
+++ b/clang/test/Analysis/unions.cpp
@@ -0,0 +1,50 @@
+// RUN: %clang_cc1 -analyze -analyzer-checker=core %s -verify
+
+namespace PR14054_reduced {
+  struct Definition;
+  struct ParseNode {
+    union {
+      Definition *lexdef;
+      ParseNode *data;
+    } pn_u;
+  };
+  struct Definition : public ParseNode { };
+
+  void CloneParseTree(ParseNode *opn, ParseNode *pn,  ParseNode *x) {
+    // This used to cause an assertion failure because:
+    // 1. The implicit operator= for unions assigns all members of the union,
+    //    not just the active one (b/c there's no way to know which is active).
+    // 2. RegionStore dutifully stored all the variants at the same offset;
+    //    the last one won.
+    // 3. We asked for the value of the first variant but got back a conjured
+    //    symbol for the second variant.
+    // 4. We ended up trying to add a base cast to a region of the wrong type.
+    //
+    // Now (at the time this test was added), we instead treat all variants of
+    // a union as different offsets, but only allow one to be active at a time.
+    *pn = *opn;
+    x = pn->pn_u.lexdef->pn_u.lexdef;
+  }
+}
+
+namespace PR14054_original {
+  struct Definition;
+  struct ParseNode {
+    union {
+      struct {
+        union {};
+        Definition *lexdef;
+      } name;
+      class {
+        int *target;
+        ParseNode *data;
+      } xmlpi;
+    } pn_u;
+  };
+  struct Definition : public ParseNode { };
+
+  void CloneParseTree(ParseNode *opn, ParseNode *pn,  ParseNode *x) {
+    pn->pn_u = opn->pn_u;
+    x = pn->pn_u.name.lexdef->pn_u.name.lexdef;
+  }
+}
-- 
2.7.4