[Diagnostics] Check integer to floating point number implicit conversions

[xbolva00]

Summary

GCC already catches these situations so we should handle it too.

GCC warns in C++ mode only (does anybody know why?). I think it is useful in C mode too.

[xbolva00]

• Added tests

[lebedev.ri]

test/Sema/ext_vector_casts.c

121

How is floating-point precision is being lost if the cast is from an integer?
I'm guessing you want to add a new diag with a more appropriate description.

[xbolva00]

test/Sema/ext_vector_casts.c

121

yeah :/ so maybe integer precision?

[lebedev.ri]

test/Sema/ext_vector_casts.c

121

It's still rather a bit too vague i'd say.
It should describe what you actually checked.

[xbolva00]

• better warning

[erichkeane]

lib/Sema/SemaChecking.cpp

110

Hmm.... I don't terribly understand how this function works. Also, comment above needs to end in a period.

Can you elaborate further as to how this works? Those are 3 pretty suspect looking magic numbers...

10918

Since there is only 1, and fltSemantics is really small (2 shorts + 2 uints), making this a 'by copy' is likely a better solution than a pointer.

10923

This else case ends up needing to be handled above, but why not also handle the other floating types?

10929

It seems like this should be checked way before we convert it to an integer or do the other float-semantics disambiguation.

10933

Is this the rounding mode that we use? Also, you might need to check this against some sort of current state for rounding mode. I know that there is an effort to do FENV_ACCESS, which this might change, right?

10939

I wonder if this call (finding the precision, 'adjusting' it, then writing to a smallstring might be a better thing to pull into its own function rather than AdjustPrecision.

[scanon]

lib/Sema/SemaChecking.cpp

110

It's attempting to compute the number of good base-10 digits (59/196 ~= log2(10)). We should really just make APFloat print the shortest round-trippable digit sequence instead. Yes, this is tricky to implement, but we don't need to implement it. There are two recent high-quality implementations available, which are both significantly faster than previous algorithms: Ryu and Swift's (https://github.com/apple/swift/blob/master/stdlib/public/runtime/SwiftDtoa.cpp). Swift's has the virtue of already being used in LLVM-family languages and having a tidy single-file implementation, but either would be perfectly usable, I think.

Neither supports float128 yet, but we could simply drop them in for float, double, and float80.

[xbolva00]

lib/Sema/SemaChecking.cpp

10918

llvm::fltSemantics has incomplete type and cannot be defined :/ Quite strange error

[xbolva00]

lib/Sema/SemaChecking.cpp

10939

I go thru LLVM code and I see we use rmNearestTiesToEven. So I am gonna change it.

[craig.topper]

lib/Sema/SemaChecking.cpp

110

Function names should start with lower case I think?

111

Can we use divideCeil from llvm/Support/MathExtras.h?

10937

Variable name should start with uppercase

[xbolva00]

lib/Sema/SemaChecking.cpp

110

float80 has precision = 64 so we can put MAX value of unsigned long long into it with no issues, or?

[xbolva00]

lib/Sema/SemaChecking.cpp

110

Other static functions here use upper case, e.g. "PrettyPrintInRange", so I follow it :)

[xbolva00]

• Addressed some comments

[xbolva00]

• Check also int128_t to float

lib/Sema/SemaChecking.cpp

110

float80 has precision = 64 so we can put MAX value of unsigned long long into it with no issues, or?

ah, int128_t

[xbolva00]

• Support for Float16

[rsmith]

lib/Sema/SemaChecking.cpp

110

LLVM will need to carry an implementation of the "shortest round-trippable digit sequence" soon regardless of what we do here, because it's part of the C++17 standard library. We should aim for an implementation that can be shared by APFloat and libc++.

[xbolva00]

lib/Sema/SemaChecking.cpp

110

You are right. But anyway, it should not block this patch, no? I can put TODO comment there.

[xbolva00]

This comment was removed by xbolva00.

[xbolva00]

Ping :)

[xbolva00]

Any comments? Waiting for approval here to move focus on some other patches :)

[efriedma]

Probably should have a test for something like float x = (__uint128_t)-1;, to make sure we print something reasonable.

lib/Sema/SemaChecking.cpp

10930

ASTContext::getFloatTypeSemantics. (Your explicit computation here is both redundant and wrong.)

[xbolva00]

lib/Sema/SemaChecking.cpp

10930

Great tip! thanks. I overlooked it :/

[xbolva00]

• Addressed comments

[efriedma]

lib/Sema/SemaChecking.cpp

10934–10935

