Wow, this actually came out very nicely. Good work, Manuel and Alex! I have a
few comments on individual pieces, but overall I'm actually pretty confident in
this. Have you run it over a large codebase yet?
> often we know when one destructor has executed that we must also execute the
> dtor of things that were generated previously; this makes the code and the
> CFG significantly more complex, though; the biggest downside seems to be that
> we potentially generate more states; not sure whether it's worth it
>
> often we do not actually need a new block (for example, if the temporary is
> created unconditionally); in that case, we could just run with a single
> block; again this would make the code more complex though, and I'm not sure
> it's worth the effort, unless we feel it's an important invariant on the CFG
We do still have this knowledge: it's whether the logical expression
immediately dominating the last temporary processed is the same as the logical
expression dominating the current temporary. That's not perfect (it doesn't
handle the `A() || B() || C()` case), but it works in the simple cases, and
handles unconditional creation as well. That said, if we do this we need to
make sure the state is cleaned up correctly, as I mention below.
I'm not sure how changing either of these behaviors would generate more states.
It seems like if you have different sets of destructors to run, the states are
already necessarily different, and once you've reached the common set of
destructors, you'd have the same opportunity to "cache out", i.e. match an
existing node in the ExplodedGraph.
================
Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:54-56
@@ -53,1 +53,5 @@
+REGISTER_TRAIT_WITH_PROGRAMSTATE(
+ InitializedTemporariesSet,
+ llvm::ImmutableSet<const CXXBindTemporaryExpr *>);
+
----------------
This needs to include the current StackFrameContext as well (for recursive
functions). You can get that from the current LocationContext.
================
Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:771
@@ +770,3 @@
+ !State->contains<InitializedTemporariesSet>(BTE));
+ State = State->add<InitializedTemporariesSet>(BTE);
+ StmtBldr.generateNode(BTE, Pred, State);
----------------
Manuel Klimek wrote:
> Alex McCarthy wrote:
> > If includeTemporaryDtorsInCFG is not set, should we avoid this state
> > modification?
> That's a good question (for Jordan ;)
>
> Another question is whether we still want to do the runCheckersForPre /
> runCheckersForPost enchilada.
We're trying to run pre/post-checks on all non-trivial statements, so yes, we
should still do it. It's //almost// simple enough that we can hoist that out of
the big switch, but I'm not sure that's correct for all statement kinds, and no
one's looked into it.
(But now that we have C++11, I'm //really// inclined to hoist the thing into a
closure, so that people don't have to write that "loop over the nodes generated
by pre-checkers" loop themselves.)
As for the state modification, what I mainly care about is that we //clean up//
the map. Since that doesn't happen when temporary destructors are off, yes, we
should be checking here. (Note that this is also important if we stop
generating conditions for every temporary destructor—we need to make sure that
the state is clean by the end of the full-expression even if we skip some of
the conditions.)
================
Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:1451
@@ -1433,1 +1450,3 @@
+ if (const CXXBindTemporaryExpr *BTE =
+ dyn_cast<CXXBindTemporaryExpr>(Condition)) {
----------------
Manuel Klimek wrote:
> Alex McCarthy wrote:
> > I think it's worth pulling this out into a new processTemporary helper
> > method rather than adding it to processBranch: you're not reusing any of
> > the logic in processBranch in the temporary dtor case.
> >
> > This would involve adding a new HandleTemporary helper (or similar) that
> > you'd call instead of HandleBranch, and that would call processTemporary
> I agree. I mainly haven't done that, as somewhere in that chain there is an
> interface, and the method is abstract. I found only one use, so I assume
> somebody else implements that interface outside of the clang codebase. I'd
> like to hear what Jordan thinks on that issue, architecture-wise.
These days the interface isn't serving us much real purpose. In theory
CoreEngine is just traversing CFGs, and doesn't actually know very much about C
ef al. It then hands the CFGElements off to its SubEngine, which actually
processes them and tells the CoreEngine where to go next if there's a branch.
ExprEngine is the particularly SubEngine that understands the AST / the
language.
In practice there's a lot of linguistic knowledge coded into CoreEngine, just
because there's a lot of linguistic knowledge in the CFG and in ProgramPoints
these days. But either way, SubEngine is more of an interface than a real base
class; I don't think Ted ever expected that we'd get subclassing more than one
level deep. It's okay to split this up into helper methods.
================
Comment at: test/Analysis/temporaries.cpp:275
@@ -228,2 +274,3 @@
+ }
void testBinaryOperatorShortcut(bool value) {
----------------
Manuel Klimek wrote:
> Alex McCarthy wrote:
> > Can you add a test that cleans up multiple temporaries in one condition?
> > Maybe something like this (not sure if this compiles, we might need to add
> > a check2(obj1, obj2) function):
> > class Dtor {
> > ~Dtor() {}
> > };
> >
> > void testMultipleTemporaries(bool value) {
> > if (value) {
> > // ~Dtor should run before ~NoReturnDtor() because construction order
> > is guaranteed by comma operator
> > if (!value || check((NoReturnDtor(), Dtor())) || value) {
> > clang_analyzer_eval(true); // no warning, unreachable code
> > }
> > }
> > }
> >
> > We could also make ~Dtor emit a warning which would only be triggered if
> > Dtor was constructed with a certain value (e.g.
> > check(Dtor(/*warning=*/true), NoReturnDtor()), then see if clang warns on
> > that error that should never happen.
> Added the test, and it works as expected. The problem is that I have no idea
> how to verify the construction order, as (according to comments I've seen in
> the code) the analyzer does not inline destructors (or at least temporary
> destructors).
We've turned that off until we can get it right, yes. We can turn that back on
after this patch. I'm happy to make this a CFG-dump test for now.
http://reviews.llvm.org/D3627
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
