[PATCH] Proposal on how to fix temporary dtors.
Manuel Klimek
klimek at google.com
Wed May 7 01:07:25 PDT 2014
Find attached the CFG for this code, which I think nicely visualizes the
proposed new flow of temp dtor decisions.
void f() {
struct Dtor {
~Dtor();
operator bool() const;
};
extern bool coin();
if (Dtor() || coin() || Dtor() ? coin() || coin() || Dtor()
: coin() && Dtor()) {
}
}
On Wed, May 7, 2014 at 9:39 AM, Manuel Klimek <klimek at google.com> wrote:
> I also thought again about the problem of where to reset the bound state
> of the bind-temporary. The problem with the current solution is that it
> puts pretty strong requirements on how the CFG looks: we need to guarantee
> that for every constructed temporary, we will control-flow through the
> decision node that branches to the destructor call. At a first glance this
> may seem benign, but when I first thought about how to construct the CFG in
> that case, I believed the "optimum" to be one where we don't have any
> decision blocks when there is no control flow, and where we use the
> knowledge from where we are in a logical operator chain to unconditionally
> flow into the next destructor block.
> For example:
> A() || B() || C() || D()
> When we are in the destructor block for D(), we *know* that we have to
> call the destructor of C(), thus we could unconditionally flow into C's
> destructor block without first flowing into its decision block (like the
> current code does).
> I'm not sure it makes sense to do that (as it's more complex), but on the
> other hand I'm not sure it makes sense to preclude that with an
> implementation detail of how we track the temp ctor calls.
>
> ================
> Comment at: lib/Analysis/CFG.cpp:3540
> @@ -3582,60 +3539,3 @@
> AbstractConditionalOperator *E, bool BindToTemporary) {
> - // First add destructors for condition expression. Even if this will
> - // unnecessarily create a block, this block will be used at least by
> the full
> - // expression.
> - autoCreateBlock();
> - CFGBlock *ConfluenceBlock = VisitForTemporaryDtors(E->getCond());
> - if (badCFG)
> - return NULL;
> - if (BinaryConditionalOperator *BCO
> - = dyn_cast<BinaryConditionalOperator>(E)) {
> - ConfluenceBlock = VisitForTemporaryDtors(BCO->getCommon());
> - if (badCFG)
> - return NULL;
> - }
> -
> - // Try to add block with destructors for LHS expression.
> - CFGBlock *LHSBlock = NULL;
> - Succ = ConfluenceBlock;
> - Block = NULL;
> - LHSBlock = VisitForTemporaryDtors(E->getTrueExpr(), BindToTemporary);
> - if (badCFG)
> - return NULL;
> -
> - // Try to add block with destructors for RHS expression;
> - Succ = ConfluenceBlock;
> - Block = NULL;
> - CFGBlock *RHSBlock = VisitForTemporaryDtors(E->getFalseExpr(),
> - BindToTemporary);
> - if (badCFG)
> - return NULL;
> -
> - if (!RHSBlock && !LHSBlock) {
> - // If neither LHS nor RHS expression had temporaries to destroy don't
> create
> - // more blocks.
> - Block = ConfluenceBlock;
> - return Block;
> - }
> -
> - Block = createBlock(false);
> - Block->setTerminator(CFGTerminator(E, true));
> - assert(Block->getTerminator().isTemporaryDtorsBranch());
> -
> - // See if this is a known constant.
> - const TryResult &KnownVal = tryEvaluateBool(E->getCond());
> -
> - if (LHSBlock) {
> - addSuccessor(Block, LHSBlock, !KnownVal.isFalse());
> - } else if (KnownVal.isFalse()) {
> - addSuccessor(Block, NULL);
> - } else {
> - addSuccessor(Block, ConfluenceBlock);
> - std::reverse(ConfluenceBlock->pred_begin(),
> ConfluenceBlock->pred_end());
> - }
> -
> - if (!RHSBlock)
> - RHSBlock = ConfluenceBlock;
> -
> - addSuccessor(Block, RHSBlock, !KnownVal.isTrue());
> -
> - return Block;
> + CFGBlock *CondBlock = VisitForTemporaryDtors(E->getCond());
> + CFGBlock *TrueBlock = VisitForTemporaryDtors(E->getTrueExpr());
> ----------------
> Alex McCarthy wrote:
> > Noob question: If we return FalseBlock below, do we need to visit
> CondBlock and TrueBlock even if they aren't returned? If we do need to
> visit them, is the order important? (Consider documenting this if this
> isn't super-obvious to someone with more clang experience than me :) )
> Yes, the CFG blocks are hooked up even if they aren't returned. Actually,
> most of the hooking up goes via the Block and Succ members, and afaiu the
> returned value is mostly for convenience (and usually equals Block).
> I'm not sure whether it makes sense to document that here, as it seems to
> be an invariant used throughout the CFG builder.
>
> ================
> Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:771
> @@ +770,3 @@
> + !State->contains<InitializedTemporariesSet>(BTE));
> + State = State->add<InitializedTemporariesSet>(BTE);
> + StmtBldr.generateNode(BTE, Pred, State);
> ----------------
> 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.
>
> ================
> Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:1451
> @@ -1433,1 +1450,3 @@
>
> + if (const CXXBindTemporaryExpr *BTE =
> + dyn_cast<CXXBindTemporaryExpr>(Condition)) {
> ----------------
> 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.
>
> ================
> Comment at: test/Analysis/temporaries.cpp:269
> @@ +268,3 @@
> + if (i < 3 && (i >= 2 || check(NoReturnDtor()))) {
> + // FIXME: Figure out why we don't run into this for i == 2.
> + // If we add if (i == 2 && (...)) we run into this case, though.
> ----------------
> Alex McCarthy wrote:
> > Don't we hit the NoReturnDtor when i == 0, which will prevent us from
> getting to i==2? This seems correct as-is.
> /me headdesks.
>
> ================
> Comment at: test/Analysis/temporaries.cpp:275
> @@ -228,2 +274,3 @@
> + }
>
> void testBinaryOperatorShortcut(bool value) {
> ----------------
> 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).
>
> http://reviews.llvm.org/D3627
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140507/d21e2fad/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: CFG-9b095d.dot.ps
Type: application/postscript
Size: 28222 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140507/d21e2fad/attachment.ps>
More information about the cfe-commits
mailing list