It looks like this is reusing an existing diagnostic; what warning flag is this controlled by? It it possible we need a separate flag for this warning?

[xbolva00]

lib/Sema/SemaChecking.cpp

10934–10935

This is in LiteralConversion.

[xbolva00]

As noted, this in under LiteralConversion (-Wconversion). GCC has it too under -Wconversion, so I think it is fine as is, or?
@rsmith

[lebedev.ri]

In D52835#1263032, @xbolva00 wrote:

As noted, this in under LiteralConversion (-Wconversion). GCC has it too under -Wconversion, so I think it is fine as is, or?
@rsmith

It's not so much about "which flag group do i need to enable to get the warning";
it's about "which flag do i need to disable to silence it", and "how fine-grained that flag is, how many warnings will get disabled".
The flags should be pretty fine-grained.

[xbolva00]

So any suggestions? LiteralPrecision? ..

[xbolva00]

• New diagnostic group

[efriedma]

Looks like the latest update is missing a test file?

[xbolva00]

• Added missing test file

[xbolva00]

Is PrecisionConversion acceptable? If not, any suggestions for name?

[xbolva00]

Ping

[xbolva00]

ping

[xbolva00]

Ping

Anything else to be done here? I would like to finish this patch to work on other things.

[xbolva00]

maybe @aaron.ballman wants to take a look too

[xbolva00]

Since I don't think we have any false positives here, maybe it would be useful to add it to -Wall or -Wextra?

[xbolva00]

Ping :)

[xbolva00]

@aaron.ballman

[xbolva00]

Since no futher comments, i think it is ok.
After day or two I will commit it.

[aaron.ballman]

lib/Sema/SemaChecking.cpp

113–115

Parameter names should be in PascalCase, not camelCase.

10920–10921

It seems wrong to early return here -- that means none of the later checks are run on system macros, but we've also not diagnosed anything as being wrong with the user's code yet.

10938

This doesn't seem like something we need to early return for?

[xbolva00]

Reverted.
Ok, I will address your comments soon.

[aaron.ballman]

In D52835#1298428, @xbolva00 wrote:

Reverted.
Ok, I will address your comments soon.

Thanks! I'm sorry for the delayed reviews -- there were wg21 meetings last week, which several of the reviewers were attending.

[lebedev.ri]

Are you sure check-clang passes with this patch?
It looks like some bots weren't happy before the revert.
(Also, is this warning enabled when building llvm? If yes, you might want to double-check stage2 build.)

[xbolva00]

Addressed comments, fixed tests

[aaron.ballman]

It looks like you removed a considerable amount of testing coverage; why?

lib/Sema/SemaChecking.cpp

10920–10921

This changes the behavior of your patch -- I think it made sense to not trigger this diagnostic in a system macro. I was suggesting that you replace the early return with braces and flip the logic around so that you only do the diagnostic work if you're not in a system macro.

[xbolva00]

I removed _Float16 related tests since some bots may fail with it, i dont know much about this custom type.
Anyway, It is fine on linux x86 64. I will restore them.

System macro - I will fix it.

[xbolva00]

• Updated. Addressed review comments.

[aaron.ballman]

test/CXX/dcl.decl/dcl.init/dcl.init.list/p7-0x.cpp

136 ↗

