patch for llvm/clang bug 25965, suppress warning diagnostic on float-to-bool conversion when in condition context

[mibintc]

Summary

Hello,
I'm submitting this patch to suppress the warning diagnostic that is reported in the bugs database as 25965. I work at Intel and we are compiling a spec test which enables -Wfloat-conversion when compiling on Linux in the gcc environment, -Werror causes the spec compilation to fail.
This patch adds a bit to Expression nodes "isCondition". This means that the expression occurs as a condition. clang knows that it is holding a condition, but as it gets deeper into the call tree, the fact that the expression is a condition gets lost and we no longer know the context.
The changes made to lib/Parse/ParseExpr.c are necessary to mark ternary expressions as conditions (i.e. the foo in: foo ? yes : no)
The 2nd set of changes to lib/Sema/SemaChecking is necessary for c++ compilations
The changes to lib/Sema/SemaStmt is necessary for do while(foo) expressions
The test case changes contain a question : should these conversion warnings be suppressed only for simple expressions, or OK to suppress generally? The patch submitted here suppresses generally for any float to bool conversion in the condition context.
This is my first patch submission, any and all comments welcome. Thanks and regards, Melanie Blower

[mibintc]

I regenerated the diff using diff -x -U999999

[joerg]

Besides "it triggers on SPEC", why should the warning be disabled here? I think it is perfectly sensible to have a warning for all six conditional contexts here.

[mibintc]

Does anyone have time to do a code review for this patch?

[mibintc]

BTW I tried a couple different approaches to suppress the message.

One of them was to call IgnoreParenImpCasts(). This worked great to suppress the unwanted diagnostic but the call also inhibited constant folding and dead code elimination, so there were many test failures. I couldn't find any documentation about when and where a call to IgnoreParenImpCasts is appropriate. Use with care.

[erichkeane]

I believe that this is a good patch that solves an open Bug. See : https://llvm.org/bugs/show_bug.cgi?id=25965

I think the SPEC case is much less compelling, but I think that using a float explicitly in a conditional is something I really wouldn't expect a warning on.

@rsmith You had commented on the bug initially suggesting suppressing the diagnostic entirely in this case. Do you still feel that way?

include/clang/AST/Expr.h

Expr(StmtClass SC, QualType T, ExprValueKind VK, ExprObjectKind OK,
     : Stmt(SC)
   {
     ExprBits.TypeDependent = TD;
     ExprBits.ValueDependent = VD;
     ExprBits.InstantiationDependent = ID;
     ExprBits.ValueKind = VK;
     ExprBits.ObjectKind = OK;
     ExprBits.ContainsUnexpandedParameterPack = ContainsUnexpandedParameterPack;
+    ExprBits.IsCondition = 0;
     setType(T);
   }
 
   /// \brief Construct an empty expression.
   explicit Expr(StmtClass SC, EmptyShell) : Stmt(SC) { }
 
 public:
   QualType getType() const { return TR; }
public:
   }
 
   /// \brief Set the bit that describes whether this expression
   /// contains an unexpanded parameter pack.
   void setContainsUnexpandedParameterPack(bool PP = true) {
     ExprBits.ContainsUnexpandedParameterPack = PP;
   }
 
+  /// \brief Whether this expression appears in condition expression context.
+  bool isCondition() const {
+    return ExprBits.IsCondition;
+  }
+
+  /// \brief Set the bit that describes whether this expression
+  /// appears in condition context e.g.: if(expr) while(expr) ?: etc.
+  void setIsCondition(bool PP = true) {
+    ExprBits.IsCondition = PP;
+  }
+
   /// getExprLoc - Return the preferred location for the arrow when diagnosing
   /// a problem with a generic expression.
   SourceLocation getExprLoc() const LLVM_READONLY;
 
   /// isUnusedResultAWarning - Return true if this immediate expression should
   /// be warned about if the result is unused.  If so, fill in expr, location,
   /// and ranges with expr to warn on and source locations/ranges appropriate
   /// for a warning.

include/clang/AST/Stmt.h

class ExprBitfields {
     unsigned : NumStmtBits;
 
     unsigned ValueKind : 2;
     unsigned ObjectKind : 2;
     unsigned TypeDependent : 1;
     unsigned ValueDependent : 1;
     unsigned InstantiationDependent : 1;
     unsigned ContainsUnexpandedParameterPack : 1;
+    unsigned IsCondition : 1;
   };
-  enum { NumExprBits = 16 };
+  enum { NumExprBits = 17 };
 
   class CharacterLiteralBitfields {
     friend class CharacterLiteral;
     unsigned : NumExprBits;
 
     unsigned Kind : 3;
   };
 

lib/Parse/ParseExpr.cpp

if (NextTokPrec == prec::Conditional) {
           TernaryMiddle = nullptr;
         }
       } else {
         // Special case handling of "X ? Y : Z" where Y is empty:
         //   logical-OR-expression '?' ':' conditional-expression   [GNU]
         TernaryMiddle = nullptr;
         Diag(Tok, diag::ext_gnu_conditional_expr);
       }
