Revert "[analyzer] Check that a member expr is valid even when the result is an lvalue."

The original intent of this commit was to catch potential null dereferences
early, but it breaks the common "home-grown offsetof" idiom (PR13927):

 (((struct Foo *)0)->member - ((struct foo *)0))

As it turns out, this appears to be legal in C, per a footnote in
C11 6.5.3.2: "Thus, &*E is equivalent to E (even if E is a null pointer)".
In C++ this issue is still open:
  http://www.open-std.org/jtc1/sc22/wg21/docs/cwg_active.html#232

We'll just have to make sure we have good path notes in the future.

This reverts r164441 / 9be016dcd1ca3986873a7b66bd4bc027309ceb59.

llvm-svn: 164958

diff --git a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
index 0e48864..007bcf5 100644 (file)
--- a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
+++ b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
@@ -1515,30 +1515,22 @@ void ExprEngine::VisitMemberExpr(const MemberExpr *M, ExplodedNode *Pred,
     return;
   }
 
+  // FIXME: Should we insert some assumption logic in here to determine
+  // if "Base" is a valid piece of memory?  Before we put this assumption
+  // later when using FieldOffset lvals (which we no longer have).
+
   // For all other cases, compute an lvalue.    
   SVal L = state->getLValue(field, baseExprVal);
   if (M->isGLValue()) {
-    ExplodedNodeSet Tmp;
-    Bldr.takeNodes(Pred);
-    evalLocation(Tmp, M, M, Pred, state, baseExprVal,
-                 /*Tag=*/0, /*isLoad=*/true);
-    Bldr.addNodes(Tmp);
-
-    const MemRegion *ReferenceRegion = 0;
     if (field->getType()->isReferenceType()) {
-      ReferenceRegion = L.getAsRegion();
-      if (!ReferenceRegion)
+      if (const MemRegion *R = L.getAsRegion())
+        L = state->getSVal(R);
+      else
         L = UnknownVal();
     }
 
-    for (ExplodedNodeSet::iterator I = Tmp.begin(), E = Tmp.end(); I != E; ++I){
-      state = (*I)->getState();
-      if (ReferenceRegion)
-        L = state->getSVal(ReferenceRegion);
-
-      Bldr.generateNode(M, (*I), state->BindExpr(M, LCtx, L), 0,
-                        ProgramPoint::PostLValueKind);
-    }
+    Bldr.generateNode(M, Pred, state->BindExpr(M, LCtx, L), 0,
+                      ProgramPoint::PostLValueKind);
   } else {
     Bldr.takeNodes(Pred);
     evalLoad(Dst, M, M, Pred, state, L);

diff --git a/clang/test/Analysis/fields.c b/clang/test/Analysis/fields.c
index a10d5a8..da0847a 100644 (file)
--- a/clang/test/Analysis/fields.c
+++ b/clang/test/Analysis/fields.c
@@ -26,10 +26,3 @@ void test() {
   Point p;
   (void)(p = getit()).x;
 }
-
-
-void testNullAddress() {
-  Point *p = 0;
-  int *px = &p->x; // expected-warning{{Access to field 'x' results in a dereference of a null pointer (loaded from variable 'p')}}
-  *px = 1; // No warning because analysis stops at the previous line.
-}

diff --git a/clang/test/Analysis/nullptr.cpp b/clang/test/Analysis/nullptr.cpp
index 80ef5fb..050c3f8 100644 (file)
--- a/clang/test/Analysis/nullptr.cpp
+++ b/clang/test/Analysis/nullptr.cpp
@@ -23,11 +23,10 @@ void foo3(void) {
   };
   char *np = nullptr;
   // casting a nullptr to anything should be caught eventually
-  int *ip = &(((struct foo *)np)->f);  // expected-warning{{Access to field 'f' results in a dereference of a null pointer (loaded from variable 'np')}}
-
-  // Analysis stops at the first problem case, so we won't actually warn here.
-  *ip = 0;
-  *np = 0;
+  int *ip = &(((struct foo *)np)->f);
+  *ip = 0;  // expected-warning{{Dereference of null pointer}}
+  // should be error here too, but analysis gets stopped
+//  *np = 0;
 }
 
 // nullptr is implemented as a zero integer value, so should be able to compare