[PATCH] [analyzer] Fix FP warnings when binding a temporary to a local static variable

[Pavel Labath]

================
Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:214
@@ +213,3 @@
+          dyn_cast<MaterializeTemporaryExpr>(Result)) {
+    if (MT->getStorageDuration() == SD_Static)
+        TR = MRMgr.getCXXExtendedTempObjectRegion(Inner);
----------------
Jordan Rose wrote:
> What about SD_Thread? The analyzer currently assumes non-parallel execution, so SD_Thread could behave like SD_Static for now.
SD_Thread added.

================
Comment at: lib/StaticAnalyzer/Core/ExprEngineCXX.cpp:150-163
@@ -149,2 +149,16 @@
           }
+        // Or a constructor for a temporary variable bound to a static
+        // reference?
+        } else if (isa<ImplicitCastExpr>(StmtElem->getStmt()) &&
+                   currStmtIdx + 2 < B->size()) {
+          CFGElement NNext = (*B)[currStmtIdx+2];
+          if (Optional<CFGStmt> NStmtElem = NNext.getAs<CFGStmt>()) {
+            if (const MaterializeTemporaryExpr *MT =
+                    dyn_cast<MaterializeTemporaryExpr>(NStmtElem->getStmt())) {
+              if (MT->getStorageDuration() == SD_Static) {
+                MemRegionManager &MRMgr = getSValBuilder().getRegionManager();
+                Target = MRMgr.getCXXExtendedTempObjectRegion(CE);
+              }
+            }
+          }
         }
----------------
Jordan Rose wrote:
> The `currStmtIdx + 2` seems really brittle. We use that for local variables because we synthesize DeclStmts that don't appear in the parent map, and because at one point I was concerned about the overhead of ParentMap lookup.  In this case, though, there may be an arbitrary number of layers between the CXXTemporaryObjectExpr and the enclosing MaterializeTemporaryExpr.
> 
> For now, it might be easier to just tweak CreateCXXTemporaryObject to not reuse the temp region if the storage duration is different.
I have completely removed this fragment and just kept the code in CreateCXXTemporaryObject. I can't say I know what is going on here (I still find the analyzer to be a pretty big black box), but it seems to work just fine without this. I have added some tests and they should prove that the reference actually ends up pointing to the right object.

================
Comment at: lib/StaticAnalyzer/Core/MemRegion.cpp:868
@@ +867,3 @@
+const CXXTempObjectRegion *
+MemRegionManager::getCXXExtendedTempObjectRegion(Expr const *Ex) {
+  return getSubRegion<CXXTempObjectRegion>(
----------------
Jordan Rose wrote:
> Not sure about this name. How about `getStaticTempObjectRegion`?
> 
> Also, `const Expr *Ex`, please. I don't know why the other one has it reversed. (I know they're both correct, but const-first is by far the convention in Clang.)
Done. Renamed.

================
Comment at: test/Analysis/stack-addr-ps.cpp:23-25
@@ -22,1 +22,5 @@
 
+void g4() {
+  static const int &x = 3; // no warning
+}
+
----------------
Jordan Rose wrote:
> Please add several more tests, and actually test that we inline the constructor and store the right values into the temporary reference. In this case there isn't a constructor at all, so you're only testing one of your modifications.
Tests added to temporaries.cpp.


http://llvm-reviews.chandlerc.com/D1133