[PATCH] D30534: [analyzer] When creating a temporary object copy, properly copy the value into it.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ created this revision.

The test case provided demonstrates a regression due to my earlier attempt to fix temporaries in https://reviews.llvm.org/D26839. Neither before nor after, we did not properly copy the symbolic rvalue of the object to the newly created memory region; in different ways though.

In the test case:

1. there are two classes - `Super` (smaller) and `Sub` (bigger);
2. a temporary object of `Sub` is created;
3. a `Sub`-specific field not present in `Super` is initialized during construction;
4. the temporary `Sub`-class object is casted to `Super` to call method `m()` that is declared in `Super`;
5. a temporary region is retroactively created to represent this-region during method call;
6. value of the `Super`-object is copied over the c++ base-object region;
7. the rest of the temporary remains uninitialized;
8. `m()` calls a virtual method `mImpl()` that resolves to the definition in `Sub`;
9. `mImpl()` uses the `Sub`-specific field initialized on step 3 but not copied on step 6, as explained on step 7;
10. a false warning regarding usage of uninitialized value is thrown.

The problem (apart from step 5 which is the reason for all the hassle, but needs AST fixes) is of course on step 6, where we should have just copied the whole `Sub` object. But - surprise! - the value of this object is no longer there in the Environment, because the cast on step 4 was already successfully modeled.

I'm uncertain of the proper solution. We could probably prevent objects from disappearing from the Environment upon unchecked-derived-to-base casts, but that'd require performance evaluations on C++-rich codebases.

For now, i propose to invalidate the `Sub`-object before binding the `Super`-object. This way at least we don't produce false accusations.

There's also one open question for the case when the value wasn't removed from the environment, but we still need to bind `Super` - i'm seeing failing tests when i disable binding `Super`, but i didn't investigate them yet.


https://reviews.llvm.org/D30534

Files:
  lib/StaticAnalyzer/Core/ExprEngine.cpp
  test/Analysis/temporaries.cpp


Index: test/Analysis/temporaries.cpp
===================================================================
--- test/Analysis/temporaries.cpp
+++ test/Analysis/temporaries.cpp
@@ -503,3 +503,30 @@
     });
   }
 }
+
+namespace CopyToTemporaryCorrectly {
+class Super {
+public:
+  void m() {
+    mImpl();
+  }
+  virtual void mImpl() = 0;
+};
+class Sub : public Super {
+public:
+  Sub(const int &p) : j(p) {}
+  virtual void mImpl() override {
+    // Used to be undefined pointer dereference because we didn't copy
+    // the subclass data (j) to the temporary object properly.
+    (void)(j + 1); // no-warning
+    if (j != 22) {
+      clang_analyzer_warnIfReached(); // no-warning
+    }
+  }
+  const int &j;
+};
+void run() {
+  int i = 22;
+  Sub(i).m();
+}
+}
Index: lib/StaticAnalyzer/Core/ExprEngine.cpp
===================================================================
--- lib/StaticAnalyzer/Core/ExprEngine.cpp
+++ lib/StaticAnalyzer/Core/ExprEngine.cpp
@@ -184,17 +184,17 @@
                                           const LocationContext *LC,
                                           const Expr *Ex,
                                           const Expr *Result) {
-  SVal V = State->getSVal(Ex, LC);
+  SVal ExV = State->getSVal(Ex, LC);
   if (!Result) {
     // If we don't have an explicit result expression, we're in "if needed"
     // mode. Only create a region if the current value is a NonLoc.
-    if (!V.getAs<NonLoc>())
+    if (!ExV.getAs<NonLoc>())
       return State;
     Result = Ex;
   } else {
     // We need to create a region no matter what. For sanity, make sure we don't
     // try to stuff a Loc into a non-pointer temporary region.
-    assert(!V.getAs<Loc>() || Loc::isLocType(Result->getType()) ||
+    assert(!ExV.getAs<Loc>() || Loc::isLocType(Result->getType()) ||
            Result->getType()->isMemberPointerType());
   }
 
@@ -259,14 +259,32 @@
     }
   }
 
-  // Try to recover some path sensitivity in case we couldn't compute the value.
-  if (V.isUnknown())
-    V = getSValBuilder().conjureSymbolVal(Result, LC, TR->getValueType(),
-                                          currBldrCtx->blockCount());
-  // Bind the value of the expression to the sub-object region, and then bind
-  // the sub-object region to our expression.
-  State = State->bindLoc(Reg, V, LC);
+  // The result expression would now point to the correct sub-region of the
+  // newly created temporary region.
   State = State->BindExpr(Result, LC, Reg);
+
+  // What remains is to copy the value of the object to the new region.
+  // FIXME: In other words, what we should always do is copy value of the
+  // Init expression (which corresponds to the bigger object) to the whole
+  // temporary region TR. However, this value is often no longer present
+  // in the Environment. If it has disappeared, we instead invalidate TR.
+  // Still, what we can do is assign the value of expression Ex (which
+  // corresponds to the sub-object) to the TR's sub-region Reg. At least,
+  // values inside Reg would be correct.
+  SVal InitV = State->getSVal(Init, LC);
+  if (InitV.isUnknown())
+    InitV = getSValBuilder().conjureSymbolVal(Result, LC, Init->getType(),
+                                              currBldrCtx->blockCount());
+  State = State->bindLoc(loc::MemRegionVal(TR), InitV, LC);
+
+  // Try to recover some path sensitivity in case we couldn't compute the value.
+  if (ExV.isUnknown())
+    ExV = getSValBuilder().conjureSymbolVal(Result, LC, Ex->getType(),
+                                            currBldrCtx->blockCount());
+
+  // FIXME: Why do we need to do that if WipeV was known to begin with?
+  State = State->bindLoc(Reg, ExV, LC);
+
   return State;
 }
 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D30534.90326.patch
Type: text/x-patch
Size: 3729 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20170302/ace17ec9/attachment-0001.bin>