[PATCH] Clang Static Analyzer support for temporary destructors

Great, thanks for the improvement!

I’ve started to dive into live variable analysis, but I think it’ll take me a while to dig into this one. I’ll ping this thread once I have more well-researched questions.

It looks like the simple case test in r205661 was commented out in favor of the more complex test. Here’s a patch to uncomment the simple test. We can also delete the simple case if you’d prefer.

0001-Re-enable-simple-case-test-that-was-introduced-in-r2.patch (1.08 KB)

Oops, no, that was my mistake. Thanks for catching this.

Hi Jordan,

I have looked into the remaining problems today; let me quickly summarize so we make sure we’re on the same page.
There are basically 2 problems left:

  1. The invariant
    As you mentioned on the rollback CL, the current layout of the CFG regarding temporary destructors violates the invariants assumed by other parts of the analyzer: namely, a CFG block with a terminator must not be empty.
    After looking into it, I fully agree that we need a principled solution here; the question is: do we want to keep the invariant, and fix the CFG with temporary destructors to match the invariant, or do we want to say the invariant doesn’t hold, and we should fix the rest of the code to not assume that invariant (and work with the current layout)? (I don’t even know whether the former is possible theoretically).
    Any hints on what your gut tells you the right solutions is would be helpful, and minimize dead-ends :wink:
    How would you want the CFG to look here?

  2. Taking all the correct branches
    So far I’m not sure I have seen a case where we’re not running into (1), but I’m not sure (the case where the temporary with the no-return dtor is the last in a sequence of binary logical ops seems to be (1) at least).
    The correct solution seems to be to add tracking lifetime of temporaries in conditionals correctly. alexmc is working on a patch here.

Cheers,
/Manuel

Hi Jordan,

I have looked into the remaining problems today; let me quickly summarize so we make sure we’re on the same page.
There are basically 2 problems left:

  1. The invariant
    As you mentioned on the rollback CL, the current layout of the CFG regarding temporary destructors violates the invariants assumed by other parts of the analyzer: namely, a CFG block with a terminator must not be empty.
    After looking into it, I fully agree that we need a principled solution here; the question is: do we want to keep the invariant, and fix the CFG with temporary destructors to match the invariant, or do we want to say the invariant doesn’t hold, and we should fix the rest of the code to not assume that invariant (and work with the current layout)? (I don’t even know whether the former is possible theoretically).
    Any hints on what your gut tells you the right solutions is would be helpful, and minimize dead-ends :wink:
    How would you want the CFG to look here?

I’m not actually sure why we care that a CFG block with a terminator is not empty. It might be because we expect the terminator condition to have been computed within the block, and that it’s trying to catch mistakes in constructing the CFG. The possible things that could be relying on this:

  • maybe we ask for the first CFGElement unilaterally if the block isn’t the entrance or exit block
  • worse, maybe we assume the block is the entrance or exit block if it’s “empty” (since the terminator isn’t included in the element count)
  • other things I haven’t thought of

But really I don’t think this is an important invariant, and if the right solution to (2) involves removing it, then so be it.

  1. Taking all the correct branches
    So far I’m not sure I have seen a case where we’re not running into (1), but I’m not sure (the case where the temporary with the no-return dtor is the last in a sequence of binary logical ops seems to be (1) at least).
    The correct solution seems to be to add tracking lifetime of temporaries in conditionals correctly. alexmc is working on a patch here.

I agree; it’s the particular value of “correctly” that makes the problem difficult. :wink: Very glad Alex has been able to spend time on this.

Jordan

Hi Jordan,

I have looked into the remaining problems today; let me quickly summarize
so we make sure we're on the same page.
There are basically 2 problems left:
1. The invariant
As you mentioned on the rollback CL, the current layout of the CFG
regarding temporary destructors violates the invariants assumed by other
parts of the analyzer: namely, a CFG block with a terminator must not be
empty.
After looking into it, I fully agree that we need a principled solution
here; the question is: do we want to keep the invariant, and fix the CFG
with temporary destructors to match the invariant, or do we want to say the
invariant doesn't hold, and we should fix the rest of the code to not
assume that invariant (and work with the current layout)? (I don't even
know whether the former is possible theoretically).
Any hints on what your gut tells you the right solutions is would be
helpful, and minimize dead-ends :wink:
How would you want the CFG to look here?

