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

[Jordan Rose]

  I was a bit concerned about the optimization in CreateCXXTemporaryObject to avoid actually creating a new region, but it looks like that's dead code now anyway. I removed it in r187160; with that, I think you can commit this safely.


================
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);
+              }
+            }
+          }
         }
----------------
Pavel Labath wrote:
> 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.
I'd say it's not a black box so much as an incredibly complicated set of interlocking gears, at least in Core. :-) The Checkers get a much simpler interface to work with.

As you said, this code is trying to tell where to construct the new object. Does it need its own region, or is there already a region for it? The local variable case does so by looking to see if the CXXConstructExpr, rather than being consumed, is actually attached to a DeclStmt. If so, the region we want to construct into is `((Type *)var)[0]`. (I'm not sure the cast is actually necessary.)

In the long run, we really ought to have something here that says we're constructing into static memory, because otherwise the region in the constructor doesn't match the region that eventually gets stored into the reference. But as a strict improvement (i.e. to silence a false positive), just making sure it ends up in the right place is fine.

================
Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:214
@@ +213,3 @@
+          dyn_cast<MaterializeTemporaryExpr>(Result)) {
+    clang::StorageDuration SD = MT->getStorageDuration();
+    // If this object is bound to a reference with static storage duration, we
----------------
Should be no need to qualify 'clang' here.


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