Add more warnings for implict conversions (e.g. double truncation to float).

[avt77]

Summary

As it's set in Bug 32048 at the moment Clang misses a lot of warnings about "possibly problematic" conversions. This patch adds such warnings in places where we have different kinds of assignments.
Alexey, could you have look at OpenMP tests - maybe it's better to change something?

[ABataev]

The OpenMP tests are correct and should remain as is

[avt77]

Ping.

[rjmccall]

Hmm. I think if you're going to push for this, you need to put more time into making sure that the diagnostics are good, and most them seem pretty questionable.

test/OpenMP/teams_distribute_simd_loop_messages.cpp

48 ↗

(On Diff #132125)

This, and all the other warnings in these OpenMP tests, is actually really misleading. It happens to be true that the result of this computation is effectively as if the value being added was just 1, but that's not what the diagnostic actually says, and it wouldn't be true for all operations or values.

test/Sema/constant-conversion.c

76 ↗

(On Diff #132125)

This doesn't seem like a good warning. The original code masks off the high bit of the value stored in the bit-field, which is a perfectly sensible operation to do. It's actually exactly the expected pattern of code for idiomatic bit manipulation.

test/Sema/conversion.c

362

This diagnostic is okay. It would be nice to somehow refer to the fact the the conversion is the conversion done at the end of the compound operation.

Although... it's not like it's unreasonable to multiply an integer by a float and then truncate back to an integer. So there needs to be some way of suppressing this warning, and I'm not sure what it would be, because you can't add a cast without changing the meaning of the operation, so you'd have to spell out the LHS twice, which is really unfortunate. We usually don't like to add warnings for code that might be the best way of expressing what it needs to express.

test/Sema/shift.c

31 ↗

(On Diff #132125)

Okay, this is really a bad diagnostic, because the value 999999 is in no way getting truncated.

test/SemaCXX/conversion.cpp

27 ↗

(On Diff #132125)

Are you implying that we don't currently warn in these cases?

test/SemaCXX/null_in_arithmetic_ops.cpp

65 ↗

(On Diff #132125)

I feel like these don't add much on top of the other warning.

[RKSimon]

lib/Sema/SemaChecking.cpp

9162

Please can you move this function as an NFC commit and then update this patch so it just shows the change in the code.

[avt77]

I removed the ambiguity with changes in CheckImplicitConversion (required by rksimon): now it's absolutely clear what was changed.
And I minimized changes in diagnostics(required by rjmccall): the only float-double truncation was changed. As result only two tests were changed. Later we could easily extend the diagnostic updates if it will be necessary (but inside another patch).

[rjmccall]

In D42728#1006260, @avt77 wrote:

I removed the ambiguity with changes in CheckImplicitConversion (required by rksimon): now it's absolutely clear what was changed.
And I minimized changes in diagnostics(required by rjmccall): the only float-double truncation was changed. As result only two tests were changed. Later we could easily extend the diagnostic updates if it will be necessary (but inside another patch).

Hmm. I'm still torn about this. I feel like at a minimum it's necessary to explain in the diagnostic that the conversion is the final conversion does as part of a compound assignment. I think the right solution is to tell CheckImplicitConversion that it's not just a straight-up conversion, it's a conversion done as part of a compound assignment, and then CheckImplicitConversion can choose to suppress or use different diagnostics based on that information. This will also more reliably avoid duplicate diagnostics.

[avt77]

I think the right solution is to tell CheckImplicitConversion that it's not just a straight-up conversion, it's a conversion done as part of a compound assignment, and then CheckImplicitConversion can choose to suppress or use different diagnostics based on that information. This will also more reliably avoid duplicate diagnostics.

I'm not sure I understand you. There is a special branch in CheckImplicitConversion related to FloatingPoint:

if (SourceBT && SourceBT->isFloatingPoint()) {
  // ...and the target is floating point...
  if (TargetBT && TargetBT->isFloatingPoint()) {
    // ...then warn if we're dropping FP rank.

And that's exactly our case. How can knowledge about "part of a compound assignment" improve the diagnostic here? Please, clarify.

[rjmccall]

In D42728#1015649, @avt77 wrote:

I'm not sure I understand you. There is a special branch in CheckImplicitConversion related to FloatingPoint:

if (SourceBT && SourceBT->isFloatingPoint()) {
  // ...and the target is floating point...
  if (TargetBT && TargetBT->isFloatingPoint()) {
    // ...then warn if we're dropping FP rank.

And that's exactly our case. How can knowledge about "part of a compound assignment" improve the diagnostic here? Please, clarify.

The diagnostic text for this warning is misleading at best when the conversion is actually a truncation of the result of a compound assignment.

[avt77]

In D42728#1016362, @rjmccall wrote:

The diagnostic text for this warning is misleading at best when the conversion is actually a truncation of the result of a compound assignment.

OK, at the moment we have:

*b += a; // expected-warning {{implicit conversion loses floating-point precision: 'double' to 'float'}}

But you'd like to see something like:

*b += a; // expected-warning {{truncation of the result of a compound assignment loses floating-point precision: 'double' to 'float'}}

Right? And you'd like to keep the current message at simple assignment, right?

[rjmccall]

In D42728#1017192, @avt77 wrote:

In D42728#1016362, @rjmccall wrote:

The diagnostic text for this warning is misleading at best when the conversion is actually a truncation of the result of a compound assignment.

OK, at the moment we have:

*b += a; // expected-warning {{implicit conversion loses floating-point precision: 'double' to 'float'}}

But you'd like to see something like:

*b += a; // expected-warning {{truncation of the result of a compound assignment loses floating-point precision: 'double' to 'float'}}

Right?

Right. I would suggest something like "implicit conversion when assigning computation result loses floating-point precision", and you should make sure that the source type is the computation result type, and the arrow should point at the operator.

And you'd like to keep the current message at simple assignment, right?

Yeah. Thanks!

[avt77]

I separated analyze of compound assignments from simple ones and created special diagnostic. Hope, now it's OK.

[rjmccall]

lib/Sema/SemaChecking.cpp

9192

This feels likely to cause redundant or inappropriate diagnostics.

9196

E->getComputationResultType()->getType()->getAs<BuiltinType>()

9198

T->getAs<BuiltinType>()

9205

Should we actually do a suppression for constant values here? We suppress them for normal assignments because it's free to coerce a constant double to float, but [fixed]myFloat *= 2.0[/fixed] actually does do a computation in [fixed]double[/fixed] that has to be truncated to [fixed]float[/fixed]. Someone using this warning to find places where unwanted implicit floating-point coercions are being used would surely want to know about this location so that they can make the literal [fixed]2.0f[/fixed].

[ABataev]

lib/Sema/SemaChecking.cpp

9147

Function names should follow coding standard, shouldWarnFloatPrecision.

9153

The names of the variables should start from capital letter, Result

9161–9162

Do you need this check? With or without it you just return true.

9189

Start from small letter function names.

9195

const BuiltinType *->const auto *

9197

const BuiltinType *->const auto *

[avt77]

lib/Sema/SemaChecking.cpp

9147

That's a problem. Look around - they use capital.

9153

And they use 'result'. Ok, I'll follow standard here.

9161–9162

Many tnx - it's a bug; I should return false.

[avt77]

lib/Sema/SemaChecking.cpp

9196

Unfortunatelly, it does not work because getComputationResultType() returns 'double':

CompoundAssignOperator 0xd2db9a0 'float' '+=' ComputeLHSTy='double' ComputeResultTy='double'

-UnaryOperator 0xd2db940 'float' lvalue prefix '*' cannot overflow

`-ImplicitCastExpr 0xd2db928 'float *' <LValueToRValue>

`-DeclRefExpr 0xd2db900 'float *' lvalue ParmVar 0xd2db780 'b' 'float *'

`-ImplicitCastExpr 0xd2db988 'double' <LValueToRValue>

`-DeclRefExpr 0xd2db960 'double' lvalue ParmVar 0xd2db6d8 'a' 'double'

(gdb) p CAOp->getComputationLHSType()->dump()
BuiltinType 0xd25a9b0 'double'

(gdb) p CAOp->getComputationResultType ()->dump()
BuiltinType 0xd25a9b0 'double'

[avt77]

It seems I fixed all requirements what I could.

[rjmccall]

lib/Sema/SemaChecking.cpp

9196

Sorry, I mean instead of E->getRHS()->getType().

[avt77]

lib/Sema/SemaChecking.cpp

9196

OK, in this case we need clear comment about getComputationResultType: I was sure that ResultType is LHS type. Could you add such a comment in Expr.h?

[avt77]

The issue with getComputationResultType was fixed.

[rjmccall]

lib/Sema/SemaChecking.cpp

9189

Looking at the other Analyze functions, it looks like you're supposed to recurse on the LHS and RHS in here: this is supposed to diagnose the entire expression tree, not just conversions in the outermost expression. The test case would be to have an unfortunate implicit conversion embedded somewhere in the LHS or RHS, like as an argument to a call that produces a subscript index or something.

[avt77]

If I understood rjmccall correctly I should do both: recurse in LHS/RHS and check the outermost expression. If it is I fixed the issue.
On the other side in one of my previous versions there was a call to AnalyzeAssignment and this call did exactly the same plus bitfield issue checking. Maybe we should return this call instead 2 explicit calls to AnalyzeImplicitConversions?

[avt77]

Sorry, I've updated the wrong debugging version - I'll fix it asap.

[avt77]

It seems I found a new problem here: with vector type it works just now (w/o this patch)

llvm/tools/clang/test/Sema/ext_vector_conversions.c:18:10: warning: implicit conversion loses integer precision: 'long long' to 'short4' (vector of 4 'short' values)

vs4 += ll; // expected-warning {{implicit conversion loses integer precision}}
    ~~ ^~

Let's wait until I'll find and fix the issue. I'll warn you when it's ready for review.

[avt77]

I've fixed the last minute issues and it works now properly.
On the other hand I'm going to continue the research about vector types: maybe it's possible to extend the current code?
But if it's possible I'd like to commit the current patch and to continue with the new one.

[rjmccall]

lib/Sema/SemaChecking.cpp

9193

It's not obvious that the bit-field special case in that function is appropriate for all compound assignments. For now, let's just leave that function for simple assignments; we can directly call AnalyzeImplicitConversions on the LHS and RHS here.

[avt77]

lib/Sema/SemaChecking.cpp

9193

There is one problem only here: if we don't support the bit-field special case here then we have several additional tests failed. But OK - I'll do it.

[avt77]

I removed cheking of bit-field case.
And I was wrong about increasing of number of failed tests: it's starnge but I was sure I saw it. OK, now we have one modified test only.

[rjmccall]

In D42728#1036743, @avt77 wrote:

I removed cheking of bit-field case.
And I was wrong about increasing of number of failed tests: it's starnge but I was sure I saw it. OK, now we have one modified test only.

Yeah, the previous behavior is just a simple recursion without any assignment-specific knowledge, so that makes sense.

LGTM.

[ABataev]

lib/Sema/SemaChecking.cpp

9527

DO you still need ICContext parameter? I don't see where it is used.

[ABataev]

lib/Sema/SemaChecking.cpp

8623–8625

Do you really need this declaration here?

9527

Hmm, seems to me you just reformatted the code. Restore the original code.

[avt77]

I fixed all issues raised by ABataev and committed the patch as revision 327618. I'll close the review in some hours.

[lebedev.ri]

In D42728#1038500, @avt77 wrote:

I fixed all issues raised by ABataev and committed the patch as revision 327618. I'll close the review in some hours.

You are aware that you can either use arc patch (best way), or at least manually add Differential revision: https://reviews.llvm.org/Dsomething line to the commit msg, so

1. the differential is auto-closed
2. the commit msg is taken from differential description
3. there is a trivial way to go from commit to differential (and vice versa)

?

[avt77]

In D42728#1038501, @lebedev.ri wrote:

In D42728#1038500, @avt77 wrote:

I fixed all issues raised by ABataev and committed the patch as revision 327618. I'll close the review in some hours.

You are aware that you can either use arc patch (best way), or at least manually add Differential revision: https://reviews.llvm.org/Dsomething line to the commit msg, so

1. the differential is auto-closed
2. the commit msg is taken from differential description
3. there is a trivial way to go from commit to differential (and vice versa)

?

If I'm sure in success I'm using "Differential revision" but sometimes I'd prefer to wait until the patch is really accepted by the system and only after that I'm closing it by hand.
But what's about arc patch? Could you point me to any docs about?

[lebedev.ri]

In D42728#1039865, @avt77 wrote:

In D42728#1038501, @lebedev.ri wrote:

In D42728#1038500, @avt77 wrote:

I fixed all issues raised by ABataev and committed the patch as revision 327618. I'll close the review in some hours.

You are aware that you can either use arc patch (best way), or at least manually add Differential revision: https://reviews.llvm.org/Dsomething line to the commit msg, so

1. the differential is auto-closed
2. the commit msg is taken from differential description
3. there is a trivial way to go from commit to differential (and vice versa)

?

If I'm sure in success I'm using "Differential revision" but sometimes I'd prefer to wait until the patch is really accepted by the system and only after that I'm closing it by hand.
But what's about arc patch? Could you point me to any docs about?

Sure, see https://llvm.org/docs/Phabricator.html#committing-a-change

[avt77]

It's committed: https://reviews.llvm.org/rC327618.

include/clang/Basic/DiagnosticSemaKinds.td

 def warn_impcast_complex_scalar : Warning<
   "implicit conversion discards imaginary component: %0 to %1">,
   InGroup<Conversion>, DefaultIgnore;
 def err_impcast_complex_scalar : Error<
   "implicit conversion from %0 to %1 is not permitted in C++">;
 def warn_impcast_float_precision : Warning<
   "implicit conversion loses floating-point precision: %0 to %1">,
   InGroup<Conversion>, DefaultIgnore;
+def warn_impcast_float_result_precision : Warning<
+  "implicit conversion when assigning computation result loses floating-point precision: %0 to %1">,
+  InGroup<Conversion>, DefaultIgnore;
 def warn_impcast_double_promotion : Warning<
   "implicit conversion increases floating-point precision: %0 to %1">,
   InGroup<DoublePromotion>, DefaultIgnore;
 def warn_impcast_integer_sign : Warning<
   "implicit conversion changes signedness: %0 to %1">,
   InGroup<SignConversion>, DefaultIgnore;
 def warn_impcast_integer_sign_conditional : Warning<
   "operand of ? changes signedness: %0 to %1">,

lib/Sema/SemaChecking.cpp

static bool IsSameFloatAfterCast(const APValue &value,
   }
 
   assert(value.isComplexFloat());
   return (IsSameFloatAfterCast(value.getComplexFloatReal(), Src, Tgt) &&
           IsSameFloatAfterCast(value.getComplexFloatImag(), Src, Tgt));
 }
 
 static void AnalyzeImplicitConversions(Sema &S, Expr *E, SourceLocation CC);
+static void CheckImplicitConversion(Sema &S, Expr *E, QualType T,
+                                    SourceLocation CC,
+                                    bool *ICContext = nullptr);
 
 static bool IsEnumConstOrFromMacro(Sema &S, Expr *E) {
   // Suppress cases where we are comparing against an enum constant.
   if (const DeclRefExpr *DR =
       dyn_cast<DeclRefExpr>(E->IgnoreParenImpCasts()))
     if (isa<EnumConstantDecl>(DR->getDecl()))
       return true;
 
static bool AnalyzeBitFieldAssignment(Sema &S, FieldDecl *Bitfield, Expr *Init,
 
   S.Diag(InitLoc, diag::warn_impcast_bitfield_precision_constant)
     << PrettyValue << PrettyTrunc << OriginalInit->getType()
     << Init->getSourceRange();
 
   return true;
 }
 
 /// Analyze the given simple or compound assignment for warning-worthy
 /// operations.
 static void AnalyzeAssignment(Sema &S, BinaryOperator *E) {
-  // Just recurse on the LHS.
-  AnalyzeImplicitConversions(S, E->getLHS(), E->getOperatorLoc());
 
   // We want to recurse on the RHS as normal unless we're assigning to
   // a bitfield.
   if (FieldDecl *Bitfield = E->getLHS()->getSourceBitField()) {
     if (AnalyzeBitFieldAssignment(S, Bitfield, E->getRHS(),
                                   E->getOperatorLoc())) {
       // Recurse, ignoring any implicit conversions on the RHS.
       return AnalyzeImplicitConversions(S, E->getRHS()->IgnoreParenImpCasts(),
                                         E->getOperatorLoc());
     }
   }
 
   AnalyzeImplicitConversions(S, E->getRHS(), E->getOperatorLoc());
 }
 
 /// Diagnose an implicit cast;  purely a helper for CheckImplicitConversion.
 static void DiagnoseImpCast(Sema &S, Expr *E, QualType SourceType, QualType T, 
                             SourceLocation CContext, unsigned diag,
                             bool pruneControlFlow = false) {
   if (pruneControlFlow) {
     S.DiagRuntimeBehavior(E->getExprLoc(), E,
                           S.PDiag(diag)
                             << SourceType << T << E->getSourceRange()
                             << SourceRange(CContext));
     return;
   }
   S.Diag(E->getExprLoc(), diag)
     << SourceType << T << E->getSourceRange() << SourceRange(CContext);
 }
 
 /// Diagnose an implicit cast;  purely a helper for CheckImplicitConversion.
 static void DiagnoseImpCast(Sema &S, Expr *E, QualType T,
                             SourceLocation CContext,
                             unsigned diag, bool pruneControlFlow = false) {
   DiagnoseImpCast(S, E, E->getType(), T, CContext, diag, pruneControlFlow);
 }
 
+/// Analyze the given compound assignment for the possible losing of
+/// floating-point precision.
+static void AnalyzeCompoundAssignment(Sema &S, BinaryOperator *E) {
+  assert(isa<CompoundAssignOperator>(E) &&
+         "Must be compound assignment operation");
+  // Recurse on the LHS and RHS in here
+  AnalyzeImplicitConversions(S, E->getLHS(), E->getOperatorLoc());
+  AnalyzeImplicitConversions(S, E->getRHS(), E->getOperatorLoc());
+
+  // Now check the outermost expression
+  const auto *ResultBT = E->getLHS()->getType()->getAs<BuiltinType>();
+  const auto *RBT = cast<CompoundAssignOperator>(E)
+                        ->getComputationResultType()
+                        ->getAs<BuiltinType>();
+
+  // If both source and target are floating points.
+  if (ResultBT && ResultBT->isFloatingPoint() && RBT && RBT->isFloatingPoint())
+    // Builtin FP kinds are ordered by increasing FP rank.
+    if (ResultBT->getKind() < RBT->getKind())
+      // We don't want to warn for system macro.
+      if (!S.SourceMgr.isInSystemMacro(E->getOperatorLoc()))
+        // warn about dropping FP rank.
+        DiagnoseImpCast(S, E->getRHS(), E->getLHS()->getType(),
+                        E->getOperatorLoc(),
+                        diag::warn_impcast_float_result_precision);
+}
 
 /// Diagnose an implicit cast from a floating point value to an integer value.
 static void DiagnoseFloatingImpCast(Sema &S, Expr *E, QualType T,
                                     SourceLocation CContext) {
   const bool IsBool = T->isSpecificBuiltinType(BuiltinType::Bool);
   const bool PruneWarnings = S.inTemplateInstantiation();
 
   Expr *InnerE = E->IgnoreParenImpCasts();
const char FirstContextCharacter =
         S.getSourceManager().getCharacterData(CC)[0];
     if (FirstContextCharacter == '{')
       return false;
   }
 
   return true;
 }
 
-static void
-CheckImplicitConversion(Sema &S, Expr *E, QualType T, SourceLocation CC,
-                        bool *ICContext = nullptr) {
+static void CheckImplicitConversion(Sema &S, Expr *E, QualType T,
+                                    SourceLocation CC, bool *ICContext) {
   if (E->isTypeDependent() || E->isValueDependent()) return;
 
   const Type *Source = S.Context.getCanonicalType(E->getType()).getTypePtr();
   const Type *Target = S.Context.getCanonicalType(T).getTypePtr();
   if (Source == Target) return;
   if (Target->isDependentType()) return;
 
   // If the conversion context location is invalid don't complain. We also
static void CheckImplicitConversion(Sema &S, Expr *E, QualType T,
 
   // Strip vector types.
   if (isa<VectorType>(Source)) {
     if (!isa<VectorType>(Target)) {
       if (S.SourceMgr.isInSystemMacro(CC))
         return;
       return DiagnoseImpCast(S, E, T, CC, diag::warn_impcast_vector_scalar);
     }
-    
     // If the vector cast is cast between two vectors of the same size, it is
     // a bitcast, not a conversion.
     if (S.Context.getTypeSize(Source) == S.Context.getTypeSize(Target))
       return;
 
     Source = cast<VectorType>(Source)->getElementType().getTypePtr();
     Target = cast<VectorType>(Target)->getElementType().getTypePtr();
   }

 /// of competing diagnostics here, -Wconversion and -Wsign-compare.
 static void AnalyzeImplicitConversions(Sema &S, Expr *OrigE,
                                        SourceLocation CC) {
   QualType T = OrigE->getType();
   Expr *E = OrigE->IgnoreParenImpCasts();
 
   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);
     CheckConditionalOperator(S, CO, CC, T);
     return;
   }
 
static void AnalyzeImplicitConversions(Sema &S, Expr *OrigE,
   if (BinaryOperator *BO = dyn_cast<BinaryOperator>(E)) {
     // Do a somewhat different check with comparison operators.
     if (BO->isComparisonOp())
       return AnalyzeComparison(S, BO);
 
     // And with simple assignments.
     if (BO->getOpcode() == BO_Assign)
       return AnalyzeAssignment(S, BO);
+    // And with compound assignments.
+    if (BO->isAssignmentOp())
+      return AnalyzeCompoundAssignment(S, BO);
   }
 
   // These break the otherwise-useful invariant below.  Fortunately,
   // we don't really need to recurse into them, because any internal
   // expressions should have been analyzed already when they were
   // built into statements.
   if (isa<StmtExpr>(E)) return;
 

test/Sema/conversion.c

void test_7631400(void) {
   char s = LONG_MAX; // expected-warning {{implicit conversion from 'long' to 'char' changes value}}
 }
 
 // <rdar://problem/7676608>: assertion for compound operators with non-integral RHS
 void f7676608(int);
 void test_7676608(void) {
   float q = 0.7f;
   char c = 5;
   f7676608(c *= q);
 }
 
 // <rdar://problem/7904686>
 void test_7904686(void) {
   const int i = -1;
   unsigned u1 = i; // expected-warning {{implicit conversion changes signedness}}  
   u1 = i; // expected-warning {{implicit conversion changes signedness}}  
 

 }
 
 
 float double2float_test1(double a) {
     return a; // expected-warning {{implicit conversion loses floating-point precision: 'double' to 'float'}}
 }
 
 void double2float_test2(double a, float *b) {
-    *b += a;
+  *b += a; // expected-warning {{implicit conversion when assigning computation result loses floating-point precision: 'double' to 'float'}}
 }
 
 float sinf (float x);
 double double2float_test3(double a) {
     return sinf(a); // expected-warning {{implicit conversion loses floating-point precision: 'double' to 'float'}}
 }
+
+float double2float_test4(double a, float b) {
+  b -= a; // expected-warning {{implicit conversion when assigning computation result loses floating-point precision: 'double' to 'float'}}
+  return b;
+}