I'm not actually sure why we care that a CFG block with a terminator is
not empty. It might be because we expect the terminator condition to have
been computed within the block, and that it's trying to catch mistakes in
constructing the CFG. The possible things that could be relying on this:

- maybe we ask for the first CFGElement unilaterally if the block isn't
the entrance or exit block
- worse, maybe we assume the block *is* the entrance or exit block if
it's "empty" (since the terminator isn't included in the element count)
- other things I haven't thought of

But really I don't think this is an important invariant, and if the right
solution to (2) involves removing it, then so be it.

Ok, I've tried to look into what exactly is happening in
ExprEngine.cpp:1355, ResolveCondition.
I have not yet found out what ResolveCondition should actually do :slight_smile:
It seems like if it's called with a logical binary operator, it's always
called with the TerminatorCondition of the given CFGBlock.
Most of the time, it will just return that condition (namely if the
condition is the last statement in the block).
If the condition is not the last statement in the block, it will drill down
through the condition into the innermost RHS of logical binary operators,
and then return that.

Can you explain what's going on here? (That's the point where the analyzer
currently assumes the block is not empty, assert(I != E) triggers).

Thanks!
/Manuel

FYI, I figured ResolveCondition out (at least I think so). I’ll send you a patch adding some code comments, where you can correct my understanding. Alex and I also now believe we have a plan how to fix the overall problem.

Jordan/Ted, could you comment on the approach described below? Manuel, please correct any mistakes in my write up :slight_smile:

Manuel did some excellent debugging over the last couple of days, and this morning he and I sold ourselves on an approach to fix the temporary destructor issue described in http://llvm.org/bugs/show_bug.cgi?id=18159 by following Jordan’s recommendation to track temporary construction. Instead of changing the liveliness algorithms, we’ll write patches that make the following changes:

  1. Add new test cases to test/Analysis/temporaries.cpp that change boolean values during condition evaluation (e.g. if (!a || !b || !(a = false) || NoReturnDtr()) )

  2. Add a flag to wrap the new temporary tracking behavior introduced below, off by default

  3. Track temporary creation in ProgramState whenever we process a CXXBindTemporaryExpr, similar to how we handle static variable initialization (sample patch attached, nowhere near ready to commit)

  4. Modify VisitBinaryOperatorForTemporaryDtors (lib/Analysis/CFG.cpp:3512) and VisitConditionalOperatorForTemporaryDtors (lib/Analysis/CFG.cpp:3624) to set the temporary cleanup block’s terminator to be the CXXBindTemporaryExpr instead of a conditional only if the new flag is turned on (exact implementation might differ slightly, maybe by using a different expr/stmt class)

  5. Add a new helper to call a temporary object’s destructor if the temporary object is tracked in ProgramState, similar to CoreEngine::HandleBranch (lib/StaticAnalyzer/Core/CoreEngine.cpp:453) and ExprEngine::processBranch (lib/StaticAnalyzer/Core/ExprEngine.cpp:1400). Alternative: modify processBranch to add special case logic for temporary destructor branching.

  6. Modify HandleBlockExit (lib/StaticAnalyzer/Core/CoreEngine.cpp:345) to handle blocks with a CXXBindTemporaryExpr terminator by calling the new helper described above

  7. Fix the assert caused by blocks with no statements and terminator conditions that fires in ResolveCondition (lib/StaticAnalyzer/Core/ExprEngine.cpp:1383). Manuel’s working on understanding this better, and it might have a separate resolution.

  8. Do some large-scale testing, then set both the new temporary tracking behavior flag and the include-temporary-dtors flag to true by default if things look good.

What do you think?

0001-Current-WIP-destructor-tracking-work.patch (6.35 KB)

