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

Jordan Rose jordan_rose at apple.com
Mon Jul 15 11:23:22 PDT 2013


  A good start! Many spot comments.


================
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);
+              }
+            }
+          }
         }
----------------
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.

================
Comment at: lib/StaticAnalyzer/Core/MemRegion.cpp:868
@@ +867,3 @@
+const CXXTempObjectRegion *
+MemRegionManager::getCXXExtendedTempObjectRegion(Expr const *Ex) {
+  return getSubRegion<CXXTempObjectRegion>(
----------------
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.)

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

================
Comment at: test/Analysis/stack-addr-ps.cpp:23-25
@@ -22,1 +22,5 @@
 
+void g4() {
+  static const int &x = 3; // no warning
+}
+
----------------
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.


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

BRANCH
  master

ARCANIST PROJECT
  clang



More information about the cfe-commits mailing list