Just a short update: I ran this over our internal codebase, and found a roughly 0.5% change in warnings. All the ones I investigated were new warnings of the form: SomeType x = ...; CHECK(some.Other() || something.Else()); // this is the macro that evaluates to a no-return destructor iff the condition is false ... doSomethingWith(x);
And now we would get a warning that 'x' is never used after being initialized (I assume because it now always thinks the noreturn dtor is run). I'll have to investigate some more. Cheers, /Manuel On Wed, May 7, 2014 at 6:55 PM, Jordan Rose <[email protected]> wrote: > 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