Jordan/Ted, could you comment on the approach described below? Manuel,
please correct any mistakes in my write up :slight_smile:

Manuel did some excellent debugging over the last couple of days, and this
morning he and I sold ourselves on an approach to fix the temporary
destructor issue described in http://llvm.org/bugs/show_bug.cgi?id=18159by following Jordan's recommendation to track temporary construction.
Instead of changing the liveliness algorithms, we'll write patches that
make the following changes:

1. Add new test cases to test/Analysis/temporaries.cpp that change boolean
values during condition evaluation (e.g. if (!a || !b || !(a = false) ||
NoReturnDtr()) )
2. Add a flag to wrap the new temporary tracking behavior introduced
below, off by default
3. Track temporary creation in ProgramState whenever we process
a CXXBindTemporaryExpr, similar to how we handle static variable
initialization (sample patch attached, nowhere near ready to commit)
4. Modify VisitBinaryOperatorForTemporaryDtors (lib/Analysis/CFG.cpp:3512)
and VisitConditionalOperatorForTemporaryDtors (lib/Analysis/CFG.cpp:3624)
to set the temporary cleanup block's terminator to be the
CXXBindTemporaryExpr instead of a conditional only if the new flag is
turned on (exact implementation might differ slightly, maybe by using a
different expr/stmt class)
5. Add a new helper to call a temporary object's destructor if the
temporary object is tracked in

I think "call the destructor" is somewhat confusing - I think the idea is
to generate the exploded state that visits the block that calls the
desctructor iff the constructor was executed (true-branch), and visits the
false-branch otherwise. This can be basically done equivalently to how we
handle known Condition SVals in processBranch.

ProgramState, similar to CoreEngine::HandleBranch
(lib/StaticAnalyzer/Core/CoreEngine.cpp:453) and ExprEngine::processBranch
(lib/StaticAnalyzer/Core/ExprEngine.cpp:1400). Alternative: modify
processBranch to add special case logic for temporary destructor branching.
6. Modify HandleBlockExit (lib/StaticAnalyzer/Core/CoreEngine.cpp:345) to
handle blocks with a CXXBindTemporaryExpr terminator by calling the new
helper described above
7. Fix the assert caused by blocks with no statements and terminator
conditions that fires in ResolveCondition
(lib/StaticAnalyzer/Core/ExprEngine.cpp:1383). Manuel's working on
understanding this better, and it might have a separate resolution.