+      if (!LHS.isInvalid()) 
+        LHS.get()->setIsCondition();
 
       if (!TryConsumeToken(tok::colon, ColonLoc)) {
         // Otherwise, we're missing a ':'.  Assume that this was a typo that
         // the user forgot. If we're not in a macro expansion, we can suggest
         // a fixit hint. If there were two spaces before the current token,
         // suggest inserting the colon in between them, otherwise insert ": ".
         SourceLocation FILoc = Tok.getLocation();
         const char *FIText = ": ";

lib/Sema/SemaChecking.cpp

if (TargetBT && TargetBT->isFloatingPoint()) {
       return;
     }
 
     // If the target is integral, always warn.
     if (TargetBT && TargetBT->isInteger()) {
       if (S.SourceMgr.isInSystemMacro(CC))
         return;
 
+      // Suppress float-to-bool diagnostic in conditions.
+      if (TargetBT->isBooleanType() && E->isCondition())
+        return;
+
       DiagnoseFloatingImpCast(S, E, T, CC);
     }
 
     // Detect the case where a call result is converted from floating-point to
     // to bool, and the final argument to the call is converted from bool, to
     // discover this typo:
     //
     //    bool b = fabs(x < 1.0);  // should be "bool b = fabs(x) < 1.0;"
void CheckBoolLikeConversion(Sema &S, Expr *E, SourceLocation CC) {
   CheckImplicitConversion(S, E->IgnoreParenImpCasts(), S.Context.BoolTy, CC);
 }
 
 /// AnalyzeImplicitConversions - Find and report any interesting
 /// implicit conversions in the given expression.  There are a couple
 /// of competing diagnostics here, -Wconversion and -Wsign-compare.
 void AnalyzeImplicitConversions(Sema &S, Expr *OrigE, SourceLocation CC) {
   QualType T = OrigE->getType();
+  bool isCondition = OrigE->isCondition();
   Expr *E = OrigE->IgnoreParenImpCasts();
+  if (isCondition)
+    E->setIsCondition(isCondition);
 
   if (E->isTypeDependent() || E->isValueDependent())
     return;
   
   // For conditional operators, we analyze the arguments as if they
   // were being fed directly into the output.
   if (isa<ConditionalOperator>(E)) {
     ConditionalOperator *CO = cast<ConditionalOperator>(E);

lib/Sema/SemaExpr.cpp

Sema::ConditionResult Sema::ActOnCondition(Scope *S, SourceLocation Loc,
                                            Expr *SubExpr, ConditionKind CK) {
   // Empty conditions are valid in for-statements.
   if (!SubExpr)
     return ConditionResult();
 
   ExprResult Cond;
   switch (CK) {
   case ConditionKind::Boolean:
+    SubExpr->setIsCondition();
     Cond = CheckBooleanCondition(Loc, SubExpr);
     break;
 
   case ConditionKind::ConstexprIf:
+    SubExpr->setIsCondition();
     Cond = CheckBooleanCondition(Loc, SubExpr, true);
     break;
 
   case ConditionKind::Switch:
     Cond = CheckSwitchCondition(Loc, SubExpr);
     break;
   }
   if (Cond.isInvalid())

lib/Sema/SemaStmt.cpp

Sema::ActOnDoStmt(SourceLocation DoLoc, Stmt *Body,
                   Expr *Cond, SourceLocation CondRParen) {
   assert(Cond && "ActOnDoStmt(): missing expression");
 
   CheckBreakContinueBinding(Cond);
   ExprResult CondResult = CheckBooleanCondition(DoLoc, Cond);
   if (CondResult.isInvalid())
     return StmtError();
   Cond = CondResult.get();
+  Cond->setIsCondition();
 
   CondResult = ActOnFinishFullExpr(Cond, DoLoc);
   if (CondResult.isInvalid())
     return StmtError();
   Cond = CondResult.get();
 
   DiagnoseUnusedExprResult(Body);
 

test/SemaCXX/warn-float-conversion.cpp

void TestOverflow() {
 
   const float LargeNumber = 1024;
   char c = LargeNumber;  // expected-warning{{implicit conversion of out of range value from 'const float' to 'char' changes value from 1024 to 127}}
   char d = 400.0 + 400.0;  // expected-warning{{implicit conversion of out of range value from 'double' to 'char' changes value from 800 to 127}}
 
   char e = 1.0 / 0.0;  // expected-warning{{implicit conversion of out of range value from 'double' to 'char' changes value from +Inf to 127}}
 }
 #endif  // OVERFLOW
+
+int m(int argc)
+{
+  float f = argc;
+  // Test that float-to-bool conversion warnings are not issued
+  // for conditions.
+ 
+
+  if (f)    
+    return 1;
+  else  
+    return f
+           ? 1 
+           : 0;
+
+  if (f+1)    //Question: also suppress for this expresson?
+    return 1;
+
+  while (f) ;
+
+  do ; while (f) ;
+
+  for (; f ; ) ;
+
+  return 0;
+}