[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...

10856

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.

10861

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

10867

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

10871

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?

10877

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

10856

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

[xbolva00]

lib/Sema/SemaChecking.cpp

10877

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?

10875

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

10868

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

[xbolva00]

lib/Sema/SemaChecking.cpp

10868

Great tip! thanks. I overlooked it :/

[xbolva00]

• Addressed comments

[efriedma]

lib/Sema/SemaChecking.cpp

10872–10873

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

10872–10873

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.

10858–10859

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.

10876

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

10858–10859

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?

lib/Sema/SemaChecking.cpp

 SourceLocation Sema::getLocationOfStringLiteralByte(const StringLiteral *SL,
                                                     unsigned ByteNo) const {
   return SL->getLocationOfByte(ByteNo, getSourceManager(), LangOpts,
                                Context.getTargetInfo());
 }
 
 /// 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)
            << 0 /*function call*/ << desiredArgCount << argCount
            << call->getSourceRange();
 
   // Highlight all the excess arguments.
   SourceRange range(call->getArg(desiredArgCount)->getBeginLoc(),
                     call->getArg(argCount - 1)->getEndLoc());
 
   return S.Diag(range.getBegin(), diag::err_typecheck_call_too_many_args)
if (Target->isBooleanType() && isa<CallExpr>(E)) {
           DiagnoseImpCast(S, E, T, CC,
                           diag::warn_impcast_floating_point_to_bool);
         }
       }
     }
     return;
   }
 
+  if (Source->isIntegerType() && Target->isFloatingType()) {
+    const llvm::fltSemantics *FloatSem = nullptr;
+    if (Target->isSpecificBuiltinType(BuiltinType::Float)) {
+      FloatSem = &llvm::APFloat::IEEEsingle();
+    } else if (Target->isSpecificBuiltinType(BuiltinType::Double)) {
+      FloatSem = &llvm::APFloat::IEEEdouble();
+    }
+
+    if (FloatSem) {
+      llvm::APFloat FloatValue(*FloatSem);
+      llvm::APSInt IntValue;
+      if (E->EvaluateAsInt(IntValue, S.Context, Expr::SE_AllowSideEffects)) {
+        if (S.SourceMgr.isInSystemMacro(CC))
+          return;
+
+        if (FloatValue.convertFromAPInt(IntValue, Source->isSignedIntegerType(),
+                                        llvm::APFloat::rmTowardZero) !=
+            llvm::APFloat::opOK) {
+          return DiagnoseImpCast(S, E, T, CC,
+                                 diag::warn_impcast_integer_precision);
+        }
+      }
+    }
+  }
+
   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 loses integer precision: 'unsigned int' to 'float2' (vector of 2 'float' values)}}
   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 -Wconversion -fsyntax-only
+
+#define shift_plus_one(x) ((1ULL << x) + 1)
+
+void test(void) {
+    float a1 = (1ULL << 31) + 1; // expected-warning {{implicit conversion loses integer precision: 'unsigned long long' to 'float'}}
+    float a2 = 1ULL << 31;
+    float a3 = shift_plus_one(31); // expected-warning {{implicit conversion loses integer precision: 'unsigned long long' to 'float'}}
+    float a4 = (1ULL << 31) - 1; // expected-warning {{implicit conversion loses integer precision: 'unsigned long long' to 'float'}}
+
+    double b1 = (1ULL << 63) + 1; // expected-warning {{implicit conversion loses integer precision: 'unsigned long long' to 'double'}}
+    double b2 = 1ULL << 63;
+    double b3 = shift_plus_one(63); // expected-warning {{implicit conversion loses integer precision: 'unsigned long long' to 'double'}}
+    double b4 = (1ULL << 63) - 1; // expected-warning {{implicit conversion loses integer precision: 'unsigned long long' to 'double'}}    
+}