[PATCH] Proposal on how to fix temporary dtors.

Alex McCarthy alexmc at google.com
Fri May 9 07:56:12 PDT 2014


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" <klimek at google.com> 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 <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/e493b5da/attachment.html>


More information about the cfe-commits mailing list