Did this patch strictly add new warnings? Do you know if it eliminated any false positives?
-Alex On May 9, 2014 4:29 AM, "Manuel Klimek" <[email protected]> wrote: > 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