(On Diff #175777)

I don't think we want the warning triggered in either of these cases -- they already have an error diagnostic on the same line for the same issue.

test/CXX/dcl.decl/dcl.init/dcl.init.list/p7-cxx11-nowarn.cpp

124 ↗

(On Diff #175777)

I think we don't want to duplicate the warnings here, either.

[xbolva00]

test/CXX/dcl.decl/dcl.init/dcl.init.list/p7-0x.cpp

136 ↗

(On Diff #175777)

Any recommended way how it should be handled?

[xbolva00]

@aaron.ballman does it make sense to warn for this case only in C/pre-C++11 mode? Since this case in C++11/14/17 is handled by -Wnarrowing, as we see from tests.

Or any other solution?

@rsmith

[aaron.ballman]

In D52835#1412416, @xbolva00 wrote:

@aaron.ballman does it make sense to warn for this case only in C/pre-C++11 mode? Since this case in C++11/14/17 is handled by -Wnarrowing, as we see from tests.

I don't think so, because code like this still needs the diagnostic even in C++11 mode, but there's no -Wnarrowing warning for it: float a1 = (1ULL << 31) + 1;

test/CXX/dcl.decl/dcl.init/dcl.init.list/p7-0x.cpp

136 ↗

(On Diff #175777)

Why is it triggering twice in the first place? Template instantiation, maybe?

include/clang/Basic/DiagnosticSemaKinds.td

def warn_impcast_literal_float_to_integer : Warning<
   InGroup<LiteralConversion>;
 def warn_impcast_literal_float_to_integer_out_of_range : Warning<
   "implicit conversion of out of range value from %0 to %1 is undefined">,
   InGroup<LiteralConversion>;
 def warn_impcast_float_integer : Warning<
   "implicit conversion turns floating-point number into integer: %0 to %1">,
   InGroup<FloatConversion>, DefaultIgnore;
 
+def warn_impcast_precision_float_to_integer : Warning<
+  "implicit conversion from %0 to %1 changes value from %2 to %3">,
+  InGroup<DiagGroup<"float-precision">>;
+
 def warn_impcast_float_to_integer : Warning<
   "implicit conversion from %0 to %1 changes value from %2 to %3">,
   InGroup<FloatOverflowConversion>, DefaultIgnore;
 def warn_impcast_float_to_integer_out_of_range : Warning<
   "implicit conversion of out of range value from %0 to %1 is undefined">,
   InGroup<FloatOverflowConversion>, DefaultIgnore;
 def warn_impcast_float_to_integer_zero : Warning<
   "implicit conversion from %0 to %1 changes non-zero value from %2 to %3">,

lib/Sema/SemaChecking.cpp

 using namespace sema;
 
 SourceLocation Sema::getLocationOfStringLiteralByte(const StringLiteral *SL,
                                                     unsigned ByteNo) const {
   return SL->getLocationOfByte(ByteNo, getSourceManager(), LangOpts,
                                Context.getTargetInfo());
 }
 
+// FIXME: Force the precision of the source value down so we don't print
+// digits which are usually useless (we don't really care here if we
+// truncate a digit by accident in edge cases).  Ideally, APFloat::toString
+// would automatically print the shortest representation, but it's a bit
+// tricky to implement.
+static void PrettyPrintFloat(const llvm::APFloat &floatValue,
+                             const llvm::fltSemantics &floatSem,
+                             SmallVectorImpl<char> &prettyFloatValue) {
+  unsigned precision = llvm::APFloat::semanticsPrecision(floatSem);
+  precision = llvm::divideCeil(precision * 59, 196);
+  floatValue.toString(prettyFloatValue, precision);
+}
+
 /// Checks that a call expression's argument count is the desired number.
 /// This is useful when doing custom type-checking.  Returns true on error.
 static bool checkArgCount(Sema &S, CallExpr *call, unsigned desiredArgCount) {
   unsigned argCount = call->getNumArgs();
   if (argCount == desiredArgCount) return false;
 
   if (argCount < desiredArgCount)
     return S.Diag(call->getEndLoc(), diag::err_typecheck_call_too_few_args)
if (IntegerValue.isUnsigned()) {
         return DiagnoseImpCast(S, E, T, CContext,
                                diag::warn_impcast_float_integer, PruneWarnings);
       }
     }
     // Warn on evaluatable floating point expression to integer conversion.
     DiagID = diag::warn_impcast_float_to_integer;
   }
 
-  // FIXME: Force the precision of the source value down so we don't print
-  // digits which are usually useless (we don't really care here if we
-  // truncate a digit by accident in edge cases).  Ideally, APFloat::toString
-  // would automatically print the shortest representation, but it's a bit
-  // tricky to implement.
   SmallString<16> PrettySourceValue;
-  unsigned precision = llvm::APFloat::semanticsPrecision(Value.getSemantics());
-  precision = (precision * 59 + 195) / 196;
-  Value.toString(PrettySourceValue, precision);
+  PrettyPrintFloat(Value, Value.getSemantics(), PrettySourceValue);
 
   SmallString<16> PrettyTargetValue;
   if (IsBool)
     PrettyTargetValue = Value.isZero() ? "false" : "true";
   else
     IntegerValue.toString(PrettyTargetValue);
 
   if (PruneWarnings) {
if (Target->isBooleanType() && isa<CallExpr>(E)) {
           DiagnoseImpCast(S, E, T, CC,
                           diag::warn_impcast_floating_point_to_bool);
         }
       }
     }
     return;
   }
 
+  if (Source->isIntegerType() && TargetBT && TargetBT->isFloatingType()) {
+    llvm::APSInt IntValue;
+    if (E->EvaluateAsInt(IntValue, S.Context, Expr::SE_AllowSideEffects)) {
+      if (S.SourceMgr.isInSystemMacro(CC))
+        return;
+      const llvm::fltSemantics &FloatSemantics =
+          S.Context.getFloatTypeSemantics(QualType(TargetBT, 0));
+      llvm::APFloat FloatValue(FloatSemantics);
+      if (FloatValue.convertFromAPInt(IntValue, Source->isSignedIntegerType(),
+                                      llvm::APFloat::rmNearestTiesToEven) !=
+          llvm::APFloat::opOK) {
+        SmallString<16> PrettyTargetValue;
+        SmallString<16> PrettySourceValue;
+        PrettyPrintFloat(FloatValue, FloatSemantics, PrettyTargetValue);
+        IntValue.toString(PrettySourceValue);
+
+        S.DiagRuntimeBehavior(
+            E->getExprLoc(), E,
+            S.PDiag(diag::warn_impcast_precision_float_to_integer)
+                << E->getType() << T << PrettySourceValue << PrettyTargetValue
+                << E->getSourceRange() << clang::SourceRange(CC));
+        return;
+      }
+    }
+  }
+
   DiagnoseNullConversion(S, E, T, CC);
 
   S.DiscardMisalignedMemberAddress(Target, E);
 
   if (!Source->isIntegerType() || !Target->isIntegerType())
     return;
 
   // TODO: remove this early return once the false positives for constant->bool

test/Sema/ext_vector_casts.c

static void splats(int i, long l, __uint128_t t, float f, double d) {
   
   vl = l + vl; // expected-warning {{implicit conversion changes signedness}}
   vl = vl + t; // expected-warning {{implicit conversion loses integer precision}}
   
   vf = 1 + vf;
   vf = l + vf;
   vf = 2.0 + vf;
   vf = d + vf; // expected-warning {{implicit conversion loses floating-point precision}}
-  vf = vf + 0xffffffff;
+  vf = vf + 0xffffffff; // expected-warning {{implicit conversion from 'unsigned int' to 'float2' (vector of 2 'float' values) changes value from 4294967295 to 4.2949673E+9}}
   vf = vf + 2.1; // expected-warning {{implicit conversion loses floating-point precision}}
   
   vd = l + vd;
   vd = vd + t;
 }

test/Sema/impcast-integer-float.c

+// RUN: %clang_cc1 %s -verify -Wfloat-precision -fsyntax-only
+
+#define shift_plus_one(x) ((1ULL << x) + 1)
+
+void test(void) {
+    float a1 = (1ULL << 31) + 1; // expected-warning {{implicit conversion from 'unsigned long long' to 'float' changes value from 2147483649 to 2.1474836E+9}}
+    float a2 = 1ULL << 31;
+    float a3 = shift_plus_one(31); // expected-warning {{implicit conversion from 'unsigned long long' to 'float' changes value from 2147483649 to 2.1474836E+9}}
+    float a4 = (1ULL << 31) - 1; // expected-warning {{implicit conversion from 'unsigned long long' to 'float' changes value from 2147483647 to 2.1474836E+9}}
+
+    double b1 = (1ULL << 63) + 1; // expected-warning {{implicit conversion from 'unsigned long long' to 'double' changes value from 9223372036854775809 to 9.223372036854775E+18}}
+    double b2 = 1ULL << 63;
+    double b3 = shift_plus_one(63); // expected-warning {{implicit conversion from 'unsigned long long' to 'double' changes value from 9223372036854775809 to 9.223372036854775E+18}}
+    double b4 = (1ULL << 63) - 1; // expected-warning {{implicit conversion from 'unsigned long long' to 'double' changes value from 9223372036854775807 to 9.223372036854775E+18}} 
+    
+    long double c1 = ((__int128)1 << 127) + 1; // expected-warning {{implicit conversion from '__int128' to 'long double' changes value from -170141183460469231731687303715884105727 to -1.7014118346046923173E+38}}
+    long double c2 = (__int128)1 << 127;
+
+    _Float16 d1 = (1ULL << 15) + 1; // expected-warning {{implicit conversion from 'unsigned long long' to '_Float16' changes value from 32769 to 3.277E+4}}
+    _Float16 d2 = 1ULL << 15;
+    _Float16 d3 = shift_plus_one(15); // expected-warning {{implicit conversion from 'unsigned long long' to '_Float16' changes value from 32769 to 3.277E+4}}
+    _Float16 d4 = (1ULL << 15) - 1; // expected-warning {{implicit conversion from 'unsigned long long' to '_Float16' changes value from 32767 to 3.277E+4}}
+
+    float e = (__uint128_t)-1; // expected-warning {{implicit conversion from '__uint128_t' (aka 'unsigned __int128') to 'float' changes value from 340282366920938463463374607431768211455 to +Inf}}
+}