r291964 - PR31631: fix bad CFG (and bogus warnings) when an if-statement has an init-statement and has binary operator as its condition.

Hans Wennborg via cfe-commits cfe-commits at lists.llvm.org
Fri Jan 13 15:58:16 PST 2017


Merged in r291978.

(Please keep the list cc'd on merge requests.)

Thanks,
Hans

On Fri, Jan 13, 2017 at 2:28 PM, Richard Smith <richard at metafoo.co.uk> wrote:
> Seems sensible to put this in clang 4.
>
> ---------- Forwarded message ----------
> From: Richard Smith via cfe-commits <cfe-commits at lists.llvm.org>
> Date: 13 January 2017 at 14:16
> Subject: r291964 - PR31631: fix bad CFG (and bogus warnings) when an
> if-statement has an init-statement and has binary operator as its condition.
> To: cfe-commits at lists.llvm.org
>
>
> Author: rsmith
> Date: Fri Jan 13 16:16:41 2017
> New Revision: 291964
>
> URL: http://llvm.org/viewvc/llvm-project?rev=291964&view=rev
> Log:
> PR31631: fix bad CFG (and bogus warnings) when an if-statement has an
> init-statement and has binary operator as its condition.
>
> Modified:
>     cfe/trunk/lib/Analysis/CFG.cpp
>     cfe/trunk/test/SemaCXX/uninitialized.cpp
>
> Modified: cfe/trunk/lib/Analysis/CFG.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Analysis/CFG.cpp?rev=291964&r1=291963&r2=291964&view=diff
> ==============================================================================
> --- cfe/trunk/lib/Analysis/CFG.cpp (original)
> +++ cfe/trunk/lib/Analysis/CFG.cpp Fri Jan 13 16:16:41 2017
> @@ -2175,19 +2175,15 @@ CFGBlock *CFGBuilder::VisitIfStmt(IfStmt
>    SaveAndRestore<LocalScope::const_iterator> save_scope_pos(ScopePos);
>
>    // Create local scope for C++17 if init-stmt if one exists.
> -  if (Stmt *Init = I->getInit()) {
> -    LocalScope::const_iterator BeginScopePos = ScopePos;
> +  if (Stmt *Init = I->getInit())
>      addLocalScopeForStmt(Init);
> -    addAutomaticObjDtors(ScopePos, BeginScopePos, I);
> -  }
>
>    // Create local scope for possible condition variable.
>    // Store scope position. Add implicit destructor.
> -  if (VarDecl *VD = I->getConditionVariable()) {
> -    LocalScope::const_iterator BeginScopePos = ScopePos;
> +  if (VarDecl *VD = I->getConditionVariable())
>      addLocalScopeForVarDecl(VD);
> -    addAutomaticObjDtors(ScopePos, BeginScopePos, I);
> -  }
> +
> +  addAutomaticObjDtors(ScopePos, save_scope_pos.get(), I);
>
>    // The block we were processing is now finished.  Make it the successor
>    // block.
> @@ -2256,36 +2252,39 @@ CFGBlock *CFGBuilder::VisitIfStmt(IfStmt
>    // removes infeasible paths from the control-flow graph by having the
>    // control-flow transfer of '&&' or '||' go directly into the then/else
>    // blocks directly.
> -  if (!I->getConditionVariable())
> -    if (BinaryOperator *Cond =
> -            dyn_cast<BinaryOperator>(I->getCond()->IgnoreParens()))
> -      if (Cond->isLogicalOp())
> -        return VisitLogicalOperator(Cond, I, ThenBlock, ElseBlock).first;
> -
> -  // Now create a new block containing the if statement.
> -  Block = createBlock(false);
> +  BinaryOperator *Cond =
> +      I->getConditionVariable()
> +          ? nullptr
> +          : dyn_cast<BinaryOperator>(I->getCond()->IgnoreParens());
> +  CFGBlock *LastBlock;
> +  if (Cond && Cond->isLogicalOp())
> +    LastBlock = VisitLogicalOperator(Cond, I, ThenBlock, ElseBlock).first;
> +  else {
> +    // Now create a new block containing the if statement.
> +    Block = createBlock(false);
>
> -  // Set the terminator of the new block to the If statement.
> -  Block->setTerminator(I);
> +    // Set the terminator of the new block to the If statement.
> +    Block->setTerminator(I);
>
> -  // See if this is a known constant.
> -  const TryResult &KnownVal = tryEvaluateBool(I->getCond());
> +    // See if this is a known constant.
> +    const TryResult &KnownVal = tryEvaluateBool(I->getCond());
>
> -  // Add the successors.  If we know that specific branches are
> -  // unreachable, inform addSuccessor() of that knowledge.
> -  addSuccessor(Block, ThenBlock, /* isReachable = */ !KnownVal.isFalse());
> -  addSuccessor(Block, ElseBlock, /* isReachable = */ !KnownVal.isTrue());
> -
> -  // Add the condition as the last statement in the new block.  This may
> create
> -  // new blocks as the condition may contain control-flow.  Any newly
> created
> -  // blocks will be pointed to be "Block".
> -  CFGBlock *LastBlock = addStmt(I->getCond());
> -
> -  // If the IfStmt contains a condition variable, add it and its
> -  // initializer to the CFG.
> -  if (const DeclStmt* DS = I->getConditionVariableDeclStmt()) {
> -    autoCreateBlock();
> -    LastBlock = addStmt(const_cast<DeclStmt *>(DS));
> +    // Add the successors.  If we know that specific branches are
> +    // unreachable, inform addSuccessor() of that knowledge.
> +    addSuccessor(Block, ThenBlock, /* isReachable = */
> !KnownVal.isFalse());
> +    addSuccessor(Block, ElseBlock, /* isReachable = */ !KnownVal.isTrue());
> +
> +    // Add the condition as the last statement in the new block.  This may
> +    // create new blocks as the condition may contain control-flow.  Any
> newly
> +    // created blocks will be pointed to be "Block".
> +    LastBlock = addStmt(I->getCond());
> +
> +    // If the IfStmt contains a condition variable, add it and its
> +    // initializer to the CFG.
> +    if (const DeclStmt* DS = I->getConditionVariableDeclStmt()) {
> +      autoCreateBlock();
> +      LastBlock = addStmt(const_cast<DeclStmt *>(DS));
> +    }
>    }
>
>    // Finally, if the IfStmt contains a C++17 init-stmt, add it to the CFG.
> @@ -3078,19 +3077,15 @@ CFGBlock *CFGBuilder::VisitSwitchStmt(Sw
>    SaveAndRestore<LocalScope::const_iterator> save_scope_pos(ScopePos);
>
>    // Create local scope for C++17 switch init-stmt if one exists.
> -  if (Stmt *Init = Terminator->getInit()) {
> -    LocalScope::const_iterator BeginScopePos = ScopePos;
> +  if (Stmt *Init = Terminator->getInit())
>      addLocalScopeForStmt(Init);
> -    addAutomaticObjDtors(ScopePos, BeginScopePos, Terminator);
> -  }
>
>    // Create local scope for possible condition variable.
>    // Store scope position. Add implicit destructor.
> -  if (VarDecl *VD = Terminator->getConditionVariable()) {
> -    LocalScope::const_iterator SwitchBeginScopePos = ScopePos;
> +  if (VarDecl *VD = Terminator->getConditionVariable())
>      addLocalScopeForVarDecl(VD);
> -    addAutomaticObjDtors(ScopePos, SwitchBeginScopePos, Terminator);
> -  }
> +
> +  addAutomaticObjDtors(ScopePos, save_scope_pos.get(), Terminator);
>
>    if (Block) {
>      if (badCFG)
>
> Modified: cfe/trunk/test/SemaCXX/uninitialized.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/uninitialized.cpp?rev=291964&r1=291963&r2=291964&view=diff
> ==============================================================================
> --- cfe/trunk/test/SemaCXX/uninitialized.cpp (original)
> +++ cfe/trunk/test/SemaCXX/uninitialized.cpp Fri Jan 13 16:16:41 2017
> @@ -1,4 +1,4 @@
> -// RUN: %clang_cc1 -fsyntax-only -Wall -Wuninitialized -Wno-unused-value
> -Wno-unused-lambda-capture -std=c++11 -verify %s
> +// RUN: %clang_cc1 -fsyntax-only -Wall -Wuninitialized -Wno-unused-value
> -Wno-unused-lambda-capture -std=c++1z -verify %s
>
>  // definitions for std::move
>  namespace std {
> @@ -1437,3 +1437,13 @@ void array_capture(bool b) {
>      [fname]{};
>    }
>  }
> +
> +void if_switch_init_stmt(int k) {
> +  if (int n = 0; (n == k || k > 5)) {}
> +
> +  if (int n; (n == k || k > 5)) {} // expected-warning {{uninitialized}}
> expected-note {{initialize}}
> +
> +  switch (int n = 0; (n == k || k > 5)) {} // expected-warning {{boolean}}
> +
> +  switch (int n; (n == k || k > 5)) {} // expected-warning
> {{uninitialized}} expected-note {{initialize}} expected-warning {{boolean}}
> +}
>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>


More information about the cfe-commits mailing list