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
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