I think with the method described above, this assert will not fire any more
(in fact, I think we'll add a new assert :wink:

I have two more question about how the static analyzer works:

  1. how are evaluated conditions cached / invalidated?
    If I have a block
    [B42]

    T: if [B23.8]

From reading processBranch, it looks like the correctness of the analysis of the terminator here relies on the result of B23.8 being available as an SVal if it was fully known at B23.8. If the result of this evaluation is guaranteed to be cached in the state, I’m not sure why doing the same for temporary destructor edges wouldn’t work (apart from that it would be harder to implement, as we’d need to make sure the ResolveCondition returns the same part of the condition from the temp dtor terminator as for the terminator condition of the branch that created it, which is something we currently don’t model).
If it’s not guaranteed to be cached, I think we’d have a different problem, because the way temporary destructors are introduced when inside and if (condition && temp()), the evaluation of the condition && temp() part gets moved a few blocks away from the terminator that decides which if-branch to take. I have not been able to come up with an example that’s problematic, though.

  1. how does inlining work, and what are the actual invariants of what has the be inside the same block
    Take the following from the dtor.cpp test:

struct CheckCustomDestructor {
bool bool_;
CheckCustomDestructor():bool_(true) {}
~CheckCustomDestructor();
operator bool() const { return bool_; }
};
bool testUnnamedCustomDestructor() {
if (CheckCustomDestructor c = CheckCustomDestructor())
return true;
return false;
}

This is a regression test for a “return of garbage” warning from the bool operator. I tried to change the CFG in VisitExprWithCleanup to always insert a new block before the temporary dtor decision blocks (as that way the CFG looks more uniform regarding how terminator edges are handled). Funnily enough this breaks the above test (and a few other ones in similarly strange ways). The only difference in the CFG I see is that the last block is split into two blocks, just before the call to the temporary destructor. I have no idea why that would ever affect the copied bool inside ‘c’, which we later get warned on. In general, it seems like I don’t have a good understanding of when putting two statements in different blocks will change how the analyzer sees them.

Any hints would be highly appreciated.

Thanks!
/Manuel

I have two more question about how the static analyzer works:

1. how are evaluated conditions cached / invalidated?
If I have a block
[B42]
...
T: if [B23.8]

From reading processBranch, it looks like the correctness of the analysis of the terminator here relies on the result of B23.8 being available as an SVal if it was fully known at B23.8. If the result of this evaluation is guaranteed to be cached in the state, I'm not sure why doing the same for temporary destructor edges wouldn't work (apart from that it would be harder to implement, as we'd need to make sure the ResolveCondition returns the same part of the condition from the temp dtor terminator as for the terminator condition of the branch that created it, which is something we currently don't model).
If it's not guaranteed to be cached, I think we'd have a different problem, because the way temporary destructors are introduced when inside and if (condition && temp()), the evaluation of the condition && temp() part gets moved a few blocks away from the terminator that decides which if-branch to take. I have not been able to come up with an example that's problematic, though.

I said this in the other e-mail, but an expression's value is intended to be live from the time it is computed to the time it is consumed. We also don't really have things set up for expressions to be consumed more than once.

"Caching" really is the wrong word. The Environment stores everything that has been evaluated until the liveness analysis says it is no longer needed.

2. how does inlining work, and what are the actual invariants of what has the be inside the same block
Take the following from the dtor.cpp test:
struct CheckCustomDestructor {
  bool bool_;
  CheckCustomDestructor():bool_(true) {}
  ~CheckCustomDestructor();
  operator bool() const { return bool_; }
};
bool testUnnamedCustomDestructor() {
  if (CheckCustomDestructor c = CheckCustomDestructor())
    return true;
  return false;
}

This is a regression test for a "return of garbage" warning from the bool operator. I tried to change the CFG in VisitExprWithCleanup to always insert a new block before the temporary dtor decision blocks (as that way the CFG looks more uniform regarding how terminator edges are handled). Funnily enough this breaks the above test (and a few other ones in similarly strange ways). The only difference in the CFG I see is that the last block is split into two blocks, just before the call to the temporary destructor. I have no idea why that would ever affect the copied bool inside 'c', which we later get warned on. In general, it seems like I don't have a good understanding of when putting two statements in different blocks will change how the analyzer sees them.

Inlining essentially jumps to a different CFG's entry block, and then jumps back when the function is different, with some fix-up before and after. It doesn't affect liveness at all. I wouldn't think that the change you describe would actually affect anything here, but the "garbage collection" that cleans up the Environment, Store, and ConstraintManager does run when entering a new CFG block, among other times. ("On exit from a function", and the conditions listed in shouldRemoveDeadBindings in ExprEngine.cpp.)

It sounds about right to me, but (4) is interesting. Ted and Anna and I have talked about similar things before. It’s a good solution for the analyzer, but it might make the rest of the compiler unhappy. IIRC, all of the existing compiler flow-sensitive analyses do include temporary destructors right now. They probably aren’t doing anything so special with them, but changing the CFG so that CXXBindTemporaryExpr can be a terminator is something that potentially affects all clients.

If we do this under a flag, what behavior should we use elsewhere? The existing thing, of visiting all the conditions again?

Jordan

The existing thing is fine. It is what we understand now.

IMO, hopefully a flag would be temporary. Ideally we’d update the compiler as well in time; having so many “views” of the CFG makes it very complex to reason about.

Btw, I think this was a red herring and I discovered the bug to be an
ordering problem of the "assignment" statement and the temp dtor statement
in the CFG. Apparently the order doesn't matter as long as we're in the
same block, but starts to matter when we jump blocks in between.

See http://reviews.llvm.org/D3603 for an attempted fix.

Jordan/Ted, could you comment on the approach described below? Manuel,
please correct any mistakes in my write up :slight_smile:

Manuel did some excellent debugging over the last couple of days, and this
morning he and I sold ourselves on an approach to fix the temporary
destructor issue described in http://llvm.org/bugs/show_bug.cgi?id=18159by following Jordan's recommendation to track temporary construction.
Instead of changing the liveliness algorithms, we'll write patches that
make the following changes:

1. Add new test cases to test/Analysis/temporaries.cpp that change boolean
values during condition evaluation (e.g. if (!a || !b || !(a = false) ||
NoReturnDtr()) )
2. Add a flag to wrap the new temporary tracking behavior introduced
below, off by default
3. Track temporary creation in ProgramState whenever we process
a CXXBindTemporaryExpr, similar to how we handle static variable
initialization (sample patch attached, nowhere near ready to commit)
4. Modify VisitBinaryOperatorForTemporaryDtors (lib/Analysis/CFG.cpp:3512)
and VisitConditionalOperatorForTemporaryDtors (lib/Analysis/CFG.cpp:3624)
to set the temporary cleanup block's terminator to be the
CXXBindTemporaryExpr instead of a conditional only if the new flag is
turned on (exact implementation might differ slightly, maybe by using a
different expr/stmt class)
5. Add a new helper to call a temporary object's destructor if the
temporary object is tracked in ProgramState, similar
to CoreEngine::HandleBranch (lib/StaticAnalyzer/Core/CoreEngine.cpp:453)
and ExprEngine::processBranch
(lib/StaticAnalyzer/Core/ExprEngine.cpp:1400). Alternative: modify
processBranch to add special case logic for temporary destructor branching.
6. Modify HandleBlockExit (lib/StaticAnalyzer/Core/CoreEngine.cpp:345) to
handle blocks with a CXXBindTemporaryExpr terminator by calling the new
helper described above
7. Fix the assert caused by blocks with no statements and terminator
conditions that fires in ResolveCondition
(lib/StaticAnalyzer/Core/ExprEngine.cpp:1383). Manuel's working on
understanding this better, and it might have a separate resolution.
8. Do some large-scale testing, then set both the new temporary tracking
behavior flag and the include-temporary-dtors flag to true by default if
things look good.

What do you think?

It sounds about right to me, but (4) is interesting. Ted and Anna and I
have talked about similar things before. It's a good solution for the
analyzer, but it might make the rest of the compiler unhappy. IIRC, all of
the existing compiler flow-sensitive analyses *do* include temporary
destructors right now. They probably aren't doing anything so special with
them, but changing the CFG so that CXXBindTemporaryExpr can be a terminator
is something that potentially affects all clients.

Given that the terminator expression with temp dtors inside logical binary
ops is basically always not evaluated, thus not in the Environment, I don't
expect much fallout from clang's warnings.
In the end, I hope we have tests for those warnings that we can just run,
and fix the warnings that break? :slight_smile:

If we do this under a flag, what behavior should we use elsewhere? The
existing thing, of visiting all the conditions again?

As Ted said, the idea would be to have the flag so everybody can test the
new behavior on their codebase and make sure we have no regressions the
tests don't cover, and then quickly replace the existing check.

Cheers,
/Manuel

Well, Clang’s warnings don’t evaluate things in the same way as the analyzer. The few warnings that do track any sort of evaluation (like -Wsometimes-uninitialized, IIRC?) can probably handle the current CFG, and probably wouldn’t be able to handle the new one.

I have a hard time thinking of which warnings the presence of temporary destructors would affect, which unfortunately probably means we don’t have many tests. But on the other hand, that means we probably won’t affect anything important.

Jordan

Jordan/Ted, could you comment on the approach described below? Manuel,
please correct any mistakes in my write up :slight_smile:

Manuel did some excellent debugging over the last couple of days, and
this morning he and I sold ourselves on an approach to fix the temporary
destructor issue described in http://llvm.org/bugs/show_bug.cgi?id=18159by following Jordan's recommendation to track temporary construction.
Instead of changing the liveliness algorithms, we'll write patches that
make the following changes:

1. Add new test cases to test/Analysis/temporaries.cpp that change
boolean values during condition evaluation (e.g. if (!a || !b || !(a =
false) || NoReturnDtr()) )
2. Add a flag to wrap the new temporary tracking behavior introduced
below, off by default
3. Track temporary creation in ProgramState whenever we process
a CXXBindTemporaryExpr, similar to how we handle static variable
initialization (sample patch attached, nowhere near ready to commit)
4. Modify VisitBinaryOperatorForTemporaryDtors
(lib/Analysis/CFG.cpp:3512) and VisitConditionalOperatorForTemporaryDtors
(lib/Analysis/CFG.cpp:3624) to set the temporary cleanup block's terminator
to be the CXXBindTemporaryExpr instead of a conditional only if the new
flag is turned on (exact implementation might differ slightly, maybe by
using a different expr/stmt class)
5. Add a new helper to call a temporary object's destructor if the
temporary object is tracked in ProgramState, similar
to CoreEngine::HandleBranch (lib/StaticAnalyzer/Core/CoreEngine.cpp:453)
and ExprEngine::processBranch
(lib/StaticAnalyzer/Core/ExprEngine.cpp:1400). Alternative: modify
processBranch to add special case logic for temporary destructor branching.
6. Modify HandleBlockExit (lib/StaticAnalyzer/Core/CoreEngine.cpp:345) to
handle blocks with a CXXBindTemporaryExpr terminator by calling the new
helper described above
7. Fix the assert caused by blocks with no statements and terminator
conditions that fires in ResolveCondition
(lib/StaticAnalyzer/Core/ExprEngine.cpp:1383). Manuel's working on
understanding this better, and it might have a separate resolution.
8. Do some large-scale testing, then set both the new temporary tracking
behavior flag and the include-temporary-dtors flag to true by default if
things look good.

What do you think?

It sounds about right to me, but (4) is interesting. Ted and Anna and I
have talked about similar things before. It's a good solution for the
analyzer, but it might make the rest of the compiler unhappy. IIRC, all of
the existing compiler flow-sensitive analyses *do* include temporary
destructors right now. They probably aren't doing anything so special with
them, but changing the CFG so that CXXBindTemporaryExpr can be a terminator
is something that potentially affects all clients.

Given that the terminator expression with temp dtors inside logical binary
ops is basically always not evaluated, thus not in the Environment, I don't
expect much fallout from clang's warnings.
In the end, I hope we have tests for those warnings that we can just run,
and fix the warnings that break? :slight_smile:

Well, Clang's warnings don't evaluate things in the same way as the
analyzer. The few warnings that *do* track any sort of evaluation (like
-Wsometimes-uninitialized, IIRC?) can probably handle the current CFG, and
probably wouldn't be able to handle the new one.

I think the DeadCode one does (that might be the same you mention, I'll
take a look). I'd be surprised if any check would be correct with the
current CFG and temporary destructors in logical operators if it matters
for the check (and if there are no logical ops, I'd assume all temp dtors
are unconditionally called anyway, so there'll not be any terminators?)

I have a hard time thinking of which warnings the presence of temporary
destructors would affect, which unfortunately probably means we don't have
many tests. But on the other hand, that means we probably won't affect
anything important.

Yea, I concur that I'd hope people have put a test on anything they really
care about :slight_smile: but that's also why the proposal is to use an
experimental-flag first, and have it run on at least our internal codebase,
which will already validate a ton of warnings...

Cheers,
/Manuel

Jordan/Ted, could you comment on the approach described below? Manuel,
please correct any mistakes in my write up :slight_smile:

Manuel did some excellent debugging over the last couple of days, and
this morning he and I sold ourselves on an approach to fix the temporary
destructor issue described in http://llvm.org/bugs/show_bug.cgi?id=18159by following Jordan's recommendation to track temporary construction.
Instead of changing the liveliness algorithms, we'll write patches that
make the following changes:

1. Add new test cases to test/Analysis/temporaries.cpp that change
boolean values during condition evaluation (e.g. if (!a || !b || !(a =
false) || NoReturnDtr()) )
2. Add a flag to wrap the new temporary tracking behavior introduced
below, off by default
3. Track temporary creation in ProgramState whenever we process
a CXXBindTemporaryExpr, similar to how we handle static variable
initialization (sample patch attached, nowhere near ready to commit)
4. Modify VisitBinaryOperatorForTemporaryDtors
(lib/Analysis/CFG.cpp:3512) and VisitConditionalOperatorForTemporaryDtors
(lib/Analysis/CFG.cpp:3624) to set the temporary cleanup block's terminator
to be the CXXBindTemporaryExpr instead of a conditional only if the new
flag is turned on (exact implementation might differ slightly, maybe by
using a different expr/stmt class)
5. Add a new helper to call a temporary object's destructor if the
temporary object is tracked in ProgramState, similar
to CoreEngine::HandleBranch (lib/StaticAnalyzer/Core/CoreEngine.cpp:453)
and ExprEngine::processBranch
(lib/StaticAnalyzer/Core/ExprEngine.cpp:1400). Alternative: modify
processBranch to add special case logic for temporary destructor branching.
6. Modify HandleBlockExit (lib/StaticAnalyzer/Core/CoreEngine.cpp:345)
to handle blocks with a CXXBindTemporaryExpr terminator by calling the new
helper described above
7. Fix the assert caused by blocks with no statements and terminator
conditions that fires in ResolveCondition
(lib/StaticAnalyzer/Core/ExprEngine.cpp:1383). Manuel's working on
understanding this better, and it might have a separate resolution.
8. Do some large-scale testing, then set both the new temporary tracking
behavior flag and the include-temporary-dtors flag to true by default if
things look good.

What do you think?

It sounds about right to me, but (4) is interesting. Ted and Anna and I
have talked about similar things before. It's a good solution for the
analyzer, but it might make the rest of the compiler unhappy. IIRC, all of
the existing compiler flow-sensitive analyses *do* include temporary
destructors right now. They probably aren't doing anything so special with
them, but changing the CFG so that CXXBindTemporaryExpr can be a terminator
is something that potentially affects all clients.

Given that the terminator expression with temp dtors inside logical
binary ops is basically always not evaluated, thus not in the Environment,
I don't expect much fallout from clang's warnings.
In the end, I hope we have tests for those warnings that we can just run,
and fix the warnings that break? :slight_smile:

Well, Clang's warnings don't evaluate things in the same way as the
analyzer. The few warnings that *do* track any sort of evaluation (like
-Wsometimes-uninitialized, IIRC?) can probably handle the current CFG, and
probably wouldn't be able to handle the new one.

I think the DeadCode one does (that might be the same you mention, I'll
take a look). I'd be surprised if any check would be correct with the
current CFG and temporary destructors in logical operators if it matters
for the check (and if there are no logical ops, I'd assume all temp dtors
are unconditionally called anyway, so there'll not be any terminators?)

I have a hard time thinking of which warnings the presence of temporary
destructors would affect, which unfortunately probably means we don't have
many tests. But on the other hand, that means we probably won't affect
anything important.

Yea, I concur that I'd hope people have put a test on anything they really
care about :slight_smile: but that's also why the proposal is to use an
experimental-flag first, and have it run on at least our internal codebase,
which will

already validate a ton of warnings...

Btw, from taking a quick look, I see 2 compiler-based warnings that pull
the statement out of a CFGTerminator (I think all others cannot be affected
by the change by design, right?):
ReachableCode.cpp and Consumed.cpp.
I think we'll need to take a good look at both anyway, at least
Consumed.cpp already seems to have problems when we encounter terminators
for temp dtor decisions.

Cheers,
/Manuel