[PATCH] Proposal on how to fix temporary dtors.

[Manuel Klimek]

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 <jordan_rose at apple.com> 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
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140509/47e3f2e7/attachment.html>